nihilist/tor
1
0
Fork 0
mirror of https://gitlab.torproject.org/tpo/core/tor.git synced 2024-11-30 23:53:32 +01:00
Code Issues Packages Projects Releases Wiki Activity

Add more error checks to socks parsing code

Browse Source 
Suggested by Linus to avoid uninitialized reads or infinite loops if
it turns out our code is buggier than we had thought.
This commit is contained in:
Nick Mathewson 2011-07-12 10:51:31 -04:00
parent 4ce302c8e5
commit 16c5a62a66
1 changed files with 20 additions and 15 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

33
src/or/buffers.c
View File

@ -1544,7 +1544,8 @@ fetch_from_buf_socks(buf_t *buf, socks_request_t *req,
else if (n_drain > 0) else if (n_drain > 0)
buf_remove_from_front(buf, n_drain); buf_remove_from_front(buf, n_drain);
} while (res == 0 && buf->head && want_length < buf->datalen); } while (res == 0 && buf->head && want_length < buf->datalen &&
buf->datalen >= 2);
return res; return res;
} }
@ -1595,12 +1596,14 @@ fetch_from_evbuffer_socks(struct evbuffer *buf, socks_request_t *req,
* will need more data than we currently have. */ * will need more data than we currently have. */
/* Loop while we have more data that we haven't given parse_socks() yet. */ /* Loop while we have more data that we haven't given parse_socks() yet. */
while (evbuffer_get_length(buf) > datalen) { do {
int free_data = 0; int free_data = 0;
const size_t last_wanted = want_length;
n_drain = 0; n_drain = 0;
data = NULL; data = NULL;
datalen = inspect_evbuffer(buf, &data, want_length, &free_data, NULL); datalen = inspect_evbuffer(buf, &data, want_length, &free_data, NULL);
want_length = 0;
res = parse_socks(data, datalen, req, log_sockstype, res = parse_socks(data, datalen, req, log_sockstype,
safe_socks, &n_drain, &want_length); safe_socks, &n_drain, &want_length);
@ -1612,21 +1615,17 @@ fetch_from_evbuffer_socks(struct evbuffer *buf, socks_request_t *req,
else if (n_drain > 0) else if (n_drain > 0)
evbuffer_drain(buf, n_drain); evbuffer_drain(buf, n_drain);
if (res) /* If res is nonzero, parse_socks() made up its mind. */ if (res == 0 && n_drain == 0 && want_length <= last_wanted) {
return res; /* If we drained nothing, and we didn't ask for more than last time,
* we're stuck in a loop. That's bad. It shouldn't be possible, but
/* If parse_socks says that we want less data than we actually tried to * let's make sure. */
give it, we've got some kind of weird situation; just exit the loop for log_warn(LD_BUG, "We seem to be caught in a parse loop; breaking out");
now.
*/
if (want_length <= datalen)
break; break;
/* Otherwise, it wants more data than we gave it. If we can provide more
* data than we gave it, we'll try to do so in the next iteration of the
* loop. If we can't, the while loop will exit. It's okay if it asked for
* more than we have total; maybe it doesn't really need so much. */
} }
buflen = evbuffer_get_length(buf);
} while (res == 0 && want_length <= buflen && buflen >= 2);
return res; return res;
} }
#endif #endif
@ -1652,6 +1651,12 @@ parse_socks(const char *data, size_t datalen, socks_request_t *req,
unsigned char usernamelen, passlen; unsigned char usernamelen, passlen;
struct in_addr in; struct in_addr in;
if (datalen < 2) {
/* We always need at least 2 bytes. */
*want_length_out = 2;
return 0;
}
if (req->socks_version == 5 && !req->got_auth) { if (req->socks_version == 5 && !req->got_auth) {
/* See if we have received authentication. Strictly speaking, we should /* See if we have received authentication. Strictly speaking, we should
also check whether we actually negotiated username/password also check whether we actually negotiated username/password