Code Review / vpp.git / commitdiff
? search:
summary | shortlog | log | commit | commitdiff | review | tree
raw | patch | inline | side by side (parent: 4306a3e)
http: status line parsing fix 76/41476/2
authorMatus Fabian <matfabia@cisco.com>
Fri, 23 Aug 2024 15:35:50 +0000 (17:35 +0200)
committerFlorin Coras <florin.coras@gmail.com>
Fri, 23 Aug 2024 17:59:46 +0000 (17:59 +0000)
Request line must only start with method name and server should
ignore at least one empty line (CRLF) received prior to the
request-line.

Type: fix
Change-Id: Ifebd992dc4c13df1a3fabfcdef9e7ee644150a21
Signed-off-by: Matus Fabian <matfabia@cisco.com>
extras/hs-test/http_test.go patch | blob | history
src/plugins/http/http.c patch | blob | history

diff --git a/extras/hs-test/http_test.go b/extras/hs-test/http_test.go
index 872f4c2..9fc426a 100644 (file)
--- a/extras/hs-test/http_test.go
+++ b/extras/hs-test/http_test.go
@@ -29,7 +29,7 @@ func init() {
                HttpStaticMacTimeTest, HttpStaticBuildInUrlGetVersionVerboseTest, HttpVersionNotSupportedTest,
                HttpInvalidContentLengthTest, HttpInvalidTargetSyntaxTest, HttpStaticPathTraversalTest, HttpUriDecodeTest,
                HttpHeadersTest, HttpStaticFileHandlerTest, HttpStaticFileHandlerDefaultMaxAgeTest, HttpClientTest, HttpClientErrRespTest, HttpClientPostFormTest,
-               HttpClientPostFileTest, HttpClientPostFilePtrTest, AuthorityFormTargetTest)
+               HttpClientPostFileTest, HttpClientPostFilePtrTest, AuthorityFormTargetTest, HttpRequestLineTest)
        RegisterNoTopoSoloTests(HttpStaticPromTest, HttpTpsTest, HttpTpsInterruptModeTest, PromConcurrentConnectionsTest,
                PromMemLeakTest, HttpClientPostMemLeakTest, HttpInvalidClientRequestMemLeakTest)
 }
@@ -867,7 +867,19 @@ func HttpInvalidRequestLineTest(s *NoTopoSuite) {
        serverAddress := s.GetInterfaceByName(TapInterfaceName).Peer.Ip4AddressString()
        vpp.Vppctl("http cli server")
 
-       resp, err := TcpSendReceive(serverAddress+":80", "GET / HTTP/1.1")
+       resp, err := TcpSendReceive(serverAddress+":80", " GET / HTTP/1.1")
+       s.AssertNil(err, fmt.Sprint(err))
+       s.AssertContains(resp, "HTTP/1.1 400 Bad Request", "invalid request line start not allowed")
+
+       resp, err = TcpSendReceive(serverAddress+":80", "\rGET / HTTP/1.1")
+       s.AssertNil(err, fmt.Sprint(err))
+       s.AssertContains(resp, "HTTP/1.1 400 Bad Request", "invalid request line start not allowed")
+
+       resp, err = TcpSendReceive(serverAddress+":80", "\nGET / HTTP/1.1")
+       s.AssertNil(err, fmt.Sprint(err))
+       s.AssertContains(resp, "HTTP/1.1 400 Bad Request", "invalid request line start not allowed")
+
+       resp, err = TcpSendReceive(serverAddress+":80", "GET / HTTP/1.1")
        s.AssertNil(err, fmt.Sprint(err))
        s.AssertContains(resp, "HTTP/1.1 400 Bad Request", "invalid framing not allowed")
 
@@ -896,6 +908,17 @@ func HttpInvalidRequestLineTest(s *NoTopoSuite) {
        s.AssertContains(resp, "HTTP/1.1 400 Bad Request", "'HTTP1.1' invalid http version not allowed")
 }
 
+func HttpRequestLineTest(s *NoTopoSuite) {
+       vpp := s.GetContainerByName("vpp").VppInstance
+       serverAddress := s.GetInterfaceByName(TapInterfaceName).Peer.Ip4AddressString()
+       vpp.Vppctl("http cli server")
+
+       resp, err := TcpSendReceive(serverAddress+":80", "\r\nGET /show/version HTTP/1.1\r\nHost:"+serverAddress+":80\r\nUser-Agent:test\r\n\r\n")
+       s.AssertNil(err, fmt.Sprint(err))
+       s.AssertContains(resp, "HTTP/1.1 200 OK")
+       s.AssertContains(resp, "<html>", "html content not found")
+}
+
 func HttpInvalidTargetSyntaxTest(s *NoTopoSuite) {
        vpp := s.GetContainerByName("vpp").VppInstance
        serverAddress := s.GetInterfaceByName(TapInterfaceName).Peer.Ip4AddressString()
diff --git a/src/plugins/http/http.c b/src/plugins/http/http.c
index f4b330a..b143893 100644 (file)
--- a/src/plugins/http/http.c
+++ b/src/plugins/http/http.c
@@ -587,10 +587,10 @@ static int
 http_parse_request_line (http_conn_t *hc, http_status_code_t *ec)
 {
   int i, target_len;
-  u32 next_line_offset;
+  u32 next_line_offset, method_offset;
 
   /* request-line = method SP request-target SP HTTP-version CRLF */
-  i = v_find_index (hc->rx_buf, 0, 0, "\r\n");
+  i = v_find_index (hc->rx_buf, 8, 0, "\r\n");
   if (i < 0)
     {
       clib_warning ("request line incomplete");
@@ -609,24 +609,40 @@ http_parse_request_line (http_conn_t *hc, http_status_code_t *ec)
       return -1;
     }
 
+  /*
+   * RFC9112 2.2:
+   * In the interest of robustness, a server that is expecting to receive and
+   * parse a request-line SHOULD ignore at least one empty line (CRLF)
+   * received prior to the request-line.
+   */
+  method_offset = hc->rx_buf[0] == '\r' && hc->rx_buf[1] == '\n' ? 2 : 0;
   /* parse method */
-  if ((i = v_find_index (hc->rx_buf, 0, next_line_offset, "GET ")) >= 0)
+  if (!memcmp (hc->rx_buf + method_offset, "GET ", 4))
     {
       HTTP_DBG (0, "GET method");
       hc->method = HTTP_REQ_GET;
-      hc->target_path_offset = i + 4;
+      hc->target_path_offset = method_offset + 4;
     }
-  else if ((i = v_find_index (hc->rx_buf, 0, next_line_offset, "POST ")) >= 0)
+  else if (!memcmp (hc->rx_buf + method_offset, "POST ", 5))
     {
       HTTP_DBG (0, "POST method");
       hc->method = HTTP_REQ_POST;
-      hc->target_path_offset = i + 5;
+      hc->target_path_offset = method_offset + 5;
     }
   else
     {
-      clib_warning ("method not implemented: %8v", hc->rx_buf);
-      *ec = HTTP_STATUS_NOT_IMPLEMENTED;
-      return -1;
+      if (hc->rx_buf[method_offset] - 'A' <= 'Z' - hc->rx_buf[method_offset])
+       {
+         clib_warning ("not method name: %8v", hc->rx_buf);
+         *ec = HTTP_STATUS_BAD_REQUEST;
+         return -1;
+       }
+      else
+       {
+         clib_warning ("method not implemented: %8v", hc->rx_buf);
+         *ec = HTTP_STATUS_NOT_IMPLEMENTED;
+         return -1;
+       }
     }
 
   /* find version */
Vector Packet Processing