diff --git a/site/tests/http.uce b/site/tests/http.uce index bc69437..e7ebdec 100644 --- a/site/tests/http.uce +++ b/site/tests/http.uce @@ -30,6 +30,8 @@ RENDER(Request& context) String uri_input = "alpha beta/?x=1&y=2"; String encoded = uri_encode(uri_input); String decoded = uri_decode(encoded); + String malformed_percent = uri_decode("% %A %GG ok%20done"); + URI empty_uri = parse_uri(""); String generated_session = session_id_create(); String query_dump = var_dump(query); String set_cookie_dump = var_dump(context.set_cookies); @@ -39,6 +41,8 @@ RENDER(Request& context) site_tests_page_start("HTTP And Session", "Request helpers, cookies, headers, URI parsing, and session lifecycle checks."); check("uri_encode() / uri_decode()", decoded == uri_input, encoded + " => " + decoded); + check("uri_decode() malformed percent literals", malformed_percent == "% %A %GG ok done", malformed_percent); + check("parse_uri() empty input", empty_uri.parts["raw"] == "", var_dump(empty_uri)); check("parse_query()", query["alpha"] == "1" && query["beta"] == "two words", query_dump); check("set_cookie()", set_cookie_dump.find("site-tests-cookie") != String::npos && set_cookie_dump.find("cookie-value") != String::npos && set_cookie_dump.find("HttpOnly") != String::npos && set_cookie_dump.find("SameSite=Lax") != String::npos, set_cookie_dump); check("response header mutation", context.header["X-Site-Tests"] == "http-suite", header_dump); diff --git a/site/tests/security_headers.uce b/site/tests/security_headers.uce index b2d40c7..5cf3a15 100644 --- a/site/tests/security_headers.uce +++ b/site/tests/security_headers.uce @@ -1,6 +1,9 @@ RENDER(Request& context) { context.header["X-UCE-Security-Test"] = "safe\r\nX-UCE-Injected: nope"; + context.header["Bad\r\nX-UCE-Injected-Name"] = "nope"; + context.set_cookies.push_back("Set-Cookie: raw=1\r\nX-UCE-Cookie-Injected: nope"); redirect("/tests/index.uce\r\nX-UCE-Redirect-Injected: nope", 299); + context.set_status(299, "OK\r\nX-UCE-Status-Injected: nope"); print("security header sanitizer test"); } diff --git a/site/tests/services.uce b/site/tests/services.uce index 94604f7..e1f4f62 100644 --- a/site/tests/services.uce +++ b/site/tests/services.uce @@ -31,10 +31,11 @@ RENDER(Request& context) bool write_ok = socket_write(sockfd, "GET /tests/index.uce HTTP/1.0\r\nHost: uce.openfu.com\r\n\r\n"); String response = socket_read(sockfd, 4096, 2); socket_close(sockfd); + bool has_nul = response.find(String("\0", 1)) != String::npos; mark( "socket_connect() / socket_write() / socket_read()", - (write_ok && response.find("200 OK") != String::npos) ? "pass" : "fail", - response.substr(0, response.length() > 220 ? 220 : response.length()) + (write_ok && response.find("200 OK") != String::npos && !has_nul) ? "pass" : "fail", + response.substr(0, response.length() > 220 ? 220 : response.length()) + (has_nul ? " [unexpected NUL]" : "") ); } else @@ -50,17 +51,22 @@ RENDER(Request& context) bool write_ok = socket_write(custom_http_sockfd, "GET /custom-test?alpha=1 HTTP/1.0\r\nHost: localhost\r\n\r\n"); String response = socket_read(custom_http_sockfd, 4096, 2); socket_close(custom_http_sockfd); - server_stop("site-tests-http"); + bool stopped = server_stop("site-tests-http"); + usleep(200000); + u64 stopped_sockfd = socket_connect("127.0.0.1", 19091); + bool listener_closed = stopped_sockfd == 0; + if(stopped_sockfd != 0) + socket_close(stopped_sockfd); mark( - "server_start_http() / SERVE_HTTP:named", - (custom_http_pid != 0 && write_ok && response.find("custom-http-named") != String::npos && response.find("query=alpha=1") != String::npos) ? "pass" : "fail", - response.substr(0, response.length() > 260 ? 260 : response.length()) + "server_start_http() / SERVE_HTTP:named / server_stop()", + (custom_http_pid != 0 && write_ok && response.find("custom-http-named") != String::npos && response.find("query=alpha=1") != String::npos && stopped && listener_closed) ? "pass" : "fail", + response.substr(0, response.length() > 260 ? 260 : response.length()) + " stopped=" + (stopped ? "true" : "false") + " listener_closed=" + (listener_closed ? "true" : "false") ); } else { - server_stop("site-tests-http"); - mark("server_start_http() / SERVE_HTTP:named", "fail", "socket_connect(127.0.0.1, 19091) returned 0; pid=" + std::to_string(custom_http_pid)); + bool stopped = server_stop("site-tests-http"); + mark("server_start_http() / SERVE_HTTP:named / server_stop()", "fail", "socket_connect(127.0.0.1, 19091) returned 0; pid=" + std::to_string(custom_http_pid) + " stopped=" + (stopped ? "true" : "false")); } u64 memfd = memcache_connect(); diff --git a/site/tests/zip.uce b/site/tests/zip.uce index 5338e98..8a1cc57 100644 --- a/site/tests/zip.uce +++ b/site/tests/zip.uce @@ -62,11 +62,41 @@ RENDER(Request& context) } check("zip_create() rejects unsafe names", unsafe_rejected, "absolute member name rejected"); + bool nul_name_rejected = false; + try + { + DTree unsafe_entries; + String nul_name = "prefix"; + nul_name.push_back((char)0x00); + nul_name += "suffix.txt"; + unsafe_entries[nul_name] = "bad"; + zip_create(path_join(base, "nul-name.zip"), unsafe_entries); + } + catch(std::exception& e) + { + nul_name_rejected = contains(e.what(), "unsafe"); + } + check("zip_create() rejects NUL entry names", nul_name_rejected, "embedded NUL member name rejected"); + + String binary_source("UCE", 3); + binary_source.push_back((char)0x00); + binary_source += "binary"; + binary_source.push_back((char)0xff); + binary_source += "payload"; + DTree binary_entries; + binary_entries["binary.dat"] = binary_source; + String binary_archive = path_join(base, "binary.zip"); + bool binary_created = zip_create(binary_archive, binary_entries); + String binary_zip_roundtrip = zip_read(binary_archive, "binary.dat"); + check("zip binary-safe String payload", binary_created && binary_zip_roundtrip == binary_source && binary_zip_roundtrip.size() == binary_source.size(), "bytes=" + std::to_string((u64)binary_zip_roundtrip.size())); + String gz_source = "UCE gzip payload\nline two\n"; String gz_body = gz_compress(gz_source); String gz_roundtrip = gz_uncompress(gz_body); + String gz_binary_body = gz_compress(binary_source); + String gz_binary_roundtrip = gz_uncompress(gz_binary_body); check("gz_compress()", gz_body.size() > gz_source.size() && (u8)gz_body[0] == 0x1f && (u8)gz_body[1] == 0x8b, "bytes=" + std::to_string((u64)gz_body.size())); - check("gz_uncompress()", gz_roundtrip == gz_source, gz_roundtrip); + check("gz_uncompress()", gz_roundtrip == gz_source && gz_binary_roundtrip == binary_source && gz_binary_roundtrip.size() == binary_source.size(), "text=" + gz_roundtrip + ", binary bytes=" + std::to_string((u64)gz_binary_roundtrip.size())); bool bad_gz_rejected = false; try diff --git a/src/fastcgi/src/fcgicc.cc b/src/fastcgi/src/fcgicc.cc index 9cad8fc..7cee1f4 100644 --- a/src/fastcgi/src/fcgicc.cc +++ b/src/fastcgi/src/fcgicc.cc @@ -36,7 +36,6 @@ #include #include -#include #include // E* #include @@ -124,39 +123,15 @@ make_http_text_response(String status_line, String body, String extra_headers = ); } -static bool -header_token_safe(String name) -{ - if(name == "") - return(false); - for(char c : name) - { - if(!(std::isalnum((unsigned char)c) || c == '-' || c == '_')) - return(false); - } - return(true); -} - -static String -header_value_sanitize(String value) -{ - for(char& c : value) - { - if(c == '\r' || c == '\n') - c = ' '; - } - return(value); -} - static String render_header_map(StringMap headers) { String result; for(auto& item : headers) { - if(!header_token_safe(item.first)) + if(!http_header_name_valid(item.first)) continue; - result += item.first + ": " + header_value_sanitize(item.second) + "\r\n"; + result += item.first + ": " + http_header_value_clean(item.second) + "\r\n"; } return(result); } @@ -167,23 +142,13 @@ render_set_cookie_headers(StringList headers) String result; for(String header : headers) { - if(header.find('\r') != String::npos || header.find('\n') != String::npos) - continue; - if(!str_starts_with(to_lower(header), "set-cookie: ")) + if(!http_set_cookie_header_valid(header)) continue; result += header + "\r\n"; } return(result); } -static String -safe_status_line(String status_line) -{ - if(status_line.find('\r') != String::npos || status_line.find('\n') != String::npos) - return("Status: 500 Internal Server Error"); - return(status_line); -} - static String http_script_root() { @@ -865,14 +830,14 @@ FastCGIServer::process_http_request(FastCGIRequest& request, String& data) if(!parse_http_message(request, data)) return; - if(request.params["SCRIPT_FILENAME"] == "" && request.params["DOCUMENT_URI"] != "") + if(resolve_http_script_filename && request.params["SCRIPT_FILENAME"] == "" && request.params["DOCUMENT_URI"] != "") { String document_root = http_script_root(); String document_uri = strip_leading_slashes(request.params["DOCUMENT_URI"]); String candidate = path_join(document_root, document_uri); String real_root = path_real(document_root); String real_candidate = path_real(candidate); - if(real_root == "" || real_candidate == "" || !path_is_within(real_candidate, real_root)) + if(real_root == "" || real_candidate == "" || !path_is_within(candidate, document_root)) { reject_http_connection(*client_sockets[request.resources.client_socket], "HTTP/1.1 404 Not Found", "script not found\n"); return; @@ -1339,7 +1304,7 @@ void FastCGIServer::assemble_output_buffer(FastCGIRequest& request, Connection* connection) { request.out = - safe_status_line(request.response_code)+"\r\n"+ + http_status_line_clean(request.response_code)+"\r\n"+ render_header_map(request.header) + render_set_cookie_headers(request.set_cookies) + "\r\n"; @@ -1354,7 +1319,7 @@ FastCGIServer::assemble_output_buffer(FastCGIRequest& request, Connection* conne request.set_status(500, "Response Too Large"); request.header.clear(); request.header["Content-Type"] = "text/plain; charset=utf-8"; - request.out = safe_status_line(request.response_code) + "\r\n" + render_header_map(request.header) + "\r\nresponse exceeded configured output limit\n"; + request.out = http_status_line_clean(request.response_code) + "\r\n" + render_header_map(request.header) + "\r\nresponse exceeded configured output limit\n"; } request.ob_stack.clear(); request.flags.output_closed = true; diff --git a/src/fastcgi/src/fcgicc.h b/src/fastcgi/src/fcgicc.h index 172d031..4d811ce 100644 --- a/src/fastcgi/src/fcgicc.h +++ b/src/fastcgi/src/fcgicc.h @@ -61,6 +61,7 @@ public: void process(int timeout_ms = -1); // timeout_ms<0 blocks forever void process_forever(); int calls_until_termination = 8; // set this to -1 to never terminate + bool resolve_http_script_filename = true; typedef unsigned RequestID; typedef std::map RequestList; diff --git a/src/lib/functionlib.cpp b/src/lib/functionlib.cpp index 7f3c3ac..de36c62 100644 --- a/src/lib/functionlib.cpp +++ b/src/lib/functionlib.cpp @@ -2,6 +2,7 @@ #define PCRE2_CODE_UNIT_WIDTH 8 #include +#include #include String var_dump(StringMap map, String prefix, String postfix) @@ -47,14 +48,14 @@ u8 hex_to_u8(String src) String to_lower(String s) { String result = s; - std::transform(result.begin(), result.end(),result.begin(), ::tolower); + std::transform(result.begin(), result.end(), result.begin(), [](unsigned char c) { return((char)std::tolower(c)); }); return(result); } String to_upper(String s) { String result = s; - std::transform(result.begin(), result.end(),result.begin(), ::toupper); + std::transform(result.begin(), result.end(), result.begin(), [](unsigned char c) { return((char)std::toupper(c)); }); return(result); } diff --git a/src/lib/sys.cpp b/src/lib/sys.cpp index 6303fcf..e8256e6 100644 --- a/src/lib/sys.cpp +++ b/src/lib/sys.cpp @@ -496,14 +496,12 @@ String socket_read(u64 sockfd, u32 max_length, u32 timeout) tv.tv_sec = timeout; tv.tv_usec = 0; setsockopt(sockfd, SOL_SOCKET, SO_RCVTIMEO, (const char*)&tv, sizeof tv); - char buf[max_length+1]; - auto byte_count = recv(sockfd, buf, sizeof(buf), 0); + if(max_length == 0) + return(""); + std::vector buf(max_length); + auto byte_count = recv(sockfd, buf.data(), buf.size(), 0); if(byte_count > 0) - { - buf[byte_count] = 0; - String result(buf, byte_count+1); - return(result); - } + return(String(buf.data(), byte_count)); return(""); } @@ -512,7 +510,7 @@ String memcache_escape_key(String key) String result; for(auto c : key) { - if(isspace(c)) + if(isspace((unsigned char)c)) c = '_'; result.append(1, c); } diff --git a/src/lib/types.h b/src/lib/types.h index 376c50e..242f63b 100644 --- a/src/lib/types.h +++ b/src/lib/types.h @@ -222,7 +222,7 @@ struct Request { bool websocket_is_binary = false; bool websocket_is_text = false; String current_unit_file = ""; - std::string params_buffer; + String params_buffer; } resources; void ob_start(); diff --git a/src/lib/uri.cpp b/src/lib/uri.cpp index 1782e64..e939de5 100644 --- a/src/lib/uri.cpp +++ b/src/lib/uri.cpp @@ -1,5 +1,6 @@ #include "uri.h" +#include #include #include @@ -174,7 +175,7 @@ String uri_decode(String q) for(u32 i = 0; i < q.length(); i++) { char c = q[i]; - if(c == '%' && q[i+1] != '%') + if(c == '%' && i + 2 < q.length() && isxdigit((unsigned char)q[i + 1]) && isxdigit((unsigned char)q[i + 2])) { result.append(1, hex_to_u8(q.substr(i+1, 2))); i += 2; @@ -197,12 +198,12 @@ String uri_encode(String q) for(u32 i = 0; i < q.length(); i++) { char c = q[i]; - if(isalnum(c) || c == '~' || c == '.' || c == '_' || c == '-') + if(isalnum((unsigned char)c) || c == '~' || c == '.' || c == '_' || c == '-') result.append(1, c); else { result.append(1, '%'); - result.append(to_hex(c)); + result.append(to_hex((u8)c, 2)); } } return(result); @@ -259,7 +260,17 @@ String encode_query(StringMap map) return(result); } -namespace { +bool http_header_name_valid(String name) +{ + if(name == "") + return(false); + for(char c : name) + { + if(!(std::isalnum((unsigned char)c) || c == '-' || c == '_')) + return(false); + } + return(true); +} String http_header_value_clean(String value) { @@ -271,6 +282,22 @@ String http_header_value_clean(String value) return(value); } +bool http_set_cookie_header_valid(String header) +{ + if(header.find('\r') != String::npos || header.find('\n') != String::npos) + return(false); + return(str_starts_with(to_lower(header), "set-cookie: ")); +} + +String http_status_line_clean(String status_line) +{ + if(status_line.find('\r') != String::npos || status_line.find('\n') != String::npos) + return("Status: 500 Internal Server Error"); + return(status_line); +} + +namespace { + String cookie_attribute_value_clean(String value) { for(char& c : value) @@ -410,6 +437,12 @@ URI parse_uri(String uri_String) { URI result; + if(uri_String == "") + { + result.parts["raw"] = ""; + return(result); + } + u8 state = 0; String current = ""; char expect = 0; diff --git a/src/lib/zip.cpp b/src/lib/zip.cpp index 75c3d77..1ca5350 100644 --- a/src/lib/zip.cpp +++ b/src/lib/zip.cpp @@ -40,6 +40,12 @@ bool zip_entry_name_safe(String name) if(name.find(":") != String::npos) return(false); + for(char c : name) + { + if(c == '\0' || (unsigned char)c < 0x20) + return(false); + } + auto parts = split(replace(name, "\\", "/"), "/"); for(auto part : parts) { @@ -330,7 +336,12 @@ String gz_uncompress(String compressed) if(!out) throw std::runtime_error("gz_uncompress(): decompression failed"); - archive_check_size("gz_uncompress", "output", out_len, "ARCHIVE_MAX_OUTPUT_BYTES", 64 * 1024 * 1024); + u64 output_limit = archive_config_u64("ARCHIVE_MAX_OUTPUT_BYTES", 64 * 1024 * 1024); + if(output_limit > 0 && out_len > output_limit) + { + mz_free(out); + throw std::runtime_error("gz_uncompress(): output exceeds configured limit"); + } String result((char*)out, out_len); mz_free(out); diff --git a/src/linux_fastcgi.cpp b/src/linux_fastcgi.cpp index 8972d21..a74330e 100644 --- a/src/linux_fastcgi.cpp +++ b/src/linux_fastcgi.cpp @@ -1298,6 +1298,7 @@ void custom_server_http_dispatcher_loop(String key) FastCGIServer dispatcher; custom_server_bind_http(dispatcher, cfg["bind"]); dispatcher.calls_until_termination = -1; + dispatcher.resolve_http_script_filename = false; dispatcher.on_complete = &custom_server_http_complete; for(;;) dispatcher.process(-1); diff --git a/tests/plugins/uce_security_smoke.py b/tests/plugins/uce_security_smoke.py index 6d1e5bf..c0ba6e9 100644 --- a/tests/plugins/uce_security_smoke.py +++ b/tests/plugins/uce_security_smoke.py @@ -14,6 +14,19 @@ def _direct_http_request(path, headers=None): connection.close() +def _frontend_http_request(path, headers=None): + request_headers = {"Host": "uce.openfu.com"} + request_headers.update(headers or {}) + connection = http.client.HTTPConnection("127.0.0.1", 80, timeout=5.0) + try: + connection.request("GET", path, headers=request_headers) + response = connection.getresponse() + body = response.read().decode("utf-8", errors="replace") + return response.status, response.getheaders(), body + finally: + connection.close() + + def register(registry): def direct_http_rejects_dotdot(context): status, headers, body = _direct_http_request("/../site/demo/hello.uce") @@ -32,13 +45,33 @@ def register(registry): def response_headers_are_sanitized(context): response = context.request("/tests/security_headers.uce") - if "X-UCE-Injected" in response.headers or "X-UCE-Redirect-Injected" in response.headers: - raise TestFailure("CRLF header injection produced an extra response header") + injected_headers = [ + "X-UCE-Injected", + "X-UCE-Injected-Name", + "X-UCE-Cookie-Injected", + "X-UCE-Redirect-Injected", + "X-UCE-Status-Injected", + ] + for header in injected_headers: + if header in response.headers: + raise TestFailure("CRLF header injection produced extra response header %s" % header) location = response.headers.get("Location", "") if "\r" in location or "\n" in location: raise TestFailure("Location header contains raw CR/LF") return "CRLF response header injection was sanitized" + def unknown_session_id_is_not_adopted(context): + attacker_id = "a" * 64 + status, headers, body = _frontend_http_request("/tests/http.uce", headers={"Cookie": "uce-site-tests=" + attacker_id}) + set_cookies = [value for name, value in headers if name.lower() == "set-cookie"] + session_cookies = [value for value in set_cookies if value.startswith("uce-site-tests=")] + if any(("uce-site-tests=" + attacker_id) in value for value in session_cookies): + raise TestFailure("session_start adopted caller supplied unknown session id") + if not session_cookies or not any("HttpOnly" in value and "SameSite=Lax" in value for value in session_cookies): + raise TestFailure("session_start did not issue a hardened replacement session cookie") + return "unknown caller-supplied session id was replaced" + registry.case("direct HTTP rejects dot-dot script traversal", direct_http_rejects_dotdot, tags=["security", "http", "internal"]) registry.case("direct HTTP ignores Script-Filename header", direct_http_ignores_script_filename_header, tags=["security", "http", "internal"]) registry.case("response headers sanitize CRLF", response_headers_are_sanitized, tags=["security", "http", "internal"]) + registry.case("unknown session ids are not adopted", unknown_session_id_is_not_adopted, tags=["security", "http", "internal"]) diff --git a/tests/plugins/uce_site_suite.py b/tests/plugins/uce_site_suite.py index cd226a9..c17e07c 100644 --- a/tests/plugins/uce_site_suite.py +++ b/tests/plugins/uce_site_suite.py @@ -32,8 +32,10 @@ def register(registry): for marker in ERROR_MARKERS: if marker in body_lower: raise Exception("response body contained error marker %r for %s" % (marker, page_path)) + if '
' in body_lower and ('>fail' in body_lower or '>failed 0<' not in body_lower): + raise Exception("site suite reported failed cases for %s" % page_path) - return "HTTP 200 with suite page marker for %s" % page_path + return "HTTP 200 with suite page marker and no failed cases for %s" % page_path return run