Deserialization

Ruby PoC (Rails environment required): # Demonstrates how corrupted payloads are handled after the fix vs. before # Requires Rails/ActiveSupport loaded poc = <<~RUBY coder = ActiveSupport::Cache::Coder.new(Marshal, Zlib) entry = ActiveSupport::Cache::Entry.new(["test"], version: "12", expires_in: 34) # Build a compressed payload payload = coder.dump_compressed(entry, 1) # Corrupt the payload by removing the first byte (invalid header/signature path) corrupted = payload.byteslice(1..-1) # With the fix: this should not raise, should return nil and report the error puts "Corrupted load result: #{coder.load(corrupted).inspect}" # Now attempt to lazy-load a normal payload up to the deserialization step lazy_entry = coder.load(payload.byteslice(0..-2)) begin puts "Lazy value: #{lazy_entry.value}" rescue => e puts "Lazy load raised: #{e.class}: #{e.message}" end # Also demonstrate uncompressed path (dump without compression) and its corruption payload_uncompressed = coder.dump(entry) corrupted_uncompressed = payload_uncompressed.byteslice(1..-1) puts "Corrupted uncompressed load: #{coder.load(corrupted_uncompressed).inspect}" RUBY puts poc

diff --git a/activesupport/lib/active_support/cache/coder.rb b/activesupport/lib/active_support/cache/coder.rb index b21d07dc6d5e2..5c018cc537a7c 100644 --- a/activesupport/lib/active_support/cache/coder.rb +++ b/activesupport/lib/active_support/cache/coder.rb @@ -39,14 +39,19 @@ def dump_compressed(entry, threshold) version = dump_version(entry.version) if entry.version version_length = version&.bytesize || -1 - packed = SIGNATURE.b - packed << [type, expires_at, version_length].pack(PACKED_TEMPLATE) - packed << version if version - packed << payload + header = [type, expires_at, version_length].pack(PACKED_TEMPLATE) + + "#{SIGNATURE}#{header}#{version}#{payload}".freeze end def load(dumped) - return @serializer.load(dumped) if !signature?(dumped) + unless signature?(dumped) + return begin + @serializer.load(dumped) + rescue => error + ActiveSupport.error_reporter.report(error, source: "active_support.cache") + end + end type = dumped.unpack1(PACKED_TYPE_TEMPLATE) expires_at = dumped.unpack1(PACKED_EXPIRES_AT_TEMPLATE) @@ -64,6 +69,7 @@ def load(dumped) private SIGNATURE = "\x00\x11".b.freeze + EMPTY_BINARY_STRING = "".b.freeze OBJECT_DUMP_TYPE = 0x01 @@ -105,7 +111,12 @@ def initialize(serializer, compressor, payload, **options) def value if !@resolved - @value = @serializer.load(@compressor ? @compressor.inflate(@value) : @value) + @value = begin + @serializer.load(@compressor ? @compressor.inflate(@value) : @value) + rescue => error + ActiveSupport.error_reporter.report(error, source: "active_support.cache") + raise DeserializationError, error.message + end @resolved = true end @value diff --git a/activesupport/lib/active_support/cache/serializer_with_fallback.rb b/activesupport/lib/active_support/cache/serializer_with_fallback.rb index 99358490a8452..cc589851fcfac 100644 --- a/activesupport/lib/active_support/cache/serializer_with_fallback.rb +++ b/activesupport/lib/active_support/cache/serializer_with_fallback.rb @@ -87,7 +87,13 @@ def dump_compressed(entry, threshold) def _load(marked) dumped = marked.byteslice(1..-1) - dumped = Zlib::Inflate.inflate(dumped) if marked.start_with?(MARK_COMPRESSED) + if marked.start_with?(MARK_COMPRESSED) + dumped = begin + Zlib::Inflate.inflate(dumped) + rescue Zlib::Error => error + raise Cache::DeserializationError, "#{error.class}: #{error.message}" + end + end Cache::Entry.unpack(marshal_load(dumped)) end diff --git a/activesupport/test/cache/cache_coder_test.rb b/activesupport/test/cache/cache_coder_test.rb index 1e37f5961a102..90d439648a56a 100644 --- a/activesupport/test/cache/cache_coder_test.rb +++ b/activesupport/test/cache/cache_coder_test.rb @@ -20,6 +20,33 @@ class CacheCoderTest < ActiveSupport::TestCase end end + test "handle corrupted payloads gracefully" do + coder = ActiveSupport::Cache::Coder.new(Marshal, Zlib) + entry = ActiveSupport::Cache::Entry.new(["test"], version: "12", expires_in: 34) + + payload = coder.dump_compressed(entry, 1) + + assert_error_reported do + assert_nil coder.load(payload.byteslice(1..-1)) + end + + lazy_entry = coder.load(payload.byteslice(0..-2)) + assert_raises ActiveSupport::Cache::DeserializationError do + lazy_entry.value + end + + payload = coder.dump(entry) + + assert_error_reported do + assert_nil coder.load(payload.byteslice(1..-1)) + end + + lazy_entry = coder.load(payload.byteslice(0..-2)) + assert_raises ActiveSupport::Cache::DeserializationError do + lazy_entry.value + end + end + test "compresses values that are larger than the threshold" do COMPRESSIBLE_ENTRIES.each do |entry| dumped = @coder.dump(entry) @@ -117,7 +144,7 @@ def dump_compressed(*) end def load(dumped) - Marshal.load(dumped.delete_prefix!("SERIALIZED:")) + Marshal.load(dumped.delete_prefix("SERIALIZED:")) end end @@ -129,7 +156,7 @@ def deflate(string) end def inflate(deflated) - Zlib.inflate(deflated.delete_prefix!("COMPRESSED:")) + Zlib.inflate(deflated.delete_prefix("COMPRESSED:")) end end diff --git a/activesupport/test/cache/serializer_with_fallback_test.rb b/activesupport/test/cache/serializer_with_fallback_test.rb index 57568f9135c59..6893d65d23017 100644 --- a/activesupport/test/cache/serializer_with_fallback_test.rb +++ b/activesupport/test/cache/serializer_with_fallback_test.rb @@ -47,6 +47,12 @@ class CacheSerializerWithFallbackTest < ActiveSupport::TestCase assert_operator compressed.bytesize, :<, uncompressed.bytesize assert_entry @entry, serializer(format).load(compressed) assert_entry @entry, serializer(format).load(uncompressed) + + unless format == :passthrough + assert_raises ActiveSupport::Cache::DeserializationError do + serializer(format).load(compressed.byteslice(0..-4)) + end + end end end diff --git a/tools/strict_warnings.rb b/tools/strict_warnings.rb index 4eae85903f9a7..b20daea67f625 100644 --- a/tools/strict_warnings.rb +++ b/tools/strict_warnings.rb @@ -20,7 +20,10 @@ class WarningError < StandardError; end # TODO: remove if https://github.com/mikel/mail/pull/1557 or similar fix %r{/lib/mail/parsers/.*statement not reached}, %r{/lib/mail/parsers/.*assigned but unused variable - disp_type_s}, - %r{/lib/mail/parsers/.*assigned but unused variable - testEof} + %r{/lib/mail/parsers/.*assigned but unused variable - testEof}, + + # Emitted by zlib + /attempt to close unfinished zstream/, ) def warn(message, ...)