diff --git a/app/controllers/legacy_api/messages_controller.rb b/app/controllers/legacy_api/messages_controller.rb index 6d78c16..239eee3 100644 --- a/app/controllers/legacy_api/messages_controller.rb +++ b/app/controllers/legacy_api/messages_controller.rb @@ -15,12 +15,9 @@ module LegacyAPI # OR an error if the message does not exist. # def message - if api_params["id"].blank? - render_parameter_error "`id` parameter is required but is missing" - return - end + message = find_message + return if performed? - message = @current_credential.server.message(api_params["id"]) message_hash = { id: message.id, token: message.token } expansions = api_params["_expansions"] @@ -111,12 +108,9 @@ module LegacyAPI # OR an error if the message does not exist. # def deliveries - if api_params["id"].blank? - render_parameter_error "`id` parameter is required but is missing" - return - end + message = find_message + return if performed? - message = @current_credential.server.message(api_params["id"]) deliveries = message.deliveries.map do |d| { id: d.id, @@ -136,5 +130,37 @@ module LegacyAPI id: api_params["id"] end + private + + # Look up the message referenced by the request's `id` parameter. + # + # The legacy API only ever identifies a message by its integer ID. The + # request body is parsed as JSON, so without validation a JSON object or + # array supplied for `id` would arrive as a Ruby Hash/Array and be passed + # straight through to the message database as a raw set of SQL conditions. + # We therefore reject anything that is not a simple scalar before it can + # reach the database and coerce the value to an integer ID. + # + # Renders an error and returns nil when the parameter is missing or is not + # a scalar; otherwise returns the matched message (raising NotFound when no + # message matches, which the actions rescue). + # + # @return [Postal::MessageDB::Message, nil] + def find_message + id = api_params["id"] + + if id.blank? + render_parameter_error "`id` parameter is required but is missing" + return + end + + unless id.is_a?(String) || id.is_a?(Integer) + render_parameter_error "`id` parameter must be a string or integer" + return + end + + @current_credential.server.message(id.to_i) + end + end end diff --git a/lib/postal/message_db/database.rb b/lib/postal/message_db/database.rb index 52f4569..415a88d 100644 --- a/lib/postal/message_db/database.rb +++ b/lib/postal/message_db/database.rb @@ -70,7 +70,7 @@ module Postal # Return the total size of all stored messages # def total_size - query("SELECT SUM(size) AS size FROM `#{database_name}`.`raw_message_sizes`").first["size"] || 0 + query("SELECT SUM(size) AS size FROM #{escape_identifier(database_name)}.`raw_message_sizes`").first["size"] || 0 end # @@ -151,11 +151,11 @@ module Postal if options[:count] sql_query << " COUNT(id) AS count" elsif options[:fields] - sql_query << (" " + options[:fields].map { |f| "`#{f}`" }.join(", ")) + sql_query << (" " + options[:fields].map { |f| escape_identifier(f) }.join(", ")) else sql_query << " *" end - sql_query << " FROM `#{database_name}`.`#{table}`" + sql_query << " FROM #{escape_identifier(database_name)}.#{escape_identifier(table)}" if options[:where].present? sql_query << (" " + build_where_string(options[:where], " AND ")) end @@ -163,7 +163,7 @@ module Postal direction = (options[:direction] || "ASC").upcase raise Postal::Error, "Invalid direction #{options[:direction]}" unless %w[ASC DESC].include?(direction) - sql_query << " ORDER BY `#{options[:order]}` #{direction}" + sql_query << " ORDER BY #{escape_identifier(options[:order])} #{direction}" end if options[:limit] @@ -211,7 +211,7 @@ module Postal # Will return the total number of affected rows. # def update(table, attributes, options = {}) - sql_query = "UPDATE `#{database_name}`.`#{table}` SET" + sql_query = "UPDATE #{escape_identifier(database_name)}.#{escape_identifier(table)} SET" sql_query << " #{hash_to_sql(attributes)}" if options[:where] sql_query << (" " + build_where_string(options[:where])) @@ -227,8 +227,8 @@ module Postal # Will return the ID of the new item. # def insert(table, attributes) - sql_query = "INSERT INTO `#{database_name}`.`#{table}`" - sql_query << (" (" + attributes.keys.map { |k| "`#{k}`" }.join(", ") + ")") + sql_query = "INSERT INTO #{escape_identifier(database_name)}.#{escape_identifier(table)}" + sql_query << (" (" + attributes.keys.map { |k| escape_identifier(k) }.join(", ") + ")") sql_query << (" VALUES (" + attributes.values.map { |v| escape(v) }.join(", ") + ")") with_mysql do |mysql| query_on_connection(mysql, sql_query) @@ -243,8 +243,8 @@ module Postal if values.empty? nil else - sql_query = "INSERT INTO `#{database_name}`.`#{table}`" - sql_query << (" (" + keys.map { |k| "`#{k}`" }.join(", ") + ")") + sql_query = "INSERT INTO #{escape_identifier(database_name)}.#{escape_identifier(table)}" + sql_query << (" (" + keys.map { |k| escape_identifier(k) }.join(", ") + ")") sql_query << " VALUES " sql_query << values.map { |v| "(" + v.map { |r| escape(r) }.join(", ") + ")" }.join(", ") query(sql_query) @@ -260,7 +260,7 @@ module Postal # Will return the total number of affected rows. # def delete(table, options = {}) - sql_query = "DELETE FROM `#{database_name}`.`#{table}`" + sql_query = "DELETE FROM #{escape_identifier(database_name)}.#{escape_identifier(table)}" sql_query << (" " + build_where_string(options[:where], " AND ")) with_mysql do |mysql| query_on_connection(mysql, sql_query) @@ -351,32 +351,41 @@ module Postal def hash_to_sql(hash, joiner = ", ") hash.map do |key, value| + column = escape_identifier(key) if value.is_a?(Array) && value.all? { |v| v.is_a?(Integer) } - "`#{key}` IN (#{value.join(', ')})" + "#{column} IN (#{value.join(', ')})" elsif value.is_a?(Array) escaped_values = value.map { |v| escape(v) }.join(", ") - "`#{key}` IN (#{escaped_values})" + "#{column} IN (#{escaped_values})" elsif value.is_a?(Hash) sql = [] value.each do |operator, inner_value| case operator when :less_than - sql << "`#{key}` < #{escape(inner_value)}" + sql << "#{column} < #{escape(inner_value)}" when :greater_than - sql << "`#{key}` > #{escape(inner_value)}" + sql << "#{column} > #{escape(inner_value)}" when :less_than_or_equal_to - sql << "`#{key}` <= #{escape(inner_value)}" + sql << "#{column} <= #{escape(inner_value)}" when :greater_than_or_equal_to - sql << "`#{key}` >= #{escape(inner_value)}" + sql << "#{column} >= #{escape(inner_value)}" end end sql.empty? ? "1=1" : sql.join(joiner) else - "`#{key}` = #{escape(value)}" + "#{column} = #{escape(value)}" end end.join(joiner) end + # Escape a value for safe use as a MySQL identifier (e.g. a column or + # table name). Identifiers are wrapped in backticks and any backtick + # within the identifier is doubled so it cannot break out of the quoting + # and inject arbitrary SQL. + def escape_identifier(identifier) + "`" + identifier.to_s.gsub("`", "``") + "`" + end + end end end diff --git a/spec/apis/legacy_api/messages/deliveries_spec.rb b/spec/apis/legacy_api/messages/deliveries_spec.rb index 2daf95f..cfc69cc 100644 --- a/spec/apis/legacy_api/messages/deliveries_spec.rb +++ b/spec/apis/legacy_api/messages/deliveries_spec.rb @@ -64,6 +64,23 @@ RSpec.describe "Legacy Messages API", type: :request do end end + # Regression test for GHSA-x2hq-rfpg-3xr5 (see message_spec.rb). A JSON + # object supplied for `id` must be rejected before reaching the database + # rather than being interpreted as a raw set of SQL conditions. + context "when the message ID is a JSON object (SQL injection attempt)" do + it "rejects it with a parameter error and never reaches the database" do + expect_any_instance_of(Server).not_to receive(:message) + post "/api/v1/messages/deliveries", + headers: { "x-server-api-key" => credential.key, + "content-type" => "application/json" }, + params: { id: { "id`=0 OR SLEEP(5)#" => "x" } }.to_json + expect(response.status).to eq 200 + parsed_body = JSON.parse(response.body) + expect(parsed_body["status"]).to eq "parameter-error" + expect(parsed_body["data"]["message"]).to match(/must be a string or integer/) + end + end + context "when the message ID exists" do let(:server) { create(:server) } let(:credential) { create(:credential, server: server) } diff --git a/spec/apis/legacy_api/messages/message_spec.rb b/spec/apis/legacy_api/messages/message_spec.rb index 202b66b..c40d756 100644 --- a/spec/apis/legacy_api/messages/message_spec.rb +++ b/spec/apis/legacy_api/messages/message_spec.rb @@ -63,6 +63,56 @@ RSpec.describe "Legacy Messages API", type: :request do end end + # Regression tests for GHSA-x2hq-rfpg-3xr5. The request body is parsed as + # JSON, so a JSON object/array supplied for `id` would otherwise arrive as + # a Ruby Hash/Array and be passed straight through to the message database + # as a raw set of SQL conditions (blind SQL injection). These must be + # rejected before reaching the database. + context "when the message ID is a JSON object (SQL injection attempt)" do + it "rejects it with a parameter error and never reaches the database" do + expect_any_instance_of(Server).not_to receive(:message) + post "/api/v1/messages/message", + headers: { "x-server-api-key" => credential.key, + "content-type" => "application/json" }, + params: { id: { "id`=0 OR SLEEP(5)#" => "x" } }.to_json + expect(response.status).to eq 200 + parsed_body = JSON.parse(response.body) + expect(parsed_body["status"]).to eq "parameter-error" + expect(parsed_body["data"]["message"]).to match(/must be a string or integer/) + end + end + + context "when the message ID is a JSON array" do + it "rejects it with a parameter error" do + post "/api/v1/messages/message", + headers: { "x-server-api-key" => credential.key, + "content-type" => "application/json" }, + params: { id: [1, 2, 3] }.to_json + expect(response.status).to eq 200 + parsed_body = JSON.parse(response.body) + expect(parsed_body["status"]).to eq "parameter-error" + expect(parsed_body["data"]["message"]).to match(/must be a string or integer/) + end + end + + context "when the message ID is provided as a numeric string" do + let(:message) { MessageFactory.outgoing(server) } + + it "is coerced to an integer and looks the message up" do + post "/api/v1/messages/message", + headers: { "x-server-api-key" => credential.key, + "content-type" => "application/json" }, + params: { id: message.id.to_s }.to_json + expect(response.status).to eq 200 + parsed_body = JSON.parse(response.body) + expect(parsed_body["status"]).to eq "success" + expect(parsed_body["data"]).to match({ + "id" => message.id, + "token" => message.token + }) + end + end + context "when the message ID exists" do let(:server) { create(:server) } let(:credential) { create(:credential, server: server) } diff --git a/spec/lib/postal/message_db/database_spec.rb b/spec/lib/postal/message_db/database_spec.rb index ff9eec4..dc9e960 100644 --- a/spec/lib/postal/message_db/database_spec.rb +++ b/spec/lib/postal/message_db/database_spec.rb @@ -14,5 +14,64 @@ describe Postal::MessageDB::Database do it "should return the current schema version" do expect(database.schema_version).to be_a Integer end + + describe "#escape_identifier" do + it "wraps a plain identifier in backticks" do + expect(database.send(:escape_identifier, "id")).to eq "`id`" + end + + it "doubles embedded backticks so the value cannot break out of the quoting" do + expect(database.send(:escape_identifier, "id`=0 OR SLEEP(5)#")) + .to eq "`id``=0 OR SLEEP(5)#`" + end + + it "coerces non-string identifiers to a string" do + expect(database.send(:escape_identifier, :token)).to eq "`token`" + end + end + + describe "#hash_to_sql" do + it "builds a simple equality condition" do + expect(database.send(:hash_to_sql, { "id" => 5 })).to eq "`id` = '5'" + end + + it "builds an IN condition for an array of integers" do + expect(database.send(:hash_to_sql, { "id" => [1, 2] })).to eq "`id` IN (1, 2)" + end + + it "builds operator conditions for a hash value" do + expect(database.send(:hash_to_sql, { "id" => { greater_than: 1 } })) + .to eq "`id` > '1'" + end + + # Regression tests for GHSA-x2hq-rfpg-3xr5: a backtick in the condition + # key must be neutralised so it cannot close the identifier quoting and + # inject arbitrary SQL. + it "neutralises a backtick injection in an equality key" do + sql = database.send(:hash_to_sql, { "id`=0 OR SLEEP(5)#" => "x" }) + expect(sql).to eq "`id``=0 OR SLEEP(5)#` = 'x'" + end + + it "neutralises a backtick injection in an IN key" do + sql = database.send(:hash_to_sql, { "id`)#" => %w[a b] }) + expect(sql).to eq "`id``)#` IN ('a', 'b')" + end + + it "neutralises a backtick injection in an operator key" do + sql = database.send(:hash_to_sql, { "id`#" => { greater_than: 1 } }) + expect(sql).to eq "`id``#` > '1'" + end + end + + describe "#select with a hostile condition key" do + # End-to-end proof against the live test database: the injected key is + # treated as a single (non-existent) column identifier, so MySQL rejects + # the query instead of executing the injected SQL. + it "does not allow SQL injection via the condition key" do + expect do + database.select("messages", where: { "id`=0 OR 1=1#" => "x" }, limit: 1) + end.to raise_error(Mysql2::Error) + end + end end end