Merge pull request #1065 from Tachi107/swagger-vuln-fix

fix(swagger/security): ensure that the requested file is in the UI directory

src/common/description.cc

@@ -15,6 +15,7 @@
 #include <pistache/http_header.h>
 
 #include <algorithm>
+#include <filesystem>
 #include <sstream>
 
 namespace Pistache::Rest
@@ -144,7 +145,7 @@ namespace Pistache::Rest
         {
             auto group = paths(name);
             auto it    = std::find_if(std::begin(group), std::end(group),
-                                   [&](const Path& p) { return p.method == method; });
+                                      [&](const Path& p) { return p.method == method; });
             return it != std::end(group);
         }
 
@@ -167,7 +168,7 @@ namespace Pistache::Rest
         {
             auto group = paths(name);
             auto it    = std::find_if(std::begin(group), std::end(group),
-                                   [&](const Path& p) { return p.method == method; });
+                                      [&](const Path& p) { return p.method == method; });
 
             if (it != std::end(group))
             {
@@ -381,7 +382,7 @@ namespace Pistache::Rest
 
     Swagger& Swagger::uiDirectory(std::string dir)
     {
-        uiDirectory_ = std::move(dir);
+        uiDirectory_ = std::filesystem::canonical(dir).string();
         return *this;
     }
 
@@ -405,9 +406,9 @@ namespace Pistache::Rest
             const auto& res = req.resource();
 
             /*
-     * @Export might be useful for routing also. Make it public or merge it with
-     * the Fragment class
-     */
+             * @Export might be useful for routing also. Make it public or merge it with
+             * the Fragment class
+             */
 
             struct Path
             {
@@ -494,9 +495,21 @@ namespace Pistache::Rest
             else if (ui.isPrefix(req))
             {
                 auto file = ui.stripPrefix(req);
-                auto path = uiDir.join(file);
-                Http::serveFile(response, path);
-                return Route::Result::Ok;
+                auto path = std::filesystem::canonical(uiDir.join(file)).string();
+
+                // Check if the requested file is contained in the uiDirectory
+                // to prevent path traversal vulnerabilities.
+                // In C++20, use std::string::starts_with()
+                if (path.rfind(uiDirectory_, 0) == 0)
+                {
+                    Http::serveFile(response, path);
+                    return Route::Result::Ok;
+                }
+                else
+                {
+                    response.send(Http::Code::Not_Found);
+                    return Route::Result::Failure;
+                }
             }
 
             else if (res == apiPath_)

tests/meson.build

@@ -36,7 +36,7 @@ pistache_test_files = [
 
 network_tests = ['net_test']
 
-flaky_tests = ['rest_swagger_server_test']
+flaky_tests = []
 
 if get_option('PISTACHE_USE_SSL')
 	pistache_test_files += 'https_server_test'

tests/rest_swagger_server_test.cc

@@ -23,7 +23,7 @@ class SwaggerEndpoint
 {
 public:
     SwaggerEndpoint(Address addr)
-        : httpEndpoint(std::make_shared<Http::Endpoint>(addr))
+        : httpEndpoint(make_shared<Http::Endpoint>(addr))
         , desc("SwaggerEndpoint API", "1.0")
     { }
 
@@ -59,46 +59,50 @@ class SwaggerEndpoint
     Rest::Router router;
 };
 
-TEST(rest_server_test, basic_test)
+TEST(rest_swagger_server_test, basic_test)
 {
     filesystem::create_directory("assets");
 
-    ofstream goodFile("assets/good.txt");
-    goodFile << "good";
-    goodFile.close();
+    ofstream("assets/good.txt") << "good";
 
-    ofstream badFile("bad.txt");
-    badFile << "bad";
-    badFile.close();
+    ofstream("bad.txt") << "bad";
 
-    Address addr(Ipv4::any(), Port(0));
+    Address addr(Ipv4::loopback(), Port(0));
     SwaggerEndpoint swagger(addr);
 
     swagger.init();
-    std::thread t(std::bind(&SwaggerEndpoint::start, swagger));
+    thread t([&swagger]() {
+        while (swagger.getPort() == 0)
+        {
+            this_thread::yield();
+        }
+
+        Port port = swagger.getPort();
+
+        cout << "CWD = " << filesystem::current_path() << endl;
+        cout << "Port = " << port << endl;
+
+        httplib::Client client("localhost", port);
+
+        // Test if we have access to files inside the UI folder.
+        auto goodRes = client.Get("/doc/good.txt");
+        // Attempt to read file outside of the UI directory should fail even if
+        // the file exists.
+        client.set_connection_timeout(1000);
+        client.set_read_timeout(1000);
+        auto badRes = client.Get("/doc/../bad.txt");
+        // Ensure the server is shut down before calling asserts that could
+        // terminate the thread without cleaning up
+        swagger.shutdown();
+
+        ASSERT_EQ(goodRes->status, 200);
+        ASSERT_EQ(goodRes->body, "good");
+
+        ASSERT_EQ(badRes->status, 404);
+        ASSERT_NE(badRes->body, "bad");
+    });
+    swagger.start();
 
-    while(swagger.getPort() == 0);
-
-    Port port = swagger.getPort();
-
-    cout << "Cores = " << hardware_concurrency() << endl;
-    cout << "CWD = " << filesystem::current_path() << endl;
-    cout << "Port = " << port << endl;
-
-    httplib::Client client("localhost", port);
-
-    // Test if we have access to files inside the UI folder.
-    auto goodRes = client.Get("/doc/good.txt");
-    ASSERT_EQ(goodRes->status, 200);
-    ASSERT_EQ(goodRes->body, "good");
-
-    // Attempt to read file outside of the UI directory should fail even if
-    // the file exists.
-    auto badRes = client.Get("/doc/../bad.txt");
-    ASSERT_EQ(badRes->status, 404);
-    ASSERT_NE(badRes->body, "bad");
-
-    swagger.shutdown();
     t.join();
     filesystem::remove_all("assets");
     filesystem::remove_all("bad.txt");

version.txt

@@ -1 +1 @@
-0.0.3.20220420
+0.0.3.20220425