Explorar el Código

Reject non-token methods in write_request_line

write_request_line checked the request target for CR/LF but concatenated
the method verbatim. A method carrying CR/LF could smuggle a whole
request ahead of the real one, and the client would take the smuggled
request's response as its own. A method with a space or an empty method
put a malformed request line on the wire.

Require the method to be a token (RFC 9110 Section 9.1) before anything
is written. All three callers (the buffered client path, open_stream and
the WebSocket handshake) go through this function and fail with
Error::Write, as they already do for a rejected target.

Claude-Session: https://claude.ai/code/session_01NTDesJQTQPEuu4o4XCu69g
yhirose hace 2 semanas
padre
commit
ad88645a83
Se han modificado 2 ficheros con 98 adiciones y 8 borrados
  1. 11 8
      httplib.h
  2. 87 0
      test/test.cc

+ 11 - 8
httplib.h

@@ -8517,11 +8517,13 @@ bool read_content(Stream &strm, T &x, size_t payload_max_length, int &status,
 
 inline ssize_t write_request_line(Stream &strm, const std::string &method,
                                   const std::string &path) {
-  // A request target must not carry CR/LF (or other control octets); otherwise
-  // a value smuggled into it splits the request line and injects headers or a
-  // whole request. The same field-value check already guards header values in
-  // check_and_write_headers and the request target in
-  // perform_websocket_handshake; apply it here too.
+  // Neither the method nor the request target may carry CR/LF (or other
+  // control octets); otherwise a value smuggled into either splits the request
+  // line and injects headers or a whole request. The method must be a token
+  // (RFC 9110 Section 9.1), which also rejects an empty method and embedded
+  // spaces. The target gets the same field-value check that already guards
+  // header values in check_and_write_headers.
+  if (!fields::is_token(method)) { return -1; }
   if (!fields::is_field_value(path)) { return -1; }
 
   std::string s = method;
@@ -15768,9 +15770,10 @@ inline bool ClientImpl::write_request(Stream &strm, Request &req,
 
     // Write request line and headers
     if (detail::write_request_line(bstrm, req.method, path_with_query) < 0) {
-      // A rejected target (e.g. CR/LF smuggled in via a decoded redirect
-      // Location under set_path_encode(false)) must fail the request cleanly
-      // instead of emitting a request-line-less, header-injecting request.
+      // A rejected method (not a token, e.g. carrying CR/LF) or target (e.g.
+      // CR/LF smuggled in via a decoded redirect Location under
+      // set_path_encode(false)) must fail the request cleanly instead of
+      // emitting a request-line-less, header-injecting request.
       error = Error::Write;
       output_error_log(error, &req);
       return false;

+ 87 - 0
test/test.cc

@@ -10141,6 +10141,93 @@ TEST(RequestLineInjectionTest, ClientRejectsCRLFTargetEndToEnd) {
   }
 }
 
+TEST(RequestLineInjectionTest, RejectsNonTokenMethod) {
+  // Methods that are tokens (RFC 9110 Section 9.1) are written verbatim,
+  // including extension methods.
+  const std::string good_methods[] = {"GET", "PROPFIND", "M-SEARCH"};
+  for (const auto &method : good_methods) {
+    detail::BufferStream strm;
+    auto n = detail::write_request_line(strm, method, "/");
+    EXPECT_GT(n, 0);
+    EXPECT_EQ(method + " / HTTP/1.1\r\n", strm.get_buffer());
+  }
+
+  // A method carrying CR/LF would split the request line and smuggle a whole
+  // request ahead of the real one. A space or an empty method corrupts the
+  // request line. All must be rejected before anything reaches the wire.
+  const std::string evil_methods[] = {
+      "GET /smuggled HTTP/1.1\r\nHost: x\r\n\r\nGET",
+      "GET\r\nInjected: pwned",
+      "GET\r",
+      "GET\n",
+      "GE T",
+      "",
+  };
+  for (const auto &evil : evil_methods) {
+    detail::BufferStream strm;
+    auto n = detail::write_request_line(strm, evil, "/");
+    EXPECT_LT(n, 0);
+    EXPECT_TRUE(strm.get_buffer().empty());
+  }
+}
+
+TEST(RequestLineInjectionTest, ClientRejectsNonTokenMethodEndToEnd) {
+  // End-to-end counterpart to RejectsNonTokenMethod: a smuggling method passed
+  // through Client::send must fail with Error::Write and no request, neither
+  // the smuggled one nor the real one, may reach the server.
+  Server svr;
+
+  std::atomic<int> request_count(0);
+  svr.set_pre_routing_handler([&](const Request &, Response &res) {
+    request_count++;
+    res.status = StatusCode::OK_200;
+    return Server::HandlerResponse::Handled;
+  });
+
+  auto port = svr.bind_to_any_port(HOST);
+  auto thread = std::thread([&]() { svr.listen_after_bind(); });
+  auto se = detail::scope_exit([&] {
+    svr.stop();
+    thread.join();
+    ASSERT_FALSE(svr.is_running());
+  });
+
+  svr.wait_until_ready();
+
+  {
+    Client cli(HOST, port);
+    // Nothing is written, so shorten the read timeout the connection would
+    // otherwise sit in.
+    cli.set_read_timeout(1, 0);
+
+    const std::string evil_methods[] = {
+        "GET /smuggled HTTP/1.1\r\nHost: x\r\n\r\nGET",
+        "GE T",
+        "",
+    };
+    for (const auto &evil : evil_methods) {
+      Request req;
+      req.method = evil;
+      req.path = "/";
+      auto res = cli.send(req);
+      EXPECT_FALSE(res);
+      EXPECT_EQ(Error::Write, res.error());
+    }
+
+    auto handle =
+        cli.open_stream("GET /smuggled HTTP/1.1\r\nHost: x\r\n\r\nGET", "/");
+    EXPECT_FALSE(handle.is_valid());
+    EXPECT_EQ(Error::Write, handle.error);
+
+    // A valid request on the same client still goes through.
+    auto res = cli.Get("/");
+    ASSERT_TRUE(res);
+    EXPECT_EQ(StatusCode::OK_200, res->status);
+  }
+
+  EXPECT_EQ(1, request_count.load());
+}
+
 // Sends a raw request and verifies that there isn't a crash or exception.
 static void test_raw_request(const std::string &req,
                              std::string *out = nullptr) {