diff --git a/lib/rb/ext/compact_protocol.c b/lib/rb/ext/compact_protocol.c index d08957e47d..257ac1e466 100644 --- a/lib/rb/ext/compact_protocol.c +++ b/lib/rb/ext/compact_protocol.c @@ -484,6 +484,7 @@ static int32_t zig_zag_to_int(uint32_t n) { } #define MAX_VARINT32_BYTES 5 /* ceil(32/7); matches protobuf wire format */ +#define MAX_VARINT32_LAST_BYTE 0x0f #define MAX_VARINT64_BYTES 10 /* ceil(64/7); matches protobuf wire format */ static uint64_t read_varint64(VALUE self) { @@ -504,7 +505,7 @@ static uint64_t read_varint64(VALUE self) { static uint32_t read_varint32(VALUE self) { int i, shift = 0; uint32_t result = 0; - for (i = 0; i < MAX_VARINT32_BYTES; i++) { + for (i = 0; i < MAX_VARINT32_BYTES - 1; i++) { int8_t b = read_byte_direct(self); result |= ((uint32_t)(b & 0x7f) << shift); if ((b & 0x80) != 0x80) { @@ -512,8 +513,15 @@ static uint32_t read_varint32(VALUE self) { } shift += 7; } - rb_exc_raise(get_protocol_exception(INT2FIX(PROTOERR_INVALID_DATA), rb_str_new2("Variable-length int over 5 bytes."))); - return 0; /* unreachable */ + + int8_t b = read_byte_direct(self); + if (RB_UNLIKELY((b & 0x80) != 0)) { + rb_exc_raise(get_protocol_exception(INT2FIX(PROTOERR_INVALID_DATA), rb_str_new2("Variable-length int over 5 bytes."))); + } + if (RB_UNLIKELY((b & ~MAX_VARINT32_LAST_BYTE) != 0)) { + rb_exc_raise(get_protocol_exception(INT2FIX(PROTOERR_INVALID_DATA), rb_str_new2("Variable-length int overflows uint32."))); + } + return result | ((uint32_t)b << shift); } static int16_t read_i16(VALUE self) { @@ -683,6 +691,9 @@ VALUE rb_thrift_compact_proto_read_string(VALUE self) { VALUE rb_thrift_compact_proto_read_binary(VALUE self) { uint32_t size = read_varint32(self); + if (RB_UNLIKELY(size > INT32_MAX)) { + rb_exc_raise(get_protocol_exception(INT2FIX(PROTOERR_SIZE_LIMIT), rb_str_new2("Binary size limit exceeded"))); + } return rb_funcall(GET_TRANSPORT(self), read_all_method_id, 1, UINT2NUM(size)); } diff --git a/lib/rb/lib/thrift/protocol/compact_protocol.rb b/lib/rb/lib/thrift/protocol/compact_protocol.rb index 8c16340e52..2c677a4bf9 100644 --- a/lib/rb/lib/thrift/protocol/compact_protocol.rb +++ b/lib/rb/lib/thrift/protocol/compact_protocol.rb @@ -28,6 +28,8 @@ class CompactProtocol < BaseProtocol TYPE_BITS = 0x07 TYPE_SHIFT_AMOUNT = 5 MAX_VARINT32_BYTES = 5 # ceil(32/7); matches protobuf wire format + VARINT32_PREFIX_BYTES = MAX_VARINT32_BYTES - 1 + MAX_VARINT32_LAST_BYTE = 0x0f MAX_VARINT_BYTES = 10 # ceil(64/7); matches protobuf wire format BYTE_MIN = -(2**7) BYTE_MAX = (2**7) - 1 @@ -420,6 +422,9 @@ def read_string def read_binary size = read_varint32() + if size > I32_MAX + raise ProtocolException.new(ProtocolException::SIZE_LIMIT, 'Binary size limit exceeded') + end trans.read_all(size) end @@ -488,13 +493,22 @@ def write_varint64(n) def read_varint32() shift = 0 result = 0 - MAX_VARINT32_BYTES.times do + VARINT32_PREFIX_BYTES.times do b = read_byte() result |= (b & 0x7f) << shift return result if (b & 0x80) != 0x80 shift += 7 end - raise ProtocolException.new(ProtocolException::INVALID_DATA, 'Variable-length int over 5 bytes.') + + b = read_byte() + if (b & 0x80) != 0 + raise ProtocolException.new(ProtocolException::INVALID_DATA, 'Variable-length int over 5 bytes.') + end + if (b & ~MAX_VARINT32_LAST_BYTE) != 0 + raise ProtocolException.new(ProtocolException::INVALID_DATA, 'Variable-length int overflows uint32.') + end + + result | (b << shift) end def read_varint64() diff --git a/lib/rb/spec/compact_protocol_spec.rb b/lib/rb/spec/compact_protocol_spec.rb index d8e7574909..c6815078ca 100644 --- a/lib/rb/spec/compact_protocol_spec.rb +++ b/lib/rb/spec/compact_protocol_spec.rb @@ -34,7 +34,7 @@ :i64 => [0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x01] } - CONTAINER_SIZE_ENCODINGS = { + VARINT32_SIZE_ENCODINGS = { (2**31) - 1 => [0xff, 0xff, 0xff, 0xff, 0x07], 2**31 => [0x80, 0x80, 0x80, 0x80, 0x08], (2**32) - 1 => [0xff, 0xff, 0xff, 0xff, 0x0f] @@ -282,7 +282,7 @@ end it "should accept container sizes within the signed 32-bit range" do - bytes = [0xf5, *CONTAINER_SIZE_ENCODINGS.fetch((2**31) - 1)] + bytes = [0xf5, *VARINT32_SIZE_ENCODINGS.fetch((2**31) - 1)] trans = Thrift::MemoryBufferTransport.new(bytes.pack("C*")) proto = Thrift::CompactProtocol.new(trans) @@ -297,9 +297,9 @@ }.each do |reader_method, container_header| [2**31, (2**32) - 1].each do |size| bytes = if reader_method == :read_map_begin - [*CONTAINER_SIZE_ENCODINGS.fetch(size), *container_header] + [*VARINT32_SIZE_ENCODINGS.fetch(size), *container_header] else - [*container_header, *CONTAINER_SIZE_ENCODINGS.fetch(size)] + [*container_header, *VARINT32_SIZE_ENCODINGS.fetch(size)] end trans = Thrift::MemoryBufferTransport.new(bytes.pack("C*")) proto = Thrift::CompactProtocol.new(trans) @@ -364,6 +364,26 @@ expect(proto.read_binary).to eq(payload) end + it "should accept binary sizes through the signed 32-bit range" do + trans = Thrift::MemoryBufferTransport.new(VARINT32_SIZE_ENCODINGS.fetch((2**31) - 1).pack("C*")) + proto = Thrift::CompactProtocol.new(trans) + expect(trans).to receive(:read_all).with((2**31) - 1).and_return("payload") + + expect(proto.read_binary).to eq("payload") + end + + it "should reject binary sizes above the signed 32-bit range before reading the payload" do + [2**31, (2**32) - 1].each do |size| + trans = Thrift::MemoryBufferTransport.new(VARINT32_SIZE_ENCODINGS.fetch(size).pack("C*")) + proto = Thrift::CompactProtocol.new(trans) + expect(trans).not_to receive(:read_all) + + expect { proto.read_binary }.to raise_error(Thrift::ProtocolException, "Binary size limit exceeded") do |error| + expect(error.type).to eq(Thrift::ProtocolException::SIZE_LIMIT) + end + end + end + it "should write a uuid" do trans = Thrift::MemoryBufferTransport.new proto = Thrift::CompactProtocol.new(trans) @@ -512,6 +532,17 @@ expect { proto.read_i32 }.not_to raise_error end + it "should reject 32-bit varints with overflowing fifth-byte payload bits" do + [0x10, 0x7f].each do |terminal_byte| + trans = Thrift::MemoryBufferTransport.new((([0x80] * 4) + [terminal_byte]).pack("C*")) + proto = Thrift::CompactProtocol.new(trans) + + expect { proto.read_i32 }.to raise_error(Thrift::ProtocolException, "Variable-length int overflows uint32.") do |error| + expect(error.type).to eq(Thrift::ProtocolException::INVALID_DATA) + end + end + end + class JankyHandler def Janky(i32arg) i32arg * 2