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>
diff --git a/extras/hs-test/http_test.go b/extras/hs-test/http_test.go
index 94cb0ca..ba0fdb3 100644
--- a/extras/hs-test/http_test.go
+++ b/extras/hs-test/http_test.go
@@ -16,10 +16,12 @@
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 @@
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"))
+ 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{
- CheckRedirect: func(req *http.Request, via []*http.Request) error {
- return http.ErrUseLastResponse
- },
- Transport: transport,
- }
+ 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 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 @@
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 @@
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 @@
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 304dd4c..d250dc6 100644
--- a/extras/hs-test/utils.go
+++ b/extras/hs-test/utils.go
@@ -3,8 +3,10 @@
import (
"fmt"
"io"
+ "net/http"
"os"
"strings"
+ "time"
)
const networkTopologyDir string = "topo-network/"
@@ -78,3 +80,17 @@
_, 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 c9912dd..7fbefd6 100644
--- a/src/plugins/http/http.h
+++ b/src/plugins/http/http.h
@@ -277,6 +277,74 @@
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 040cdca..f433238 100644
--- a/src/plugins/http_static/static_server.c
+++ b/src/plugins/http_static/static_server.c
@@ -357,7 +357,7 @@
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 @@
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 @@
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 @@
hs->cache_pool_index = ce_index;
done:
-
+ vec_free (sanitized_path);
hs->content_type = type;
start_send_data (hs, sc);
if (!hs->data)