AVRO-4293: [c] Bound allocation when decoding length-prefixed values and collections

lang/c/src/avro_private.h

@@ -27,6 +27,7 @@ extern "C" {
 
 #include "avro/errors.h"
 #include "avro/platform.h"
+#include "avro/schema.h"
 
 #ifdef HAVE_CONFIG_H
 /* This is only true for now in the autotools build */
@@ -95,5 +96,9 @@ extern "C" {
 #define nullstrcmp(s1, s2) \
     (((s1) && (s2)) ? strcmp(s1, s2) : ((s1) || (s2)))
 
+/* Collection allocation limits, shared between the read and skip paths. */
+int64_t avro_min_bytes_per_element(avro_schema_t schema);
+void avro_collection_limits(int64_t *zero_byte, int64_t *structural);
+
 CLOSE_EXTERN
 #endif

lang/c/src/consume-binary.c

@@ -53,12 +53,16 @@ read_array(avro_reader_t reader, const avro_encoding_t * enc,
 
 	check_prefix(rval, enc->read_long(reader, &block_count),
 		     "Cannot read array block count: ");
+	if (block_count == INT64_MIN) {
+		avro_set_error("Invalid array block count");
+		return EINVAL;
+	}
 	check(rval, avro_consumer_call(consumer, array_start_block,
 				       1, block_count, ud));
 
 	while (block_count != 0) {
 		if (block_count < 0) {
-			block_count = block_count * -1;
+			block_count = -block_count;
 			check_prefix(rval, enc->read_long(reader, &block_size),
 				     "Cannot read array block size: ");
 		}
@@ -77,6 +81,10 @@ read_array(avro_reader_t reader, const avro_encoding_t * enc,
 
 		check_prefix(rval, enc->read_long(reader, &block_count),
 			     "Cannot read array block count: ");
+		if (block_count == INT64_MIN) {
+			avro_set_error("Invalid array block count");
+			return EINVAL;
+		}
 		check(rval, avro_consumer_call(consumer, array_start_block,
 					       0, block_count, ud));
 	}
@@ -96,12 +104,16 @@ read_map(avro_reader_t reader, const avro_encoding_t * enc,
 
 	check_prefix(rval, enc->read_long(reader, &block_count),
 		     "Cannot read map block count: ");
+	if (block_count == INT64_MIN) {
+		avro_set_error("Invalid map block count");
+		return EINVAL;
+	}
 	check(rval, avro_consumer_call(consumer, map_start_block,
 				       1, block_count, ud));
 
 	while (block_count != 0) {
 		if (block_count < 0) {
-			block_count = block_count * -1;
+			block_count = -block_count;
 			check_prefix(rval, enc->read_long(reader, &block_size),
 				     "Cannot read map block size: ");
 		}
@@ -135,6 +147,10 @@ read_map(avro_reader_t reader, const avro_encoding_t * enc,
 
 		check_prefix(rval, enc->read_long(reader, &block_count),
 			     "Cannot read map block count: ");
+		if (block_count == INT64_MIN) {
+			avro_set_error("Invalid map block count");
+			return EINVAL;
+		}
 		check(rval, avro_consumer_call(consumer, map_start_block,
 					       0, block_count, ud));
 	}

lang/c/src/datum_skip.c

@@ -17,6 +17,7 @@
 
 #include "avro_private.h"
 #include "avro/errors.h"
+#include <inttypes.h>
 #include <stdlib.h>
 #include <errno.h>
 #include <string.h>
@@ -30,17 +31,38 @@ static int skip_array(avro_reader_t reader, const avro_encoding_t * enc,
 	int64_t i;
 	int64_t block_count;
 	int64_t block_size;
+	int64_t skipped = 0;
+	int64_t zero_byte, structural, limit;
+
+	/* Zero-byte elements skip with no per-item input, so bound them tightly;
+	 * other elements consume bytes (bounded by EOF) so only the structural
+	 * cap applies. Skipping allocates nothing, but an unbounded per-item loop
+	 * is a CPU exhaustion. */
+	avro_collection_limits(&zero_byte, &structural);
+	limit = (avro_min_bytes_per_element(writers_schema->items) > 0)
+		    ? structural : zero_byte;
 
 	check_prefix(rval, enc->read_long(reader, &block_count),
 		     "Cannot read array block count: ");
 
 	while (block_count != 0) {
 		if (block_count < 0) {
-			block_count = block_count * -1;
+			if (block_count == INT64_MIN) {
+				avro_set_error("Invalid array block count");
+				return EINVAL;
+			}
+			block_count = -block_count;
 			check_prefix(rval, enc->read_long(reader, &block_size),
 				     "Cannot read array block size: ");
 		}
 
+		if (block_count > limit || skipped > limit - block_count) {
+			avro_set_error("Cannot skip a collection of more than %"
+				       PRId64 " elements", limit);
+			return EINVAL;
+		}
+		skipped += block_count;
+
 		for (i = 0; i < block_count; i++) {
 			check_prefix(rval, avro_skip_data(reader, writers_schema->items),
 				     "Cannot skip array element: ");
@@ -57,16 +79,32 @@ static int skip_map(avro_reader_t reader, const avro_encoding_t * enc,
 {
 	int rval;
 	int64_t i, block_count;
+	int64_t skipped = 0;
+	int64_t zero_byte, structural;
+
+	/* Map entries always carry a >= 1 byte key, so the structural cap applies
+	 * (they are never zero-byte). */
+	avro_collection_limits(&zero_byte, &structural);
 
 	check_prefix(rval, enc->read_long(reader, &block_count),
 		     "Cannot read map block count: ");
 	while (block_count != 0) {
 		int64_t block_size;
 		if (block_count < 0) {
-			block_count = block_count * -1;
+			if (block_count == INT64_MIN) {
+				avro_set_error("Invalid map block count");
+				return EINVAL;
+			}
+			block_count = -block_count;
 			check_prefix(rval, enc->read_long(reader, &block_size),
 				     "Cannot read map block size: ");
 		}
+		if (block_count > structural || skipped > structural - block_count) {
+			avro_set_error("Cannot skip a collection of more than %"
+				       PRId64 " elements", structural);
+			return EINVAL;
+		}
+		skipped += block_count;
 		for (i = 0; i < block_count; i++) {
 			check_prefix(rval, enc->skip_string(reader),
 				     "Cannot skip map key: ");

lang/c/src/encoding.h

@@ -104,5 +104,15 @@ typedef struct avro_encoding_t avro_encoding_t;
 extern const avro_encoding_t avro_binary_encoding;	/* in
 							 * encoding_binary 
 							 */
+
+/*
+ * Returns the number of bytes still available to read from a memory-backed
+ * reader, or -1 when the amount remaining is unknown (e.g. file readers).
+ * Defined in io.c. Used to reject a length prefix that exceeds the data
+ * actually available before allocating for it, to guard against an
+ * out-of-memory attack from a malicious or truncated input.
+ */
+int64_t avro_reader_bytes_available(avro_reader_t reader);
+
 CLOSE_EXTERN
 #endif

lang/c/src/encoding_binary.c

@@ -21,6 +21,7 @@
 #include "encoding.h"
 #include <stdlib.h>
 #include <limits.h>
+#include <stdint.h>
 #include <errno.h>
 #include <string.h>
 
@@ -125,13 +126,37 @@ static int64_t size_int(avro_writer_t writer, const int32_t i)
 static int read_bytes(avro_reader_t reader, char **bytes, int64_t * len)
 {
 	int rval;
+	int64_t available;
 	check_prefix(rval, read_long(reader, len),
 		     "Cannot read bytes length: ");
 	if (*len < 0) {
 		avro_set_error("Invalid bytes length: %" PRId64, *len);
 		return EINVAL;
 	}
-	*bytes = (char *) avro_malloc(*len + 1);
+	/* Reject a declared length that exceeds the data actually available
+	 * before allocating for it, to guard against an out-of-memory attack
+	 * from a malicious or truncated input. Only enforced when the reader
+	 * can report the amount remaining. */
+	available = avro_reader_bytes_available(reader);
+	if (available >= 0 && *len > available) {
+		avro_set_error("Bytes length %" PRId64
+			       " exceeds %" PRId64 " bytes available",
+			       *len, available);
+		return EINVAL;
+	}
+	/* Bound the length so the +1 (NUL terminator) cannot overflow either
+	 * the size_t allocation size here or the int64_t len + 1 computed by the
+	 * AVRO_BYTES caller in value-read.c. On 64-bit platforms SIZE_MAX >
+	 * INT64_MAX, so the size_t check alone would let len == INT64_MAX through
+	 * and make len + 1 overflow (undefined behavior); the INT64_MAX - 1 bound
+	 * rejects it. */
+	if ((uint64_t) *len > (uint64_t) (SIZE_MAX - 1)
+	    || *len > INT64_MAX - 1) {
+		avro_set_error("Bytes length %" PRId64
+			       " exceeds the maximum allocatable size", *len);
+		return EINVAL;
+	}
+	*bytes = (char *) avro_malloc((size_t) *len + 1);
 	if (!*bytes) {
 		avro_set_error("Cannot allocate buffer for bytes value");
 		return ENOMEM;
@@ -180,15 +205,39 @@ size_bytes(avro_writer_t writer, const char *bytes, const int64_t len)
 static int read_string(avro_reader_t reader, char **s, int64_t *len)
 {
 	int64_t  str_len = 0;
+	int64_t  available;
 	int rval;
 	check_prefix(rval, read_long(reader, &str_len),
 		     "Cannot read string length: ");
 	if (str_len < 0) {
 		avro_set_error("Invalid string length: %" PRId64, str_len);
 		return EINVAL;
 	}
+	/* Reject a declared length that exceeds the data actually available
+	 * before allocating for it, to guard against an out-of-memory attack
+	 * from a malicious or truncated input. Only enforced when the reader
+	 * can report the amount remaining. */
+	available = avro_reader_bytes_available(reader);
+	if (available >= 0 && str_len > available) {
+		avro_set_error("String length %" PRId64
+			       " exceeds %" PRId64 " bytes available",
+			       str_len, available);
+		return EINVAL;
+	}
+	/* Bound the length so the +1 (NUL terminator) cannot overflow either
+	 * the size_t allocation size (undersizing the buffer, leading to an
+	 * out-of-bounds read/write) or the int64_t returned in *len. On 64-bit
+	 * platforms SIZE_MAX > INT64_MAX, so the size_t check alone would let
+	 * str_len == INT64_MAX through and make str_len + 1 overflow (undefined
+	 * behavior); the INT64_MAX - 1 bound rejects it. */
+	if ((uint64_t) str_len > (uint64_t) (SIZE_MAX - 1)
+	    || str_len > INT64_MAX - 1) {
+		avro_set_error("String length %" PRId64
+			       " exceeds the maximum allocatable size", str_len);
+		return EINVAL;
+	}
 	*len = str_len + 1;
-	*s = (char *) avro_malloc(*len);
+	*s = (char *) avro_malloc((size_t) str_len + 1);
 	if (!*s) {
 		avro_set_error("Cannot allocate buffer for string value");
 		return ENOMEM;

lang/c/src/io.c

@@ -20,6 +20,7 @@
 #include "avro/errors.h"
 #include "avro/io.h"
 #include "avro_private.h"
+#include "encoding.h"
 #include <stdio.h>
 #include <stdlib.h>
 #include <errno.h>
@@ -198,6 +199,19 @@ avro_read_memory(struct _avro_reader_memory_t *reader, void *buf, int64_t len)
 	return 0;
 }
 
+int64_t
+avro_reader_bytes_available(avro_reader_t reader)
+{
+	if (is_memory_io(reader)) {
+		struct _avro_reader_memory_t *mem_reader =
+		    avro_reader_to_memory(reader);
+		return mem_reader->len - mem_reader->read;
+	}
+	/* File readers buffer internally and cannot cheaply report how many
+	 * bytes remain in the underlying file, so the amount is unknown. */
+	return -1;
+}
+
 #define bytes_available(reader) (reader->end - reader->cur)
 #define buffer_reset(reader) {reader->cur = reader->end = reader->buffer;}
 

lang/c/src/map.c

@@ -106,16 +106,31 @@ int avro_raw_map_get_or_create(avro_raw_map_t *map, const char *key,
 		el = (char *) raw_entry + sizeof(avro_raw_map_entry_t);
 		is_new = 0;
 	} else {
+		char  *key_copy;
+		size_t  key_size;
 		i = map->elements.element_count;
 		avro_raw_map_entry_t  *raw_entry =
 		    (avro_raw_map_entry_t *) avro_raw_array_append(&map->elements);
-		raw_entry->key = avro_strdup(key);
-		st_insert((st_table *) map->indices_by_key,
-			  (st_data_t) raw_entry->key, (st_data_t) i);
 		if (!raw_entry) {
-			avro_str_free((char*)raw_entry->key);
+			/* avro_raw_array_append returns NULL on allocation
+			 * failure; check before dereferencing it. */
+			return -ENOMEM;
+		}
+		/* Duplicate the key with a NULL-safe allocation: avro_strdup()
+		 * memcpy()s into the buffer without checking it, so it would
+		 * crash on OOM. Only index the entry once the copy succeeds; on
+		 * failure, roll back the element we just appended so the map is
+		 * not left with a half-initialized (NULL-key) entry. */
+		key_size = strlen(key) + 1;
+		key_copy = avro_str_alloc(key_size);
+		if (!key_copy) {
+			map->elements.element_count--;
 			return -ENOMEM;
 		}
+		memcpy(key_copy, key, key_size);
+		raw_entry->key = key_copy;
+		st_insert((st_table *) map->indices_by_key,
+			  (st_data_t) raw_entry->key, (st_data_t) i);
 		el = ((char *) raw_entry) + sizeof(avro_raw_map_entry_t);
 		is_new = 1;
 	}