Trac issueshttps://gitlab.torproject.org/legacy/trac/-/issues2020-06-13T15:38:36Zhttps://gitlab.torproject.org/legacy/trac/-/issues/23512Bandwidth stats info leak upon close of circuits with queued cells2020-06-13T15:38:36ZGeorge KadianakisBandwidth stats info leak upon close of circuits with queued cellsWe received a tor bug bounty report from `jaym` about a congestion attack variant that can cause bandwidth stats watermark.
The bug uses the fact that Tor increments the _read bytes counter_ before adding the cell to the output buffer:...We received a tor bug bounty report from `jaym` about a congestion attack variant that can cause bandwidth stats watermark.
The bug uses the fact that Tor increments the _read bytes counter_ before adding the cell to the output buffer: If the circuit gets killed before the cell gets relayed to the next hop, then the _write bytes counter_ will never be updated, making the _read bytes counter_ having a higher value than the _write bytes counter_. The attacker could exploit this assymetry to find relays using their bandwidth graph.
The attacker can kill the circuit using the OOM killer by saturating its output queue with cells until `circuits_handle_oom()` gets called and kills the circuit.
We should figure out whether this attack is practical (the paper claims it is) and whether it's worthwhile fixing it. Just fixing this issue won't solve the general issue of congestion attacks, and it might even allow other kinds of attacks.
The most practical fix right now seem to be to hack circuit_handle_oom()` to actually decrement the read counters before killing a circuit. However, that's a very specific fix that might solve this very specific bug, but leave the rest of the bug class open.
Another approach would be removing the bandwidth graphs, or aggregating them over a greater period of time, or adding noise. We should consider these approaches carefully since bandwidth graphs see great use by academic papers and also by relay operators (to gauge their contribution).Tor: 0.3.4.x-finalhttps://gitlab.torproject.org/legacy/trac/-/issues/20081potential memory corruption in or/buffers.c (not exploitable)2020-06-13T15:01:11ZGeorge Kadianakispotential memory corruption in or/buffers.c (not exploitable)Bug reported by _Guido Vranken_ through hackerone.
We believe it's not an exploitable issue, but it's still a bug worth fixing.
Here follows the report:
----
In `or/buffer.s.c`:
```
/** Return the allocation size we'd like to use to ...Bug reported by _Guido Vranken_ through hackerone.
We believe it's not an exploitable issue, but it's still a bug worth fixing.
Here follows the report:
----
In `or/buffer.s.c`:
```
/** Return the allocation size we'd like to use to hold <b>target</b>
* bytes. */
static inline size_t
preferred_chunk_size(size_t target)
{
size_t sz = MIN_CHUNK_ALLOC;
while (CHUNK_SIZE_WITH_ALLOC(sz) < target) {
sz <<= 1;
}
return sz;
}
```
```
#define MIN_CHUNK_ALLOC 256
#define CHUNK_SIZE_WITH_ALLOC(memlen) ((memlen) - CHUNK_HEADER_LEN)
```
CHUNK_HEADER_LEN is usually around 30 bytes or so.
The problem with `preferred_chunk_size` is that for a large `size_t target`, the function will return 0.
If you compile this program with `-m32`:
```
#include <stdio.h>
#include <stdint.h>
#define FLEXIBLE_ARRAY_MEMBER /**/
#define DEBUG_CHUNK_ALLOC
/** A single chunk on a buffer. */
typedef struct chunk_t {
struct chunk_t *next; /**< The next chunk on the buffer. */
size_t datalen; /**< The number of bytes stored in this chunk */
size_t memlen; /**< The number of usable bytes of storage in <b>mem</b>. */
#ifdef DEBUG_CHUNK_ALLOC
size_t DBG_alloc;
#endif
char *data; /**< A pointer to the first byte of data stored in <b>mem</b>. */
uint32_t inserted_time; /**< Timestamp in truncated ms since epoch
* when this chunk was inserted. */
char mem[FLEXIBLE_ARRAY_MEMBER]; /**< The actual memory used for storage in
* this chunk. */
} chunk_t;
#if defined(__GNUC__) && __GNUC__ > 3
#define STRUCT_OFFSET(tp, member) __builtin_offsetof(tp, member)
#else
#define STRUCT_OFFSET(tp, member) \
((off_t) (((char*)&((tp*)0)->member)-(char*)0))
#endif
#define MIN_CHUNK_ALLOC 256
#define CHUNK_HEADER_LEN STRUCT_OFFSET(chunk_t, mem[0])
#define CHUNK_SIZE_WITH_ALLOC(memlen) ((memlen) - CHUNK_HEADER_LEN)
static inline size_t
preferred_chunk_size(size_t target)
{
size_t sz = MIN_CHUNK_ALLOC;
while (CHUNK_SIZE_WITH_ALLOC(sz) < target) {
sz <<= 1;
}
return sz;
}
int main(void)
{
size_t i = 1024;
printf("i is %08X, preferred_chunk_size is %08X\n", i, preferred_chunk_size(i)); i <<= 1;
printf("i is %08X, preferred_chunk_size is %08X\n", i, preferred_chunk_size(i)); i <<= 1;
printf("i is %08X, preferred_chunk_size is %08X\n", i, preferred_chunk_size(i)); i <<= 1;
printf("i is %08X, preferred_chunk_size is %08X\n", i, preferred_chunk_size(i)); i <<= 1;
printf("i is %08X, preferred_chunk_size is %08X\n", i, preferred_chunk_size(i)); i <<= 1;
printf("i is %08X, preferred_chunk_size is %08X\n", i, preferred_chunk_size(i)); i <<= 1;
printf("i is %08X, preferred_chunk_size is %08X\n", i, preferred_chunk_size(i)); i <<= 1;
printf("i is %08X, preferred_chunk_size is %08X\n", i, preferred_chunk_size(i)); i <<= 1;
printf("i is %08X, preferred_chunk_size is %08X\n", i, preferred_chunk_size(i)); i <<= 1;
printf("i is %08X, preferred_chunk_size is %08X\n", i, preferred_chunk_size(i)); i <<= 1;
printf("i is %08X, preferred_chunk_size is %08X\n", i, preferred_chunk_size(i)); i <<= 1;
printf("i is %08X, preferred_chunk_size is %08X\n", i, preferred_chunk_size(i)); i <<= 1;
printf("i is %08X, preferred_chunk_size is %08X\n", i, preferred_chunk_size(i)); i <<= 1;
printf("i is %08X, preferred_chunk_size is %08X\n", i, preferred_chunk_size(i)); i <<= 1;
printf("i is %08X, preferred_chunk_size is %08X\n", i, preferred_chunk_size(i)); i <<= 1;
printf("i is %08X, preferred_chunk_size is %08X\n", i, preferred_chunk_size(i)); i <<= 1;
printf("i is %08X, preferred_chunk_size is %08X\n", i, preferred_chunk_size(i)); i <<= 1;
printf("i is %08X, preferred_chunk_size is %08X\n", i, preferred_chunk_size(i)); i <<= 1;
printf("i is %08X, preferred_chunk_size is %08X\n", i, preferred_chunk_size(i)); i <<= 1;
printf("i is %08X, preferred_chunk_size is %08X\n", i, preferred_chunk_size(i)); i <<= 1;
printf("i is %08X, preferred_chunk_size is %08X\n", i, preferred_chunk_size(i)); i <<= 1;
printf("i is %08X, preferred_chunk_size is %08X\n", i, preferred_chunk_size(i)); i <<= 1;
return 0;
}
```
the output is:
```
i is 00000400, preferred_chunk_size is 00000800
i is 00000800, preferred_chunk_size is 00001000
i is 00001000, preferred_chunk_size is 00002000
i is 00002000, preferred_chunk_size is 00004000
i is 00004000, preferred_chunk_size is 00008000
i is 00008000, preferred_chunk_size is 00010000
i is 00010000, preferred_chunk_size is 00020000
i is 00020000, preferred_chunk_size is 00040000
i is 00040000, preferred_chunk_size is 00080000
i is 00080000, preferred_chunk_size is 00100000
i is 00100000, preferred_chunk_size is 00200000
i is 00200000, preferred_chunk_size is 00400000
i is 00400000, preferred_chunk_size is 00800000
i is 00800000, preferred_chunk_size is 01000000
i is 01000000, preferred_chunk_size is 02000000
i is 02000000, preferred_chunk_size is 04000000
i is 04000000, preferred_chunk_size is 08000000
i is 08000000, preferred_chunk_size is 10000000
i is 10000000, preferred_chunk_size is 20000000
i is 20000000, preferred_chunk_size is 40000000
i is 40000000, preferred_chunk_size is 80000000
i is 80000000, preferred_chunk_size is 00000000
```
The danger is that the return value of `preferred_chunk_size` is always used as a parameter to `tor_malloc` or `tor_realloc`. It is called at these places:
In `buf_pullup`:
```
210 newsize = CHUNK_SIZE_WITH_ALLOC(preferred_chunk_size(capacity));
211 newhead = chunk_grow(buf->head, newsize);
```
In `buf_new_with_capacity`:
```
283 /** Create and return a new buf with default chunk capacity <b>size</b>.
284 */
285 buf_t *
286 buf_new_with_capacity(size_t size)
287 {
288 buf_t *b = buf_new();
289 b->default_chunk_size = preferred_chunk_size(size);
290 return b;
291 }
```
In `buf_add_chunk_with_capacity`:
```
401 /** Append a new chunk with enough capacity to hold <b>capacity</b> bytes to
402 * the tail of <b>buf</b>. If <b>capped</b>, don't allocate a chunk bigger
403 * than MAX_CHUNK_ALLOC. */
404 static chunk_t *
405 buf_add_chunk_with_capacity(buf_t *buf, size_t capacity, int capped)
406 {
407 chunk_t *chunk;
408
409 if (CHUNK_ALLOC_SIZE(capacity) < buf->default_chunk_size) {
410 chunk = chunk_new_with_alloc_size(buf->default_chunk_size);
411 } else if (capped && CHUNK_ALLOC_SIZE(capacity) > MAX_CHUNK_ALLOC) {
412 chunk = chunk_new_with_alloc_size(MAX_CHUNK_ALLOC);
413 } else {
414 chunk = chunk_new_with_alloc_size(preferred_chunk_size(capacity));
415 }
```
`buf_new_with_capacity` is currently called nowhere except for tests.
`buf_add_chunk_with_capacity` is called at various places but currently not with the `apped` parameter set to 0.
However, `buf_pullup` is called at various places and the call to `preferred_chunk_size` is reachable. Whether it is reachable with a parameter large enough that it will return 0 I'm not sure about.
```
int
tor_main(int argc, char *argv[])
{
buf_t* buf;
char* string;
size_t string_len;
size_t i;
buf = buf_new();
string_len = 0x00001000;
string = tor_malloc(string_len);
for (i = 0; i < 507904; i++)
{
write_to_buf(string, string_len, buf);
}
write_to_buf(string, 0x3FFFFFA, buf);
free(string);
buf_pullup(buf, 0x90000000);
}
```
What will happen is that `buf_pullup` will call `hunk_grow`
```
140 static inline chunk_t *
141 chunk_grow(chunk_t *chunk, size_t sz)
142 {
143 off_t offset;
144 size_t memlen_orig = chunk->memlen;
145 tor_assert(sz > chunk->memlen);
146 offset = chunk->data - chunk->mem;
147 chunk = tor_realloc(chunk, CHUNK_ALLOC_SIZE(sz));
148 chunk->memlen = sz;
149 chunk->data = chunk->mem + offset;
```
`tor_realloc` will in effect be called with a size parameter of 0. Whether and how much legitimate heap memory `realloc` will allocate might be implementation-dependent. The point is that the
following lines might overwrite heap memory:
```
148 chunk->memlen = sz;
149 chunk->data = chunk->mem + offset;
```
since `hunk` is a memory area that has just been allocated to 0 (or 1, after correction) bytes.
The whole scenario is not very likely considering Tor's frugal memory consumption but it is nonetheless a programming fault in the buffers "API" that could lead to stability issues. Especially if you ever
expand the use of `buf_pullup`, `buf_new_with_capacity`, and/or uncapped `buf_add_chunk_with_capacity`, it'll be wise to hard-limit the amounts of right-shifts in `preferred_chunk_size` (a
single unintended negative integer -> size_t can be conducive in establishing an exploitation path).Tor: 0.2.9.x-finalNick MathewsonNick Mathewsonhttps://gitlab.torproject.org/legacy/trac/-/issues/19418i2d_RSAPublicKey retval ignored in multiple callsites2020-06-13T14:58:40ZGeorge Kadianakisi2d_RSAPublicKey retval ignored in multiple callsitesHello. Here follows a bug report by `Guido Vranken` received through the hackerone program.
There are various places in the codebase where we don't check the retval of `i2d_RSA_PublicKey()` (or its callers). That function can fail in ca...Hello. Here follows a bug report by `Guido Vranken` received through the hackerone program.
There are various places in the codebase where we don't check the retval of `i2d_RSA_PublicKey()` (or its callers). That function can fail in cases of OOM, and this is something we should be handling correctly. This is a similar case to #17686.Tor: 0.3.1.x-finalNick MathewsonNick Mathewsonhttps://gitlab.torproject.org/legacy/trac/-/issues/19281Potential heap corruption via `write_escaped_data` in control.c2020-06-13T14:58:12ZGeorge KadianakisPotential heap corruption via `write_escaped_data` in control.cHere follows another bug report by guido from hackerone.
We think this is safe to list on trac, because the `write_escaped_data` function is only called from the control API, and we believe it's not possible to force the control API to ...Here follows another bug report by guido from hackerone.
We think this is safe to list on trac, because the `write_escaped_data` function is only called from the control API, and we believe it's not possible to force the control API to write enough amounts of data to trigger this bug (since descriptors, dir documents, and cells are all capped in size, and this bug requires message lengths close to `INT_MAX`).
Bug report follows:
----
```
/** Given a <b>len</b>-character string in <b>data</b>, made of lines
* terminated by CRLF, allocate a new string in *<b>out</b>, and copy the
* contents of <b>data</b> into *<b>out</b>, adding a period before any period
* that appears at the start of a line, and adding a period-CRLF line at
* the end. Replace all LF characters sequences with CRLF. Return the number
* of bytes in *<b>out</b>.
*/
STATIC size_t
write_escaped_data(const char *data, size_t len, char **out)
{
size_t sz_out = len+8;
char *outp;
const char *start = data, *end;
int i;
int start_of_line;
for (i=0; i<(int)len; ++i) {
if (data[i]== '\n')
sz_out += 2; /* Maybe add a CR; maybe add a dot. */
}
*out = outp = tor_malloc(sz_out+1);
end = data+len;
start_of_line = 1;
while (data < end) {
if (*data == '\n') {
if (data > start && data[-1] != '\r')
*outp++ = '\r';
start_of_line = 1;
} else if (*data == '.') {
if (start_of_line) {
start_of_line = 0;
*outp++ = '.';
}
} else {
start_of_line = 0;
}
*outp++ = *data++;
}
if (outp < *out+2 || fast_memcmp(outp-2, "\r\n", 2)) {
*outp++ = '\r';
*outp++ = '\n';
}
*outp++ = '.';
*outp++ = '\r';
*outp++ = '\n';
*outp = '\0'; /* NUL-terminate just in case. */
tor_assert((outp - *out) <= (int)sz_out);
return outp - *out;
}
```
There are two potential vulnerabilities lurking here:
1. If the input size (`len`) >= 0x80000000, then this loop will not execute at all:
```
for (i=0; i<(int)len; ++i) {
if (data[i]== '\n')
sz_out += 2; /* Maybe add a CR; maybe add a dot. */
}
```
Because the condition `i<(int)len` is effectively `i<(negative number)` and `i` is intialized to 0, this can never be true. As a result of this, the output buffer (whose size is based on sz_out)
is too small to hold the result for an input buffer containing '\n' characters.
Triggering this is typically only feasible on a 64-bit system, because if the input buffer is >= 0x80000000 bytes, then sz_out is set to 0x80000008 bytes, and allocating such an amount twice (one for the
input buffer, and one for the output buffer) is not possible on a 32-bit system.
2. If the equation (number of '\n' characters in input buffer * 2 + size of input buffer) exceeds 0xFFFFFFFF, then this will cause heap corruption on a 32-bit system, because sz_out overflows.Tor: 0.3.2.x-finalNick MathewsonNick Mathewsonhttps://gitlab.torproject.org/legacy/trac/-/issues/19223Potential heap corruption in do_getpass in routerkeys.c2020-06-13T14:58:10ZGeorge KadianakisPotential heap corruption in do_getpass in routerkeys.cHello,
this is a bug by Guido Vranken from our bug bounty program. This bug is not triggerable in the current codebase, but it's still a good idea to fix, for future safety.
Here follows the bug report as received:
----
`do_getpass` ...Hello,
this is a bug by Guido Vranken from our bug bounty program. This bug is not triggerable in the current codebase, but it's still a good idea to fix, for future safety.
Here follows the bug report as received:
----
`do_getpass` contains this code:
```
if (twice) {
const char msg[] = "One more time:";
size_t p2len = strlen(prompt) + 1;
if (p2len < sizeof(msg))
p2len = sizeof(msg);
prompt2 = tor_malloc(strlen(prompt)+1);
memset(prompt2, ' ', p2len);
memcpy(prompt2 + p2len - sizeof(msg), msg, sizeof(msg));
buf2 = tor_malloc_zero(buflen);
}
```
There is only one call to this function in the code for which twice == 1:
```
if (do_getpass("Enter new passphrase:", pwbuf0, sizeof(pwbuf0), 1,
get_options()) < 0) {
log_warn(LD_OR, "NO/failed passphrase");
return -1;
}
```
This will not trigger a memory corruption, but if the first parameter had been shorter, it would:
Compile and run like this:
```
$ gcc -fomit-frame-pointer -fsanitize=address do_getpass.c
$ ./a.out "Enter new passphrase:"
$ ./a.out "Enter new passphrase"
$ ./a.out "Enter new passphras"
$ ./a.out "Enter new passphra"
$ ./a.out "Enter new passphr"
$ ./a.out "Enter new passph"
$ ./a.out "Enter new passp"
$ ./a.out "Enter new pass"
$ ./a.out "Enter new pas"
==7883== ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60040000dffe at pc 0x400c0a bp 0x7fff8d9c22e0 sp 0x7fff8d9c22d8
...
...
```
So it's not really a vulnerability at present, but I thought I'd mention it to
you since it struck me as odd and it could become a problem if you pass a
dynamic, potentially short string (for ex. created with snprintf) to
do_getpass.Tor: 0.2.9.x-finalhttps://gitlab.torproject.org/legacy/trac/-/issues/19222base64_decode() unreachable heap corruption on 32-bit systems2020-06-13T14:58:09ZGeorge Kadianakisbase64_decode() unreachable heap corruption on 32-bit systemsHello,
this is a bug by `Guido Vranken` from our bug bounty program. After analysis, we found that there are no codepaths that allow the attacker to specify such a big input size to `base64_decode()` hence this bug should not be exploit...Hello,
this is a bug by `Guido Vranken` from our bug bounty program. After analysis, we found that there are no codepaths that allow the attacker to specify such a big input size to `base64_decode()` hence this bug should not be exploitable. More checking should be done, and there might be more instances of this rounding pattern around our codebase.
Here follows the bug report as received:
----
```
int
base64_decode(char *dest, size_t destlen, const char *src, size_t srclen)
{
...
...
if (destlen < (srclen*3)/4)
return -1;
if (destlen > SIZE_T_CEILING)
return -1;
```
The problem here is that the multiplication (by 3) occurs before the division (by 4).
For source strings larger than 0xFFFFFFFF / 3 == 0x55555555, an overflow will occur within this calculation. If the result of the overflow-affected calculation is smaller than what ```destlen``` is, then
this check will be passed and memory will be corrupted.Tor: 0.3.0.x-finalhttps://gitlab.torproject.org/legacy/trac/-/issues/19150Pointer overflow in memarea_alloc()2020-06-13T14:57:45ZGeorge KadianakisPointer overflow in memarea_alloc()There is a pointer overflow in `memarea_alloc()`:
```
if (chunk->next_mem+sz > chunk->U_MEM+chunk->mem_size) {
```
It does not seem to be RCE exploitable, since in all places in `routerparse.c` where memareas are used, we restrict th...There is a pointer overflow in `memarea_alloc()`:
```
if (chunk->next_mem+sz > chunk->U_MEM+chunk->mem_size) {
```
It does not seem to be RCE exploitable, since in all places in `routerparse.c` where memareas are used, we restrict the input size to 128kb or so (through `MAX_LINE_LENGTH` and `MAX_UNPARSED_OBJECT_SIZE`).
However, we should still fix this to plug any DoS threats and for future code correctness.
The bug was found by Guido Vranken through the hackerone bug bounty program.Tor: 0.2.8.x-finalNick MathewsonNick Mathewsonhttps://gitlab.torproject.org/legacy/trac/-/issues/18189Control connection pre-auth local DoS when bufferevents enabled2020-06-13T14:58:46ZGeorge KadianakisControl connection pre-auth local DoS when bufferevents enabledHere is another report by _Guido Vranken_ through hackerone. Please credit him appropriately.
BTW, while this is not a serious vulnerability (local DoS), we should consider making it even harder for people to enable bufferevents, or rip...Here is another report by _Guido Vranken_ through hackerone. Please credit him appropriately.
BTW, while this is not a serious vulnerability (local DoS), we should consider making it even harder for people to enable bufferevents, or ripping them out completely, if we don't plan to develop them further in the future.
---
## Bug report
In `control.c`, this is the loop that retrieves data from the input buffer of the connection, or returns if no complete linefreed-terminated line is available (connection_fetch_from_buf_line() returns 0).
```
4225 while (1) {
4226 size_t last_idx;
4227 int r;
4228 /* First, fetch a line. */
4229 do {
4230 data_len = conn->incoming_cmd_len - conn->incoming_cmd_cur_len;
4231 r = connection_fetch_from_buf_line(TO_CONN(conn),
4232 conn->incoming_cmd+conn->incoming_cmd_cur_len,
4233 &data_len);
4234 if (r == 0)
4235 /* Line not all here yet. Wait. */
4236 return 0;
4237 else if (r == -1) {
4238 if (data_len + conn->incoming_cmd_cur_len > MAX_COMMAND_LINE_LENGTH) {
4239 connection_write_str_to_buf("500 Line too long.\r\n", conn);
4240 connection_stop_reading(TO_CONN(conn));
4241 connection_mark_and_flush(TO_CONN(conn));
4242 }
4243 while (conn->incoming_cmd_len < data_len+conn->incoming_cmd_cur_len)
4244 conn->incoming_cmd_len *= 2;
4245 conn->incoming_cmd = tor_realloc(conn->incoming_cmd,
4246 conn->incoming_cmd_len);
4247 }
4248 } while (r != 1);
```
If connection_fetch_from_buf_line() returns -1, this means that the buffer (conn->incoming_cmd) is not large enough. conn->incoming_cmd_len is then increased to a size sufficiently large to hold the incoming command (lines 4243 - 4246). In order for this to work, data_len must be set to this required size by connection_fetch_from_buf_line().
If libevent bufferevents are not enabled, then connection_fetch_from_buf_line() is simply a proxy function for fetch_from_buf_line():
```
3785 }) ELSE_IF_NO_BUFFEREVENT {
3786 return fetch_from_buf_line(conn->inbuf, data, data_len);
3787 }
```
This function will indeed set *data_len to the required size if the present buffer size is too small (line 2255):
```
2241 int
2242 fetch_from_buf_line(buf_t *buf, char *data_out, size_t *data_len)
2243 {
2244 size_t sz;
2245 off_t offset;
2246
2247 if (!buf->head)
2248 return 0;
2249
2250 offset = buf_find_offset_of_char(buf, '\n');
2251 if (offset < 0)
2252 return 0;
2253 sz = (size_t) offset;
2254 if (sz+2 > *data_len) {
2255 *data_len = sz + 2;
2256 return -1;
2257 }
2258 fetch_from_buf(data_out, sz+1, buf);
2259 data_out[sz+1] = '\0';
2260 *data_len = sz+1;
2261 return 1;
2262 }
```
However, if libevent bufferevents are enabled (by ./configuring tor with --enable-bufferevents), then the code on lines (3770 - 3784) is executed instead:
```
3765 int
3766 connection_fetch_from_buf_line(connection_t *conn, char *data,
3767 size_t *data_len)
3768 {
3769 IF_HAS_BUFFEREVENT(conn, {
3770 int r;
3771 size_t eol_len=0;
3772 struct evbuffer *input = bufferevent_get_input(conn->bufev);
3773 struct evbuffer_ptr ptr =
3774 evbuffer_search_eol(input, NULL, &eol_len, EVBUFFER_EOL_LF);
3775 if (ptr.pos == -1)
3776 return 0; /* No EOL found. */
3777 if ((size_t)ptr.pos+eol_len >= *data_len) {
3778 return -1; /* Too long */
3779 }
3780 *data_len = ptr.pos+eol_len;
3781 r = evbuffer_remove(input, data, ptr.pos+eol_len);
3782 tor_assert(r >= 0);
3783 data[ptr.pos+eol_len] = '\0';
3784 return 1;
3785 }) ELSE_IF_NO_BUFFEREVENT {
3786 return fetch_from_buf_line(conn->inbuf, data, data_len);
3787 }
3788 }
```
Following the size check on line 3777, *data_len is not altered and thus remains the same as before the invocation.
For incoming data larger than the initial buffer size (1024 bytes) and contains a linefeed character past 1024 bytes, this sends the control connection input routine into an infinite loop.
## Proof of concept
$ ./configure --enable-bufferevents && make -j4
Now start tor with this torrc:
ControlPort 9999
then in another terminal:
$ cat genpoc.py
import sys
sys.stdout.write((chr(0x63) * 2000) + chr(0x0A) )
$ python genpoc.py >poc
$ ncat localhost 9999 <poc
tor now hangs and has to be killed with force (kill -9 <pid>).
## Inter-protocol exploit
Since the only two prerequisites of the attack are:
- Input longer than 1024 bytes
- Input contains linefeed character after byte 1024
it's easy to think of other ways of making tor hang than manually creating a connection for this purpose.
```
$ cat genpoc2.py
print "curl http://localhost:9999/{}".format("x" * 1200)
$ python genpoc2.py >poc.sh
$ bash poc.sh
```
This also causes tor to hang, because curl is sending this to tor:
```
GET /xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx HTTP/1.1
User-Agent: curl/7.35.0
Host: localhost:9999
Accept: */*
```
which is data that adheres to the prerequisites.
Thus, a person running tor with the control server running locally while also using a regular browser can be DoSed via:
```
html
<img src='http://localhost:9999/xxxxxxxxxxxxxxxxxxx...'>
```
GuidoTor: 0.2.9.x-finalhttps://gitlab.torproject.org/legacy/trac/-/issues/18162Potential heap corruption in smartlist_add(), smartlist_insert()2020-06-13T14:53:49ZGeorge KadianakisPotential heap corruption in smartlist_add(), smartlist_insert()Here follows vulnerability report by `Guido Vranken` reported through the Tor bug bounty program.
The attack requires minimum 16GB of available memory on the victim's host, so it's quite hard to be exploited.
## Walkthrough of the vuln...Here follows vulnerability report by `Guido Vranken` reported through the Tor bug bounty program.
The attack requires minimum 16GB of available memory on the victim's host, so it's quite hard to be exploited.
## Walkthrough of the vulnerability
`smartlist_add` and `smartlist_insert` both invoke `smartlist_ensure_capacity` prior adding an element to the list in order to ensure that sufficient memory is available, to `exit()` if not enough memory is available and to detect requests for an invalid size:
```
static INLINE void
smartlist_ensure_capacity(smartlist_t *sl, int size)
{
#if SIZEOF_SIZE_T > SIZEOF_INT
#define MAX_CAPACITY (INT_MAX)
#else
#define MAX_CAPACITY (int)((SIZE_MAX / (sizeof(void*))))
#define ASSERT_CAPACITY
#endif
if (size > sl->capacity) {
int higher = sl->capacity;
if (PREDICT_UNLIKELY(size > MAX_CAPACITY/2)) {
#ifdef ASSERT_CAPACITY
/* We don't include this assertion when MAX_CAPACITY == INT_MAX,
* since int size; (size <= INT_MAX) makes analysis tools think we're
* doing something stupid. */
tor_assert(size <= MAX_CAPACITY);
#endif
higher = MAX_CAPACITY;
} else {
while (size > higher)
higher *= 2;
}
sl->capacity = higher;
sl->list = tor_reallocarray(sl->list, sizeof(void*),
((size_t)sl->capacity));
}
#undef ASSERT_CAPACITY
#undef MAX_CAPACITY
}
```
On a typical 64-bit system, `SIZEOF_INT` is 4 and `SIZEOF_SIZE_T` is 8. Consequently, `MAX_CAPACITY` is `INT_MAX`, which is 0x7FFFFFFF as can be seen in torint.h:
```
#ifndef INT_MAX
#if (SIZEOF_INT == 4)
#define INT_MAX 0x7fffffffL
#elif (SIZEOF_INT == 8)
#define INT_MAX 0x7fffffffffffffffL
#else
#error "Can't define INT_MAX"
#endif
#endif
```
So `MAX_CAPACITY` is 0x7FFFFFFF. Now assume that that many (0x7FFFFFFF) items have already been added to a smartlist via smartlist_add(sl, value).
smartlist_add() is:
```
void
smartlist_add(smartlist_t *sl, void *element)
{
smartlist_ensure_capacity(sl, sl->num_used+1);
sl->list[sl->num_used++] = element;
}
```
If `sl->num_used` is 0x7FFFFFFF prior to invoking `smartlist_add`, then the next `smartlist_add` is effectively:
```
void
smartlist_add(smartlist_t *sl, void *element)
{
smartlist_ensure_capacity(sl, -2147483648);
sl->list[2147483647] = element;
sl->num_used = -2147483648
}
```
This is the case since we are dealing with a signed 32 bit integer, and 2147483647 + 1 equals -2147483647.
All of the code in `smartlist_ensure_capacity` is wrapped inside the following `if` block:
```
if (size > sl->capacity) {
}
```
The expression -2147483648 > 2147483647 equals false, thus the code inside the block is not executed.
What actually causes the segmentation fault is that a negative 32 bit integer is used to compute a the location of array index on a 64 bit memory layout, ie., the next call to smartlist_add is effectively:
```
void
smartlist_add(smartlist_t *sl, void *element)
{
smartlist_ensure_capacity(sl, -2147483647); // Note that this is effective do-nothing code, as explained above
sl->list[-2147483648] = element;
sl->num_used = -2147483647
}
```
## Discussion
The requirement for 16 gigabytes of memory is considerable.
Triggering the vulnerability obviously also requires some code path which will invoke `smartlist_add` or `smartlist_insert` upon the same smartlist at the attacker's behest. Moreover, such a code path may have the side effect that it requires a separate allocation for each object that is added to the list; `smartlist_add` takes a pointer argument after all -- usually, but not always, this pointer refers to freshly allocated memory. Exceptions to this rule are static strings and pointers to a place in a large string or buffer that was already extant.
Once a vulnerable code path has been discovered, then it ultimately boils down to how much memory a user's machine is able to allocate in order to corrupt the heap.
Despite these constraints, smartlists form a considerable portion of the infrastructure of your code (I count some 380+ occurrences of `smartlist_add`/`smartlist_insert` in the .c files using grep, that is excluding the test/ directory) and as such it's probably wise to revise the checks in `smartlist_ensure_capacity`.Tor: 0.2.8.x-finalNick MathewsonNick Mathewson