THRIFT-6145: Validate Ruby Compact decoder bounds

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,16 +505,23 @@ 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) {
       return result;
     }
     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));
 }
 

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()

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