THRIFT-5310: Harden Ruby binary protocol integer bounds

lib/rb/README.md

@@ -155,6 +155,14 @@ the generated `FIELDS` metadata hash for field names such as `fields`, but it
 is a source-compatible break if your application referenced the old constants
 directly. Regenerate Ruby code and update those constant references atomically.
 
+Ruby binary protocol writers now enforce signed Thrift integer ranges before
+serializing values. `byte`, `i16`, `i32`, and `i64` writes reject values outside
+their declared signed ranges, and binary/string/container sizes must fit in a
+non-negative signed `i32` length. Older releases could silently wrap or clip
+some out-of-range values, such as writing `255` as a byte that read back as
+`-1`. This applies to both `Thrift::BinaryProtocol` and
+`Thrift::BinaryProtocolAccelerated`.
+
 ### 0.23.0
 
 The documented source-build flow now effectively requires Ruby `2.7+`.

lib/rb/ext/binary_protocol_accelerated.c

@@ -31,13 +31,59 @@ VALUE rb_thrift_binary_proto_native_qmark(VALUE self) {
   return Qtrue;
 }
 
-
-
 static int VERSION_1;
 static int VERSION_MASK;
 static int TYPE_MASK;
 static ID rbuf_ivar_id;
 
+static int64_t checked_integer_value(VALUE value) {
+  if (!RB_INTEGER_TYPE_P(value)) {
+    rb_raise(rb_eTypeError, "integer argument expected");
+  }
+
+  return NUM2LL(value);
+}
+
+static int64_t checked_integer_range(VALUE value, int64_t min, int64_t max) {
+  int64_t integer = checked_integer_value(value);
+
+  if (integer < min || integer > max) {
+    rb_raise(rb_eRangeError, "integer out of bounds");
+  }
+
+  return integer;
+}
+
+static int8_t checked_byte_value(VALUE value) {
+  return (int8_t)checked_integer_range(value, INT8_MIN, INT8_MAX);
+}
+
+static int16_t checked_i16_value(VALUE value) {
+  return (int16_t)checked_integer_range(value, INT16_MIN, INT16_MAX);
+}
+
+static int32_t checked_i32_value(VALUE value) {
+  return (int32_t)checked_integer_range(value, INT32_MIN, INT32_MAX);
+}
+
+static int32_t checked_size_value(VALUE value) {
+  return (int32_t)checked_integer_range(value, 0, INT32_MAX);
+}
+
+static int64_t checked_i64_value(VALUE value) {
+  return checked_integer_range(value, INT64_MIN, INT64_MAX);
+}
+
+static int32_t checked_string_length(VALUE str) {
+  long length = RSTRING_LEN(str);
+
+  if (length > INT32_MAX) {
+    rb_raise(rb_eRangeError, "string too long");
+  }
+
+  return (int32_t)length;
+}
+
 static void write_byte_direct(VALUE trans, int8_t b) {
   WRITE(trans, (char*)&b, 1);
 }
@@ -83,7 +129,7 @@ static void write_string_direct(VALUE trans, VALUE str) {
     rb_raise(rb_eStandardError, "Value should be a string");
   }
   str = convert_to_utf8_byte_buffer(str);
-  write_i32_direct(trans, (int32_t)RSTRING_LEN(str));
+  write_i32_direct(trans, checked_string_length(str));
   rb_funcall(trans, write_method_id, 1, str);
 }
 
@@ -124,22 +170,23 @@ VALUE rb_thrift_binary_proto_write_message_begin(VALUE self, VALUE name, VALUE t
   VALUE strict_write = GET_STRICT_WRITE(self);
 
   if (strict_write == Qtrue) {
-    write_i32_direct(trans, VERSION_1 | FIX2INT(type));
+    int8_t message_type = checked_byte_value(type);
+    write_i32_direct(trans, VERSION_1 | message_type);
     write_string_direct(trans, name);
-    write_i32_direct(trans, FIX2INT(seqid));
+    write_i32_direct(trans, checked_i32_value(seqid));
   } else {
     write_string_direct(trans, name);
-    write_byte_direct(trans, FIX2INT(type));
-    write_i32_direct(trans, FIX2INT(seqid));
+    write_byte_direct(trans, checked_byte_value(type));
+    write_i32_direct(trans, checked_i32_value(seqid));
   }
 
   return Qnil;
 }
 
 VALUE rb_thrift_binary_proto_write_field_begin(VALUE self, VALUE name, VALUE type, VALUE id) {
   VALUE trans = GET_TRANSPORT(self);
-  write_byte_direct(trans, FIX2INT(type));
-  write_i16_direct(trans, FIX2INT(id));
+  write_byte_direct(trans, checked_byte_value(type));
+  write_i16_direct(trans, checked_i16_value(id));
 
   return Qnil;
 }
@@ -151,17 +198,17 @@ VALUE rb_thrift_binary_proto_write_field_stop(VALUE self) {
 
 VALUE rb_thrift_binary_proto_write_map_begin(VALUE self, VALUE ktype, VALUE vtype, VALUE size) {
   VALUE trans = GET_TRANSPORT(self);
-  write_byte_direct(trans, FIX2INT(ktype));
-  write_byte_direct(trans, FIX2INT(vtype));
-  write_i32_direct(trans, FIX2INT(size));
+  write_byte_direct(trans, checked_byte_value(ktype));
+  write_byte_direct(trans, checked_byte_value(vtype));
+  write_i32_direct(trans, checked_size_value(size));
 
   return Qnil;
 }
 
 VALUE rb_thrift_binary_proto_write_list_begin(VALUE self, VALUE etype, VALUE size) {
   VALUE trans = GET_TRANSPORT(self);
-  write_byte_direct(trans, FIX2INT(etype));
-  write_i32_direct(trans, FIX2INT(size));
+  write_byte_direct(trans, checked_byte_value(etype));
+  write_i32_direct(trans, checked_size_value(size));
 
   return Qnil;
 }
@@ -178,25 +225,25 @@ VALUE rb_thrift_binary_proto_write_bool(VALUE self, VALUE b) {
 
 VALUE rb_thrift_binary_proto_write_byte(VALUE self, VALUE byte) {
   CHECK_NIL(byte);
-  write_byte_direct(GET_TRANSPORT(self), NUM2INT(byte));
+  write_byte_direct(GET_TRANSPORT(self), checked_byte_value(byte));
   return Qnil;
 }
 
 VALUE rb_thrift_binary_proto_write_i16(VALUE self, VALUE i16) {
   CHECK_NIL(i16);
-  write_i16_direct(GET_TRANSPORT(self), FIX2INT(i16));
+  write_i16_direct(GET_TRANSPORT(self), checked_i16_value(i16));
   return Qnil;
 }
 
 VALUE rb_thrift_binary_proto_write_i32(VALUE self, VALUE i32) {
   CHECK_NIL(i32);
-  write_i32_direct(GET_TRANSPORT(self), NUM2INT(i32));
+  write_i32_direct(GET_TRANSPORT(self), checked_i32_value(i32));
   return Qnil;
 }
 
 VALUE rb_thrift_binary_proto_write_i64(VALUE self, VALUE i64) {
   CHECK_NIL(i64);
-  write_i64_direct(GET_TRANSPORT(self), NUM2LL(i64));
+  write_i64_direct(GET_TRANSPORT(self), checked_i64_value(i64));
   return Qnil;
 }
 
@@ -224,7 +271,7 @@ VALUE rb_thrift_binary_proto_write_binary(VALUE self, VALUE buf) {
   CHECK_NIL(buf);
   VALUE trans = GET_TRANSPORT(self);
   buf = force_binary_encoding(buf);
-  write_i32_direct(trans, (int32_t)RSTRING_LEN(buf));
+  write_i32_direct(trans, checked_string_length(buf));
   rb_funcall(trans, write_method_id, 1, buf);
   return Qnil;
 }

lib/rb/lib/thrift/protocol/binary_protocol.rb

@@ -23,6 +23,14 @@ class BinaryProtocol < BaseProtocol
     VERSION_MASK = 0xffff0000
     VERSION_1 = 0x80010000
     TYPE_MASK = 0x000000ff
+    BYTE_MIN = -2**7
+    BYTE_MAX = 2**7 - 1
+    I16_MIN = -2**15
+    I16_MAX = 2**15 - 1
+    I32_MIN = -2**31
+    I32_MAX = 2**31 - 1
+    I64_MIN = -2**63
+    I64_MAX = 2**63 - 1
 
     attr_reader :strict_read, :strict_write
 
@@ -37,11 +45,10 @@ def initialize(trans, strict_read = true, strict_write = true)
     end
 
     def write_message_begin(name, type, seqid)
-      # this is necessary because we added (needed) bounds checking to
-      # write_i32, and 0x80010000 is too big for that.
       if strict_write
-        write_i16(VERSION_1 >> 16)
-        write_i16(type)
+        raise ::TypeError, 'integer argument expected' unless type.is_a?(Integer)
+        raise RangeError if type < BYTE_MIN || type > BYTE_MAX
+        trans.write([VERSION_1 | type].pack('N'))
         write_string(name)
         write_i32(seqid)
       else
@@ -65,17 +72,17 @@ def write_field_stop
     def write_map_begin(ktype, vtype, size)
       write_byte(ktype)
       write_byte(vtype)
-      write_i32(size)
+      write_i32_size(size)
     end
 
     def write_list_begin(etype, size)
       write_byte(etype)
-      write_i32(size)
+      write_i32_size(size)
     end
 
     def write_set_begin(etype, size)
       write_byte(etype)
-      write_i32(size)
+      write_i32_size(size)
     end
 
     def write_bool(bool)
@@ -84,24 +91,29 @@ def write_bool(bool)
 
     def write_byte(byte)
       raise 'nil argument not allowed!' if byte.nil?
-      raise RangeError if byte < -2**31 || byte >= 2**32
+      raise ::TypeError, 'integer argument expected' unless byte.is_a?(Integer)
+      raise RangeError if byte < BYTE_MIN || byte > BYTE_MAX
       trans.write([byte].pack('c'))
     end
 
     def write_i16(i16)
       raise 'nil argument not allowed!' if i16.nil?
+      raise ::TypeError, 'integer argument expected' unless i16.is_a?(Integer)
+      raise RangeError if i16 < I16_MIN || i16 > I16_MAX
       trans.write([i16].pack('n'))
     end
 
     def write_i32(i32)
       raise 'nil argument not allowed!' if i32.nil?
-      raise RangeError if i32 < -2**31 || i32 >= 2**31
+      raise ::TypeError, 'integer argument expected' unless i32.is_a?(Integer)
+      raise RangeError if i32 < I32_MIN || i32 > I32_MAX
       trans.write([i32].pack('N'))
     end
 
     def write_i64(i64)
       raise 'nil argument not allowed!' if i64.nil?
-      raise RangeError if i64 < -2**63 || i64 >= 2**64
+      raise ::TypeError, 'integer argument expected' unless i64.is_a?(Integer)
+      raise RangeError if i64 < I64_MIN || i64 > I64_MAX
       hi = i64 >> 32
       lo = i64 & 0xffffffff
       trans.write([hi, lo].pack('N2'))
@@ -120,7 +132,7 @@ def write_string(str)
 
     def write_binary(buf)
       raise 'nil argument not allowed!' if buf.nil?
-      write_i32(buf.bytesize)
+      write_i32_size(buf.bytesize)
       trans.write(buf)
     end
 
@@ -247,6 +259,15 @@ def read_uuid
     def to_s
       "binary(#{super.to_s})"
     end
+
+    private
+
+    def write_i32_size(size)
+      raise 'nil argument not allowed!' if size.nil?
+      raise ::TypeError, 'integer argument expected' unless size.is_a?(Integer)
+      raise RangeError if size < 0 || size > I32_MAX
+      trans.write([size].pack('N'))
+    end
   end
 
   class BinaryProtocolFactory < BaseProtocolFactory

lib/rb/spec/binary_protocol_spec_shared.rb

@@ -110,15 +110,15 @@
     end
   end
 
-  it "should clip numbers out of signed range" do
-    (128..255).each do |i|
-      @prot.write_byte(i)
-      expect(@trans.read(1)).to eq([i].pack('c'))
+  it "should reject bytes outside the signed byte range" do
+    [-129, 128, 255, 2**65].each do |i|
+      expect { @prot.write_byte(i) }.to raise_error(RangeError)
     end
+    expect(@trans.available).to eq(0)
   end
 
-  it "errors out with a Bignum" do
-    expect { @prot.write_byte(2**65) }.to raise_error(RangeError)
+  it "should reject non-integer bytes" do
+    expect { @prot.write_byte(1.5) }.to raise_error(TypeError)
   end
 
   it "should error gracefully when trying to write a nil byte" do
@@ -131,13 +131,19 @@
     [-2**15, -1024, 17, 0, -10000, 1723, 2**15-1].each do |i|
       @prot.write_i16(i)
     end
-    # and try something out of signed range, it should clip
-    @prot.write_i16(2**15 + 5)
 
-    expect(@trans.read(@trans.available)).to eq("\200\000\374\000\000\021\000\000\330\360\006\273\177\377\200\005")
+    expect(@trans.read(@trans.available)).to eq("\200\000\374\000\000\021\000\000\330\360\006\273\177\377")
+  end
+
+  it "should reject i16 values outside the signed i16 range" do
+    [-2**15 - 1, 2**15, 2**65].each do |i|
+      expect { @prot.write_i16(i) }.to raise_error(RangeError)
+    end
+    expect(@trans.available).to eq(0)
+  end
 
-    # a Bignum should error
-    # lambda { @prot.write_i16(2**65) }.should raise_error(RangeError)
+  it "should reject non-integer i16 values" do
+    expect { @prot.write_i16(1.5) }.to raise_error(TypeError)
   end
 
   it "should error gracefully when trying to write a nil i16" do
@@ -150,13 +156,16 @@
     [-2**31, -123123, -2532, -3, 0, 2351235, 12331, 2**31-1].each do |i|
       @prot.write_i32(i)
     end
-    # try something out of signed range, it should clip
     expect(@trans.read(@trans.available)).to eq("\200\000\000\000" + "\377\376\037\r" + "\377\377\366\034" + "\377\377\377\375" + "\000\000\000\000" + "\000#\340\203" + "\000\0000+" + "\177\377\377\377")
-    [2 ** 31 + 5, 2 ** 65 + 5].each do |i|
+    [-2 ** 31 - 1, 2 ** 31, 2 ** 65 + 5].each do |i|
       expect { @prot.write_i32(i) }.to raise_error(RangeError)
     end
   end
 
+  it "should reject non-integer i32 values" do
+    expect { @prot.write_i32(1.5) }.to raise_error(TypeError)
+  end
+
   it "should error gracefully when trying to write a nil i32" do
     expect { @prot.write_i32(nil) }.to raise_error(StandardError, 'nil argument not allowed!')
   end
@@ -167,7 +176,6 @@
     [-2**63, -12356123612323, -23512351, -234, 0, 1231, 2351236, 12361236213, 2**63-1].each do |i|
       @prot.write_i64(i)
     end
-    # try something out of signed range, it should clip
     expect(@trans.read(@trans.available)).to eq(["\200\000\000\000\000\000\000\000",
       "\377\377\364\303\035\244+]",
       "\377\377\377\377\376\231:\341",
@@ -177,13 +185,30 @@
       "\000\000\000\000\000#\340\204",
       "\000\000\000\002\340\311~\365",
       "\177\377\377\377\377\377\377\377"].join(""))
-    expect { @prot.write_i64(2 ** 65 + 5) }.to raise_error(RangeError)
+    [-2 ** 63 - 1, 2 ** 63, 2 ** 65 + 5].each do |i|
+      expect { @prot.write_i64(i) }.to raise_error(RangeError)
+    end
+  end
+
+  it "should reject non-integer i64 values" do
+    expect { @prot.write_i64(1.5) }.to raise_error(TypeError)
   end
 
   it "should error gracefully when trying to write a nil i64" do
     expect { @prot.write_i64(nil) }.to raise_error(StandardError, 'nil argument not allowed!')
   end
 
+  it "should reject out-of-range field ids" do
+    expect { @prot.write_field_begin('foo', Thrift::Types::DOUBLE, -2**15 - 1) }.to raise_error(RangeError)
+    expect { @prot.write_field_begin('foo', Thrift::Types::DOUBLE, 2**15) }.to raise_error(RangeError)
+  end
+
+  it "should reject out-of-range container sizes" do
+    expect { @prot.write_map_begin(Thrift::Types::STRING, Thrift::Types::LIST, -1) }.to raise_error(RangeError)
+    expect { @prot.write_list_begin(Thrift::Types::I16, 2**31) }.to raise_error(RangeError)
+    expect { @prot.write_set_begin(Thrift::Types::I16, 2**31) }.to raise_error(RangeError)
+  end
+
   it "should write a double" do
     # try a random scattering of values, including min/max
     values = [Float::MIN, -1231.15325, -123123.23, -23.23515123, 0, 12351.1325, 523.23, Float::MAX]