iemejia commented on code in PR #3851:
URL: https://github.com/apache/avro/pull/3851#discussion_r3564663334
##########
lang/ruby/lib/avro/data_file.rb:
##########
@@ -319,10 +340,22 @@ def decompress(compressed)
# (without the RFC1950 header & checksum). See the docs for
# inflateInit2 in https://www.zlib.net/manual.html
zstream = Zlib::Inflate.new(-Zlib::MAX_WBITS)
- data = zstream.inflate(compressed)
- data << zstream.finish
+ limit = DataFile.max_decompress_length
+ data = "".b
+ # Inflate in chunks so an over-large (or malicious) block is rejected
+ # before its full decompressed form is materialized in memory.
+ append = lambda do |chunk|
+ data << chunk
+ if data.bytesize > limit
+ raise DecompressionSizeError, "Decompressed block size exceeds the
maximum allowed of #{limit} bytes"
+ end
+ end
Review Comment:
Fixed. The deflate path now checks the running length against the limit
before appending each inflated chunk to `data`, so an over-limit block is
rejected without first materializing it. Pushed in 6830ef5.
##########
lang/ruby/lib/avro/data_file.rb:
##########
@@ -374,11 +429,29 @@ def load_snappy!
end
class ZstandardCodec
+ # Size of the compressed input fed to the streaming decompressor per
step,
+ # so the decompressed output can be bounded incrementally.
+ ZSTD_DECOMPRESS_CHUNK_SIZE = 16 * 1024
+
def codec_name; 'zstandard'; end
def decompress(data)
load_zstandard!
- Zstd.decompress(data)
+ limit = DataFile.max_decompress_length
+ stream = Zstd::StreamingDecompress.new
+ uncompressed = +''.b
+ offset = 0
+ size = data.bytesize
+ # Decompress the block in chunks so an over-large (or malicious) block
is
+ # rejected before its full decompressed form is materialized in memory.
+ while offset < size
+ uncompressed << stream.decompress(data.byteslice(offset,
ZSTD_DECOMPRESS_CHUNK_SIZE))
+ offset += ZSTD_DECOMPRESS_CHUNK_SIZE
+ if uncompressed.bytesize > limit
+ raise DecompressionSizeError, "Decompressed block size exceeds the
maximum allowed of #{limit} bytes"
+ end
Review Comment:
Same fix applied to the zstd streaming path: the size check now runs before
appending each decompressed chunk to the accumulator. Pushed in 6830ef5.
##########
lang/ruby/test/test_codec_limits.rb:
##########
@@ -0,0 +1,90 @@
+# frozen_string_literal: true
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# https://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+require 'test_help'
+
+# A block with a very high compression ratio can expand to far more memory than
+# its compressed size. These tests ensure that decompressing such a block
raises
+# an error instead of allocating without bound.
+class TestCodecLimits < Test::Unit::TestCase
+ LIMIT = 1024
+
+ def with_limit(bytes)
+ previous = ENV['AVRO_MAX_DECOMPRESS_LENGTH']
+ ENV['AVRO_MAX_DECOMPRESS_LENGTH'] = bytes.to_s
+ yield
+ ensure
+ if previous.nil?
+ ENV.delete('AVRO_MAX_DECOMPRESS_LENGTH')
+ else
+ ENV['AVRO_MAX_DECOMPRESS_LENGTH'] = previous
+ end
+ end
+
+ def oversized_payload
+ ("\x00".b * (LIMIT * 8))
+ end
+
+ def test_deflate_rejects_oversized_block
+ codec = Avro::DataFile::DeflateCodec.new
+ compressed = codec.compress(oversized_payload)
+ with_limit(LIMIT) do
+ assert_raise(Avro::DataFile::DecompressionSizeError) do
+ codec.decompress(compressed)
+ end
+ end
+ end
+
+ def test_deflate_allows_block_within_limit
+ codec = Avro::DataFile::DeflateCodec.new
+ payload = ('the quick brown fox ' * 10).b
+ compressed = codec.compress(payload)
+ with_limit(LIMIT) do
+ assert_equal payload, codec.decompress(compressed)
+ end
+ end
+
+ def test_snappy_rejects_declared_oversized_length
+ begin
+ require 'snappy'
+ rescue LoadError
+ omit('snappy gem not available')
+ end
+ codec = Avro::DataFile::SnappyCodec.new
+ compressed = codec.compress(oversized_payload)
+ with_limit(LIMIT) do
+ assert_raise(Avro::DataFile::DecompressionSizeError) do
+ codec.decompress(compressed)
+ end
+ end
+ end
+
+ def test_zstandard_rejects_oversized_block
+ begin
+ require 'zstd-ruby'
+ rescue LoadError
+ omit('zstd-ruby gem not available')
+ end
+ codec = Avro::DataFile::ZstandardCodec.new
+ compressed = codec.compress(oversized_payload)
+ with_limit(LIMIT) do
+ assert_raise(Avro::DataFile::DecompressionSizeError) do
+ codec.decompress(compressed)
+ end
+ end
+ end
Review Comment:
Added a test that decodes a zstd block within the limit and asserts the
payload round-trips, so the streaming path is exercised for the success case
too. Pushed in 6830ef5.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
