diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md index 8656d26fe5..95cb710931 100644 --- a/activesupport/CHANGELOG.md +++ b/activesupport/CHANGELOG.md @@ -1,3 +1,54 @@ +* Raise `ActiveSupport::MessageEncryptor::InvalidMessage` from + `ActiveSupport::MessageEncryptor#decrypt_and_verify` regardless of cipher. + Previously, when a `MessageEncryptor` was using a non-AEAD cipher such as + AES-256-CBC, a corrupt or tampered message would raise + `ActiveSupport::MessageVerifier::InvalidSignature`. Now, all ciphers raise + the same error: + + ```ruby + encryptor = ActiveSupport::MessageEncryptor.new("x" * 32, cipher: "aes-256-gcm") + message = encryptor.encrypt_and_sign("message") + encryptor.decrypt_and_verify(message.next) + # => raises ActiveSupport::MessageEncryptor::InvalidMessage + + encryptor = ActiveSupport::MessageEncryptor.new("x" * 32, cipher: "aes-256-cbc") + message = encryptor.encrypt_and_sign("message") + encryptor.decrypt_and_verify(message.next) + # BEFORE: + # => raises ActiveSupport::MessageVerifier::InvalidSignature + # AFTER: + # => raises ActiveSupport::MessageEncryptor::InvalidMessage + ``` + + *Jonathan Hefner* + +* Support `nil` original values when using `ActiveSupport::MessageVerifier#verify`. + Previously, `MessageVerifier#verify` did not work with `nil` original + values, though both `MessageVerifier#verified` and + `MessageEncryptor#decrypt_and_verify` do: + + ```ruby + encryptor = ActiveSupport::MessageEncryptor.new(secret) + message = encryptor.encrypt_and_sign(nil) + + encryptor.decrypt_and_verify(message) + # => nil + + verifier = ActiveSupport::MessageVerifier.new(secret) + message = verifier.generate(nil) + + verifier.verified(message) + # => nil + + verifier.verify(message) + # BEFORE: + # => raises ActiveSupport::MessageVerifier::InvalidSignature + # AFTER: + # => nil + ``` + + *Jonathan Hefner* + * Maintain `html_safe?` on html_safe strings when sliced with `slice`, `slice!`, or `chr` method. Previously, `html_safe?` was only maintained when the html_safe strings were sliced diff --git a/activesupport/lib/active_support/message_encryptor.rb b/activesupport/lib/active_support/message_encryptor.rb index d5255ea3ed..0a0873b5a8 100644 --- a/activesupport/lib/active_support/message_encryptor.rb +++ b/activesupport/lib/active_support/message_encryptor.rb @@ -86,7 +86,7 @@ module ActiveSupport # # crypt.rotate old_secret, cipher: "aes-256-cbc" class MessageEncryptor < Messages::Codec - prepend Messages::Rotator::Encryptor + prepend Messages::Rotator cattr_accessor :use_authenticated_message_encryption, instance_accessor: false, default: false cattr_accessor :default_message_encryptor_serializer, instance_accessor: false, default: :marshal @@ -174,7 +174,7 @@ def initialize(secret, sign_secret = nil, cipher: nil, digest: nil, serializer: # specified when verifying the message; otherwise, verification will fail. # (See #decrypt_and_verify.) def encrypt_and_sign(value, **options) - sign(encrypt(serialize_with_metadata(value, **options))) + create_message(value, **options) end # Decrypt and verify a message. We need to verify the message in order to @@ -195,9 +195,13 @@ def encrypt_and_sign(value, **options) # encryptor.decrypt_and_verify(message, purpose: "greeting") # => nil # def decrypt_and_verify(message, **options) - deserialize_with_metadata(decrypt(verify(message)), **options) - rescue TypeError, ArgumentError, ::JSON::ParserError - raise InvalidMessage + catch_and_raise :invalid_message_format, as: InvalidMessage do + catch_and_raise :invalid_message_serialization, as: InvalidMessage do + catch_and_ignore :invalid_message_content do + read_message(message, **options) + end + end + end end # Given a cipher, returns the key length of the cipher to help generate the key of desired size @@ -205,13 +209,21 @@ def self.key_len(cipher = default_cipher) OpenSSL::Cipher.new(cipher).key_len end + def create_message(value, **options) # :nodoc: + sign(encrypt(serialize_with_metadata(value, **options))) + end + + def read_message(message, **options) # :nodoc: + deserialize_with_metadata(decrypt(verify(message)), **options) + end + private def sign(data) - @verifier ? @verifier.generate(data) : data + @verifier ? @verifier.create_message(data) : data end def verify(data) - @verifier ? @verifier.verify(data) : data + @verifier ? @verifier.read_message(data) : data end def encrypt(data) @@ -239,7 +251,9 @@ def decrypt(encrypted_message) # Currently the OpenSSL bindings do not raise an error if auth_tag is # truncated, which would allow an attacker to easily forge it. See # https://github.com/ruby/openssl/issues/63 - raise InvalidMessage if aead_mode? && auth_tag.bytesize != AUTH_TAG_LENGTH + if aead_mode? && auth_tag.bytesize != AUTH_TAG_LENGTH + throw :invalid_message_format, "truncated auth_tag" + end cipher.decrypt cipher.key = @secret @@ -251,8 +265,8 @@ def decrypt(encrypted_message) decrypted_data = cipher.update(encrypted_data) decrypted_data << cipher.final - rescue OpenSSLCipherError - raise InvalidMessage + rescue OpenSSLCipherError => error + throw :invalid_message_format, error end def length_after_encode(length_before_encode) @@ -281,7 +295,7 @@ def extract_part(encrypted_message, rindex, length) if encrypted_message[index - SEPARATOR.length, SEPARATOR.length] == SEPARATOR encrypted_message[index, length] else - raise InvalidMessage + throw :invalid_message_format, "missing separator" end end diff --git a/activesupport/lib/active_support/message_verifier.rb b/activesupport/lib/active_support/message_verifier.rb index 785551703f..d107a7f806 100644 --- a/activesupport/lib/active_support/message_verifier.rb +++ b/activesupport/lib/active_support/message_verifier.rb @@ -119,7 +119,7 @@ module ActiveSupport # @verifier = ActiveSupport::MessageVerifier.new("secret", url_safe: true) # @verifier.generate("signed message") #=> URL-safe string class MessageVerifier < Messages::Codec - prepend Messages::Rotator::Verifier + prepend Messages::Rotator class InvalidSignature < StandardError; end @@ -145,7 +145,7 @@ def initialize(secret, digest: nil, serializer: nil, url_safe: false) # tampered_message = signed_message.chop # editing the message invalidates the signature # verifier.valid_message?(tampered_message) # => false def valid_message?(message) - !!extract_encoded(message) + !!catch_and_ignore(:invalid_message_format) { extract_encoded(message) } end # Decodes the signed message using the +MessageVerifier+'s secret. @@ -186,10 +186,13 @@ def valid_message?(message) # verifier.verified(message, purpose: "greeting") # => nil # def verified(message, **options) - encoded = extract_encoded(message) - deserialize_with_metadata(decode(encoded), **options) if encoded - rescue ArgumentError => error - raise unless error.message.include?("invalid base64") + catch_and_ignore :invalid_message_format do + catch_and_raise :invalid_message_serialization do + catch_and_ignore :invalid_message_content do + read_message(message, **options) + end + end + end end # Decodes the signed message using the +MessageVerifier+'s secret. @@ -220,8 +223,14 @@ def verified(message, **options) # verifier.verify(message) # => "bye" # verifier.verify(message, purpose: "greeting") # => raises InvalidSignature # - def verify(*args, **options) - verified(*args, **options) || raise(InvalidSignature) + def verify(message, **options) + catch_and_raise :invalid_message_format, as: InvalidSignature do + catch_and_raise :invalid_message_serialization do + catch_and_raise :invalid_message_content, as: InvalidSignature do + read_message(message, **options) + end + end + end end # Generates a signed message for the provided value. @@ -259,9 +268,17 @@ def verify(*args, **options) # specified when verifying the message; otherwise, verification will fail. # (See #verified and #verify.) def generate(value, **options) + create_message(value, **options) + end + + def create_message(value, **options) # :nodoc: sign_encoded(encode(serialize_with_metadata(value, **options))) end + def read_message(message, **options) # :nodoc: + deserialize_with_metadata(decode(extract_encoded(message)), **options) + end + private def sign_encoded(encoded) digest = generate_digest(encoded) @@ -269,14 +286,18 @@ def sign_encoded(encoded) end def extract_encoded(signed) - return if signed.nil? || !signed.valid_encoding? + if signed.nil? || !signed.valid_encoding? + throw :invalid_message_format, "invalid message string" + end if separator_index = separator_index_for(signed) encoded = signed[0, separator_index] digest = signed[separator_index + SEPARATOR_LENGTH, digest_length_in_hex] end - return unless digest_matches_data?(digest, encoded) + unless digest_matches_data?(digest, encoded) + throw :invalid_message_format, "mismatched digest" + end encoded end diff --git a/activesupport/lib/active_support/messages/codec.rb b/activesupport/lib/active_support/messages/codec.rb index 1c459364d4..10f860ceae 100644 --- a/activesupport/lib/active_support/messages/codec.rb +++ b/activesupport/lib/active_support/messages/codec.rb @@ -32,6 +32,8 @@ def encode(data, url_safe: @url_safe) def decode(encoded, url_safe: @url_safe) url_safe ? ::Base64.urlsafe_decode64(encoded) : ::Base64.strict_decode64(encoded) + rescue ArgumentError => error + throw :invalid_message_format, error end def serialize(data) @@ -40,6 +42,23 @@ def serialize(data) def deserialize(serialized) serializer.load(serialized) + rescue StandardError => error + throw :invalid_message_serialization, error + end + + def catch_and_ignore(throwable, &block) + catch throwable do + return block.call + end + nil + end + + def catch_and_raise(throwable, as: nil, &block) + error = catch throwable do + return block.call + end + error = as.new(error.to_s) if as + raise error end end end diff --git a/activesupport/lib/active_support/messages/metadata.rb b/activesupport/lib/active_support/messages/metadata.rb index 9ee2f85357..5b8ccf6870 100644 --- a/activesupport/lib/active_support/messages/metadata.rb +++ b/activesupport/lib/active_support/messages/metadata.rb @@ -22,7 +22,7 @@ def serialize_with_metadata(data, **metadata) if has_metadata && !use_message_serializer_for_metadata? data_string = serialize_to_json_safe_string(data) envelope = wrap_in_metadata_envelope({ "message" => data_string }, **metadata) - ActiveSupport::JSON.encode(envelope) + serialize_to_json(envelope) else data = wrap_in_metadata_envelope({ "data" => data }, **metadata) if has_metadata serialize(data) @@ -31,16 +31,17 @@ def serialize_with_metadata(data, **metadata) def deserialize_with_metadata(message, **expected_metadata) if dual_serialized_metadata_envelope_json?(message) - envelope = ActiveSupport::JSON.decode(message) + envelope = deserialize_from_json(message) extracted = extract_from_metadata_envelope(envelope, **expected_metadata) - deserialize_from_json_safe_string(extracted["message"]) if extracted + deserialize_from_json_safe_string(extracted["message"]) else deserialized = deserialize(message) if metadata_envelope?(deserialized) - extracted = extract_from_metadata_envelope(deserialized, **expected_metadata) - extracted["data"] if extracted + extract_from_metadata_envelope(deserialized, **expected_metadata)["data"] + elsif expected_metadata.none? { |k, v| v } + deserialized else - deserialized if expected_metadata.none? { |k, v| v } + throw :invalid_message_content, "missing metadata" end end end @@ -58,8 +59,15 @@ def wrap_in_metadata_envelope(hash, expires_at: nil, expires_in: nil, purpose: n def extract_from_metadata_envelope(envelope, purpose: nil) hash = envelope["_rails"] - return if hash["exp"] && Time.now.utc >= parse_expiry(hash["exp"]) - return if hash["pur"] != purpose&.to_s + + if hash["exp"] && Time.now.utc >= parse_expiry(hash["exp"]) + throw :invalid_message_content, "expired" + end + + if hash["pur"] != purpose&.to_s + throw :invalid_message_content, "mismatched purpose" + end + hash end @@ -89,6 +97,19 @@ def parse_expiry(expires_at) end end + def serialize_to_json(data) + ActiveSupport::JSON.encode(data) + end + + def deserialize_from_json(serialized) + ActiveSupport::JSON.decode(serialized) + rescue ::JSON::ParserError => error + # Throw :invalid_message_format instead of :invalid_message_serialization + # because here a parse error is due to a bad message rather than an + # incompatible `self.serializer`. + throw :invalid_message_format, error + end + def serialize_to_json_safe_string(data) encode(serialize(data), url_safe: false) end diff --git a/activesupport/lib/active_support/messages/rotator.rb b/activesupport/lib/active_support/messages/rotator.rb index 8822799d37..180545208f 100644 --- a/activesupport/lib/active_support/messages/rotator.rb +++ b/activesupport/lib/active_support/messages/rotator.rb @@ -20,21 +20,23 @@ def fall_back_to(fallback) self end - module Encryptor # :nodoc: - include Rotator - - def decrypt_and_verify(message, on_rotation: @on_rotation, **options) + def read_message(message, on_rotation: @on_rotation, **options) + if @rotations.empty? super(message, **options) - rescue MessageEncryptor::InvalidMessage, MessageVerifier::InvalidSignature - run_rotations(on_rotation) { |encryptor| encryptor.decrypt_and_verify(message, **options) } || raise - end - end + else + thrown, error = catch_rotation_error do + return super(message, **options) + end - module Verifier # :nodoc: - include Rotator + @rotations.each do |rotation| + catch_rotation_error do + value = rotation.read_message(message, **options) + on_rotation&.call + return value + end + end - def verified(message, on_rotation: @on_rotation, **options) - super(message, **options) || run_rotations(on_rotation) { |verifier| verifier.verified(message, **options) } + throw thrown, error end end @@ -43,13 +45,14 @@ def build_rotation(*args, **options) self.class.new(*args, *@args.drop(args.length), **@options, **options) end - def run_rotations(on_rotation) - @rotations.find do |rotation| - if message = yield(rotation) rescue next - on_rotation&.call - return message + def catch_rotation_error(&block) + error = catch :invalid_message_format do + error = catch :invalid_message_serialization do + return [nil, block.call] end + return [:invalid_message_serialization, error] end + [:invalid_message_format, error] end end end diff --git a/activesupport/test/message_encryptor_test.rb b/activesupport/test/message_encryptor_test.rb index 7564a9dd35..ab0aaacec3 100644 --- a/activesupport/test/message_encryptor_test.rb +++ b/activesupport/test/message_encryptor_test.rb @@ -164,7 +164,7 @@ def assert_not_decrypted(value) end def assert_not_verified(value) - assert_raise(ActiveSupport::MessageVerifier::InvalidSignature) do + assert_raise(ActiveSupport::MessageEncryptor::InvalidMessage) do @encryptor.decrypt_and_verify(value) end end diff --git a/activesupport/test/message_encryptors_test.rb b/activesupport/test/message_encryptors_test.rb index 573c41f970..769aafbfc3 100644 --- a/activesupport/test/message_encryptors_test.rb +++ b/activesupport/test/message_encryptors_test.rb @@ -48,7 +48,7 @@ def make_coordinator def roundtrip(message, encryptor, decryptor = encryptor) decryptor.decrypt_and_verify(encryptor.encrypt_and_sign(message)) - rescue ActiveSupport::MessageVerifier::InvalidSignature + rescue ActiveSupport::MessageEncryptor::InvalidMessage nil end end diff --git a/activesupport/test/message_verifier_test.rb b/activesupport/test/message_verifier_test.rb index da8d5e6a2d..20ae9d6392 100644 --- a/activesupport/test/message_verifier_test.rb +++ b/activesupport/test/message_verifier_test.rb @@ -38,6 +38,12 @@ def test_simple_round_tripping assert_equal @data, @verifier.verify(message) end + def test_round_tripping_nil + message = @verifier.generate(nil) + assert_nil @verifier.verified(message) + assert_nil @verifier.verify(message) + end + def test_verified_returns_false_on_invalid_message assert_not @verifier.verified("purejunk") end diff --git a/activesupport/test/messages/message_encryptor_rotator_test.rb b/activesupport/test/messages/message_encryptor_rotator_test.rb index fff0eb71c5..1c5daf09d5 100644 --- a/activesupport/test/messages/message_encryptor_rotator_test.rb +++ b/activesupport/test/messages/message_encryptor_rotator_test.rb @@ -10,14 +10,6 @@ class MessageEncryptorRotatorTest < ActiveSupport::TestCase assert_rotate [cipher: "aes-256-gcm"], [cipher: "aes-256-cbc"] end - test "rotate serializer" do - assert_rotate [serializer: JSON], [serializer: Marshal] - end - - test "rotate serializer when message has purpose" do - assert_rotate [serializer: JSON], [serializer: Marshal], purpose: "purpose" - end - test "rotate verifier secret when using non-authenticated encryption" do with_authenticated_encryption(false) do assert_rotate \ @@ -49,7 +41,7 @@ def encode(data, encryptor, **options) def decode(message, encryptor, **options) encryptor.decrypt_and_verify(message, **options) - rescue ActiveSupport::MessageVerifier::InvalidSignature + rescue ActiveSupport::MessageEncryptor::InvalidMessage nil end diff --git a/activesupport/test/messages/message_rotator_tests.rb b/activesupport/test/messages/message_rotator_tests.rb index 4f9e0eb4d0..bd90a7416e 100644 --- a/activesupport/test/messages/message_rotator_tests.rb +++ b/activesupport/test/messages/message_rotator_tests.rb @@ -18,6 +18,23 @@ module MessageRotatorTests assert_rotate [url_safe: true], [url_safe: false] end + test "rotate serializer" do + assert_rotate [serializer: JSON], [serializer: Marshal] + end + + test "rotate serializer when message has purpose" do + assert_rotate [serializer: JSON], [serializer: Marshal], purpose: "purpose" + end + + test "rotate serializer that raises a custom deserialization error" do + serializer = Class.new do + def self.dump(*); ""; end + def self.load(*); raise Class.new(StandardError); end + end + + assert_rotate [serializer: serializer], [serializer: JSON], [serializer: Marshal] + end + test "rotate secret and options" do assert_rotate [secret("new"), url_safe: true], [secret("old"), url_safe: false] end