Fix a theoretical bug where a short request packet yields an oob read

2 files changed, 19 insertions, 3 deletions

diff --git a/src/ot_http.c b/src/ot_http.c
index fc49333..1b5281c 100644
--- a/src/ot_http.c
+++ b/src/ot_http.c
@@ -762,8 +762,8 @@ ssize_t http_handle_request(const int64_t sock, struct ot_workstruct *ws) {
     }
   }
 
-  /* This one implicitely tests strlen < 5, too -- remember, it is \n terminated */
-  if (memcmp(read_ptr, "GET /", 5))
+  /* The request need not be NUL terminated and can be shorter than the literal */
+  if (ws->request_size < 5 || memcmp(read_ptr, "GET /", 5))
     HTTPERROR_400;
 
   /* Skip leading '/' */
@@ -786,7 +786,7 @@ ssize_t http_handle_request(const int64_t sock, struct ot_workstruct *ws) {
   if ((*write_ptr == 'a') || (*write_ptr == '?'))
     http_handle_announce(sock, ws, read_ptr);
 #ifdef WANT_FULLSCRAPE
-  else if (!memcmp(write_ptr, "scrape HTTP/", 12))
+  else if (ws->request + ws->request_size - write_ptr >= 12 && !memcmp(write_ptr, "scrape HTTP/", 12))
     http_handle_fullscrape(sock, ws);
 #endif
   /* This is the hardcore match for scrape */
diff --git a/tests/integration/test_keepalive.py b/tests/integration/test_keepalive.py
index 2c724fc..f9aa468 100644
--- a/tests/integration/test_keepalive.py
+++ b/tests/integration/test_keepalive.py
@@ -107,6 +107,22 @@ class KeepAlive(TrackerTestCase):
         finally:
             c.close()
 
+    def test_a_tiny_split_request_does_not_overread(self):
+        """A request that arrives in pieces and completes as a buffer shorter
+        than the dispatch literals (\"GET /\" etc.) used to read past the
+        exactly sized buffer. It must be rejected without a sanitizer hit."""
+        c = self.tracker.connect()
+        try:
+            c.sock.sendall(b"\r\n")
+            time.sleep(0.05)
+            c.sock.sendall(b"\r\n")          # completes as a 4-byte buffer
+            time.sleep(0.1)
+        finally:
+            c.close()
+        # the tracker must still answer; tearDown's stop() surfaces any sanitizer abort
+        answer = self.tracker.announce(self.hash_for("after-tiny"), port=6881, left=0)
+        self.assertNotIn(b"failure reason", answer, "the tracker stopped serving after a tiny request")
+
     def test_a_split_request_answered_by_a_worker_does_not_spin(self):
         """A reply that a worker produces does not go through the send path,
         so the request stays in the buffer the read loop is reading. The loop