[tor-commits] [tor/master] Add more error checks to socks parsing code

[nickm at torproject.org]

commit 16c5a62a66098274dde726c8e02110238866fe0b
Author: Nick Mathewson <nickm at torproject.org>
Date:   Tue Jul 12 10:51:31 2011 -0400

    Add more error checks to socks parsing code
    
    Suggested by Linus to avoid uninitialized reads or infinite loops if
    it turns out our code is buggier than we had thought.
---
 src/or/buffers.c |   35 ++++++++++++++++++++---------------
 1 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/src/or/buffers.c b/src/or/buffers.c
index 1310b42..b7567bf 100644
--- a/src/or/buffers.c
+++ b/src/or/buffers.c
@@ -1544,7 +1544,8 @@ fetch_from_buf_socks(buf_t *buf, socks_request_t *req,
     else if (n_drain > 0)
       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;
 }
@@ -1595,12 +1596,14 @@ fetch_from_evbuffer_socks(struct evbuffer *buf, socks_request_t *req,
    * will need more data than we currently have. */
 
   /* 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;
+    const size_t last_wanted = want_length;
     n_drain = 0;
     data = NULL;
     datalen = inspect_evbuffer(buf, &data, want_length, &free_data, NULL);
 
+    want_length = 0;
     res = parse_socks(data, datalen, req, log_sockstype,
                       safe_socks, &n_drain, &want_length);
 
@@ -1612,20 +1615,16 @@ fetch_from_evbuffer_socks(struct evbuffer *buf, socks_request_t *req,
     else if (n_drain > 0)
       evbuffer_drain(buf, n_drain);
 
-    if (res) /* If res is nonzero, parse_socks() made up its mind. */
-      return res;
-
-    /* If parse_socks says that we want less data than we actually tried to
-       give it, we've got some kind of weird situation; just exit the loop for
-       now.
-    */
-    if (want_length <= datalen)
+    if (res == 0 && n_drain == 0 && want_length <= last_wanted) {
+      /* 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
+       * let's make sure. */
+      log_warn(LD_BUG, "We seem to be caught in a parse loop; breaking out");
       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;
 }
@@ -1652,6 +1651,12 @@ parse_socks(const char *data, size_t datalen, socks_request_t *req,
   unsigned char usernamelen, passlen;
   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) {
     /* See if we have received authentication.  Strictly speaking, we should
        also check whether we actually negotiated username/password