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 <acozzette@google.com>

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

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;

t/issue375/.gitignore

@@ -0,0 +1 @@
+issue375

t/issue375/issue375.c

@@ -0,0 +1,24 @@
+#include <assert.h>
+#include <stdlib.h>
+
+#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;
+}

t/issue375/issue375.proto

@@ -0,0 +1,7 @@
+syntax = "proto2";
+
+package issue375;
+
+message TestMessage {
+  repeated int32 nums = 1;
+}