Skip to content

Commit

Permalink
fix(swagger/security): ensure that the requested file is in the UI di…
Browse files Browse the repository at this point in the history
…rectory

The Rest::Swagger class didn't check if the file asked from the user was
contained in the UI directory, thus allowing users to access arbitrary
files in the filesystem.

Thanks to Kirill Efimov (@Kirill89) and the Snyk Security team for
finding and reporting the vulnerability to the Pistache team.
  • Loading branch information
Tachi107 committed Apr 25, 2022
1 parent 9b529cf commit 4ba6da0
Show file tree
Hide file tree
Showing 4 changed files with 60 additions and 43 deletions.
31 changes: 22 additions & 9 deletions src/common/description.cc
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include <pistache/http_header.h>

#include <algorithm>
#include <filesystem>
#include <sstream>

namespace Pistache::Rest
Expand Down Expand Up @@ -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);
}

Expand All @@ -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))
{
Expand Down Expand Up @@ -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;
}

Expand All @@ -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
{
Expand Down Expand Up @@ -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_)
Expand Down
2 changes: 1 addition & 1 deletion tests/meson.build
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
68 changes: 36 additions & 32 deletions tests/rest_swagger_server_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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")
{ }

Expand Down Expand Up @@ -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");
Expand Down
2 changes: 1 addition & 1 deletion version.txt
Original file line number Diff line number Diff line change
@@ -1 +1 @@
0.0.3.20220420
0.0.3.20220425

0 comments on commit 4ba6da0

Please sign in to comment.