| Age | Commit message (Collapse) | Author | Files | Lines |
|---|
|
|
|
|
|
|
|
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
|
|
Update issue 140
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.
|
|
The memset() filled also the extensions field, which was just
waiting for a crash to happen.
|
|
|
|
|
|
|
|
|
|
|
|
Also updated descriptor.proto from protobuf-3.0.0.
|
|
Update issue 136
Status: FixedInGit
|
|
|
|
|
|
|
|
|
|
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().
|
|
Attempts to verify all the properties defined in the security model,
while also being portable and able to run on many platforms.
|
|
Update issue 126
Status: FixedInGit
|
|
|
|
|
|
Update issue 126
Status: Started
|
|
|
|
Update issue 130
Status: Started
|
|
|
|
|
|
|
|
|
|
|
|
The version in PB_PROTO_HEADER_VERSION can be bumped whenever there
is a breaking change to the generated files, and it will then alert
to the difference.
Update issue 129
Status: FixedInGit
|
|
Update issue 106
Status: FixedInGit
|
|
This avoids possible namespace conflicts with other macros.
|
|
Update issue 82
Status: FixedInGit
|
|
|
|
|
|
Update issue 128
Status: FixedInGit
|
|
|
|
|
|
|
|
|
|
Usage like:
MyMessage foo = MyMessage_init_default;
MyMessage_init_default will initialize to default values defined in .proto.
MyMessage_init_zero will initialize to null/zero values. Same results as {}
or {0}, but will avoid compiler warnings by initializing everything explicitly.
Update issue 79
Status: FixedInGit
|
Petteri Aimonen