From 74b2bc43fbe61e3a04fe3f5cc9f817be307e13e1 Mon Sep 17 00:00:00 2001 From: Tobias Stoeckmann Date: Tue, 9 Apr 2019 11:59:20 -0400 Subject: [PATCH 1/9] 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) { From 0fa95308fe5fcce8842530fcae5a49188856e6ac Mon Sep 17 00:00:00 2001 From: Tobias Stoeckmann Date: Sun, 31 Mar 2019 17:33:11 +0200 Subject: [PATCH 2/9] 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; } From c10011532e524846bce300a791f51f298b223f6a Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 9 Apr 2019 12:03:22 -0400 Subject: [PATCH 3/9] 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. From 684b396ce5c0a4d5ea70ec01a22d6d368819c873 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 20 Sep 2018 14:34:44 -0400 Subject: [PATCH 4/9] Remove another needless typedef --- src/or/rephist.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/or/rephist.h b/src/or/rephist.h index c464b34f7c..d2f6c66df7 100644 --- a/src/or/rephist.h +++ b/src/or/rephist.h @@ -117,9 +117,7 @@ extern uint32_t rephist_total_num; #ifdef TOR_UNIT_TESTS extern int onion_handshakes_requested[MAX_ONION_HANDSHAKE_TYPE+1]; extern int onion_handshakes_assigned[MAX_ONION_HANDSHAKE_TYPE+1]; -typedef struct bw_array_t bw_array_t; extern bw_array_t *write_array; #endif #endif - From 05d25d06b62a9ee2cc77e44a66be2d9a95cae636 Mon Sep 17 00:00:00 2001 From: teor Date: Tue, 16 Apr 2019 15:39:45 +1000 Subject: [PATCH 5/9] rephist: fix an undeclared type compilation error In 0.3.4 and later, we declare write_array as: extern struct bw_array_t *write_array; ... typedef struct bw_array_t bw_array_t; But in 0.2.9, we declare write_array as: typedef struct bw_array_t bw_array_t; extern bw_array_t *write_array; And then again in rephist.c: typedef struct bw_array_t bw_array_t; So some compilers fail with a duplicate declaration error. We backport 684b396ce5, which removes the duplicate declaration. And this commit deals with the undeclared type error. Backports a single line from merge commit 813019cc57. Fixes bug 30184; not in any released version of Tor. --- src/or/rephist.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/or/rephist.h b/src/or/rephist.h index d2f6c66df7..303cd74f7a 100644 --- a/src/or/rephist.h +++ b/src/or/rephist.h @@ -117,7 +117,7 @@ extern uint32_t rephist_total_num; #ifdef TOR_UNIT_TESTS extern int onion_handshakes_requested[MAX_ONION_HANDSHAKE_TYPE+1]; extern int onion_handshakes_assigned[MAX_ONION_HANDSHAKE_TYPE+1]; -extern bw_array_t *write_array; +extern struct bw_array_t *write_array; #endif #endif From 031ed59dbaf62d9cebd09f98f563c228fe6822f6 Mon Sep 17 00:00:00 2001 From: teor Date: Wed, 17 Apr 2019 11:14:05 +1000 Subject: [PATCH 6/9] test/relay: add a missing typedef In 0.3.4 and later, these functions are declared in rephist.h: STATIC uint64_t find_largest_max(bw_array_t *b); STATIC void commit_max(bw_array_t *b); STATIC void advance_obs(bw_array_t *b); But in 0.2.9, they are declared in rephist.c and test_relay.c. So compilers fail with a "must use 'struct' tag" error. We add the missing struct typedef in test_relay.c, to match the declarations in rephist.c. (Merge commit 813019cc57 moves these functions into rephist.h instead.) Fixes bug 30184; not in any released version of Tor. --- src/test/test_relay.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/test/test_relay.c b/src/test/test_relay.c index 57dcb2406a..d4ebb23382 100644 --- a/src/test/test_relay.c +++ b/src/test/test_relay.c @@ -19,6 +19,8 @@ static or_circuit_t * new_fake_orcirc(channel_t *nchan, channel_t *pchan); static void test_relay_append_cell_to_circuit_queue(void *arg); + +typedef struct bw_array_t bw_array_t; uint64_t find_largest_max(bw_array_t *b); void commit_max(bw_array_t *b); void advance_obs(bw_array_t *b); From c35aded00a19f26f3124584e7d0f561cf579efec Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Tue, 12 Mar 2019 20:11:51 +0200 Subject: [PATCH 7/9] Fix #28525 changes file that is breaking CI. --- changes/bug28525 | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/changes/bug28525 b/changes/bug28525 index 392a9265e5..988ffb2192 100644 --- a/changes/bug28525 +++ b/changes/bug28525 @@ -1,8 +1,7 @@ - o Minor bugfixes (address selection): + o Minor features (address selection): - Make Tor aware of the RFC 6598 (Carrier Grade NAT) IP range, which is the subnet 100.64.0.0/10. This is deployed by many ISPs as an alternative to RFC 1918 that does not break existing internal networks. This patch fixes security issues caused by RFC 6518 by blocking control ports on these addresses and warns users if client ports or ExtORPorts are listening on - a RFC 6598 address. Fixes bug 28525; bugfix on 0.4.1.1-alpha. Patch by - Neel Chauhan. + a RFC 6598 address. Closes ticket 28525. Patch by Neel Chauhan. From 519556ef2cb43b76967ea25372db093d41b6fc4a Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Tue, 12 Mar 2019 20:11:51 +0200 Subject: [PATCH 8/9] Fix #28525 changes file that is breaking CI. --- changes/bug28525 | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/changes/bug28525 b/changes/bug28525 index 392a9265e5..988ffb2192 100644 --- a/changes/bug28525 +++ b/changes/bug28525 @@ -1,8 +1,7 @@ - o Minor bugfixes (address selection): + o Minor features (address selection): - Make Tor aware of the RFC 6598 (Carrier Grade NAT) IP range, which is the subnet 100.64.0.0/10. This is deployed by many ISPs as an alternative to RFC 1918 that does not break existing internal networks. This patch fixes security issues caused by RFC 6518 by blocking control ports on these addresses and warns users if client ports or ExtORPorts are listening on - a RFC 6598 address. Fixes bug 28525; bugfix on 0.4.1.1-alpha. Patch by - Neel Chauhan. + a RFC 6598 address. Closes ticket 28525. Patch by Neel Chauhan. From 37bd7fa50d0901a87084b71299cc8c8786cd1cd8 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 9 Apr 2019 13:14:28 -0400 Subject: [PATCH 9/9] 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); } } -