diff --git a/ChangeLog b/ChangeLog index faeccb65..6760a4ff 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,17 @@ Version 0.19.0 - 2023-06-15 + Fixed path traversal vulnerability in file uploads when + generate_random_filename_on_upload is disabled. + Fixed TOCTOU race in file_response by replacing stat-then-open with + open-then-fstat; added O_NOFOLLOW on non-Windows. + Fixed file descriptor leaks in file_response on lseek failure and + zero-size file paths. + Fixed NULL pointer dereference when MHD_get_connection_info returns + nullptr for TCP_NODELAY. + Fixed uninitialized _file_size in file_info. + Fixed auth skip path bypass via path traversal (e.g. /public/../protected). + Fixed use of free() instead of MHD_free() for digest auth username. + Fixed unchecked write error during file upload. Considering family_url as part of the priority when selecting a URL to match. More explicit selection of C++ version. Ability to handle multiple parameters with the same name on the URL. diff --git a/src/file_response.cpp b/src/file_response.cpp index 669f5e8b..3e915413 100644 --- a/src/file_response.cpp +++ b/src/file_response.cpp @@ -32,24 +32,29 @@ struct MHD_Response; namespace httpserver { MHD_Response* file_response::get_raw_response() { - struct stat sb; +#ifndef _WIN32 + int fd = open(filename.c_str(), O_RDONLY | O_NOFOLLOW); +#else + int fd = open(filename.c_str(), O_RDONLY); +#endif + if (fd == -1) return nullptr; - // Deny everything but regular files - if (stat(filename.c_str(), &sb) == 0) { - if (!S_ISREG(sb.st_mode)) return nullptr; - } else { + struct stat sb; + if (fstat(fd, &sb) != 0 || !S_ISREG(sb.st_mode)) { + close(fd); return nullptr; } - int fd = open(filename.c_str(), O_RDONLY); - if (fd == -1) return nullptr; - off_t size = lseek(fd, 0, SEEK_END); - if (size == (off_t) -1) return nullptr; + if (size == (off_t) -1) { + close(fd); + return nullptr; + } if (size) { return MHD_create_response_from_fd(size, fd); } else { + close(fd); return MHD_create_response_from_buffer(0, nullptr, MHD_RESPMEM_PERSISTENT); } } diff --git a/src/http_request.cpp b/src/http_request.cpp index d58842b3..1de47519 100644 --- a/src/http_request.cpp +++ b/src/http_request.cpp @@ -351,7 +351,7 @@ std::string_view http_request::get_digested_user() const { cache->digested_user = EMPTY; if (digested_user_c != nullptr) { cache->digested_user = digested_user_c; - free(digested_user_c); + MHD_free(digested_user_c); } return cache->digested_user; diff --git a/src/http_utils.cpp b/src/http_utils.cpp index 43b522bb..695292a3 100644 --- a/src/http_utils.cpp +++ b/src/http_utils.cpp @@ -271,6 +271,21 @@ const std::string http_utils::generate_random_upload_filename(const std::string& return ret_filename; } +std::string http_utils::sanitize_upload_filename(const std::string& filename) { + if (filename.empty()) return ""; + + // Find the basename: take everything after the last '/' or '\' + std::string::size_type pos = filename.find_last_of("/\\"); + std::string basename = (pos != std::string::npos) ? filename.substr(pos + 1) : filename; + + // Reject empty basename, ".", and ".." + if (basename.empty() || basename == "." || basename == "..") { + return ""; + } + + return basename; +} + std::string get_ip_str(const struct sockaddr *sa) { if (!sa) throw std::invalid_argument("socket pointer is null"); diff --git a/src/httpserver/file_info.hpp b/src/httpserver/file_info.hpp index f78c55fa..8fd4f9e9 100644 --- a/src/httpserver/file_info.hpp +++ b/src/httpserver/file_info.hpp @@ -42,7 +42,7 @@ class file_info { file_info() = default; private: - size_t _file_size; + size_t _file_size = 0; std::string _file_system_file_name; std::string _content_type; std::string _transfer_encoding; diff --git a/src/httpserver/http_utils.hpp b/src/httpserver/http_utils.hpp index 35a5a45a..8e44c15b 100644 --- a/src/httpserver/http_utils.hpp +++ b/src/httpserver/http_utils.hpp @@ -272,6 +272,8 @@ class http_utils { static std::string standardize_url(const std::string&); static const std::string generate_random_upload_filename(const std::string& directory); + + static std::string sanitize_upload_filename(const std::string& filename); }; #define COMPARATOR(x, y, op) { \ diff --git a/src/webserver.cpp b/src/webserver.cpp index 20690cab..92d754a2 100644 --- a/src/webserver.cpp +++ b/src/webserver.cpp @@ -699,7 +699,11 @@ MHD_Result webserver::post_iterator(void *cls, enum MHD_ValueKind kind, if (mr->ws->generate_random_filename_on_upload) { file.set_file_system_file_name(http_utils::generate_random_upload_filename(mr->ws->file_upload_dir)); } else { - file.set_file_system_file_name(mr->ws->file_upload_dir + "/" + std::string(filename)); + std::string safe_name = http_utils::sanitize_upload_filename(filename); + if (safe_name.empty()) { + return MHD_NO; + } + file.set_file_system_file_name(mr->ws->file_upload_dir + "/" + safe_name); } // to not append to an already existing file, delete an already existing file unlink(file.get_file_system_file_name().c_str()); @@ -731,6 +735,9 @@ MHD_Result webserver::post_iterator(void *cls, enum MHD_ValueKind kind, if (size > 0) { mr->upload_ostrm->write(data, size); + if (!mr->upload_ostrm->good()) { + return MHD_NO; + } } // update the file size in the map @@ -774,13 +781,40 @@ std::shared_ptr webserver::internal_error_page(details::modded_re } bool webserver::should_skip_auth(const std::string& path) const { + // Normalize path: resolve ".." and "." segments to prevent bypass + std::string normalized; + { + std::vector segments; + std::string::size_type start = 0; + // Skip leading slash + if (!path.empty() && path[0] == '/') { + start = 1; + } + while (start < path.size()) { + auto end = path.find('/', start); + if (end == std::string::npos) end = path.size(); + std::string seg = path.substr(start, end - start); + if (seg == "..") { + if (!segments.empty()) segments.pop_back(); + } else if (!seg.empty() && seg != ".") { + segments.push_back(seg); + } + start = end + 1; + } + normalized = "/"; + for (size_t i = 0; i < segments.size(); i++) { + if (i > 0) normalized += "/"; + normalized += segments[i]; + } + } + for (const auto& skip_path : auth_skip_paths) { - if (skip_path == path) return true; + if (skip_path == normalized) return true; // Support wildcard suffix (e.g., "/public/*") if (skip_path.size() > 2 && skip_path.back() == '*' && skip_path[skip_path.size() - 2] == '/') { std::string prefix = skip_path.substr(0, skip_path.size() - 1); - if (path.compare(0, prefix.size(), prefix) == 0) return true; + if (normalized.compare(0, prefix.size(), prefix) == 0) return true; } } return false; @@ -1028,7 +1062,7 @@ MHD_Result webserver::answer_to_connection(void* cls, MHD_Connection* connection const MHD_ConnectionInfo * conninfo = MHD_get_connection_info(connection, MHD_CONNECTION_INFO_CONNECTION_FD); - if (static_cast(cls)->tcp_nodelay) { + if (conninfo != nullptr && static_cast(cls)->tcp_nodelay) { int yes = 1; setsockopt(conninfo->connect_fd, IPPROTO_TCP, TCP_NODELAY, reinterpret_cast(&yes), sizeof(int)); } diff --git a/test/integ/authentication.cpp b/test/integ/authentication.cpp index d280cbd6..57440b2e 100644 --- a/test/integ/authentication.cpp +++ b/test/integ/authentication.cpp @@ -1090,6 +1090,39 @@ LT_BEGIN_AUTO_TEST(authentication_suite, auth_empty_skip_paths) ws.stop(); LT_END_AUTO_TEST(auth_empty_skip_paths) +// Test that path traversal cannot bypass auth skip paths +// Requesting /public/../protected should NOT skip auth +LT_BEGIN_AUTO_TEST(authentication_suite, auth_skip_path_traversal_bypass) + webserver ws = create_webserver(PORT) + .auth_handler(centralized_auth_handler) + .auth_skip_paths({"/public/*"}); + + simple_resource sr; + LT_ASSERT_EQ(true, ws.register_resource("protected", &sr)); + LT_ASSERT_EQ(true, ws.register_resource("public/info", &sr)); + ws.start(false); + + curl_global_init(CURL_GLOBAL_ALL); + std::string s; + CURL *curl; + CURLcode res; + long http_code = 0; // NOLINT(runtime/int) + + // /public/../protected should normalize to /protected, which requires auth + curl = curl_easy_init(); + curl_easy_setopt(curl, CURLOPT_URL, "localhost:" PORT_STRING "/public/../protected"); + curl_easy_setopt(curl, CURLOPT_HTTPGET, 1L); + curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, writefunc); + curl_easy_setopt(curl, CURLOPT_WRITEDATA, &s); + res = curl_easy_perform(curl); + curl_easy_getinfo(curl, CURLINFO_RESPONSE_CODE, &http_code); + LT_ASSERT_EQ(res, 0); + LT_CHECK_EQ(http_code, 401); // Should require auth, not be skipped + curl_easy_cleanup(curl); + + ws.stop(); +LT_END_AUTO_TEST(auth_skip_path_traversal_bypass) + LT_BEGIN_AUTO_TEST_ENV() AUTORUN_TESTS() LT_END_AUTO_TEST_ENV() diff --git a/test/integ/file_upload.cpp b/test/integ/file_upload.cpp index 97cae716..5361c1d0 100644 --- a/test/integ/file_upload.cpp +++ b/test/integ/file_upload.cpp @@ -964,6 +964,96 @@ LT_BEGIN_AUTO_TEST(file_upload_suite, file_upload_with_content_type) ws->stop(); LT_END_AUTO_TEST(file_upload_with_content_type) +// Send file with a crafted filename for path traversal testing +static std::pair send_file_with_traversal_name(int port, const char* crafted_filename) { + curl_global_init(CURL_GLOBAL_ALL); + + CURL *curl = curl_easy_init(); + + curl_mime *form = curl_mime_init(curl); + curl_mimepart *field = curl_mime_addpart(form); + curl_mime_name(field, TEST_KEY); + // Use the real file for data, but override the filename + curl_mime_filedata(field, TEST_CONTENT_FILEPATH); + curl_mime_filename(field, crafted_filename); + + CURLcode res; + std::string url = "localhost:" + std::to_string(port) + "/upload"; + curl_easy_setopt(curl, CURLOPT_URL, url.c_str()); + curl_easy_setopt(curl, CURLOPT_MIMEPOST, form); + + res = curl_easy_perform(curl); + long http_code = 0; // NOLINT [runtime/int] + curl_easy_getinfo(curl, CURLINFO_RESPONSE_CODE, &http_code); + + curl_easy_cleanup(curl); + curl_mime_free(form); + return {res, http_code}; +} + +// Test that path traversal filenames are rejected +LT_BEGIN_AUTO_TEST(file_upload_suite, file_upload_path_traversal_rejected) + string upload_directory = "upload_test_dir"; + MKDIR(upload_directory.c_str()); + + int port = PORT + 2; + auto ws = std::make_unique(create_webserver(port) + .file_upload_target(httpserver::FILE_UPLOAD_DISK_ONLY) + .file_upload_dir(upload_directory)); + // NOT using generate_random_filename_on_upload - this is the vulnerable path + ws->start(false); + LT_CHECK_EQ(ws->is_running(), true); + + print_file_upload_resource resource; + LT_ASSERT_EQ(true, ws->register_resource("upload", &resource)); + + // Attempt path traversal with "../escape" + send_file_with_traversal_name(port, "../escape"); + // The server should reject the upload (MHD_NO causes connection close) + // The key check is that no file was created outside the upload dir + LT_CHECK_EQ(file_exists("escape"), false); + LT_CHECK_EQ(file_exists("./escape"), false); + + ws->stop(); + + // Clean up + rmdir(upload_directory.c_str()); +LT_END_AUTO_TEST(file_upload_path_traversal_rejected) + +// Test that sanitize keeps the basename for normal filenames +LT_BEGIN_AUTO_TEST(file_upload_suite, file_upload_sanitize_keeps_basename) + string upload_directory = "upload_test_dir"; + MKDIR(upload_directory.c_str()); + + int port = PORT + 3; + auto ws = std::make_unique(create_webserver(port) + .file_upload_target(httpserver::FILE_UPLOAD_DISK_ONLY) + .file_upload_dir(upload_directory)); + ws->start(false); + LT_CHECK_EQ(ws->is_running(), true); + + print_file_upload_resource resource; + LT_ASSERT_EQ(true, ws->register_resource("upload", &resource)); + + // Upload with a path-like filename — should strip to just "myfile.txt" + auto res = send_file_with_traversal_name(port, "some/path/myfile.txt"); + LT_ASSERT_EQ(res.first, 0); + LT_ASSERT_EQ(res.second, 201); + + // The file should be created with only the basename + map> files = resource.get_files(); + LT_CHECK_EQ(files.size(), 1); + auto file = files.begin()->second.begin(); + string expected_path = upload_directory + "/myfile.txt"; + LT_CHECK_EQ(file->second.get_file_system_file_name(), expected_path); + + ws->stop(); + + // Clean up + unlink(expected_path.c_str()); + rmdir(upload_directory.c_str()); +LT_END_AUTO_TEST(file_upload_sanitize_keeps_basename) + LT_BEGIN_AUTO_TEST_ENV() AUTORUN_TESTS() LT_END_AUTO_TEST_ENV()