Skip to content

Commit 39d2f72

Browse files
petergoldsteinclaude
petergoldstein
and
claude
committed
Bound decompressed and reply value sizes (GHSA-3553-vcg5-72jw)
- Values flagged as compressed were inflated with no size limit, so a ~130 KB stored item could expand to 128 MB on read (about 1000:1), whatever the client's compress or serializer settings. A new decompressed_max_bytes option (default 128 MiB, nil for no limit) caps it: the built-in Deflate and Gzip compressors inflate in chunks and raise UnmarshalError as soon as the output passes the limit. Custom compressors whose decompress takes only the data keep working, without the limit. - The size in a VA reply was passed straight to IO#read, which allocates that many bytes up front, so a hostile server could make the client allocate gigabytes. Sizes over 1 GiB (memcached's largest item) or negative now raise DalliError before any read, on the single-key paths and in the pipelined parser. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
1 parent f8f7a21 commit 39d2f72

9 files changed

Lines changed: 174 additions & 8 deletions

‎lib/dalli/compressor.rb‎

Lines changed: 27 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,28 @@ def self.compress(data)
1313
Zlib::Deflate.deflate(data)
1414
end
1515

16-
def self.decompress(data)
17-
Zlib::Inflate.inflate(data)
16+
# max_bytes caps the decompressed size; inflating stops and raises as soon
17+
# as the output passes it, so a small compressed value can't expand into
18+
# gigabytes of memory.
19+
def self.decompress(data, max_bytes: nil)
20+
return Zlib::Inflate.inflate(data) unless max_bytes
21+
22+
inflate_within_limit(Zlib::Inflate.new, data, max_bytes)
23+
end
24+
25+
def self.inflate_within_limit(inflater, data, max_bytes)
26+
out = String.new(encoding: Encoding::BINARY)
27+
inflater.inflate(data) { |chunk| append_within_limit(out, chunk, max_bytes) }
28+
append_within_limit(out, inflater.finish, max_bytes)
29+
ensure
30+
inflater.close
31+
end
32+
33+
def self.append_within_limit(out, chunk, max_bytes)
34+
out << chunk if chunk
35+
return out if out.bytesize <= max_bytes
36+
37+
raise Dalli::UnmarshalError, "Decompressed value exceeds #{max_bytes} bytes (decompressed_max_bytes)"
1838
end
1939
end
2040

@@ -32,9 +52,11 @@ def self.compress(data)
3252
io.string
3353
end
3454

35-
def self.decompress(data)
36-
io = StringIO.new(data, 'rb')
37-
Zlib::GzipReader.new(io).read
55+
def self.decompress(data, max_bytes: nil)
56+
return Zlib::GzipReader.new(StringIO.new(data, 'rb')).read unless max_bytes
57+
58+
# MAX_WBITS + 16 makes zlib read the gzip format (and check its CRC)
59+
Compressor.inflate_within_limit(Zlib::Inflate.new(Zlib::MAX_WBITS + 16), data, max_bytes)
3860
end
3961
end
4062
end

‎lib/dalli/protocol/connection_manager.rb‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -187,14 +187,27 @@ def read_line
187187

188188
# JRuby doesn't support IO#timeout=, so use custom readfull implementation
189189
# CRuby 3.3+ has IO#timeout= which makes IO#read work with timeouts
190+
# memcached can't store an item larger than 1 GiB (its -I maximum), so a
191+
# reply claiming more (or a negative size) is malformed or hostile.
192+
# IO#read allocates the full count up front, so check before reading.
193+
MAX_READ_BYTES = (1024 * 1024 * 1024) + 2 # plus the value's trailing "\r\n"
194+
195+
def check_read_size!(count)
196+
return if count.between?(0, MAX_READ_BYTES)
197+
198+
raise Dalli::DalliError, "Reply size #{count} from #{name} is out of range"
199+
end
200+
190201
if RUBY_ENGINE == 'jruby'
191202
def read(count)
203+
check_read_size!(count)
192204
@sock.readfull(count)
193205
rescue SystemCallError, *TIMEOUT_ERRORS, *SSL_ERRORS, EOFError => e
194206
error_on_request!(e)
195207
end
196208
else
197209
def read(count)
210+
check_read_size!(count)
198211
read_bytes(count)
199212
rescue SystemCallError, *TIMEOUT_ERRORS, *SSL_ERRORS, EOFError => e
200213
error_on_request!(e)

‎lib/dalli/protocol/response_processor.rb‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,8 @@ class ResponseProcessor
2424
SERVER_ERROR = 'SERVER_ERROR'
2525

2626
VA_PREFIX = 'VA '
27+
# memcached can't store an item larger than 1 GiB (its -I maximum)
28+
MAX_VALUE_BYTES = 1024 * 1024 * 1024
2729
HD_PREFIX = 'HD '
2830
FLAGS_TOKEN_PREFIX = ' f'
2931
CAS_TOKEN_PREFIX = ' c'
@@ -265,6 +267,7 @@ def getk_response_from_buffer(buf, offset = 0)
265267
# still has a body -- just its terminator -- so it's parsed below.
266268
return [tokens.first == MN, header_len] if body_len.zero? && tokens.first != VA
267269

270+
check_value_size!(body_len)
268271
resp_size = header_len + body_len + TERMINATOR.length
269272
# The header is in the buffer, but the body is not. As we don't have
270273
# a complete response, don't advance the buffer
@@ -301,6 +304,7 @@ def va_response_from_buffer(buf, offset, term_idx)
301304
end
302305
return nil if size.nil? || size.zero?
303306

307+
check_value_size!(size)
304308
header_len = term_idx - offset + TERMINATOR.length
305309
resp_size = header_len + size + TERMINATOR.length
306310
return [0] unless buf.bytesize >= offset + resp_size
@@ -425,6 +429,14 @@ def value_from_tokens(tokens, flag)
425429
end
426430
end
427431

432+
# A pipelined reply claiming an impossible size would otherwise have the
433+
# buffer wait for (and accumulate) that many bytes
434+
def check_value_size!(size)
435+
return if size.between?(0, MAX_VALUE_BYTES)
436+
437+
raise Dalli::DalliError, "Reply value size #{size} is out of range"
438+
end
439+
428440
def read_line
429441
@io_source.read_line&.chomp!(TERMINATOR)
430442
end

‎lib/dalli/protocol/value_compressor.rb‎

Lines changed: 27 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,10 @@ class ValueCompressor
1515
compress: true,
1616
compressor: ::Dalli::Compressor,
1717
# min byte size to attempt compression
18-
compression_min_size: 4 * 1024 # 4K
18+
compression_min_size: 4 * 1024, # 4K
19+
# max size a stored value may decompress to (nil: no limit). Guards
20+
# against a small compressed value expanding into gigabytes on read.
21+
decompressed_max_bytes: 128 * 1024 * 1024 # 128 MiB
1922
}.freeze
2023

2124
OPTIONS = DEFAULTS.keys.freeze
@@ -34,8 +37,16 @@ def store(value, req_options, bitflags)
3437
end
3538

3639
def retrieve(value, bitflags)
37-
compressed = bitflags.anybits?(Flags::COMPRESSED)
38-
compressed ? compressor.decompress(value) : value
40+
return value unless bitflags.anybits?(Flags::COMPRESSED)
41+
42+
max_bytes = @compression_options[:decompressed_max_bytes]
43+
# Custom compressors that only define decompress(data) keep working,
44+
# without the limit
45+
if max_bytes && compressor_accepts_max_bytes?
46+
compressor.decompress(value, max_bytes: max_bytes)
47+
else
48+
compressor.decompress(value)
49+
end
3950

4051
# TODO: We likely want to move this rescue into the Dalli::Compressor / Dalli::GzipCompressor
4152
# itself, since not all compressors necessarily use Zlib. For now keep it here, so the behavior
@@ -44,6 +55,19 @@ def retrieve(value, bitflags)
4455
raise UnmarshalError, "Unable to uncompress value: #{$ERROR_INFO.message}"
4556
end
4657

58+
def compressor_accepts_max_bytes?
59+
return @compressor_accepts_max_bytes if defined?(@compressor_accepts_max_bytes)
60+
61+
@compressor_accepts_max_bytes =
62+
begin
63+
compressor.method(:decompress).parameters.any? do |type, name|
64+
%i[key keyreq].include?(type) && name == :max_bytes
65+
end
66+
rescue NameError # an object without a reflectable decompress method
67+
false
68+
end
69+
end
70+
4771
def compress_by_default?
4872
@compression_options[:compress]
4973
end

‎test/integration/test_compressor.rb‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,24 @@ def self.decompress(data)
2424
end
2525
end
2626

27+
# Anyone who can write to memcached can store a small value flagged as
28+
# compressed that expands enormously; reading it must stop at the limit
29+
it 'refuses to decompress a value past decompressed_max_bytes' do
30+
memcached(p, 29_199) do |_dc|
31+
bomb = Dalli::Compressor.compress("\0" * (16 * 1024 * 1024))
32+
sock = TCPSocket.new('127.0.0.1', 29_199)
33+
sock.write("ms bomb #{bomb.bytesize} F#{Dalli::Flags::COMPRESSED}\r\n#{bomb}\r\n")
34+
35+
assert_equal "HD\r\n", sock.gets
36+
sock.close
37+
38+
capped = Dalli::Client.new('127.0.0.1:29199', decompressed_max_bytes: 1024 * 1024)
39+
40+
assert_raises(Dalli::UnmarshalError) { capped.get('bomb') }
41+
assert_equal 16 * 1024 * 1024, Dalli::Client.new('127.0.0.1:29199').get('bomb').bytesize
42+
end
43+
end
44+
2745
it 'support a custom compressor' do
2846
memcached(p, 29_199) do |_dc|
2947
memcache = Dalli::Client.new('127.0.0.1:29199', { compressor: NoopCompressor })

‎test/protocol/test_connection_manager.rb‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,16 @@ def closed?
117117
end
118118
end
119119

120+
describe '#read size check' do
121+
# IO#read allocates the whole count up front, so an impossible size from
122+
# a hostile server is rejected before reading
123+
it 'rejects sizes over the largest item memcached can store, and negative sizes' do
124+
[(1024 * 1024 * 1024) + 3, 4 * 1024 * 1024 * 1024, -1].each do |count|
125+
assert_raises(Dalli::DalliError) { connection_manager.read(count) }
126+
end
127+
end
128+
end
129+
120130
describe 'failure counting' do
121131
let(:manager) { Dalli::Protocol::ConnectionManager.new('localhost', 11_211, :tcp, { socket_max_failures: 2 }) }
122132

‎test/protocol/test_response_processor.rb‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -511,6 +511,12 @@ def expect_read_data(data, size)
511511
assert_nil processor.key_from_va_line("VA 5 f0 s5\r\n")
512512
end
513513

514+
it 'rejects a pipelined reply that claims an impossible value size' do
515+
["VA 4294967296 f0 kfoo s4294967296\r\n", "VA 4294967296 s4294967296 f0 kfoo\r\n"].each do |line|
516+
assert_raises(Dalli::DalliError) { processor.getk_response_from_buffer(line.b) }
517+
end
518+
end
519+
514520
it 'skips a bodyless error reply instead of treating it as the end of the pipeline' do
515521
error = "CLIENT_ERROR bad command line format\r\n"
516522
buf = "#{error}VA 1 f0 kfoo s1\r\nx\r\nMN\r\n".b

‎test/protocol/test_value_compressor.rb‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -431,6 +431,38 @@
431431
end
432432
end
433433

434+
describe 'decompressed_max_bytes' do
435+
let(:data) { 'abc' * 100_000 }
436+
let(:compressed) { Dalli::Compressor.compress(data) }
437+
let(:bitflags) { Dalli::Flags::COMPRESSED }
438+
439+
it 'defaults to 128 MiB' do
440+
assert_equal 128 * 1024 * 1024, Dalli::Protocol::ValueCompressor::DEFAULTS[:decompressed_max_bytes]
441+
end
442+
443+
it 'rejects values that decompress past the configured limit' do
444+
vc = Dalli::Protocol::ValueCompressor.new(decompressed_max_bytes: 1024)
445+
446+
assert_raises(Dalli::UnmarshalError) { vc.retrieve(compressed, bitflags) }
447+
end
448+
449+
it 'can be disabled with nil' do
450+
vc = Dalli::Protocol::ValueCompressor.new(decompressed_max_bytes: nil)
451+
452+
assert_equal data.b, vc.retrieve(compressed, bitflags).b
453+
end
454+
455+
it 'still calls a custom compressor that only defines decompress(data)' do
456+
custom = Class.new do
457+
def self.compress(data) = Zlib::Deflate.deflate(data)
458+
def self.decompress(data) = Zlib::Inflate.inflate(data)
459+
end
460+
vc = Dalli::Protocol::ValueCompressor.new(compressor: custom, decompressed_max_bytes: 1024)
461+
462+
assert_equal data.b, vc.retrieve(compressed, bitflags).b
463+
end
464+
end
465+
434466
describe 'retrieve' do
435467
let(:raw_value) { SecureRandom.hex(8) }
436468
let(:decompressed_dummy) { SecureRandom.hex(8) }

‎test/test_compressor.rb‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,3 +18,32 @@
1818
))
1919
end
2020
end
21+
22+
# Decompressing with max_bytes stops as soon as the output passes the limit,
23+
# so a small compressed value can't expand into gigabytes of memory.
24+
[Dalli::Compressor, Dalli::GzipCompressor].each do |compressor|
25+
describe "#{compressor}.decompress with max_bytes" do
26+
let(:data) { 'abc' * 100_000 }
27+
let(:compressed) { compressor.compress(data) }
28+
29+
it 'returns values within the limit unchanged' do
30+
assert_equal data.b, compressor.decompress(compressed, max_bytes: data.bytesize).b
31+
end
32+
33+
it 'raises UnmarshalError for values that decompress past the limit' do
34+
error = assert_raises(Dalli::UnmarshalError) { compressor.decompress(compressed, max_bytes: 1024) }
35+
36+
assert_match(/exceeds 1024 bytes/, error.message)
37+
end
38+
39+
it 'stops a decompression bomb without inflating it fully' do
40+
bomb = compressor.compress("\0" * (64 * 1024 * 1024))
41+
42+
assert_raises(Dalli::UnmarshalError) { compressor.decompress(bomb, max_bytes: 1024 * 1024) }
43+
end
44+
45+
it 'has no limit without max_bytes' do
46+
assert_equal data.b, compressor.decompress(compressed).b
47+
end
48+
end
49+
end

0 commit comments

Comments
 (0)