diff --git a/SECURITY_REVIEW.md b/SECURITY_REVIEW.md index e5bec05..ff51634 100644 --- a/SECURITY_REVIEW.md +++ b/SECURITY_REVIEW.md @@ -44,18 +44,20 @@ This comprehensive security review analyzes the Simple Object Server C++23 appli ## High-Risk Issues -### 4. **Missing Security Headers** +### 4. **~~Missing Security Headers~~ [FIXED]** - **Location**: HTTP response handling throughout -- **Risk**: HIGH - Missing standard security headers -- **Issue**: No implementation of standard security headers -- **Recommendation**: Add security headers to all responses: - ```cpp - resp->addHeader("X-Frame-Options", "DENY"); - resp->addHeader("X-Content-Type-Options", "nosniff"); - resp->addHeader("X-XSS-Protection", "1; mode=block"); - resp->addHeader("Strict-Transport-Security", "max-age=31536000"); - resp->addHeader("Content-Security-Policy", "default-src 'self'"); - ``` +- **Risk**: ~~HIGH~~ RESOLVED - Security headers now implemented +- **Fix Implemented**: + - Added `add_security_headers()` method in server.cpp + - Headers added to all HTTP responses across all endpoints + - Implemented headers: + - `X-Frame-Options: DENY` - Prevents clickjacking + - `X-Content-Type-Options: nosniff` - Prevents MIME sniffing + - `X-XSS-Protection: 1; mode=block` - Legacy XSS protection + - `Content-Security-Policy` - Restrictive CSP preventing external resources + - `Referrer-Policy: strict-origin-when-cross-origin` - Controls referrer info + - `Permissions-Policy` - Disables unnecessary browser features + - Note: HSTS header commented out by default (requires HTTPS configuration) ### 5. **Insufficient Input Validation** - **Location**: Multiple endpoints (put_handler.cpp, update_handler.cpp) diff --git a/src/HttpController.cpp b/src/HttpController.cpp index 08b375a..f8914b6 100644 --- a/src/HttpController.cpp +++ b/src/HttpController.cpp @@ -12,6 +12,10 @@ void HttpController::getIndex(const drogon::HttpRequestPtr &req, auto resp = drogon::HttpResponse::newHttpResponse(); resp->setBody(welcome_page()); resp->setContentTypeCode(drogon::CT_TEXT_HTML); + auto server = Server::getInstance(); + if (server) { + server->add_security_headers(resp); + } callback(resp); } @@ -24,6 +28,11 @@ void HttpController::getHash(const drogon::HttpRequestPtr &req, } else { auto resp = drogon::HttpResponse::newHttpResponse(); resp->setStatusCode(drogon::k500InternalServerError); + // Try to add security headers if server instance is available + auto srv = Server::getInstance(); + if (srv) { + srv->add_security_headers(resp); + } callback(resp); } } @@ -37,6 +46,11 @@ void HttpController::getVersion(const drogon::HttpRequestPtr &req, } else { auto resp = drogon::HttpResponse::newHttpResponse(); resp->setStatusCode(drogon::k500InternalServerError); + // Try to add security headers if server instance is available + auto srv = Server::getInstance(); + if (srv) { + srv->add_security_headers(resp); + } callback(resp); } } @@ -50,6 +64,11 @@ void HttpController::checkExists(const drogon::HttpRequestPtr &req, } else { auto resp = drogon::HttpResponse::newHttpResponse(); resp->setStatusCode(drogon::k500InternalServerError); + // Try to add security headers if server instance is available + auto srv = Server::getInstance(); + if (srv) { + srv->add_security_headers(resp); + } callback(resp); } } @@ -62,6 +81,11 @@ void HttpController::getDirectory(const drogon::HttpRequestPtr &req, } else { auto resp = drogon::HttpResponse::newHttpResponse(); resp->setStatusCode(drogon::k500InternalServerError); + // Try to add security headers if server instance is available + auto srv = Server::getInstance(); + if (srv) { + srv->add_security_headers(resp); + } callback(resp); } } @@ -74,6 +98,11 @@ void HttpController::uploadObject(const drogon::HttpRequestPtr &req, } else { auto resp = drogon::HttpResponse::newHttpResponse(); resp->setStatusCode(drogon::k500InternalServerError); + // Try to add security headers if server instance is available + auto srv = Server::getInstance(); + if (srv) { + srv->add_security_headers(resp); + } callback(resp); } } @@ -86,6 +115,11 @@ void HttpController::updateObject(const drogon::HttpRequestPtr &req, } else { auto resp = drogon::HttpResponse::newHttpResponse(); resp->setStatusCode(drogon::k500InternalServerError); + // Try to add security headers if server instance is available + auto srv = Server::getInstance(); + if (srv) { + srv->add_security_headers(resp); + } callback(resp); } } @@ -99,6 +133,11 @@ void HttpController::getMetadata(const drogon::HttpRequestPtr &req, } else { auto resp = drogon::HttpResponse::newHttpResponse(); resp->setStatusCode(drogon::k500InternalServerError); + // Try to add security headers if server instance is available + auto srv = Server::getInstance(); + if (srv) { + srv->add_security_headers(resp); + } callback(resp); } } @@ -111,6 +150,11 @@ void HttpController::deleteObject(const drogon::HttpRequestPtr &req, } else { auto resp = drogon::HttpResponse::newHttpResponse(); resp->setStatusCode(drogon::k500InternalServerError); + // Try to add security headers if server instance is available + auto srv = Server::getInstance(); + if (srv) { + srv->add_security_headers(resp); + } callback(resp); } } @@ -121,6 +165,10 @@ void HttpController::getStatus(const drogon::HttpRequestPtr &req, nlohmann::json response = {{"result", "success"}, {"status", "ok"}}; resp->setBody(response.dump()); resp->setContentTypeCode(drogon::CT_APPLICATION_JSON); + auto server = Server::getInstance(); + if (server) { + server->add_security_headers(resp); + } callback(resp); } @@ -133,6 +181,11 @@ void HttpController::getObject(const drogon::HttpRequestPtr &req, } else { auto resp = drogon::HttpResponse::newHttpResponse(); resp->setStatusCode(drogon::k500InternalServerError); + // Try to add security headers if server instance is available + auto srv = Server::getInstance(); + if (srv) { + srv->add_security_headers(resp); + } callback(resp); } } @@ -142,6 +195,10 @@ void HttpController::getRoot(const drogon::HttpRequestPtr &req, auto resp = drogon::HttpResponse::newHttpResponse(); resp->setBody(welcome_page()); resp->setContentTypeCode(drogon::CT_TEXT_HTML); + auto server = Server::getInstance(); + if (server) { + server->add_security_headers(resp); + } callback(resp); } @@ -154,6 +211,11 @@ void HttpController::getAny(const drogon::HttpRequestPtr &req, } else { auto resp = drogon::HttpResponse::newHttpResponse(); resp->setStatusCode(drogon::k500InternalServerError); + // Try to add security headers if server instance is available + auto srv = Server::getInstance(); + if (srv) { + srv->add_security_headers(resp); + } callback(resp); } } diff --git a/src/put_handler.cpp b/src/put_handler.cpp index 7f602fa..8be38ac 100644 --- a/src/put_handler.cpp +++ b/src/put_handler.cpp @@ -32,6 +32,7 @@ void PutHandler::handle_upload_object(const drogon::HttpRequestPtr& req, std::fu std::map params; if (!server_.validate_write_request(req, resp, {}, params)) { // No required params now since token is in header + server_.add_security_headers(resp); callback(resp); return; } @@ -47,6 +48,7 @@ void PutHandler::handle_upload_object(const drogon::HttpRequestPtr& req, std::fu nlohmann::json response = {{"result", "error"}, {"error", "File too large"}}; resp->setBody(response.dump()); resp->setContentTypeCode(drogon::CT_APPLICATION_JSON); + server_.add_security_headers(resp); callback(resp); return; } @@ -62,6 +64,7 @@ void PutHandler::handle_upload_object(const drogon::HttpRequestPtr& req, std::fu nlohmann::json response = {{"result", "error"}, {"error", "Failed to parse multipart data"}}; resp->setBody(response.dump()); resp->setContentTypeCode(drogon::CT_APPLICATION_JSON); + server_.add_security_headers(resp); callback(resp); return; } @@ -80,6 +83,7 @@ void PutHandler::handle_upload_object(const drogon::HttpRequestPtr& req, std::fu nlohmann::json response = {{"result", "error"}, {"error", "No file provided in upload"}}; resp->setBody(response.dump()); resp->setContentTypeCode(drogon::CT_APPLICATION_JSON); + server_.add_security_headers(resp); callback(resp); return; } @@ -128,6 +132,7 @@ void PutHandler::handle_upload_object(const drogon::HttpRequestPtr& req, std::fu nlohmann::json response = {{"result", "error"}, {"error", "Missing or invalid required metadata field: labeltags (must be non-empty array of label:tag pairs)"}}; resp->setBody(response.dump()); resp->setContentTypeCode(drogon::CT_APPLICATION_JSON); + server_.add_security_headers(resp); callback(resp); return; } @@ -174,6 +179,7 @@ void PutHandler::handle_upload_object(const drogon::HttpRequestPtr& req, std::fu nlohmann::json response = {{"result", "error"}, {"error", "Failed to create temporary file"}}; resp->setBody(response.dump()); resp->setContentTypeCode(drogon::CT_APPLICATION_JSON); + server_.add_security_headers(resp); callback(resp); return; } @@ -189,6 +195,7 @@ void PutHandler::handle_upload_object(const drogon::HttpRequestPtr& req, std::fu resp->setBody(response.dump()); resp->setContentTypeCode(drogon::CT_APPLICATION_JSON); std::filesystem::remove(temp_path); + server_.add_security_headers(resp); callback(resp); return; } @@ -203,6 +210,7 @@ void PutHandler::handle_upload_object(const drogon::HttpRequestPtr& req, std::fu nlohmann::json response = {{"result", "error"}, {"error", "Failed to calculate hash"}}; resp->setBody(response.dump()); resp->setContentTypeCode(drogon::CT_APPLICATION_JSON); + server_.add_security_headers(resp); callback(resp); return; } @@ -246,12 +254,14 @@ void PutHandler::handle_upload_object(const drogon::HttpRequestPtr& req, std::fu resp->setContentTypeCode(drogon::CT_APPLICATION_JSON); // Attempt to clean up the moved file if index fails try { if (std::filesystem::exists(final_path)) std::filesystem::remove(final_path); } catch(...) {}; + server_.add_security_headers(resp); callback(resp); return; } resp->setBody(nlohmann::json({{"result", "success"}, {"hash", hash}}).dump()); resp->setContentTypeCode(drogon::CT_APPLICATION_JSON); + server_.add_security_headers(resp); callback(resp); } diff --git a/src/server.cpp b/src/server.cpp index 9a7a0da..64422c2 100644 --- a/src/server.cpp +++ b/src/server.cpp @@ -199,6 +199,7 @@ void Server::setup_routes() { void Server::handle_cors_preflight(const drogon::HttpRequestPtr& req, std::function&& callback) { auto resp = drogon::HttpResponse::newHttpResponse(); add_cors_headers(req, resp); + add_security_headers(resp); resp->setStatusCode(drogon::k204NoContent); callback(resp); } @@ -240,6 +241,33 @@ void Server::add_cors_headers(const drogon::HttpRequestPtr& req, const drogon::H } } +void Server::add_security_headers(const drogon::HttpResponsePtr& res) { + // Add security headers to prevent common web vulnerabilities + + // Prevent clickjacking attacks + res->addHeader("X-Frame-Options", "DENY"); + + // Prevent MIME type sniffing + res->addHeader("X-Content-Type-Options", "nosniff"); + + // Enable XSS filter in browsers (legacy, but still useful for older browsers) + res->addHeader("X-XSS-Protection", "1; mode=block"); + + // Enforce HTTPS (only add if we're sure the server uses HTTPS) + // Note: Commented out by default as it requires HTTPS to be configured + // res->addHeader("Strict-Transport-Security", "max-age=31536000; includeSubDomains"); + + // Content Security Policy - restrictive by default + // This prevents loading of external resources and inline scripts + res->addHeader("Content-Security-Policy", "default-src 'self'; script-src 'none'; style-src 'self' 'unsafe-inline'; img-src 'self' data:; font-src 'self'; connect-src 'self'; frame-ancestors 'none'"); + + // Referrer Policy - don't leak referrer information + res->addHeader("Referrer-Policy", "strict-origin-when-cross-origin"); + + // Permissions Policy (formerly Feature Policy) - disable unnecessary browser features + res->addHeader("Permissions-Policy", "geolocation=(), microphone=(), camera=(), payment=(), usb=(), magnetometer=(), gyroscope=(), accelerometer=()"); +} + std::string Server::join(const std::vector& strings, const std::string& delimiter) { if (strings.empty()) return ""; @@ -261,6 +289,7 @@ void Server::handle_get_object(const drogon::HttpRequestPtr& req, std::function< nlohmann::json response = {{"result", "error"}, {"error", "Couldn't find: " + key}}; resp->setBody(response.dump()); resp->setContentTypeCode(drogon::CT_APPLICATION_JSON); + add_security_headers(resp); callback(resp); return; } @@ -273,12 +302,14 @@ void Server::handle_get_object(const drogon::HttpRequestPtr& req, std::function< nlohmann::json response = {{"result", "error"}, {"error", "Hash recognised, but object missing for: " + entry.hash}}; resp->setBody(response.dump()); resp->setContentTypeCode(drogon::CT_APPLICATION_JSON); + add_security_headers(resp); callback(resp); return; } // Send file auto resp = drogon::HttpResponse::newFileResponse(file_path.string()); + add_security_headers(resp); callback(resp); } @@ -292,6 +323,7 @@ void Server::handle_get_hash(const drogon::HttpRequestPtr& req, std::functionsetBody(response.dump()); resp->setContentTypeCode(drogon::CT_APPLICATION_JSON); + add_security_headers(resp); callback(resp); return; } @@ -300,6 +332,7 @@ void Server::handle_get_hash(const drogon::HttpRequestPtr& req, std::functionsetBody(response.dump()); resp->setContentTypeCode(drogon::CT_APPLICATION_JSON); + add_security_headers(resp); callback(resp); } @@ -312,6 +345,7 @@ void Server::handle_get_directory(const drogon::HttpRequestPtr& /*req*/, std::fu nlohmann::json response = {{"result", "error"}, {"error", "Failed to retrieve directory listing"}}; resp->setBody(response.dump()); resp->setContentTypeCode(drogon::CT_APPLICATION_JSON); + add_security_headers(resp); callback(resp); return; } @@ -326,6 +360,7 @@ void Server::handle_get_directory(const drogon::HttpRequestPtr& /*req*/, std::fu nlohmann::json response = {{"result", "success"}, {"entries", entries_array}}; resp->setBody(response.dump()); resp->setContentTypeCode(drogon::CT_APPLICATION_JSON); + add_security_headers(resp); callback(resp); } @@ -356,6 +391,7 @@ void Server::handle_get_metadata(const drogon::HttpRequestPtr& req, std::functio resp->setBody(response.dump()); } resp->setContentTypeCode(drogon::CT_APPLICATION_JSON); + add_security_headers(resp); callback(resp); } @@ -374,6 +410,7 @@ void Server::handle_delete_object(const drogon::HttpRequestPtr& req, std::functi { std::map params; if (!validate_write_request(req, resp, {"hash"}, params)) { + add_security_headers(resp); callback(resp); return; } @@ -394,6 +431,7 @@ void Server::handle_delete_object(const drogon::HttpRequestPtr& req, std::functi nlohmann::json response = {{"result", "error"}, {"error", "Failed to remove some or all associated tags"}}; resp->setBody(response.dump()); resp->setContentTypeCode(drogon::CT_APPLICATION_JSON); + add_security_headers(resp); callback(resp); return; } @@ -417,6 +455,7 @@ void Server::handle_delete_object(const drogon::HttpRequestPtr& req, std::functi nlohmann::json response = {{"result", "success"}}; resp->setBody(response.dump()); resp->setContentTypeCode(drogon::CT_APPLICATION_JSON); + add_security_headers(resp); callback(resp); } @@ -431,6 +470,7 @@ void Server::handle_exists(const drogon::HttpRequestPtr& req, std::functionsetBody(response.dump()); resp->setContentTypeCode(drogon::CT_APPLICATION_JSON); + add_security_headers(resp); callback(resp); } @@ -448,6 +488,7 @@ void Server::handle_get_version(const drogon::HttpRequestPtr& req, std::function nlohmann::json response = {{"result", "failed"}, {"error", "Failed to get version for: " + key}}; resp->setBody(response.dump()); resp->setContentTypeCode(drogon::CT_APPLICATION_JSON); + add_security_headers(resp); callback(resp); return; } @@ -460,6 +501,7 @@ void Server::handle_get_version(const drogon::HttpRequestPtr& req, std::function } resp->setBody(response.dump()); resp->setContentTypeCode(drogon::CT_APPLICATION_JSON); + add_security_headers(resp); callback(resp); } diff --git a/src/server.hpp b/src/server.hpp index 7cc1bf8..54d612a 100644 --- a/src/server.hpp +++ b/src/server.hpp @@ -50,6 +50,7 @@ public: void handle_exists(const drogon::HttpRequestPtr& req, std::function&& callback, const std::string& key); void handle_cors_preflight(const drogon::HttpRequestPtr& req, std::function&& callback); void add_cors_headers(const drogon::HttpRequestPtr& req, const drogon::HttpResponsePtr& res); + void add_security_headers(const drogon::HttpResponsePtr& res); private: void setup_routes(); diff --git a/src/update_handler.cpp b/src/update_handler.cpp index 1ef3fe5..90c7940 100644 --- a/src/update_handler.cpp +++ b/src/update_handler.cpp @@ -13,6 +13,7 @@ void UpdateHandler::handle_update_object(const drogon::HttpRequestPtr& req, std: // Validate authentication and rate limit (no required query params, just auth) if (!server_.validate_write_request(req, resp, {}, params)) { + server_.add_security_headers(resp); callback(resp); return; } @@ -30,6 +31,7 @@ void UpdateHandler::handle_update_object(const drogon::HttpRequestPtr& req, std: nlohmann::json response = {{"result", "error"}, {"error", "Invalid JSON body"}}; resp->setBody(response.dump()); resp->setContentTypeCode(drogon::CT_APPLICATION_JSON); + server_.add_security_headers(resp); callback(resp); return; } @@ -44,6 +46,7 @@ void UpdateHandler::handle_update_object(const drogon::HttpRequestPtr& req, std: nlohmann::json response = {{"result", "error"}, {"error", "Failed to parse multipart data"}}; resp->setBody(response.dump()); resp->setContentTypeCode(drogon::CT_APPLICATION_JSON); + server_.add_security_headers(resp); callback(resp); return; } @@ -92,6 +95,7 @@ void UpdateHandler::handle_update_object(const drogon::HttpRequestPtr& req, std: nlohmann::json response = {{"result", "error"}, {"error", "Unsupported content type. Use application/json or form data."}}; resp->setBody(response.dump()); resp->setContentTypeCode(drogon::CT_APPLICATION_JSON); + server_.add_security_headers(resp); callback(resp); return; } @@ -102,6 +106,7 @@ void UpdateHandler::handle_update_object(const drogon::HttpRequestPtr& req, std: nlohmann::json response = {{"result", "error"}, {"error", "Missing 'hash' or 'metadata' field in request body"}}; resp->setBody(response.dump()); resp->setContentTypeCode(drogon::CT_APPLICATION_JSON); + server_.add_security_headers(resp); callback(resp); return; } @@ -116,6 +121,7 @@ void UpdateHandler::handle_update_object(const drogon::HttpRequestPtr& req, std: nlohmann::json response = {{"result", "error"}, {"error", "Object not found for hash: " + hash}}; resp->setBody(response.dump()); resp->setContentTypeCode(drogon::CT_APPLICATION_JSON); + server_.add_security_headers(resp); callback(resp); return; } @@ -136,6 +142,7 @@ void UpdateHandler::handle_update_object(const drogon::HttpRequestPtr& req, std: nlohmann::json response = {{"result", "error"}, {"error", "Failed to update metadata for hash: " + hash}}; resp->setBody(response.dump()); resp->setContentTypeCode(drogon::CT_APPLICATION_JSON); + server_.add_security_headers(resp); callback(resp); return; } @@ -143,6 +150,7 @@ void UpdateHandler::handle_update_object(const drogon::HttpRequestPtr& req, std: nlohmann::json response = {{"result", "success"}, {"hash", hash}}; resp->setBody(response.dump()); resp->setContentTypeCode(drogon::CT_APPLICATION_JSON); + server_.add_security_headers(resp); callback(resp); } diff --git a/testing/test_security_headers.sh b/testing/test_security_headers.sh new file mode 100755 index 0000000..364dd16 --- /dev/null +++ b/testing/test_security_headers.sh @@ -0,0 +1,67 @@ +#!/bin/bash + +set -euo pipefail + +SCRIPT_DIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) +URL="${1:-http://127.0.0.1:7703}" + +echo "Testing security headers at $URL" +echo "======================================" + +# Function to check if a header exists and print its value +check_header() { + local endpoint="$1" + local header="$2" + local expected="$3" + + echo "" + echo "Testing $header on $endpoint" + + # Get headers using curl -I (HEAD request) or curl -i (for full response) + response=$(curl -s -I "$URL$endpoint" 2>/dev/null || curl -s -i -X GET "$URL$endpoint" 2>/dev/null | head -n 20) + + # Check if header exists (case-insensitive) + if echo "$response" | grep -qi "^$header:"; then + value=$(echo "$response" | grep -i "^$header:" | sed 's/[^:]*: *//' | tr -d '\r\n') + echo " ✓ Found: $value" + if [ ! -z "$expected" ] && [ "$value" != "$expected" ]; then + echo " WARNING: Expected '$expected'" + fi + else + echo " ✗ Missing $header" + return 1 + fi +} + +# Test endpoints +echo "" +echo "1. Testing /status endpoint (no auth required)" +echo "-----------------------------------------------" +check_header "/status" "X-Frame-Options" "DENY" +check_header "/status" "X-Content-Type-Options" "nosniff" +check_header "/status" "X-XSS-Protection" "1; mode=block" +check_header "/status" "Content-Security-Policy" "" +check_header "/status" "Referrer-Policy" "strict-origin-when-cross-origin" +check_header "/status" "Permissions-Policy" "" + +echo "" +echo "2. Testing / endpoint (welcome page)" +echo "-------------------------------------" +check_header "/" "X-Frame-Options" "DENY" +check_header "/" "X-Content-Type-Options" "nosniff" + +echo "" +echo "3. Testing authenticated endpoint /dir" +echo "---------------------------------------" +response=$(curl -s -i "$URL/dir" 2>/dev/null | head -n 30) +echo "$response" | grep -E "^(X-Frame-Options|X-Content-Type-Options|X-XSS-Protection|Content-Security-Policy|Referrer-Policy|Permissions-Policy):" || echo "Headers shown above" + +echo "" +echo "4. Testing error response (404)" +echo "--------------------------------" +response=$(curl -s -i "$URL/nonexistent" 2>/dev/null | head -n 30) +echo "$response" | grep -E "^(X-Frame-Options|X-Content-Type-Options|X-XSS-Protection|Content-Security-Policy|Referrer-Policy|Permissions-Policy):" || echo "Headers shown above" + +echo "" +echo "======================================" +echo "Security headers test complete" \ No newline at end of file