From 4b35da564c56d7302ca0d7a9bd0ca8c882c2b408 Mon Sep 17 00:00:00 2001 From: Dmytro Shteflyuk Date: Tue, 19 May 2026 21:15:57 -0400 Subject: [PATCH] THRIFT-5310: Harden Ruby binary protocol integer bounds --- lib/rb/README.md | 8 ++ lib/rb/ext/binary_protocol_accelerated.c | 85 ++++++++++++++----- lib/rb/lib/thrift/protocol/binary_protocol.rb | 43 +++++++--- lib/rb/spec/binary_protocol_spec_shared.rb | 55 ++++++++---- 4 files changed, 146 insertions(+), 45 deletions(-) diff --git a/lib/rb/README.md b/lib/rb/README.md index f241a4e7e94..0675c695580 100644 --- a/lib/rb/README.md +++ b/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+`. diff --git a/lib/rb/ext/binary_protocol_accelerated.c b/lib/rb/ext/binary_protocol_accelerated.c index 791dfa70fed..d5d31ed31cf 100644 --- a/lib/rb/ext/binary_protocol_accelerated.c +++ b/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,13 +170,14 @@ 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; @@ -138,8 +185,8 @@ VALUE rb_thrift_binary_proto_write_message_begin(VALUE self, VALUE name, VALUE t 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; } diff --git a/lib/rb/lib/thrift/protocol/binary_protocol.rb b/lib/rb/lib/thrift/protocol/binary_protocol.rb index 6957aa8c2f0..340d6d553a2 100644 --- a/lib/rb/lib/thrift/protocol/binary_protocol.rb +++ b/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 diff --git a/lib/rb/spec/binary_protocol_spec_shared.rb b/lib/rb/spec/binary_protocol_spec_shared.rb index 852cb095ec6..a878fd6d78f 100644 --- a/lib/rb/spec/binary_protocol_spec_shared.rb +++ b/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]