From: Petteri Aimonen Date: Mon, 8 Sep 2014 14:34:16 +0000 (+0300) Subject: Protect against size_t overflows in pb_dec_bytes/pb_dec_string. X-Git-Tag: 5.0.2~186^2~190 X-Git-Url: https://gerrit.automotivelinux.org/gerrit/gitweb?a=commitdiff_plain;h=d2099cc8f1adb33d427a44a5e32ed27b647c7168;p=apps%2Fagl-service-can-low-level.git Protect against size_t overflows in pb_dec_bytes/pb_dec_string. Possible consequences of bug: 1) Denial of service by causing a crash Possible when all of the following apply: - Untrusted data is passed to pb_decode() - The top-level message contains a static string field as the first field. Causes a single write of '0' byte to 1 byte before the message struct. 2) Remote code execution Possible when all of the following apply: - 64-bit platform - The message or a submessage contains a static/pointer string field. - Decoding directly from a custom pb_istream_t - bytes_left on the stream is set to larger than 4 GB Causes a write of up to 4 GB of data past the string field. 3) Possible heap corruption or remote code execution Possible when all of the following apply: - less than 64-bit platform - The message or a submessage contains a pointer-type bytes field. Causes a write of sizeof(pb_size_t) bytes of data past a 0-byte long malloc()ed buffer. On many malloc() implementations, this causes at most a crash. However, remote code execution through a controlled jump cannot be ruled out. -- Detailed analysis follows In the following consideration, I define "platform bitness" as equal to number of bits in size_t datatype. Therefore most 8-bit platforms are regarded as 16-bit for the purposes of this discussion. 1. The overflow in pb_dec_string The overflow happens in this computation: uint32_t size; size_t alloc_size; alloc_size = size + 1; There are two ways in which the overflow can occur: In the uint32_t addition, or in the cast to size_t. This depends on the platform bitness. On 32- and 64-bit platforms, the size has to be UINT32_MAX for the overflow to occur. In that case alloc_size will be 0. On 16-bit platforms, overflow will happen whenever size is more than UINT16_MAX, and resulting alloc_size is attacker controlled. For static fields, the alloc_size value is just checked against the field data size. For pointer fields, the alloc_size value is passed to malloc(). End result in both cases is the same, the storage is 0 or just a few bytes in length. On 16-bit platforms, another overflow occurs in the call to pb_read(), when passing the original size. An attacker will want the passed value to be larger than the alloc_size, therefore the only reasonable choice is to have size = UINT16_MAX and alloc_size = 0. Any larger multiple will truncate to the same values. At this point we have read atleast the tag and the string length of the message, i.e. atleast 3 bytes. The maximum initial value for stream bytes_left is SIZE_MAX, thus at this point at most SIZE_MAX-3 bytes are remaining. On 32-bit and 16-bit platforms this means that the size passed to pb_read() is always larger than the number of remaining bytes. This causes pb_read() to fail immediately, before reading any bytes. On 64-bit platforms, it is possible for the bytes_left value to be set to a value larger than UINT32_MAX, which is the wraparound point in size calculation. In this case pb_read() will succeed and write up to 4 GB of attacker controlled data over the RAM that comes after the string field. On all platforms, there is an unconditional write of a terminating null byte. Because the size of size_t typically reflects the size of the processor address space, a write at UINT16_MAX or UINT32_MAX bytes after the string field actually wraps back to before the string field. Consequently, on 32-bit and 16-bit platforms, the bug causes a single write of '0' byte at one byte before the string field. If the string field is in the middle of a message, this will just corrupt other data in the message struct. Because the message contents is attacker controlled anyway, this is a non-issue. However, if the string field is the first field in the top-level message, it can corrupt other data on the stack/heap before it. Typically a single '0' write at a location not controlled by attacker is enough only for a denial-of-service attack. When using pointer fields and malloc(), the attacker controlled alloc_size will cause a 0-size allocation to happen. By the same logic as before, on 32-bit and 16-bit platforms this causes a '0' byte write only. On 64-bit platforms, however, it will again allow up to 4 GB of malicious data to be written over memory, if the stream length allows the read. 2. The overflow in pb_dec_bytes This overflow happens in the PB_BYTES_ARRAY_T_ALLOCSIZE macro: The computation is done in size_t data type this time. This means that an overflow is possible only when n is larger than SIZE_MAX - offsetof(..). The offsetof value in this case is equal to sizeof(pb_size_t) bytes. Because the incoming size value is limited to 32 bits, no overflow can happen here on 64-bit platforms. The size will be passed to pb_read(). Like before, on 32-bit and 16-bit platforms the read will always fail before writing anything. This leaves only the write of bdest->size as exploitable. On statically allocated fields, the size field will always be allocated, regardless of alloc_size. In this case, no buffer overflow is possible here, but user code could possibly use the attacker controlled size value and read past a buffer. If the field is allocated through malloc(), this will allow a write of sizeof(pb_size_t) attacker controlled bytes to past a 0-byte long buffer. In typical malloc implementations, this will either fit in unused alignment padding area, or cause a heap corruption and a crash. Under very exceptional situation it could allow attacker to influence the behaviour of malloc(), possibly jumping into an attacker-controlled location and thus leading to remote code execution. --- diff --git a/pb_decode.c b/pb_decode.c index 37f30707..d1efd1b5 100644 --- a/pb_decode.c +++ b/pb_decode.c @@ -1072,32 +1072,35 @@ static bool checkreturn pb_dec_fixed64(pb_istream_t *stream, const pb_field_t *f static bool checkreturn pb_dec_bytes(pb_istream_t *stream, const pb_field_t *field, void *dest) { uint32_t size; + size_t alloc_size; pb_bytes_array_t *bdest; if (!pb_decode_varint32(stream, &size)) return false; + if (size > PB_SIZE_MAX) + PB_RETURN_ERROR(stream, "bytes overflow"); + + alloc_size = PB_BYTES_ARRAY_T_ALLOCSIZE(size); + if (size > alloc_size) + PB_RETURN_ERROR(stream, "size too large"); + if (PB_ATYPE(field->type) == PB_ATYPE_POINTER) { #ifndef PB_ENABLE_MALLOC PB_RETURN_ERROR(stream, "no malloc support"); #else - if (!allocate_field(stream, dest, PB_BYTES_ARRAY_T_ALLOCSIZE(size), 1)) + if (!allocate_field(stream, dest, alloc_size, 1)) return false; bdest = *(pb_bytes_array_t**)dest; #endif } else { - if (PB_BYTES_ARRAY_T_ALLOCSIZE(size) > field->data_size) + if (alloc_size > field->data_size) PB_RETURN_ERROR(stream, "bytes overflow"); bdest = (pb_bytes_array_t*)dest; } - - if (size > PB_SIZE_MAX) - { - PB_RETURN_ERROR(stream, "bytes overflow"); - } bdest->size = (pb_size_t)size; return pb_read(stream, bdest->bytes, size); @@ -1114,6 +1117,9 @@ static bool checkreturn pb_dec_string(pb_istream_t *stream, const pb_field_t *fi /* Space for null terminator */ alloc_size = size + 1; + if (alloc_size < size) + PB_RETURN_ERROR(stream, "size too large"); + if (PB_ATYPE(field->type) == PB_ATYPE_POINTER) { #ifndef PB_ENABLE_MALLOC