| Age | Commit message (Collapse) | Author | Files | Lines |
|---|
|
|
|
|
|
Changed to use simple indexing instead of while (count--) in buf_read()/buf_write(),
because the count overflowed from 0 to max on the last iteration. While the unsigned
integer overflow is defined and behaviour was correct, making this simple change
allowed enabling the sanitizer which might catch true errors elsewhere in the code.
|
|
|
|
This commit adds a new FT_INLINE allocation type that forces bytes
fields to be inlined into the struct. E.g., pb_byte_t my_bytes[32].
This requires max_size for the bytes field. The FT_INLINE type is
represented as a new LTYPE: FT_LTYPE_FIXED_LENGTH_BYTES.
This commit also updates the documentation with FT_INLINE and
FT_LTYPE_FIXED_LENGTH_BYTES.
Added an AUTHORS file in apparent order of appearance in the git log
history from $(git log --all).
|
|
Fixes a potential security issue (#205). Only relevant if the user
code writes untrusted data to _count fields, but this is allowed as
per the security model.
|
|
|
|
This supports platforms where uint8_t does not exist.
If you are using a custom pb_syshdr.h, this may require adding
definitions for uint_least8_t etc.
|
|
This was never very clean code, but it was fast. Hopefully
compilers are smart enough to optimize it away, or the speed
difference is not very large. This should be checked.
However working code is always more important than fast code,
and the previous way couldn't really work for platforms that
do not have byte-sized memory access. Related to PR #191.
|
|
This commit changes the prototype for pb_istream_from_buffer from:
pb_istream_t pb_istream_from_buffer(uint8_t *buf, size_t bufsize);
to
pb_istream_t pb_istream_from_buffer(const uint8_t *buf, size_t bufsize);
This allows pb_istream_from_buffer users to point to const buffers
without having to inspect code (to ensure practical const-ness) and then be
forced to manually cast away const.
In order to not break compatibility with existing programs (by
introducing a const/non-const union in the pb_istream_t state) we simply
cast away the const in pb_istream_from_buffer and re-apply it when
possible in the callbacks. Unfortunately we lose any compiler help in
the callbacks to ensure we are treating the buffer as const but manual
inspection is easy enough.
|
|
|
|
* gcc 5.0 and 5.1 appear to take issue with this line and generates the
following error:
/home/nitro/tmp/nanopb/pb_decode.c: In function ‘pb_decode_noinit’:
/home/nitro/tmp/nanopb/pb_decode.c:889:60: error: conversion to ‘uint8_t {aka unsigned char}’ from ‘int’ may alter its value [-Werror=conversion]
fields_seen[iter.required_field_index >> 3] |= (uint8_t)(1 << (iter.required_field_index & 7));
^
* This seems like a compiler bug, but this workaround is harmless.
|
|
Update issue 148
Status: FixedInGit
|
|
Update issue 149
Status: FixedInGit
|
|
Update issue 131
Status: FixedInGit
|
|
Fixes crashes / memory leaks when using pointer type fields.
Also fixes initialization of which_oneof fields.
|
|
Basic test included, should probably add an oneof to the AllTypes test also.
Update issue 131
Status: Started
|
|
Because Issue #139 now allows limiting integer fields, it is good
to check the values received from other protobuf libraries against
the lower limits.
|
|
This allows overriding the integer field types to e.g. uint8_t for
saving RAM.
Update issue 139
Status: FixedInGit
|
|
There was a memory leak when:
1) A statically allocated submessage or
2) an extension field submessage
contained
A) a pointer-type field or
B) a submessage that further contained a pointer-type field.
This was because pb_release() didn't recurse into non-pointer fields.
Update issue 138
Status: FixedInGit
|
|
This makes the behaviour more consistent with non-extension fields,
and also makes sure that all 'found' fields of extensions are initially
false.
|
|
|
|
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.
|
|
This check will help to detect bugs earlier, and is quite lightweight
compared to malloc() anyway.
|
|
If a required or optional field appeared twice in the message data,
pb_decode will overwrite the old data with new one. That is fine, but
with submessage fields, it didn't release the allocated subfields before
overwriting.
This bug can manifest if all of the following conditions are true:
1. There is a message with a "optional" or "required" submessage field
that has type:FT_POINTER.
2. The submessage contains atleast one field with type:FT_POINTER.
3. The message data to be decoded has the submessage field twice in it.
|
|
There was a double-free bug in pb_release() because it didn't set size fields
to zero after deallocation. Most commonly this happens if pb_decode() fails,
internally calls pb_release() and then application code also calls pb_release().
|
|
This avoids possible namespace conflicts with other macros.
|
|
Update issue 82
Status: FixedInGit
|
|
Update issue 128
Status: FixedInGit
|
|
|
|
Back in design phase the code used realloc() for freeing the memory
also. However, this is not entirely portable, and therefore the finished
implementation used free() separately.
There were some remnants of the size = 0 code in the allocate_field()
code, which made it somewhat confusing. This change makes it clearer
that size = 0 is not allowed (and not used by nanopb).
|
|
Update issue 120
Status: FixedInGit
|
|
The multiplication in allocate_field could potentially overflow,
leading to allocating too little memory. This could subsequently
allow an attacker to cause a write past the buffer, overwriting
other memory contents.
The attack is possible if untrusted message data is decoded using
nanopb, and the message type includes a pointer-type string or bytes
field, or a repeated numeric field. Submessage fields are not
affected.
This issue only affects systems that have been compiled with
PB_ENABLE_MALLOC enabled. Only version nanopb-0.2.7 is affected,
as prior versions do not include this functionality.
Update issue 117
Status: FixedInGit
|
|
Update issue 112
Status: FixedInGit
|
|
Update issue 91
Status: FixedInGit
|
|
|
|
|
|
|
|
This makes the internal logic much simpler, and also keeps the datatypes
more similar between STATIC/POINTER cases. It will still be a bit cumbersome
to use because of variable length array member. Macros PB_BYTES_ARRAY_T(n) and
PB_BYTES_ARRAY_T_ALLOCSIZE(n) have been added to make life a bit easier.
This has the drawback that it is no longer as easy to use externally allocated
byte array as input for bytes field in pointer mode. However, this is still
easy to do using callbacks, so it shouldn't be a large issue.
|
|
|
|
|
|
|
|
|
|
Apparently int32 values that are negative must be cast into int64 first
before being encoded. Because uint32 still needs to be cast to uint64,
the cases for int32 and uint32 had to be separated.
Update issue 97
Status: FixedInGit
|
|
For PB_BUFFER_ONLY configuration, this gives 20% speedup without
increasing code size.
|
|
These have been deprecated since nanopb-0.1.6 (some since 0.1.3).
Equivalent functions with better interface are available in the API.
Update issue 91
Status: FixedInGit
|
|
For compliance with MISRA C rules (issue 91).
|
|
|
|
|
|
Testing is still needed. Also only 'optional' extension fields
are supported now, 'repeated' fields are not yet supported.
|
Tobba
Petteri Aimonen
Bernhard Krämer
Tom Roeder
Andrew Ruder
Kyle Manna