Incorrect IP Logging for Failed Login Attempts and Potential Vulnerability to IP Spoofing

[zylxpl]

Prerequisites

• [X] I have checked the Wiki and Discussions and found no answer

• [X] I have searched other issues and found no duplicates

• [X] I want to report a bug and not ask a question or ask for help

• [X] I have set up AdGuard Home correctly and configured clients to use it. (Use the Discussions for help with installing and configuring clients.)

Platform (OS and CPU architecture)

Linux, ARM64

Installation

Docker

Setup

Other (please mention in the description)

AdGuard Home version

v0.107.48

Action

AHD behind rev proxy. On failed login attempts, the logs show the reverse proxy's IP address instead of the real user's IP address. Successful login attempts, however, are logged with the correct real user IP.

Expected result

If the reverse proxy is trusted, the logs should always display the real user IP address, retrieved for example from the XFF header.

Actual result

Failed login attempts are logged with the reverse proxy's IP address, which is not useful for auditing purposes. In contrast, successful login attempts are logged with the correct user IP address even for untrusted proxies - this might be vulnerable to IP spoofing attacks, where malicious actors could forge the X-Forwarded-For header to hide their identity.

Additional information and/or screenshots

[PixelHir]

Same issue over here, can successfully reproduce on a host networking container, with traefik as reverse proxy. (proxy is to https port, not http)

IP not censored as its iCloud private relay anyways

[bytebone]

This issue is both frustrating and interesting. You'd think this is a silly issue to have and easy to solve. But the deeper I dig the more confused I become.

The log for successful logins comes from here https://github.com/AdguardTeam/AdGuardHome/blob/7d479baba6ec14fec0fd9c701a5cf005f6944410/internal/home/authhttp.go#L187 Here, the value ip is used, which comes from line 170, just above, and the function realIP(). This function is defined in line 77, and pulls the IP from the different proxy headers, as you would expect. If there are no such headers, it falls back to the first X-Forwarded-For header, and then to the net/http request.RemoteAddr value. A good looking hierarchy.

Just 5 lines above the successful login, the error is printed, and the method is completely different: https://github.com/AdguardTeam/AdGuardHome/blob/7d479baba6ec14fec0fd9c701a5cf005f6944410/internal/home/authhttp.go#L175-L185 Suddenly we have a whole new function for printing errors, which in this case does not use the, already defined and properly saturated, ip value from before, but a new logIP, which is pulled in line 141 from the golang net/http type request.RemoteAddr. This is always the client IP and never considers proxy IPs. After assigning remoteIP (the always client IP value) to logIP, the latter get's possibly re-set: https://github.com/AdguardTeam/AdGuardHome/blob/7d479baba6ec14fec0fd9c701a5cf005f6944410/internal/home/authhttp.go#L178-L180 So if the ip (the real IP pulled from the proxy headers) is part of the trustedProxies setting in the AGH-Config, the logIP would be overwritten with the ip.

One just has to wonder - why would my real, non-proxy IP be in the trustedProxies of the AGH config? Isn't this a condition that will never realistically be true? Of so, of course failed logins will always show the proxy IP.

As for a fix - I can think of two rather quick and easy solutions:

1. When checking if the request remote address is part of the trusted proxies, use the proper value for the check:

cookie, err := globalContext.auth.newCookie(req, remoteIP)
	if err != nil {
		logIP := remoteIP
		if globalContext.auth.trustedProxies.Contains(remoteIP) {
			logIP = ip.String()
		}

		writeErrorWithIP(r, w, http.StatusForbidden, logIP, "%s", err)

		return
	}

This doesn't seem proper as, when handling proxy IPs, you should usually work your way up the proxy IP chain one by one, checking every time if the current IP is trusted or not. In this implementation, you just check if the lowest IP of the chain is trusted, and if it is, skip all the way to the top of the chain.

2. Skip the trustedProxy check alltogether and directly use the realIP

cookie, err := globalContext.auth.newCookie(req, remoteIP)
	if err != nil {
		writeErrorWithIP(r, w, http.StatusForbidden, ip, "%s", err)

		return
	}

Depending on how you see the function of the trustedProxy setting, this may or may not be tolerable. If it's just aimed at DoH requests, this solution should be fine. After all, this entire file is aimed at web-UI auth. If it's meant to apply to all HTTP requests, the whole HTTP stack may need a larger rework, as the order of operations seems fundamentally flawed to my amateur eye.

I also wonder, since we're passing the remoteIP (which will be a local IP in the reverse proxy setup) to the newCookie function, if there are any security implications from this, since the cookie isn't created for my individual IP. Wouldn't anyone and everyone's cookie be assigned to the same IP, since it is created for the reverse proxy IP?

In any case, as you can see, it's not as easy as you'd imagine. Since it's probably a larger (maybe even philosophical) issue, I won't attempt a patch. But I still hope that we can get this worked on, because it shouldn't be this complicated in the first place.