From b8d820fc2adb677e8d891c4daf5c5ce250998ab8 Mon Sep 17 00:00:00 2001
From: Martin Chang <marty1885@users.noreply.github.com>
Date: Thu, 24 Jun 2021 13:04:19 +0800
Subject: [PATCH] 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 <antao2002@gmail.com>
Bug discovered by: oToToT <https://github.com/oToToT>
---
 lib/src/StaticFileRouter.cc               | 25 ++++++++++++++++++----
 lib/tests/integration_test/client/main.cc | 26 +++++++++++++++++++++++
 2 files changed, 47 insertions(+), 4 deletions(-)

diff --git a/lib/src/StaticFileRouter.cc b/lib/src/StaticFileRouter.cc
index 55771218ab..8f06825932 100644
--- 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
index e150854887..7d96481ac3 100644
--- 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);