From 77ab447b7f00882de70c17eb9ab4269c459821de Mon Sep 17 00:00:00 2001
From: MarcoFalke <falke.marco@gmail.com>
Date: Fri, 10 Jul 2020 22:51:52 +0200
Subject: [PATCH] Merge #19140: tests: Avoid fuzzer-specific nullptr
 dereference in libevent when handling PROXY requests
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

20d31bdd92cc2ad9b8d26ed80da73bbcd6016144 tests: Avoid fuzzer-specific nullptr dereference in libevent when handling PROXY requests (practicalswift)

Pull request description:

  Avoid constructing requests that will be interpreted by libevent as PROXY requests to avoid triggering a `nullptr` dereference. Split out from #19074 as suggested by MarcoFalke.

  The dereference (`req->evcon->http_server`) takes place in `evhttp_parse_request_line` and is a consequence of our hacky but necessary use of the internal function `evhttp_parse_firstline_` in the `http_request` fuzzing harness.

  The suggested workaround is not aesthetically pleasing, but it successfully avoids the troublesome code path.

  `" http:// HTTP/1.1\n"` was a crashing input prior to this workaround.

  Before this PR:

  ```
  $ echo " http:// HTTP/1.1" > input
  $ src/test/fuzz/http_request input
  src/test/fuzz/http_request: Running 1 inputs 1 time(s) each.
  Running: input
  AddressSanitizer:DEADLYSIGNAL
  =================================================================
  ==27905==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000108 (pc 0x55a169b7e053 bp 0x7ffd452f1160 sp 0x7ffd452f10e0 T0)
  ==27905==The signal is caused by a READ memory access.
  ==27905==Hint: address points to the zero page.
      #0 0x55a169b7e053 in evhttp_parse_request_line depends/work/build/x86_64-pc-linux-gnu/libevent/2.1.11-stable-36daee64dc1/http.c:1883:37
      #1 0x55a169b7d9ae in evhttp_parse_firstline_ depends/work/build/x86_64-pc-linux-gnu/libevent/2.1.11-stable-36daee64dc1/http.c:2041:7
      #2 0x55a1687f624e in test_one_input(std::vector<unsigned char, std::allocator<unsigned char> > const&) src/test/fuzz/http_request.cpp:51:9
  …
  $ echo $?
  1
  ```

  After this PR:

  ```
  $ echo " http:// HTTP/1.1" > input
  $ src/test/fuzz/http_request input
  src/test/fuzz/http_request: Running 1 inputs 1 time(s) each.
  Running: input
  Executed input in 0 ms
  ***
  *** NOTE: fuzzing was not performed, you have only
  ***       executed the target code on a fixed set of inputs.
  ***
  $ echo $?
  0
  ```

  See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets).

  Happy fuzzing :)

Top commit has no ACKs.

Tree-SHA512: 7a6b68e52cbcd6c117487e74e47760fe03566bec09b0bb606afb3b652edfd22186ab8244e8e27c38cef3fd0d4a6c237fe68b2fd22e0970c349e4ab370cf3e304
---
 src/test/fuzz/http_request.cpp | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/src/test/fuzz/http_request.cpp b/src/test/fuzz/http_request.cpp
index 4104c5574d..e588757abc 100644
--- a/src/test/fuzz/http_request.cpp
+++ b/src/test/fuzz/http_request.cpp
@@ -7,6 +7,7 @@
 #include <test/fuzz/FuzzedDataProvider.h>
 #include <test/fuzz/fuzz.h>
 #include <test/fuzz/util.h>
+#include <util/strencodings.h>
 
 #include <event2/buffer.h>
 #include <event2/http.h>
@@ -31,7 +32,14 @@ void test_one_input(const std::vector<uint8_t>& buffer)
     assert(evbuf != nullptr);
     const std::vector<uint8_t> http_buffer = ConsumeRandomLengthByteVector(fuzzed_data_provider, 4096);
     evbuffer_add(evbuf, http_buffer.data(), http_buffer.size());
-    if (evhttp_parse_firstline_(evreq, evbuf) != 1 || evhttp_parse_headers_(evreq, evbuf) != 1) {
+    // Avoid constructing requests that will be interpreted by libevent as PROXY requests to avoid triggering
+    // a nullptr dereference. The dereference (req->evcon->http_server) takes place in evhttp_parse_request_line
+    // and is a consequence of our hacky but necessary use of the internal function evhttp_parse_firstline_ in
+    // this fuzzing harness. The workaround is not aesthetically pleasing, but it successfully avoids the troublesome
+    // code path. " http:// HTTP/1.1\n" was a crashing input prior to this workaround.
+    const std::string http_buffer_str = ToLower({http_buffer.begin(), http_buffer.end()});
+    if (http_buffer_str.find(" http://") != std::string::npos || http_buffer_str.find(" https://") != std::string::npos ||
+        evhttp_parse_firstline_(evreq, evbuf) != 1 || evhttp_parse_headers_(evreq, evbuf) != 1) {
         evbuffer_free(evbuf);
         evhttp_request_free(evreq);
         return;