Fix single layer directory traversal in StaticFileRouter (#901)

The StaticFileRouter can access file in the immediate parent directory if the client sends a specially crafted, non RFC conforming HTTP 1.x request. By sending a HTTP request without a "/" predicating the path. The StaticFileRouter fails to detect directory traversal since it checks for "/../" in the path. This PR fixes the issue by detecting if there's potential for directory traversal. If true, we follow the path and detect if it reaches out of the document root at any point. Also added 2 new tests for edge cases in static file serving. (Not related to the bug). Co-authored-by: an-tao <[email protected]> Bug discovered by: oToToT <https://github.com/oToToT>
drogonframework · Jun 24, 2021 · b8d820f · b8d820f
1 parent 0656754
commit b8d820f
Show file tree

Hide file tree

Showing 2 changed files with 47 additions and 4 deletions.
diff --git a/lib/src/StaticFileRouter.cc b/lib/src/StaticFileRouter.cc
@@ -59,11 +59,28 @@ void StaticFileRouter::route(
     std::function<void(const HttpResponsePtr &)> &&callback)
 {
     const std::string &path = req->path();
-    if (path.find("/../") != std::string::npos)
+    if (path.find("..") != std::string::npos)
     {
-        // Downloading files from the parent folder is forbidden.
-        callback(app().getCustomErrorHandler()(k403Forbidden));
-        return;
+        auto directories = utils::splitString(path, "/");
+        int traversalDepth = 0;
+        for (const auto &dir : directories)
+        {
+            if (dir == "..")
+            {
+                traversalDepth--;
+            }
+            else if (dir != ".")
+            {
+                traversalDepth++;
+            }
+
+            if (traversalDepth < 0)
+            {
+                // Downloading files from the parent folder is forbidden.
+                callback(app().getCustomErrorHandler()(k403Forbidden));
+                return;
+            }
+        }
     }
 
     auto lPath = path;

diff --git a/lib/tests/integration_test/client/main.cc b/lib/tests/integration_test/client/main.cc
@@ -681,6 +681,32 @@ void doTest(const HttpClientPtr &client, std::shared_ptr<test::Case> TEST_CTX)
                             CHECK((*json)["P2"] == "test");
                         });
 
+    // Using .. to access a upper directory should be permitted as long as
+    // it never leaves the document root
+    req = HttpRequest::newHttpRequest();
+    req->setMethod(drogon::Get);
+    req->setPath("/a-directory/../index.html");
+    client->sendRequest(req,
+                        [req, TEST_CTX](ReqResult result,
+                                        const HttpResponsePtr &resp) {
+                            REQUIRE(result == ReqResult::Ok);
+                            CHECK(resp->getBody().length() == indexLen);
+                        });
+
+    // . (current directory) shall also be allowed
+    req = HttpRequest::newHttpRequest();
+    req->setMethod(drogon::Get);
+    req->setPath("/a-directory/./page.html");
+    client->sendRequest(req,
+                        [req, TEST_CTX, body](ReqResult result,
+                                              const HttpResponsePtr &resp) {
+                            REQUIRE(result == ReqResult::Ok);
+                            CHECK(resp->getBody().length() == indexImplicitLen);
+                            CHECK(std::equal(body->begin(),
+                                             body->end(),
+                                             resp->getBody().begin()));
+                        });
+
     // Test exception handling
     req = HttpRequest::newHttpRequest();
     req->setMethod(drogon::Get);