From a714989094a67315d17642e142c62a2e5abebb7e Mon Sep 17 00:00:00 2001 From: Joshua Haberman Date: Tue, 3 Mar 2009 22:20:57 -0800 Subject: Detect overflow (unlikely except for malicious input). --- pbstream.c | 44 +++++++++++++++++++++++++++----------------- pbstream.h | 3 +++ pbstream_lowlevel.h | 12 +++++++----- 3 files changed, 37 insertions(+), 22 deletions(-) diff --git a/pbstream.c b/pbstream.c index 9732797..b613e33 100644 --- a/pbstream.c +++ b/pbstream.c @@ -187,7 +187,9 @@ static pbstream_status_t get_BYTES(struct pbstream_parse_state *s, uint8_t *buf, CHECK(get_v_uint32_t(&b, &tmp)); s->offset += (b-buf); /* advance past length varint. */ wvtov_delimited(tmp, &d->v.delimited, s->offset); - s->offset = d->v.delimited.offset + d->v.delimited.len; /* skip bytes */ + size_t new_offset = s->offset + d->v.delimited.len; /* skip bytes */ + if (unlikely(new_offset < s->offset)) return PBSTREAM_ERROR_OVERFLOW; + s->offset = new_offset; return PBSTREAM_STATUS_OK; } @@ -203,6 +205,7 @@ static pbstream_status_t get_MESSAGE(struct pbstream_parse_state *s, uint8_t *bu if (unlikely(++s->top == s->limit)) return PBSTREAM_ERROR_STACK_OVERFLOW; s->top->fieldset = d->field->fieldset; s->top->end_offset = d->v.delimited.offset + d->v.delimited.len; + if (unlikely(s->top->end_offset < s->offset)) return PBSTREAM_ERROR_OVERFLOW; return PBSTREAM_STATUS_OK; } @@ -240,21 +243,25 @@ pbstream_status_t parse_tag(uint8_t **buf, struct pbstream_tag *tag) return PBSTREAM_STATUS_OK; } -pbstream_status_t parse_wire_value(uint8_t **buf, size_t offset, +pbstream_status_t parse_wire_value(uint8_t *buf, size_t *offset, pbstream_wire_type_t wt, union pbstream_wire_value *wv) { +#define READ(expr) CHECK(expr); *offset += (b-buf) + uint8_t *b = buf; switch(wt) { case PBSTREAM_WIRE_TYPE_VARINT: - CHECK(get_v_uint64_t(buf, &wv->varint)); break; + READ(get_v_uint64_t(&b, &wv->varint)); break; case PBSTREAM_WIRE_TYPE_64BIT: - CHECK(get_f_uint64_t(buf, &wv->_64bit)); break; + READ(get_f_uint64_t(&b, &wv->_64bit)); break; case PBSTREAM_WIRE_TYPE_32BIT: - CHECK(get_f_uint32_t(buf, &wv->_32bit)); break; + READ(get_f_uint32_t(&b, &wv->_32bit)); break; case PBSTREAM_WIRE_TYPE_DELIMITED: - wv->delimited.offset = offset; - CHECK(get_v_uint32_t(buf, &wv->delimited.len)); - *buf += wv->delimited.len; + wv->delimited.offset = *offset; + READ(get_v_uint32_t(&b, &wv->delimited.len)); + size_t new_offset = *offset + wv->delimited.len; + if (new_offset < *offset) return PBSTREAM_ERROR_OVERFLOW; + *offset += new_offset; break; case PBSTREAM_WIRE_TYPE_START_GROUP: case PBSTREAM_WIRE_TYPE_END_GROUP: @@ -263,20 +270,24 @@ pbstream_status_t parse_wire_value(uint8_t **buf, size_t offset, return PBSTREAM_STATUS_OK; } -pbstream_status_t skip_wire_value(uint8_t **buf, pbstream_wire_type_t wt) +pbstream_status_t skip_wire_value(uint8_t *buf, size_t *offset, + pbstream_wire_type_t wt) { + uint8_t *b = buf; switch(wt) { case PBSTREAM_WIRE_TYPE_VARINT: - CHECK(skip_v_uint64_t(buf)); break; + READ(skip_v_uint64_t(&b)); break; case PBSTREAM_WIRE_TYPE_64BIT: - CHECK(skip_f_uint64_t(buf)); break; + READ(skip_f_uint64_t(&b)); break; case PBSTREAM_WIRE_TYPE_32BIT: - CHECK(skip_f_uint32_t(buf)); break; + READ(skip_f_uint32_t(&b)); break; case PBSTREAM_WIRE_TYPE_DELIMITED: { /* Have to get (not skip) the length to skip the bytes. */ uint32_t len; - CHECK(get_v_uint32_t(buf, &len)); - *buf += len; + READ(get_v_uint32_t(&b, &len)); + size_t new_offset = *offset + len; + if (new_offset < *offset) return PBSTREAM_ERROR_OVERFLOW; + *offset += new_offset; break; } case PBSTREAM_WIRE_TYPE_START_GROUP: @@ -284,6 +295,7 @@ pbstream_status_t skip_wire_value(uint8_t **buf, pbstream_wire_type_t wt) return PBSTREAM_ERROR_GROUP; /* deprecated, no plans to support. */ } return PBSTREAM_STATUS_OK; +#undef READ } struct pbstream_field *pbstream_find_field(struct pbstream_fieldset* fs, @@ -333,9 +345,7 @@ pbstream_status_t pbstream_parse_field(struct pbstream_parse_state *s, unknown_value: wv->type = tag.wire_type; - b = buf; - CHECK(parse_wire_value(&b, s->offset, tag.wire_type, &wv->v)); - s->offset += (b-buf); + CHECK(parse_wire_value(buf, &s->offset, tag.wire_type, &wv->v)); return unknown_value_status; } diff --git a/pbstream.h b/pbstream.h index 6830689..6ad6685 100644 --- a/pbstream.h +++ b/pbstream.h @@ -153,6 +153,9 @@ typedef enum pbstream_status { // Input was nested more than PBSTREAM_MAX_STACK deep. PBSTREAM_ERROR_STACK_OVERFLOW = -4, + // The input data caused the pb's offset (a size_t) to overflow. + PBSTREAM_ERROR_OVERFLOW = -5, + /** NONFATAL ERRORS: the input was invalid, but we can continue if desired. */ // A value was encountered that was not defined in the .proto file. The diff --git a/pbstream_lowlevel.h b/pbstream_lowlevel.h index 2fa31a3..c25b16d 100644 --- a/pbstream_lowlevel.h +++ b/pbstream_lowlevel.h @@ -23,18 +23,20 @@ struct pbstream_tag { }; /* Parses a single tag from the character data starting at buf, and updates - * buf to point one past the bytes that were consumed. */ + * buf to point one past the bytes that were consumed. buf will be incremented + * by at most ten bytes. */ pbstream_status_t parse_tag(uint8_t **buf, struct pbstream_tag *tag); /* Parses a wire value with the given type (which must have been obtained from - * a tag that was just parsed) and updates buf to point to one past the bytes - * that were consumed. */ -pbstream_status_t parse_wire_value(uint8_t **buf, size_t offset, + * a tag that was just parsed) and adds the number of bytes that were consumed + * to *offset. */ +pbstream_status_t parse_wire_value(uint8_t *buf, size_t *offset, pbstream_wire_type_t wt, union pbstream_wire_value *wv); /* Like the above, but discards the wire value instead of saving it. */ -pbstream_status_t skip_wire_value(uint8_t **buf, pbstream_wire_type_t wt); +pbstream_status_t skip_wire_value(uint8_t *buf, size_t *offset, + pbstream_wire_type_t wt); /* Looks the given field number up in the fieldset, and returns the * corresponding pbstream_field definition (or NULL if this field number -- cgit v1.2.3