Caddy: Caddy allows writing fake IP addresses to log file

[fd0]

1. What version of Caddy are you running (caddy -version)?

Caddy 0.9.0

2. What are you trying to do?

Running a web server which writes correct log files.

3. What is your entire Caddyfile?

fd0.me, www.fd0.me {
    root /srv/web/fd0.me/htdocs
    log /srv/web/fd0.me/logs/access.log
    errors /srv/web/fd0.me/logs/error.log

    header / Strict-Transport-Security max-age=31536000

    ext .html .thtml
    templates / .thtml
}

4. How did you run Caddy (give the full command and describe the execution environment)?

Debian stable
caddy -conf=/etc/caddy/Caddyfile -log=/dev/stderr -email=root@XXX -agree=true -pidfile=/var/run/caddy/caddy.pid

5. What did you expect to see?

The IP address of the HTTP client written to the log files.

6. What did you see instead (give full error messages and/or log)?

The contents of the HTTP request header X-Forwarded-For (which can be completely controlled by the client) is written to the log file instead of the client's real IP address. So by e.g. using cURL I can fake log entries like this:

$ curl -H 'X-Forwarded-For: 127.0.0.1' https://fd0.me/secret

The logfile then contains the following line:

127.0.0.1 - [10/Aug/2016:18:55:51 +0200] "GET /secret HTTP/1.1" 404 14

There is no trace of the requesting IP address in the log file at all. In fact, whatever is transmitted in this header's value is written to the logfile.

7. How can someone who is starting from scratch reproduce this behavior as minimally as possible?

Start caddy locally on port 9000 with a Caddyfile that configures an access log file. Then use cURL to write arbitrary data to the log file like this:

$ curl -H 'X-Forwarded-For: foobar this is a test' http://localhost:9000

The logfile will then contain:

foobar this is a test - [10/Aug/2016:18:58:11 +0200] "GET / HTTP/1.1" 200 5

[fd0]

This may be security relevant for people relying on caddy to write accurate access logs

[mholt]

I don't see this as a security issue. Besides, if someone wanted to be malicious with an IP address, they could just perform IP spoofing and really mess things up.

We print the value of this field on purpose because if the request was proxied for another IP that's the one we want to log, generally. I think it came from a pull request way back when, but I can't find it right now.

[fd0]

I disagree, spoofing the IP address in a way that a TCP connection is established is hard to do remotely, since the server's response packets must reach the attacker. It is only realistic for attackers who are on the upstream path of the server, e.g. at the data center or at the ISP. Otherwise, even if attackers successfully send out IP packets with a fake source IP address, the response packets are routed to the original owner of the IP address and no TCP connection is established.

At the moment caddy logs whatever the client sent in the X-Forwarded-For header, so the IP address field in access log files written by an instance of caddy is not trustworthy at all. Unfortunately, that's also the case for all logs written since this feature was implemented. That's unexpected, to say the least, and is very different from what other HTTP servers like Apache and nginx do by default.

I think logging the header value instead of the real remote IP address should be disabled by default and only enabled explicitly with e.g. a config option. That's the way it is handled with most other HTTP server software. This feature is only needed when a (trustworthy!) reverse proxy or load balancer, which save the original IP address to this header and throws away the value the original client sent there, is used in front of caddy. I think that this is not the typical use case.

The X-Forwarded-For header is also inserted by (forward) HTTP proxies e.g. at larger organisations. In this case, caddy will log the internal IP address of the user visiting the site. Usually this will be an RFC1918 ("private") IP address, so it's of no use for me as a server administrator.

[fd0]

Here is a short explanation with a diagram that explains why IP spoofing with TCP is hard to do: http://security.stackexchange.com/a/105680

[mholt]

since the server's response packets must reach the attacker.

Not necessarily, an attacker might just be DDOSing and doesn't care about responses.

At the moment caddy logs whatever the client sent in the X-Forwarded-For header ... That's unexpected

Perhaps to some -- I can understand that. It just depends on the situation like you said. I suppose we can already log this field manually by using {>X-Forwarded-For}.

This behavior was introduced here -- @nemothekid - what do you think?

[nemosupremo]

You're right that this isn't how nginx operates by default. A quick fix, if you believe this to be a serious security issue would be to just remove the 3 offending lines.

A longer term fix would be to figure out how we turn this into a config parameter, either by directive, or another replacer (ex, real_ip which is X-Fwd -> Remote, and keeping remote as the TCP ip).

[fd0]

since the server's response packets must reach the attacker.

Not necessarily, an attacker might just be DDOSing and doesn't care about responses.

But that's a completely different scenario: DDoS vs. making caddy log fake IP addresses. I think that's not the point.

[meyskens]

A propper fix may be how Expressjs does it a trust proxy option that you can set/unset so it knows if it is behind one or not.

[mholt]

Is it a common situation to be receiving requests from both proxied and non-proxied clients? In other words, if you know that X-Forwarded-For is going to contain the IP address you want, then the site owner could just use {>X-Forwaded-For} instead of {remote}. This is only hard to solve if we have to guess which one to use on the site owner's behalf. I'd rather not introduce new configuration just for this.

So, assuming that BOTH values aren't needed by the same site very often or ever, I'm leaning towards just removing those 3 lines.

[slightfoot]

We already have a plugin that enables the feature of accepting X-Forwarded-For and/or only from certain IP ranges.
https://caddyserver.com/docs/realip

Why would you also have this in built-in? To be honest the plugin provides a lot more control, and I didn't even expect Caddy to pay attention to the X-Forwarded-For until I read this issue ticket I thought it didn't.

It's normal to only pay attention to the X-Forwarded-For only from say the CIDR of your Load Balancers, so the Plugin is the way to go.

[mholt]

@slightfoot

Why would you also have this in built-in? To be honest the plugin provides a lot more control

I agree with you. The reason is that the realip plugin didn't exist a year ago.

Anyone opposed to just removing the 3 lines? If not I can do this tomorrow.

[fd0]

:+1: from me, I think it's a good idea to remove the lines and only log the connecting IP address by default

[fd0]

Thanks!