iemejia commented on code in PR #3858: URL: https://github.com/apache/avro/pull/3858#discussion_r3568093091
########## lang/c/tests/test_avro_4293.c: ########## @@ -0,0 +1,545 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to you under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or + * implied. See the License for the specific language governing + * permissions and limitations under the License. + */ + +/* + * A bytes/string value is encoded as a length prefix followed by that many + * bytes of data. A malicious or truncated input can declare a huge length with + * little or no actual data, which previously caused a correspondingly huge + * allocation before the shortfall was noticed. When the reader knows how many + * bytes remain (a memory-backed reader), the declared length must be rejected + * before allocating for it. + */ + +#include <stdio.h> +#include <stdlib.h> +#include <string.h> +#include <stdint.h> +#include <errno.h> +#include <avro.h> + +/* Portable set/unset of the collection-limit environment variable: MSVC has no + * setenv/unsetenv, so use _putenv_s (an empty value unsets the variable). */ +#ifdef _WIN32 +static void set_collection_limit(const char *value) +{ + _putenv_s("AVRO_MAX_COLLECTION_ITEMS", value); +} +static void unset_collection_limit(void) +{ + _putenv_s("AVRO_MAX_COLLECTION_ITEMS", ""); +} +#else +static void set_collection_limit(const char *value) +{ + setenv("AVRO_MAX_COLLECTION_ITEMS", value, 1); +} +static void unset_collection_limit(void) +{ + unsetenv("AVRO_MAX_COLLECTION_ITEMS"); +} +#endif + +/* Decodes buf against the given schema and returns the avro_value_read rc. + * A non-zero rc means the read was rejected. */ +static int try_decode(avro_value_iface_t *iface, const char *buf, size_t buf_size) Review Comment: Fixed — the header now states it returns the avro_value_read rc (0 ok, >0 rejected) or -1 on harness setup failure, and that callers distinguish the two. ########## lang/c/tests/test_avro_4293.c: ########## @@ -0,0 +1,541 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to you under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or + * implied. See the License for the specific language governing + * permissions and limitations under the License. + */ + +/* + * A bytes/string value is encoded as a length prefix followed by that many + * bytes of data. A malicious or truncated input can declare a huge length with + * little or no actual data, which previously caused a correspondingly huge + * allocation before the shortfall was noticed. When the reader knows how many + * bytes remain (a memory-backed reader), the declared length must be rejected + * before allocating for it. + */ + +#include <stdio.h> +#include <stdlib.h> +#include <string.h> +#include <stdint.h> +#include <errno.h> +#include <avro.h> + +/* Portable set/unset of the collection-limit environment variable: MSVC has no + * setenv/unsetenv, so use _putenv_s (an empty value unsets the variable). */ +#ifdef _WIN32 +static void set_collection_limit(const char *value) +{ + _putenv_s("AVRO_MAX_COLLECTION_ITEMS", value); +} +static void unset_collection_limit(void) +{ + _putenv_s("AVRO_MAX_COLLECTION_ITEMS", ""); +} +#else +static void set_collection_limit(const char *value) +{ + setenv("AVRO_MAX_COLLECTION_ITEMS", value, 1); +} +static void unset_collection_limit(void) +{ + unsetenv("AVRO_MAX_COLLECTION_ITEMS"); +} +#endif + +/* Decodes buf against the given schema and returns the avro_value_read rc. + * A non-zero rc means the read was rejected. */ +static int try_decode(avro_value_iface_t *iface, const char *buf, size_t buf_size) +{ + int rc; + avro_value_t decoded; + avro_reader_t reader; + + rc = avro_generic_value_new(iface, &decoded); + if (rc != 0) { + fprintf(stderr, "avro_generic_value_new failed: %s\n", avro_strerror()); + return -1; + } + + reader = avro_reader_memory(buf, buf_size); + if (reader == NULL) { + fprintf(stderr, "avro_reader_memory failed\n"); + avro_value_decref(&decoded); + return -1; + } + + rc = avro_value_read(reader, &decoded); + + avro_reader_free(reader); + avro_value_decref(&decoded); + return rc; +} + +static int check_rejects_oversized(const char *schema_literal, const char *label) +{ + avro_schema_t schema = NULL; + avro_value_iface_t *iface = NULL; + int rc; + int ret = EXIT_FAILURE; + + /* + * A length prefix declaring 127 bytes (zig-zag long 254 -> varint + * 0xFE 0x01), followed by no data at all. The reader has 0 bytes + * remaining after the prefix, so a declared length of 127 must be + * rejected before allocating. + */ + const char oversized[] = { (char) 0xFE, (char) 0x01 }; + + if (avro_schema_from_json_length(schema_literal, strlen(schema_literal), + &schema) != 0) { + fprintf(stderr, "%s: failed to parse schema: %s\n", label, avro_strerror()); + return EXIT_FAILURE; + } + + iface = avro_generic_class_from_schema(schema); + if (iface == NULL) { + fprintf(stderr, "%s: failed to create iface\n", label); + avro_schema_decref(schema); + return EXIT_FAILURE; + } + + rc = try_decode(iface, oversized, sizeof(oversized)); + if (rc == 0) { + fprintf(stderr, "%s: FAIL - oversized length was accepted\n", label); + } else if (rc != EINVAL) { + /* The availability checks reject with EINVAL before allocating. + * A different error (e.g. ENOSPC from failing to read after a + * large allocation) would indicate the pre-fix behavior. */ + fprintf(stderr, "%s: FAIL - rejected with rc=%d (expected EINVAL): %s\n", + label, rc, avro_strerror()); + } else { + fprintf(stderr, "%s: oversized length rejected as expected: %s\n", + label, avro_strerror()); + ret = EXIT_SUCCESS; + } + + avro_value_iface_decref(iface); + avro_schema_decref(schema); + return ret; +} + +/* Decodes an enum whose declared symbol index is out of range for the schema. + * The index must be rejected before it is stored, rather than accepted as an + * out-of-bounds ordinal. */ +static int check_enum_index_rejected(const char *encoded, size_t size, const char *label) +{ + const char *schema_literal = + "{\"type\":\"enum\",\"name\":\"E\",\"symbols\":[\"A\",\"B\"]}"; + avro_schema_t schema = NULL; + avro_value_iface_t *iface = NULL; + int rc; + int ret = EXIT_FAILURE; + + if (avro_schema_from_json_length(schema_literal, strlen(schema_literal), + &schema) != 0) { + fprintf(stderr, "%s: failed to parse schema: %s\n", label, avro_strerror()); + return EXIT_FAILURE; + } + + iface = avro_generic_class_from_schema(schema); + if (iface == NULL) { + fprintf(stderr, "%s: failed to create iface\n", label); + avro_schema_decref(schema); + return EXIT_FAILURE; + } + + rc = try_decode(iface, encoded, size); + if (rc == 0) { + fprintf(stderr, "%s: FAIL - out-of-range enum index was accepted\n", label); + } else { + fprintf(stderr, "%s: out-of-range enum index rejected as expected: %s\n", + label, avro_strerror()); + ret = EXIT_SUCCESS; + } + + avro_value_iface_decref(iface); + avro_schema_decref(schema); + return ret; +} + +static int check_accepts_valid(void) +{ + avro_schema_t schema = NULL; + avro_value_iface_t *iface = NULL; + int rc; + int ret = EXIT_FAILURE; + const char *text = NULL; + size_t text_size = 0; + avro_value_t decoded; + avro_reader_t reader; + + /* A well-formed 3-byte string "abc": length 3 (zig-zag 6 -> 0x06), + * then the bytes 'a','b','c'. This must still decode. */ + const char valid[] = { 0x06, 'a', 'b', 'c' }; + + if (avro_schema_from_json_literal("\"string\"", &schema) != 0) { + fprintf(stderr, "valid: failed to parse schema: %s\n", avro_strerror()); + return EXIT_FAILURE; + } + iface = avro_generic_class_from_schema(schema); + if (iface == NULL) { + fprintf(stderr, "valid: failed to create iface\n"); + avro_schema_decref(schema); + return EXIT_FAILURE; + } + + if (avro_generic_value_new(iface, &decoded) != 0) { + avro_value_iface_decref(iface); + avro_schema_decref(schema); + return EXIT_FAILURE; + } + reader = avro_reader_memory(valid, sizeof(valid)); + if (reader == NULL) { + fprintf(stderr, "valid: avro_reader_memory failed\n"); + avro_value_decref(&decoded); + avro_value_iface_decref(iface); + avro_schema_decref(schema); + return EXIT_FAILURE; + } + rc = avro_value_read(reader, &decoded); + if (rc == 0 && avro_value_get_string(&decoded, &text, &text_size) == 0 && + text_size == 4 && strcmp(text, "abc") == 0) { + fprintf(stderr, "valid: well-formed string decoded as expected\n"); + ret = EXIT_SUCCESS; + } else { + fprintf(stderr, "valid: FAIL - well-formed string did not decode (rc=%d)\n", rc); + } + + avro_reader_free(reader); + avro_value_decref(&decoded); + avro_value_iface_decref(iface); + avro_schema_decref(schema); + return ret; +} + +/* An array of nulls within the configured limit: null elements occupy zero + * bytes, so a moderate declared count is legitimate and must not be rejected by + * the available-bytes check (it is instead bounded by the zero-byte item cap). */ +static int check_accepts_null_array(void) +{ + avro_schema_t schema = NULL; + avro_value_iface_t *iface = NULL; + avro_value_t decoded; + avro_reader_t reader; + int rc; + int ret = EXIT_FAILURE; + size_t count = 0; + + /* count 127 (zig-zag 254 -> 0xFE 0x01), then the end-of-array marker 0. */ + const char null_array[] = { (char) 0xFE, (char) 0x01, 0x00 }; + + if (avro_schema_from_json_length("{\"type\":\"array\",\"items\":\"null\"}", + strlen("{\"type\":\"array\",\"items\":\"null\"}"), + &schema) != 0) { + fprintf(stderr, "null-array: failed to parse schema: %s\n", avro_strerror()); + return EXIT_FAILURE; + } + iface = avro_generic_class_from_schema(schema); + if (iface == NULL) { + avro_schema_decref(schema); + return EXIT_FAILURE; + } + if (avro_generic_value_new(iface, &decoded) != 0) { + avro_value_iface_decref(iface); + avro_schema_decref(schema); + return EXIT_FAILURE; + } + reader = avro_reader_memory(null_array, sizeof(null_array)); + if (reader == NULL) { + fprintf(stderr, "null-array: avro_reader_memory failed\n"); + avro_value_decref(&decoded); + avro_value_iface_decref(iface); + avro_schema_decref(schema); + return EXIT_FAILURE; + } + rc = avro_value_read(reader, &decoded); + if (rc == 0 && avro_value_get_size(&decoded, &count) == 0 && count == 127) { + fprintf(stderr, "null-array: 127 nulls decoded, not falsely rejected\n"); + ret = EXIT_SUCCESS; + } else { + fprintf(stderr, "null-array: FAIL (rc=%d count=%zu): %s\n", + rc, count, avro_strerror()); + } + + avro_reader_free(reader); + avro_value_decref(&decoded); + avro_value_iface_decref(iface); + avro_schema_decref(schema); + return ret; +} + +/* + * Zero-byte-element collection allocation limit + */ + +/* Encode a long as an Avro zig-zag varint into buf; returns the byte count. */ +static size_t encode_long(int64_t n, char *buf) +{ + uint64_t z = ((uint64_t) n << 1) ^ (uint64_t) (n >> 63); Review Comment: Already portable — encode_long derives the zig-zag sign mask from `(n < 0) ? ~0 : 0` rather than `n >> 63`, so it doesn't depend on signed-shift behavior (test_avro_4293.c:292). ########## lang/c/src/encoding_binary.c: ########## @@ -180,15 +201,35 @@ 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 the + * size_t allocation size, which could otherwise undersize the buffer + * and lead to an out-of-bounds read/write. */ + if ((uint64_t) str_len > (uint64_t) (SIZE_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) { Review Comment: Already handled — read_string bounds str_len with `str_len > INT64_MAX - 1` (in addition to SIZE_MAX-1) before computing str_len + 1 (encoding_binary.c:233-234). ########## lang/c/src/encoding_binary.c: ########## @@ -125,13 +126,33 @@ 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 the + * size_t allocation size, which could otherwise undersize the buffer + * and lead to an out-of-bounds read/write. */ + if ((uint64_t) *len > (uint64_t) (SIZE_MAX - 1)) { + avro_set_error("Bytes length %" PRId64 + " exceeds the maximum allocatable size", *len); + return EINVAL; + } Review Comment: Already handled — read_bytes has the same `*len > INT64_MAX - 1` bound before the +1 used by the AVRO_BYTES caller (encoding_binary.c:153-154). ########## lang/c/src/value-read.c: ########## @@ -38,6 +40,170 @@ static int read_value(avro_reader_t reader, avro_value_t *dest); +/* + * Minimum number of bytes a single value of the given schema can occupy on the + * wire. Used to reject an array/map block count that could not be backed by the + * bytes remaining. A type that can encode to zero bytes (null) returns 0, which + * disables the collection check for it (so an array of nulls is not falsely + * rejected). A recursion guard breaks self-referencing schemas. + */ +static int64_t +min_bytes_per_element(avro_schema_t schema, int depth) +{ + if (schema == NULL) { + return 0; + } + switch (avro_typeof(schema)) { + case AVRO_NULL: + return 0; + case AVRO_FLOAT: + return 4; + case AVRO_DOUBLE: + return 8; + case AVRO_FIXED: + return avro_schema_fixed_size(schema); + case AVRO_RECORD: { + size_t n = avro_schema_record_size(schema); + int64_t total = 0; + size_t i; + if (depth > 64) { + /* Safety net for a pathologically deep (or cyclic) + * schema. Return 0 (a valid conservative lower bound) + * rather than over-estimating: if the true minimum + * cannot be computed, treat the element as possibly + * zero-byte so the tighter zero-byte collection cap is + * applied instead of the much larger structural cap. */ + return 0; + } + for (i = 0; i < n; i++) { + avro_schema_t field = + avro_schema_record_field_get_by_index(schema, i); + int64_t field_min = min_bytes_per_element(field, depth + 1); + /* Saturate rather than overflow: a wrapped (negative) + * total would disable the collection check. */ + if (field_min > INT64_MAX - total) { + return INT64_MAX; + } + total += field_min; + } + return total; + } + case AVRO_LINK: + return min_bytes_per_element(avro_schema_link_target(schema), + depth + 1); + default: + /* boolean, int, long, bytes, string, enum, union, array, map: + * all encode to at least one byte. */ + return 1; + } +} + +/* Non-static wrapper so the skip path (datum_skip.c) can compute the minimum + * on-wire size of a collection's element schema. */ +int64_t +avro_min_bytes_per_element(avro_schema_t schema) +{ + return min_bytes_per_element(schema, 0); +} + +/* + * Maximum number of zero-byte-encoded collection elements (e.g. an array of + * nulls) to allocate from a single decode. Such elements consume no input, so + * the bytes-remaining check cannot bound their count; without a cap a tiny + * payload can declare a huge block count and exhaust memory. Overridable via + * the AVRO_MAX_COLLECTION_ITEMS environment variable. + */ +#define AVRO_DEFAULT_MAX_COLLECTION_ITEMS ((int64_t) 10000000) + +/* + * Structural cap on the number of elements in any array or map (an overflow / + * defense-in-depth guard), matching the historical Integer.MAX_VALUE - 8 limit. + * Non-zero-byte elements are also bounded by the bytes remaining. + */ +#define AVRO_DEFAULT_MAX_COLLECTION_STRUCTURAL ((int64_t) 2147483639) + +/* + * Populate the zero-byte and structural collection limits. The + * AVRO_MAX_COLLECTION_ITEMS environment variable, when a non-negative integer, + * caps both; otherwise the tighter zero-byte default and the structural default + * are used. Shared with the skip path (see avro_collection_limits). + */ +void +avro_collection_limits(int64_t *zero_byte, int64_t *structural) +{ + const char *env = getenv("AVRO_MAX_COLLECTION_ITEMS"); + if (env != NULL && *env != '\0') { + char *end; + long long value = strtoll(env, &end, 10); + if (*end == '\0' && value >= 0) { + /* Clamp to INT64_MAX and to SIZE_MAX: the block count is + * later cast to size_t in the read loop, so a limit above + * SIZE_MAX would permit a count that truncates on cast + * (notably on 32-bit) and weaken the bound. */ + uint64_t v = (uint64_t) value; + if (v > (uint64_t) INT64_MAX) { + v = (uint64_t) INT64_MAX; + } + if (v > (uint64_t) SIZE_MAX) { + v = (uint64_t) SIZE_MAX; + } + *zero_byte = *structural = (int64_t) v; + return; + } + } + *zero_byte = AVRO_DEFAULT_MAX_COLLECTION_ITEMS; + *structural = AVRO_DEFAULT_MAX_COLLECTION_STRUCTURAL; +} + +/* + * Reject a collection (array or map) block that could not be backed by the + * input, before appending. + * + * For elements with a positive minimum on-wire size, the declared count is + * checked against the bytes remaining. For zero-byte elements (e.g. an array of + * nulls), which consume no input and so cannot be bounded by the bytes + * remaining, the cumulative count is checked against a configurable limit. + */ +static int +ensure_collection_available(avro_reader_t reader, int64_t existing, + int64_t count, int64_t min_bytes) +{ + int64_t available; + int64_t zero_byte, structural; + if (count <= 0) { + return 0; + } + avro_collection_limits(&zero_byte, &structural); + if (min_bytes > 0) { Review Comment: Already handled — avro_collection_limits() is fetched once before the block loop (value-read.c:223 / :281) and passed into ensure_collection_available, so it isn't re-parsed per block. ########## lang/c/src/value-read.c: ########## @@ -330,9 +534,19 @@ read_value(avro_reader_t reader, avro_value_t *dest) case AVRO_ENUM: { int64_t val; + int symbol_count; + avro_schema_t enum_schema; check_prefix(rval, avro_binary_encoding. read_long(reader, &val), "Cannot read enum value: "); + enum_schema = avro_value_get_schema(dest); + symbol_count = + avro_schema_enum_number_of_symbols(enum_schema); + if (val < 0 || val >= symbol_count) { Review Comment: Already handled — the enum path checks is_avro_enum(enum_schema) and rejects a non-enum/NULL schema before the range check, so a negative number_of_symbols can't make it accept out-of-range values (value-read.c:548). ########## 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); Review Comment: Correct — st_insert() allocating via avro_new() without a NULL check is a pre-existing OOM-safety gap in st.c, outside this PR's scope. Tracked as a dedicated follow-up: AVRO-4305 ("Make C st_insert OOM-safe"). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
