Fix 9 security vulnerabilities found in audit

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.

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);
     }
 }

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;

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");
 

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;

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) { \

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<http_response> 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<std::string> 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<webserver*>(cls)->tcp_nodelay) {
+    if (conninfo != nullptr && static_cast<webserver*>(cls)->tcp_nodelay) {
         int yes = 1;
         setsockopt(conninfo->connect_fd, IPPROTO_TCP, TCP_NODELAY, reinterpret_cast<char*>(&yes), sizeof(int));
     }

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()

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<CURLcode, int32_t> 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<webserver>(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<webserver>(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<string, map<string, httpserver::http::file_info>> 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()