From 74b2bc43fbe61e3a04fe3f5cc9f817be307e13e1 Mon Sep 17 00:00:00 2001 From: Tobias Stoeckmann Date: Tue, 9 Apr 2019 11:59:20 -0400 Subject: Protect buffers against INT_MAX datalen overflows. Many buffer functions have a hard limit of INT_MAX for datalen, but this limitation is not enforced in all functions: - buf_move_all may exceed that limit with too many chunks - buf_move_to_buf exceeds that limit with invalid buf_flushlen argument - buf_new_with_data may exceed that limit (unit tests only) This patch adds some annotations in some buf_pos_t functions to guarantee that no out of boundary access could occur even if another function lacks safe guards against datalen overflows. [This is a backport of the submitted patch to 0.2.9, where the buf_move_to_buf and buf_new_with_data functions did not exist.] --- src/or/buffers.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/or/buffers.c b/src/or/buffers.c index 89382d1d8e..394ba0ccb8 100644 --- a/src/or/buffers.c +++ b/src/or/buffers.c @@ -394,6 +394,10 @@ buf_free(buf_t *buf) { if (!buf) return; + if (BUG(buf_out->datalen >= INT_MAX || buf_in->datalen >= INT_MAX)) + return; + if (BUG(buf_out->datalen >= INT_MAX - buf_in->datalen)) + return; buf_clear(buf); buf->magic = 0xdeadbeef; @@ -1034,6 +1038,7 @@ buf_find_pos_of_char(char ch, buf_pos_t *out) static inline int buf_pos_inc(buf_pos_t *pos) { + tor_assert(pos->pos < INT_MAX - 1); ++pos->pos; if (pos->pos == (off_t)pos->chunk->datalen) { if (!pos->chunk->next) @@ -1925,6 +1930,7 @@ buf_find_offset_of_char(buf_t *buf, char ch) { chunk_t *chunk; off_t offset = 0; + tor_assert(buf->datalen < INT_MAX); for (chunk = buf->head; chunk; chunk = chunk->next) { char *cp = memchr(chunk->data, ch, chunk->datalen); if (cp) @@ -2044,6 +2050,7 @@ assert_buf_ok(buf_t *buf) for (ch = buf->head; ch; ch = ch->next) { total += ch->datalen; tor_assert(ch->datalen <= ch->memlen); + tor_assert(ch->datalen < INT_MAX); tor_assert(ch->data >= &ch->mem[0]); tor_assert(ch->data <= &ch->mem[0]+ch->memlen); if (ch->data == &ch->mem[0]+ch->memlen) { -- cgit v1.2.3-54-g00ecf From 0fa95308fe5fcce8842530fcae5a49188856e6ac Mon Sep 17 00:00:00 2001 From: Tobias Stoeckmann Date: Sun, 31 Mar 2019 17:33:11 +0200 Subject: Check return value of buf_move_to_buf for error. If the concatenation of connection buffer and the buffer of linked connection exceeds INT_MAX bytes, then buf_move_to_buf returns -1 as an error value. This value is currently casted to size_t (variable n_read) and will erroneously lead to an increasement of variable "max_to_read". This in turn can be used to call connection_buf_read_from_socket to store more data inside the buffer than expected and clogging the connection buffer. If the linked connection buffer was able to overflow INT_MAX, the call of buf_move_to_buf would have previously internally triggered an integer overflow, corrupting the state of the connection buffer. Signed-off-by: Tobias Stoeckmann --- src/or/connection.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/or/connection.c b/src/or/connection.c index 791fd95c27..4f636eeb8c 100644 --- a/src/or/connection.c +++ b/src/or/connection.c @@ -3581,6 +3581,10 @@ connection_read_to_buf(connection_t *conn, ssize_t *max_to_read, if (conn->linked_conn) { result = move_buf_to_buf(conn->inbuf, conn->linked_conn->outbuf, &conn->linked_conn->outbuf_flushlen); + if (BUG(result<0)) { + log_warn(LD_BUG, "reading from linked connection buffer failed."); + return -1; + } } else { result = 0; } -- cgit v1.2.3-54-g00ecf From c10011532e524846bce300a791f51f298b223f6a Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 9 Apr 2019 12:03:22 -0400 Subject: Changes file for bug30041 --- changes/bug30041 | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changes/bug30041 diff --git a/changes/bug30041 b/changes/bug30041 new file mode 100644 index 0000000000..801c8f67ac --- /dev/null +++ b/changes/bug30041 @@ -0,0 +1,5 @@ + o Minor bugfixes (hardening): + - Verify in more places that we are not about to create a buffer + with more than INT_MAX bytes, to avoid possible OOB access in the event + of bugs. Fixes bug 30041; bugfix on 0.2.0.16. Found and fixed by + Tobias Stoeckmann. -- cgit v1.2.3-54-g00ecf From 37bd7fa50d0901a87084b71299cc8c8786cd1cd8 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 9 Apr 2019 13:14:28 -0400 Subject: Modify "Protect buffers against INT_MAX datalen overflows." for 0.2.9 --- src/or/buffers.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/or/buffers.c b/src/or/buffers.c index 394ba0ccb8..b36e4ab509 100644 --- a/src/or/buffers.c +++ b/src/or/buffers.c @@ -394,10 +394,6 @@ buf_free(buf_t *buf) { if (!buf) return; - if (BUG(buf_out->datalen >= INT_MAX || buf_in->datalen >= INT_MAX)) - return; - if (BUG(buf_out->datalen >= INT_MAX - buf_in->datalen)) - return; buf_clear(buf); buf->magic = 0xdeadbeef; @@ -2067,4 +2063,3 @@ assert_buf_ok(buf_t *buf) tor_assert(buf->datalen == total); } } - -- cgit v1.2.3-54-g00ecf