TEST_CODEOWNERS/rails
3
0
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases Wiki Activity

Use throw for message error handling control flow

Browse Source 
There are multiple points of failure when processing a message with
`MessageEncryptor` or `MessageVerifier`, and there several ways we might
want to handle those failures.  For example, swallowing a failure with
`MessageVerifier#verified`, or raising a specific exception with
`MessageVerifier#verify`, or conditionally ignoring a failure when
rotations are configured.

Prior to this commit, the _internal_ logic of handling failures was
implemented using a mix of `nil` return values and raised exceptions.
This commit reimplements the internal logic using `throw` and a few
precisely targeted `rescue`s.  This accomplishes several things:

* Allow rotation of serializers for `MessageVerifier`.  Previously,
  errors from a `MessageVerifier`'s initial serializer were never
  rescued.  Thus, the serializer could not be rotated:

    ```ruby
    old_verifier = ActiveSupport::MessageVerifier.new("secret", serializer: Marshal)
    new_verifier = ActiveSupport::MessageVerifier.new("secret", serializer: JSON)
    new_verifier.rotate(serializer: Marshal)

    message = old_verifier.generate("message")

    new_verifier.verify(message)
    # BEFORE:
    # => raises JSON::ParserError
    # AFTER:
    # => "message"
    ```

* Allow rotation of serializers for `MessageEncryptor` when using a
  non-standard initial serializer.  Similar to `MessageVerifier`, the
  serializer could not be rotated when the initial serializer raised an
  error other than `TypeError` or `JSON::ParserError`, such as
  `Psych::SyntaxError` or a custom error.

* Raise `MessageEncryptor::InvalidMessage` from `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 `MessageVerifier::InvalidSignature` due to reliance on
  `MessageVerifier` for verification.  Now, the verification mechanism
  is transparent to the user:

    ```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
    ```

* Support `nil` original value when using `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("x" * 32)
    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
    ```

* Improve performance of verifying a message when it has expired and one
  or more rotations have been configured:

    ```ruby
    # frozen_string_literal: true
    require "benchmark/ips"
    require "active_support/all"

    verifier = ActiveSupport::MessageVerifier.new("new secret")
    verifier.rotate("old secret")

    message = verifier.generate({ "data" => "x" * 100 }, expires_at: 1.day.ago)

    Benchmark.ips do |x|
      x.report("expired message") do
        verifier.verified(message)
      end
    end
    ```

  __Before__

    ```
    Warming up --------------------------------------
         expired message     1.442k i/100ms
    Calculating -------------------------------------
         expired message     14.403k (± 1.7%) i/s -     72.100k in   5.007382s
    ```

  __After__

    ```
    Warming up --------------------------------------
         expired message     1.995k i/100ms
    Calculating -------------------------------------
         expired message     19.992k (± 2.0%) i/s -    101.745k in   5.091421s
    ```

Fixes #47185.
This commit is contained in:
Jonathan Hefner 2023-02-07 13:56:59 -06:00
parent 21a3b52ba0
commit d3917f5fdd
11 changed files with 201 additions and 57 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

51
activesupport/CHANGELOG.md
View File

@ -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

36
activesupport/lib/active_support/message_encryptor.rb
View File

@ -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

41
activesupport/lib/active_support/message_verifier.rb
View File

@ -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

19
activesupport/lib/active_support/messages/codec.rb
View File

@ -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

37
activesupport/lib/active_support/messages/metadata.rb
View File

@ -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

37
activesupport/lib/active_support/messages/rotator.rb
View File

@ -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

2
activesupport/test/message_encryptor_test.rb
View File

@ -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

2
activesupport/test/message_encryptors_test.rb
View File

@ -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

6
activesupport/test/message_verifier_test.rb
View File

@ -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

10
activesupport/test/messages/message_encryptor_rotator_test.rb
View File

@ -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

17
activesupport/test/messages/message_rotator_tests.rb
View File

@ -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