commit 596b203317f16ab624d374f811426a516912db25
author Ed Tanous <edtanous@google.com> Mon Sep 13 10:32:22 2021 -0700
committer Ed Tanous <ed@tanous.net> Tue Sep 14 23:41:56 2021 +0000
tree 31de3874779b00640a678169e0499e3d0a35447f
parent 9a69d5a5efde522fccac2fcbf50052a73aaab671

Fill in request earlier

Because http connection can possibly fail on a bad host header, we need
to fill in the request object earlier in the flow.  This has the
potential to lead to use after free bugs, although in practice, we have
null checks, so it's a lower likelihood.

Tested:
Ran
redfishtool -S Always -A Session -u root -p 0penBmc -vvvvvvvvv -r 192.168.7.2 raw GET /redfish/v1/Managers/bmc

and verified it returned a 200 and the managers data structure.

Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I789d50f67dff14b769ccefacf55530b0b7c5c7cd

http/http_connection.hpp

diff --git a/http/http_connection.hpp b/http/http_connection.hpp
index 815bea5..ae0477a 100644
--- a/http/http_connection.hpp
+++ b/http/http_connection.hpp
@@ -314,13 +314,16 @@
     {
         cancelDeadlineTimer();
 
+        crow::Request& thisReq = req.emplace(parser->release());
+        thisReq.session = userSession;
+
         // Fetch the client IP address
         readClientIp();
 
         // Check for HTTP version 1.1.
-        if (parser->get().version() == 11)
+        if (thisReq.version() == 11)
         {
-            if (parser->get()[boost::beast::http::field::host].empty())
+            if (thisReq.getHeaderValue(boost::beast::http::field::host).empty())
             {
                 res.result(boost::beast::http::status::bad_request);
                 completeRequest();
@@ -329,18 +332,14 @@
         }
 
         BMCWEB_LOG_INFO << "Request: "
-                        << " " << this << " HTTP/"
-                        << parser->get().version() / 10 << "."
-                        << parser->get().version() % 10 << ' '
-                        << parser->get().method_string() << " "
-                        << parser->get().target() << " " << req->ipAddress;
-        req.emplace(parser->release());
-        req->session = userSession;
+                        << " " << this << " HTTP/" << thisReq.version() / 10
+                        << "." << thisReq.version() % 10 << ' '
+                        << thisReq.methodString() << " " << thisReq.target()
+                        << " " << thisReq.ipAddress;
         try
         {
-            // causes life time issue
-            req->urlView = boost::urls::url_view(req->target());
-            req->url = req->urlView.encoded_path();
+            thisReq.urlView = boost::urls::url_view(thisReq.target());
+            thisReq.url = thisReq.urlView.encoded_path();
         }
         catch (std::exception& p)
         {
@@ -350,7 +349,7 @@
         res.setCompleteRequestHandler(nullptr);
         res.isAliveHelper = [this]() -> bool { return isAlive(); };
 
-        req->ioService = static_cast<decltype(req->ioService)>(
+        thisReq.ioService = static_cast<decltype(thisReq.ioService)>(
             &adaptor.get_executor().context());
 
         if (res.completed)
@@ -363,19 +362,19 @@
                               [self] { self->completeRequest(); });
         });
 
-        if (req->isUpgrade() &&
+        if (thisReq.isUpgrade() &&
             boost::iequals(
-                req->getHeaderValue(boost::beast::http::field::upgrade),
+                thisReq.getHeaderValue(boost::beast::http::field::upgrade),
                 "websocket"))
         {
-            handler->handleUpgrade(*req, res, std::move(adaptor));
+            handler->handleUpgrade(thisReq, res, std::move(adaptor));
             // delete lambda with self shared_ptr
             // to enable connection destruction
             res.setCompleteRequestHandler(nullptr);
             return;
         }
         auto asyncResp = std::make_shared<bmcweb::AsyncResp>(res);
-        handler->handle(*req, asyncResp);
+        handler->handle(thisReq, asyncResp);
     }
 
     bool isAlive()