Skip to content

Don't require proxy ip to use X-Forwarded-For header #104

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ii8 wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Don't require proxy ip to use X-Forwarded-For header #104

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 29 additions & 39 deletions darkhttpd.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,11 +329,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;
Expand Down Expand Up @@ -1008,9 +1007,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"
Expand Down Expand Up @@ -1235,20 +1234,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;
Expand Down Expand Up @@ -1412,8 +1399,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)
Expand All @@ -1423,20 +1409,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); \
Expand All @@ -1446,12 +1418,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,
Expand Down Expand Up @@ -1482,7 +1463,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
Expand Down Expand Up @@ -1972,8 +1953,17 @@ static int parse_request(struct connection *conn) {
conn->referer = parse_field(conn, "Referer: ");
conn->user_agent = parse_field(conn, "User-Agent: ");
conn->authorization = parse_field(conn, "Authorization: ");
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, "X-Forwarded-For: ");
if (conn->forwarded_for != NULL) {
if ((tmp = strchr(conn->forwarded_for, ',')) != NULL)
*tmp = '\0';
}
}
parse_range_field(conn);
return 1;
}
Expand Down Expand Up @@ -2174,7 +2164,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, "<a href=\"");
Expand Down
2 changes: 1 addition & 1 deletion devel/fuzz_parse_request.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) {
/* Enable some server options to exercise more code paths. */
trusted_ip = "0.0.0.0";
want_log_forwarded_for = 1;
logfile = stdout;

struct connection *conn = new_connection();
Expand Down
4 changes: 2 additions & 2 deletions devel/run-tests
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,9 +78,9 @@ runtests() {
kill $PID
wait $PID

echo "===> run --trusted-ip tests"
echo "===> run --log-forwarded-for tests"
rm -f test_xff.log
./a.out $DIR --port $PORT --addr $ADDR --log test_xff.log --trusted-ip $ADDR \
./a.out $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
Expand Down
58 changes: 29 additions & 29 deletions devel/test_log.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}")

Expand All @@ -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.")
Expand All @@ -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__':
Expand Down
36 changes: 18 additions & 18 deletions devel/test_log_xff.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand All @@ -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):
Expand All @@ -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)

Expand Down