diff --git a/darkhttpd.c b/darkhttpd.c index 75259c1..1247118 100644 --- a/darkhttpd.c +++ b/darkhttpd.c @@ -330,11 +330,10 @@ static FILE *logfile = NULL; static char *pidfile_name = NULL; /* NULL = no pidfile */ static int want_chroot = 0, want_daemon = 0, want_accf = 0, want_keepalive = 1, want_server_id = 1, want_single_file = 0, - want_hide_dotfiles = 0; + want_hide_dotfiles = 0, want_log_forwarded_for = 0; static char *server_hdr = NULL; static char *auth_key = NULL; /* NULL or "Basic base64_of_password" */ static char *custom_hdrs = NULL; -static char *trusted_ip = NULL; /* Address of a trusted reverse proxy */ static uint64_t num_requests = 0, total_in = 0, total_out = 0; static int accepting = 1; /* set to 0 to stop accept()ing */ static int syslog_enabled = 0; @@ -1013,9 +1012,9 @@ static void usage(const char *argv0) { "\t\tEnable basic authentication. This is *INSECURE*: passwords\n" "\t\tare sent unencrypted over HTTP, plus the password is visible\n" "\t\tin ps(1) to other users on the system.\n\n"); - printf("\t--trusted-ip ip\n" - "\t\tIf the request comes from this IP, the X-Forwarded-For header\n" - "\t\tcontent is used in the log instead of the connection peer IP.\n\n"); + printf("\t--log-forwarded-for\n" + "\t\tUse the first value from the X-Forwarded-For header in the\n" + "\t\trequest log instead of the connection peer IP.\n\n"); printf("\t--header 'Header: Value'\n" "\t\tAdd a custom header to all responses.\n" "\t\tThis option can be specified multiple times, in which case\n" @@ -1245,20 +1244,8 @@ static void parse_commandline(const int argc, char *argv[]) { xasprintf(&auth_key, "Basic %s", key); free(key); } - else if (strcmp(argv[i], "--trusted-ip") == 0) { - if (++i >= argc) - errx(1, "missing ip after --trusted-ip"); - struct in_addr a4; -#ifdef HAVE_INET6 - struct in6_addr a6; - if (inet_pton(AF_INET, argv[i], &a4) != 1 && - inet_pton(AF_INET6, argv[i], &a6) != 1) -#else - if (inet_pton(AF_INET, argv[i], &a4) != 1) -#endif - errx(1, "invalid ip address specified for --trusted-ip: `%s'", argv[i]); - - trusted_ip = argv[i]; + else if (strcmp(argv[i], "--log-forwarded-for") == 0) { + want_log_forwarded_for = 1; } else if (strcmp(argv[i], "--forward-https") == 0) { forward_to_https = 1; @@ -1433,8 +1420,7 @@ static char *clf_date(char *dest, const time_t when) { /* Add a connection's details to the logfile. */ static void log_connection(const struct connection *conn) { char *safe_method, *safe_url, *safe_referer, *safe_user_agent, - dest[CLF_DATE_LEN]; - char *safe_forwarded = NULL; + *safe_forwarded_for, dest[CLF_DATE_LEN]; const char *log_ip; if (logfile == NULL) @@ -1444,20 +1430,6 @@ static void log_connection(const struct connection *conn) { if (conn->method == NULL) return; /* invalid - didn't parse - maybe too long */ - log_ip = get_address_text(&conn->client); - - if (conn->forwarded_for != NULL && strcasecmp(log_ip, trusted_ip) == 0) { - /* X-Forwarded-For can be a comma separated list. - We want the first IP (the client), not the whole string. */ - char *comma = strchr(conn->forwarded_for, ','); - if (comma != NULL) - *comma = '\0'; - - safe_forwarded = xmalloc(strlen(conn->forwarded_for) * 3 + 1); - logencode(conn->forwarded_for, safe_forwarded); - log_ip = safe_forwarded; - } - #define make_safe(x) do { \ if (conn->x) { \ safe_##x = xmalloc(strlen(conn->x)*3 + 1); \ @@ -1467,12 +1439,21 @@ static void log_connection(const struct connection *conn) { } \ } while(0) +#define use_safe(x) safe_##x ? safe_##x : "" + + if (want_log_forwarded_for) { + make_safe(forwarded_for); + log_ip = use_safe(forwarded_for); + } else { + safe_forwarded_for = NULL; + log_ip = get_address_text(&conn->client); + } + make_safe(method); make_safe(url); make_safe(referer); make_safe(user_agent); -#define use_safe(x) safe_##x ? safe_##x : "" if (syslog_enabled) { syslog(LOG_INFO, "%s - - %s \"%s %s HTTP/1.1\" %d %llu \"%s\" \"%s\"\n", log_ip, @@ -1503,7 +1484,7 @@ static void log_connection(const struct connection *conn) { free_safe(url); free_safe(referer); free_safe(user_agent); - if (safe_forwarded != NULL) free(safe_forwarded); + free_safe(forwarded_for); #undef make_safe #undef use_safe @@ -1978,8 +1959,17 @@ static int parse_request(struct connection *conn) { conn->referer = parse_field(conn, "\nReferer: "); conn->user_agent = parse_field(conn, "\nUser-Agent: "); conn->authorization = parse_field(conn, "\nAuthorization: "); - if (trusted_ip != NULL) + if (want_log_forwarded_for) { + /* We select the leftmost value in the first header. Note that this + * can be easily spoofed by the client or an intermediary proxy, + * therefore it must not be used for anything security related + * like access control. We use it only for logging. */ conn->forwarded_for = parse_field(conn, "\nX-Forwarded-For: "); + if (conn->forwarded_for != NULL) { + if ((tmp = strchr(conn->forwarded_for, ',')) != NULL) + *tmp = '\0'; + } + } parse_range_field(conn); return 1; } @@ -2213,7 +2203,7 @@ static void generate_dir_listing(struct connection *conn, const char *path, */ char safe_url[MAXNAMLEN*3 + 1]; char buf[DIR_LIST_MTIME_SIZE]; - + urlencode(list[i]->name, safe_url); append(listing, " run --trusted-ip tests" + echo "===> run --log-forwarded-for tests" rm -f test_xff.log - ./darkhttpd $DIR --port $PORT --addr $ADDR --log test_xff.log --trusted-ip $ADDR \ + ./darkhttpd $DIR --port $PORT --addr $ADDR --log test_xff.log --log-forwarded-for \ >>test.out.stdout 2>>test.out.stderr & PID=$! kill -0 $PID || exit 1 diff --git a/devel/test_log.py b/devel/test_log.py index c991c30..0e6e404 100755 --- a/devel/test_log.py +++ b/devel/test_log.py @@ -27,7 +27,7 @@ def _wait_and_check_log(self, unique_marker, timeout=2.0): if unique_marker in content: return content time.sleep(0.1) - + # If we fail, print the content we did find to help debugging self.fail(f"Marker '{unique_marker}' not found in {LOG_FILE} within {timeout}s.\nLog content:\n{content}") @@ -44,38 +44,38 @@ def test_log_sanitization_quotes(self): expected_part = f'Ref-{unique_id} %22hacker%22' self.get("/sanit_quote", req_hdrs={"Referer": bad_referer}) - + content = self._wait_and_check_log(unique_id) - self.assertIn(expected_part, content, + self.assertIn(expected_part, content, "Double quotes in Referer must be escaped as %22 to prevent log injection.") def test_log_truncation_newlines(self): """ Security Test: Log Injection Protection via Newlines. - - darkhttpd parses headers by reading until \r or \n. - Therefore, if a client sends a header with a newline, darkhttpd + + darkhttpd parses headers by reading until \r or \n. + Therefore, if a client sends a header with a newline, darkhttpd should stop parsing the value at that point. - + The result should be a truncated log entry, NOT a new log line (injection). """ unique_id = random_str() injection_attempt = "127.0.0.1 - - [FAKE LOG ENTRY]" - + # We try to inject a newline into the Referer. # darkhttpd is expected to stop parsing at \n. bad_referer = f"Start-{unique_id}\n{injection_attempt}" - + self.get("/sanit_newline", req_hdrs={"Referer": bad_referer}) content = self._wait_and_check_log(unique_id) - + # 1. Verify truncation: We should see the start, but NOT the injection attempt in the same string context # darkhttpd logic: splits header at \n. 'Referer' becomes just "Start-{unique_id}" self.assertIn(f"Start-{unique_id}", content) - self.assertNotIn(injection_attempt, content, + self.assertNotIn(injection_attempt, content, "The part after the newline should not appear in the log (header parsing should stop at newline).") - + # 2. Verify no duplicate unique_id (ensure the line didn't split into two valid looking lines) count = content.count(unique_id) self.assertEqual(count, 1, "Injection should not result in duplicate markers.") @@ -85,53 +85,53 @@ def test_empty_headers(self): Test that empty headers are logged as empty quotes "" rather than skipped or malformed. """ unique_url = f"/empty_hdrs_{random_str()}" - - # We explicitly set Referer to empty. - # Note: We don't test User-Agent here because TestHelper/Python requests + + # We explicitly set Referer to empty. + # Note: We don't test User-Agent here because TestHelper/Python requests # usually force a default python UA if one isn't provided or is empty. self.get(unique_url, req_hdrs={"Referer": ""}) - + content = self._wait_and_check_log(unique_url) - + found_line = False for line in content.splitlines(): if unique_url in line: found_line = True # Format is: ... "REFERER" "USER_AGENT" # If Referer is empty, we expect: ... "" "..." - self.assertIn('"" "', line, + self.assertIn('"" "', line, f"Empty Referer should be logged as \"\". Logged line: {line}") break - + self.assertTrue(found_line, "Could not find the specific log line for empty headers test.") def test_xff_ignored_by_default(self): """ Security Test: X-Forwarded-For must be IGNORED by default. - - Unless the server is started with --trusted-ip, it must log the + + Unless the server is started with --log-forwarded-for, it must log the actual connection IP (127.0.0.1), not the spoofed header. """ fake_ip = "10.6.6.6" unique_url = f"/xff_test_{random_str()}" - + self.get(unique_url, req_hdrs={"X-Forwarded-For": fake_ip}) - + content = self._wait_and_check_log(unique_url) - + target_line = "" for line in content.splitlines(): if unique_url in line: target_line = line break - + self.assertTrue(target_line, "Log line not found for XFF test") - + # Log starts with IP. Check it is 127.0.0.1 - self.assertTrue(target_line.startswith("127.0.0.1"), + self.assertTrue(target_line.startswith("127.0.0.1"), f"Server must log real IP (127.0.0.1) by default. Logged: {target_line}") - - self.assertNotIn(fake_ip, target_line, + + self.assertNotIn(fake_ip, target_line, "Spoofed X-Forwarded-For IP must not appear in log by default.") if __name__ == '__main__': diff --git a/devel/test_log_xff.py b/devel/test_log_xff.py index 9daad7b..9bcfb7b 100755 --- a/devel/test_log_xff.py +++ b/devel/test_log_xff.py @@ -41,12 +41,12 @@ def test_xff_single_ipv4(self): fake_ip = "10.10.10.10" time_marker = random_str() url = f"/xff_single-{time_marker}" - + self.get(url, req_hdrs={"X-Forwarded-For": fake_ip}) - + line = self._wait_and_check_log(url) # Log line format: IP - - [DATE] "GET ..." - self.assertTrue(line.startswith(fake_ip + " "), + self.assertTrue(line.startswith(fake_ip + " "), f"Expected log to start with {fake_ip}, but got: {line}") def test_xff_multiple_ipv4(self): @@ -59,35 +59,35 @@ def test_xff_multiple_ipv4(self): proxy_ip = "5.6.7.8" # Standard XFF format: client, proxy1, proxy2 header_val = f"{client_ip}, {proxy_ip}, {proxy_ip}" - + time_marker = random_str() url = f"/xff_multi-{time_marker}" - + self.get(url, req_hdrs={"X-Forwarded-For": header_val}) - + line = self._wait_and_check_log(url) - + # Should start with the client IP - self.assertTrue(line.startswith(client_ip + " "), + self.assertTrue(line.startswith(client_ip + " "), f"Expected log to start with first IP ({client_ip}). Log line: {line}") - + # Should NOT contain the proxy IP (it should be stripped) - self.assertNotIn(proxy_ip, line, + self.assertNotIn(proxy_ip, line, "The second IP in the list should be stripped from the log.") def test_xff_ipv6(self): """ - Test a single IPv6 address. + Test a single IPv6 address. Important to ensure logic doesn't break on colons (:). """ ipv6 = "2001:db8::1" time_marker = random_str() url = f"/xff_ipv6-{time_marker}" - + self.get(url, req_hdrs={"X-Forwarded-For": ipv6}) - + line = self._wait_and_check_log(url) - self.assertTrue(line.startswith(ipv6 + " "), + self.assertTrue(line.startswith(ipv6 + " "), f"Expected log to start with IPv6 {ipv6}. Log line: {line}") def test_xff_ipv6_list(self): @@ -98,14 +98,14 @@ def test_xff_ipv6_list(self): client_ipv6 = "2001:0db8:85a3:0000:0000:8a2e:0370:7334" proxy_ip = "192.168.1.1" header_val = f"{client_ipv6}, {proxy_ip}" - + time_marker = random_str() url = f"/xff_ipv6_list-{time_marker}" - + self.get(url, req_hdrs={"X-Forwarded-For": header_val}) - + line = self._wait_and_check_log(url) - self.assertTrue(line.startswith(client_ipv6 + " "), + self.assertTrue(line.startswith(client_ipv6 + " "), f"Expected log to start with {client_ipv6}. Log line: {line}") self.assertNotIn(proxy_ip, line)