http_static: sanitize path before file read

Romove dot segments from requested target path before start reading file in file handler to prevent path traversal. Type: fix Change-Id: I3bdd3e9d7fffd33c9c8c608169c1dc73423b7078 Signed-off-by: Matus Fabian <matfabia@cisco.com>
author: Matus Fabian <matfabia@cisco.com> 2024-05-28 13:39:13 +0200
committer: Florin Coras <florin.coras@gmail.com> 2024-05-28 20:42:30 +0000
commit: 5409d330020b19ab909838e734e29ab71c36a14f (patch)
tree: d290fd755a494827be0bc3f31cbdb3887939cb0e
parent: a93c85a5793852b6edda20bc1100fa9fabd0eb29 (diff)
4 files changed, 122 insertions, 30 deletions
diff --git a/extras/hs-test/http_test.go b/extras/hs-test/http_test.go
index 94cb0cad064..ba0fdb31a1a 100644
--- a/extras/hs-test/http_test.go
+++ b/extras/hs-test/http_test.go
@@ -16,10 +16,12 @@ func init() {
 	registerNoTopoTests(NginxHttp3Test, NginxAsServerTest,
 		NginxPerfCpsTest, NginxPerfRpsTest, NginxPerfWrkTest, HeaderServerTest,
 		HttpStaticMovedTest, HttpStaticNotFoundTest, HttpCliMethodNotAllowedTest,
-		HttpCliBadRequestTest)
+		HttpCliBadRequestTest, HttpStaticPathTraversalTest)
 	registerNoTopoSoloTests(HttpStaticPromTest)
 }
 
+const wwwRootPath = "/tmp/www_root"
+
 func HttpTpsTest(s *NsSuite) {
 	iface := s.getInterfaceByName(clientInterface)
 	client_ip := iface.ip4AddressString()
@@ -108,21 +110,31 @@ func HttpStaticPromTest(s *NoTopoSuite) {
 	s.assertNil(err)
 }
 
+func HttpStaticPathTraversalTest(s *NoTopoSuite) {
+	vpp := s.getContainerByName("vpp").vppInstance
+	vpp.container.exec("mkdir -p " + wwwRootPath)
+	vpp.container.exec("mkdir -p " + "/tmp/secret_folder")
+	vpp.container.createFile("/tmp/secret_folder/secret_file.txt", "secret")
+	serverAddress := s.getInterfaceByName(tapInterfaceName).peer.ip4AddressString()
+	s.log(vpp.vppctl("http static server www-root " + wwwRootPath + " uri tcp://" + serverAddress + "/80 debug"))
+
+	client := newHttpClient()
+	req, err := http.NewRequest("GET", "http://"+serverAddress+":80/../secret_folder/secret_file.txt", nil)
+	s.assertNil(err, fmt.Sprint(err))
+	resp, err := client.Do(req)
+	s.assertNil(err, fmt.Sprint(err))
+	defer resp.Body.Close()
+	s.assertEqual(404, resp.StatusCode)
+}
+
 func HttpStaticMovedTest(s *NoTopoSuite) {
 	vpp := s.getContainerByName("vpp").vppInstance
-	vpp.container.exec("mkdir -p /tmp/tmp.aaa")
-	vpp.container.createFile("/tmp/tmp.aaa/index.html", "<http><body><p>Hello</p></body></http>")
+	vpp.container.exec("mkdir -p " + wwwRootPath + "/tmp.aaa")
+	vpp.container.createFile(wwwRootPath+"/tmp.aaa/index.html", "<http><body><p>Hello</p></body></http>")
 	serverAddress := s.getInterfaceByName(tapInterfaceName).peer.ip4AddressString()
-	s.log(vpp.vppctl("http static server www-root /tmp uri tcp://" + serverAddress + "/80 debug"))
-
-	transport := http.DefaultTransport
-	transport.(*http.Transport).Proxy = nil
-	client := &http.Client{
-		CheckRedirect: func(req *http.Request, via []*http.Request) error {
-			return http.ErrUseLastResponse
-		},
-		Transport: transport,
-	}
+	s.log(vpp.vppctl("http static server www-root " + wwwRootPath + " uri tcp://" + serverAddress + "/80 debug"))
+
+	client := newHttpClient()
 	req, err := http.NewRequest("GET", "http://"+serverAddress+":80/tmp.aaa", nil)
 	s.assertNil(err, fmt.Sprint(err))
 	resp, err := client.Do(req)
@@ -134,12 +146,11 @@ func HttpStaticMovedTest(s *NoTopoSuite) {
 
 func HttpStaticNotFoundTest(s *NoTopoSuite) {
 	vpp := s.getContainerByName("vpp").vppInstance
+	vpp.container.exec("mkdir -p " + wwwRootPath)
 	serverAddress := s.getInterfaceByName(tapInterfaceName).peer.ip4AddressString()
-	s.log(vpp.vppctl("http static server www-root /tmp uri tcp://" + serverAddress + "/80 debug"))
+	s.log(vpp.vppctl("http static server www-root " + wwwRootPath + " uri tcp://" + serverAddress + "/80 debug"))
 
-	transport := http.DefaultTransport
-	transport.(*http.Transport).Proxy = nil
-	client := &http.Client{Transport: transport}
+	client := newHttpClient()
 	req, err := http.NewRequest("GET", "http://"+serverAddress+":80/notfound.html", nil)
 	s.assertNil(err, fmt.Sprint(err))
 	resp, err := client.Do(req)
@@ -153,9 +164,7 @@ func HttpCliMethodNotAllowedTest(s *NoTopoSuite) {
 	serverAddress := s.getInterfaceByName(tapInterfaceName).peer.ip4AddressString()
 	vpp.vppctl("http cli server")
 
-	transport := http.DefaultTransport
-	transport.(*http.Transport).Proxy = nil
-	client := &http.Client{Transport: transport}
+	client := newHttpClient()
 	req, err := http.NewRequest("POST", "http://"+serverAddress+":80/test", nil)
 	s.assertNil(err, fmt.Sprint(err))
 	resp, err := client.Do(req)
@@ -171,9 +180,7 @@ func HttpCliBadRequestTest(s *NoTopoSuite) {
 	serverAddress := s.getInterfaceByName(tapInterfaceName).peer.ip4AddressString()
 	vpp.vppctl("http cli server")
 
-	transport := http.DefaultTransport
-	transport.(*http.Transport).Proxy = nil
-	client := &http.Client{Transport: transport}
+	client := newHttpClient()
 	req, err := http.NewRequest("GET", "http://"+serverAddress+":80", nil)
 	s.assertNil(err, fmt.Sprint(err))
 	resp, err := client.Do(req)
@@ -187,9 +194,7 @@ func HeaderServerTest(s *NoTopoSuite) {
 	serverAddress := s.getInterfaceByName(tapInterfaceName).peer.ip4AddressString()
 	vpp.vppctl("http cli server")
 
-	transport := http.DefaultTransport
-	transport.(*http.Transport).Proxy = nil
-	client := &http.Client{Transport: transport}
+	client := newHttpClient()
 	req, err := http.NewRequest("GET", "http://"+serverAddress+":80/show/version", nil)
 	s.assertNil(err, fmt.Sprint(err))
 	resp, err := client.Do(req)
diff --git a/extras/hs-test/utils.go b/extras/hs-test/utils.go
index 304dd4c241b..d250dc64519 100644
--- a/extras/hs-test/utils.go
+++ b/extras/hs-test/utils.go
@@ -3,8 +3,10 @@ package main
 import (
 	"fmt"
 	"io"
+	"net/http"
 	"os"
 	"strings"
+	"time"
 )
 
 const networkTopologyDir string = "topo-network/"
@@ -78,3 +80,17 @@ func (s *Stanza) saveToFile(fileName string) error {
 	_, err = io.Copy(fo, strings.NewReader(s.content))
 	return err
 }
+
+// newHttpClient creates [http.Client] with disabled proxy and redirects, it also sets timeout to 30seconds.
+func newHttpClient() *http.Client {
+	transport := http.DefaultTransport
+	transport.(*http.Transport).Proxy = nil
+	transport.(*http.Transport).DisableKeepAlives = true
+	client := &http.Client{
+		Transport: transport,
+		Timeout:   time.Second * 30,
+		CheckRedirect: func(req *http.Request, via []*http.Request) error {
+			return http.ErrUseLastResponse
+		}}
+	return client
+}
diff --git a/src/plugins/http/http.h b/src/plugins/http/http.h
index c9912dd6db8..7fbefd667f4 100644
--- a/src/plugins/http/http.h
+++ b/src/plugins/http/http.h
@@ -277,6 +277,74 @@ http_state_is_tx_valid (http_conn_t *hc)
 	  state == HTTP_STATE_WAIT_APP_METHOD);
 }
 
+/**
+ * Remove dot segments from path (RFC3986 section 5.2.4)
+ *
+ * @param path Path to sanitize.
+ *
+ * @return New vector with sanitized path.
+ *
+ * The caller is always responsible to free the returned vector.
+ */
+always_inline u8 *
+http_path_remove_dot_segments (u8 *path)
+{
+  u32 *segments = 0, *segments_len = 0, segment_len;
+  u8 *new_path = 0;
+  int i, ii;
+
+  if (!path)
+    return vec_new (u8, 0);
+
+  segments = vec_new (u32, 1);
+  /* first segment */
+  segments[0] = 0;
+  /* find all segments */
+  for (i = 1; i < (vec_len (path) - 1); i++)
+    {
+      if (path[i] == '/')
+	vec_add1 (segments, i + 1);
+    }
+  /* dummy tail */
+  vec_add1 (segments, vec_len (path));
+
+  /* scan all segments for "." and ".." */
+  segments_len = vec_new (u32, vec_len (segments) - 1);
+  for (i = 0; i < vec_len (segments_len); i++)
+    {
+      segment_len = segments[i + 1] - segments[i];
+      if (segment_len == 2 && path[segments[i]] == '.')
+	segment_len = 0;
+      else if (segment_len == 3 && path[segments[i]] == '.' &&
+	       path[segments[i] + 1] == '.')
+	{
+	  segment_len = 0;
+	  /* remove parent (if any) */
+	  for (ii = i - 1; ii >= 0; ii--)
+	    {
+	      if (segments_len[ii])
+		{
+		  segments_len[ii] = 0;
+		  break;
+		}
+	    }
+	}
+      segments_len[i] = segment_len;
+    }
+
+  /* we might end with empty path, so return at least empty vector */
+  new_path = vec_new (u8, 0);
+  /* append all valid segments */
+  for (i = 0; i < vec_len (segments_len); i++)
+    {
+      if (segments_len[i])
+	vec_add (new_path, path + segments[i], segments_len[i]);
+    }
+  vec_free (segments);
+  vec_free (segments_len);
+  return new_path;
+}
+
 #endif /* SRC_PLUGINS_HTTP_HTTP_H_ */
 
 /*
diff --git a/src/plugins/http_static/static_server.c b/src/plugins/http_static/static_server.c
index 040cdca9d7a..f433238dcb1 100644
--- a/src/plugins/http_static/static_server.c
+++ b/src/plugins/http_static/static_server.c
@@ -357,7 +357,7 @@ try_file_handler (hss_main_t *hsm, hss_session_t *hs, http_req_method_t rt,
 		  u8 *request)
 {
   http_status_code_t sc = HTTP_STATUS_OK;
-  u8 *path;
+  u8 *path, *sanitized_path;
   u32 ce_index;
   http_content_type_t type;
 
@@ -367,6 +367,9 @@ try_file_handler (hss_main_t *hsm, hss_session_t *hs, http_req_method_t rt,
 
   type = content_type_from_request (request);
 
+  /* Remove dot segments to prevent path traversal */
+  sanitized_path = http_path_remove_dot_segments (request);
+
   /*
    * Construct the file to open
    * Browsers are capable of sporadically including a leading '/'
@@ -374,9 +377,9 @@ try_file_handler (hss_main_t *hsm, hss_session_t *hs, http_req_method_t rt,
   if (!request)
     path = format (0, "%s%c", hsm->www_root, 0);
   else if (request[0] == '/')
-    path = format (0, "%s%s%c", hsm->www_root, request, 0);
+    path = format (0, "%s%s%c", hsm->www_root, sanitized_path, 0);
   else
-    path = format (0, "%s/%s%c", hsm->www_root, request, 0);
+    path = format (0, "%s/%s%c", hsm->www_root, sanitized_path, 0);
 
   if (hsm->debug_level > 0)
     clib_warning ("%s '%s'", (rt == HTTP_REQ_GET) ? "GET" : "POST", path);
@@ -419,7 +422,7 @@ try_file_handler (hss_main_t *hsm, hss_session_t *hs, http_req_method_t rt,
   hs->cache_pool_index = ce_index;
 
 done:
-
+  vec_free (sanitized_path);
   hs->content_type = type;
   start_send_data (hs, sc);
   if (!hs->data)
author	Matus Fabian <matfabia@cisco.com>	2024-05-28 13:39:13 +0200
committer	Florin Coras <florin.coras@gmail.com>	2024-05-28 20:42:30 +0000
commit	5409d330020b19ab909838e734e29ab71c36a14f (patch)
tree	d290fd755a494827be0bc3f31cbdb3887939cb0e
parent	a93c85a5793852b6edda20bc1100fa9fabd0eb29 (diff)