Skip to content

Commit

Permalink
Fix buffer overflow when encoding bytes with size set to 65535 (#547)
Browse files Browse the repository at this point in the history
On platforms where size_t equals pb_size_t, for example AVR where both
are 16-bit, or x86 and ARM when PB_FIELD_32BIT is defined, the buffer size
checks in pb_write() and pb_enc_submessage can overflow if a bytes field
has size close to maximum size value. This causes read and write out of bounds.

This issue can cause a security vulnerability if the size of a bytes field
in the structure given to pb_encode() is untrusted. Note that pb_decode()
has correct bounds checking and will reject too large values.
  • Loading branch information
PetteriAimonen committed Jun 22, 2020
1 parent 0bf6bef commit 7b44235
Show file tree
Hide file tree
Showing 4 changed files with 64 additions and 3 deletions.
11 changes: 8 additions & 3 deletions pb_encode.c
Original file line number Diff line number Diff line change
Expand Up @@ -104,8 +104,11 @@ bool checkreturn pb_write(pb_ostream_t *stream, const pb_byte_t *buf, size_t cou
{
if (count > 0 && stream->callback != NULL)
{
if (stream->bytes_written + count > stream->max_size)
if (stream->bytes_written + count < stream->bytes_written ||
stream->bytes_written + count > stream->max_size)
{
PB_RETURN_ERROR(stream, "stream full");
}

#ifdef PB_BUFFER_ONLY
if (!buf_write(stream, buf, count))
Expand Down Expand Up @@ -849,6 +852,7 @@ static bool checkreturn pb_enc_fixed32(pb_ostream_t *stream, const pb_field_t *f
static bool checkreturn pb_enc_bytes(pb_ostream_t *stream, const pb_field_t *field, const void *src)
{
const pb_bytes_array_t *bytes = NULL;
size_t allocsize;

bytes = (const pb_bytes_array_t*)src;

Expand All @@ -858,8 +862,9 @@ static bool checkreturn pb_enc_bytes(pb_ostream_t *stream, const pb_field_t *fie
return pb_encode_string(stream, NULL, 0);
}

if (PB_ATYPE(field->type) == PB_ATYPE_STATIC &&
PB_BYTES_ARRAY_T_ALLOCSIZE(bytes->size) > field->data_size)
allocsize = PB_BYTES_ARRAY_T_ALLOCSIZE(bytes->size);
if (allocsize < bytes->size ||
(PB_ATYPE(field->type) == PB_ATYPE_STATIC && allocsize > field->data_size))
{
PB_RETURN_ERROR(stream, "bytes size exceeded");
}
Expand Down
21 changes: 21 additions & 0 deletions tests/regression/issue_547/SConscript
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
# Regression test for issue #547:
# Buffer overflow when encoding bytes with size set to 65535

Import("env")

env.NanopbProto("test.proto")

# Define the compilation options
opts = env.Clone()
opts.Append(CPPDEFINES = {'PB_FIELD_32BIT': 1})

# Build new version of core
strict = opts.Clone()
strict.Append(CFLAGS = strict['CORECFLAGS'])
strict.Object("pb_encode_fields32.o", "$NANOPB/pb_encode.c")
strict.Object("pb_common_fields32.o", "$NANOPB/pb_common.c")

# Build and run test
test = opts.Program(["test.c", "test.pb.c", "pb_encode_fields32.o", "pb_common_fields32.o"])

env.RunTest(test)
28 changes: 28 additions & 0 deletions tests/regression/issue_547/test.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
#include <string.h>
#include <pb_encode.h>
#include <unittests.h>
#include "test.pb.h"

int main()
{
pb_byte_t buf[512];
MyMessage msg = MyMessage_init_zero;
pb_ostream_t stream = pb_ostream_from_buffer(buf, sizeof(buf));

msg.mybytes.size = 0xFFFFFFFF;

if (pb_encode(&stream, MyMessage_fields, &msg))
{
fprintf(stderr, "Failure: expected pb_encode() to fail.\n");
return 1;
}
else if (strcmp(PB_GET_ERROR(&stream), "bytes size exceeded") != 0)
{
fprintf(stderr, "Unexpected encoding error: %s\n", PB_GET_ERROR(&stream));
return 2;
}
else
{
return 0;
}
}
7 changes: 7 additions & 0 deletions tests/regression/issue_547/test.proto
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
syntax = "proto2";

import "nanopb.proto";

message MyMessage {
required bytes mybytes = 1 [(nanopb).max_size = 512];
}

0 comments on commit 7b44235

Please sign in to comment.