From d58d7ca271bbf3b3761188e86650d4be0557708c Mon Sep 17 00:00:00 2001 From: Adam Cozzette Date: Thu, 16 May 2019 11:39:40 -0700 Subject: [PATCH] Fixed out-of-bounds read The scan_length_prefixed_data() function returns the number of bytes taken up by a varint length delimiter, plus the actual value of that delimiter. Since it returns a uint32_t, a delimiter of 2^32 - 1 (or close to that) could cause the return value to overflow and result in an incorrect value. At first I tried to fix it by making scan_length_prefixed_data() use a size_t for its result, but I realized this would have no effect on 32-bit systems. To fix the problem for 32-bit, I changed the function to return early if the length is 2 GiB or more (protobuf messages are not allowed to be that large). I kept the size_t change anyway, since the result will ultimately be stored in a size_t (ScannedMember.len) and we might as well stay consistent with that. Signed-off-by: Adam Cozzette --- Makefile.am | 17 +++++++++++++++++ protobuf-c/protobuf-c.c | 15 +++++++++++---- t/issue375/.gitignore | 1 + t/issue375/issue375.c | 24 ++++++++++++++++++++++++ t/issue375/issue375.proto | 7 +++++++ 5 files changed, 60 insertions(+), 4 deletions(-) create mode 100644 t/issue375/.gitignore create mode 100644 t/issue375/issue375.c create mode 100644 t/issue375/issue375.proto diff --git a/Makefile.am b/Makefile.am index 6c836a3..aee2d84 100644 --- a/Makefile.am +++ b/Makefile.am @@ -250,6 +250,23 @@ BUILT_SOURCES += \ EXTRA_DIST += \ t/issue251/issue251.proto +# Issue #375 +check_PROGRAMS += \ + t/issue375/issue375 +TESTS += \ + t/issue375/issue375 +t_issue375_issue375_SOURCES = \ + t/issue375/issue375.c \ + t/issue375/issue375.pb-c.c +t_issue375_issue375_LDADD = \ + protobuf-c/libprotobuf-c.la +t/issue375/issue375.pb-c.c t/issue375/issue375.pb-c.h: $(top_builddir)/protoc-c/protoc-gen-c$(EXEEXT) $(top_srcdir)/t/issue375/issue375.proto + $(AM_V_GEN)@PROTOC@ --plugin=protoc-gen-c=$(top_builddir)/protoc-c/protoc-gen-c$(EXEEXT) -I$(top_srcdir) --c_out=$(top_builddir) $(top_srcdir)/t/issue375/issue375.proto +BUILT_SOURCES += \ + t/issue375/issue375.pb-c.c t/issue375/issue375.pb-c.h +EXTRA_DIST += \ + t/issue375/issue375.proto + endif # CROSS_COMPILING endif # BUILD_COMPILER diff --git a/protobuf-c/protobuf-c.c b/protobuf-c/protobuf-c.c index 5d857e1..c057870 100644 --- a/protobuf-c/protobuf-c.c +++ b/protobuf-c/protobuf-c.c @@ -2103,18 +2103,18 @@ struct _ScannedMember { const uint8_t *data; /**< Pointer to field data. */ }; -static inline uint32_t +static inline size_t scan_length_prefixed_data(size_t len, const uint8_t *data, size_t *prefix_len_out) { unsigned hdr_max = len < 5 ? len : 5; unsigned hdr_len; - uint32_t val = 0; + size_t val = 0; unsigned i; unsigned shift = 0; for (i = 0; i < hdr_max; i++) { - val |= (data[i] & 0x7f) << shift; + val |= ((size_t)data[i] & 0x7f) << shift; shift += 7; if ((data[i] & 0x80) == 0) break; @@ -2125,8 +2125,15 @@ scan_length_prefixed_data(size_t len, const uint8_t *data, } hdr_len = i + 1; *prefix_len_out = hdr_len; + if (val > INT_MAX) { + // Protobuf messages should always be less than 2 GiB in size. + // We also want to return early here so that hdr_len + val does + // not overflow on 32-bit systems. + PROTOBUF_C_UNPACK_ERROR("length prefix of %lu is too large", val); + return 0; + } if (hdr_len + val > len) { - PROTOBUF_C_UNPACK_ERROR("data too short after length-prefix of %u", val); + PROTOBUF_C_UNPACK_ERROR("data too short after length-prefix of %lu", val); return 0; } return hdr_len + val; diff --git a/t/issue375/.gitignore b/t/issue375/.gitignore new file mode 100644 index 0000000..4657e6d --- /dev/null +++ b/t/issue375/.gitignore @@ -0,0 +1 @@ +issue375 diff --git a/t/issue375/issue375.c b/t/issue375/issue375.c new file mode 100644 index 0000000..c808043 --- /dev/null +++ b/t/issue375/issue375.c @@ -0,0 +1,24 @@ +#include +#include + +#include "t/issue375/issue375.pb-c.h" + +int main(void) { + // This buffer represents some serialized bytes where we have a length + // delimiter of 2^32 - 1 bytes for a particular repeated int32 field. + // We want to make sure that parsing a length delimiter this large does + // not cause a problematic integer overflow. + uint8_t buffer[] = { + // Field 1 with wire type 2 (length-delimited) + 0x0a, + // Varint length delimiter: 2^32 - 1 + 0xff, 0xff, 0xff, 0xff, 0x0f, + }; + // The parser should detect that this message is malformed and return + // null. + Issue375__TestMessage* m = + issue375__test_message__unpack(NULL, sizeof(buffer), buffer); + assert(m == NULL); + + return EXIT_SUCCESS; +} diff --git a/t/issue375/issue375.proto b/t/issue375/issue375.proto new file mode 100644 index 0000000..0566fae --- /dev/null +++ b/t/issue375/issue375.proto @@ -0,0 +1,7 @@ +syntax = "proto2"; + +package issue375; + +message TestMessage { + repeated int32 nums = 1; +}