We received a vulnerability disclosure due to how we pick a remote IP address.

[didip]

Disclosure URL: https://adam-p.ca/blog/2022/03/x-forwarded-for/

This is an attempt to address the situation.

1. We no longer configure SetIPLookups on default.

2. We address the two different SetIPLookups confusion in two different place by removing both of them.

3. We add a new, explicit way, for user to define how IP address should be picked up.

Tests are all updated to use the new method of picking IP address.

This will be a backward incompatible change so version number has to be bumped to 8.

[adam-p]

Overall I think it's a good approach.

Instead of a fixed list of supported headers, I think you should support any arbitrary header. X-Forwarded-For (and maybe Forwarded, in the future) requires special handling (because it's a list). RemoteAddr needs special handling (because it's not a header). But all other headers used for this purpose are single-value IPs. As it stands, you don't support Fastly-Client-IP, True-Client-IP, X-Azure-SocketIP, or any of the arbitrary (and sometimes unique) headers used for this purpose. So, there's a whole category of single-IP headers that I think you can handle without hard-coding. (But make sure to run the user input through http.CanonicalHeaderKey().)

(It's also a little confusing for there to be IndexFromRight for the single-IP headers. If the list and non-list headers were treated differently, maybe there wouldn't have to be.)

If you really want to restrict header use to just a few that are supported, I think you should use enum constants rather than strings. It's too easy to think that you can put whatever header you want there. Or make a typo when adding a supported one. (And then it'll silently not rate-limit, which I'll mention below.)

I would like there to be more explanation of why IndexFromRight depends on your network architecture. But maybe that's outside the scope of this.

I don't think you should have HeaderIndexFromRight. Instead you should combine all the XFF headers into one list. If the user is checking the IP added by a reverse proxy they own (which they should), then probably it's in the last XFF header, but see the "hypothetical attack" here, where I suggest it might be possible (maybe) for an attacker to force the creation of another header. Anyway, the best thing to do is to treat all the XFF headers as a single list.

I don't like how it (still) "fails open" (doesn't rate-limit) when there's a failure to get the IP. I worry that if the tollbooth user misconfigures it, tollbooth will silently seem like it's working but not actually do anything. (Yes, the dev should actually test, but...)

If you stuck with a fixed set of supported headers, then one thing you could do is panic if the user calls lmt.SetIPLookup with an unsupported Name. Yeah, it's a runtime failure, which isn't great, but it'll probably happen on startup, so it's easy to notice. Alternatively, you could use typed enum constants instead of strings, which will effectively be a compile-time check.

If, as I suggest above, you support arbitrary single-IP headers, it's trickier. My preference would still be to panic, but I can see why that's not super attractive. Instead of happening at server-startup time, it would happen when the first request came in. (tollbooth is configured to look at Fastly-Typo-IP; request has Fastly-Client-IP, but the typo header isn't found; so no IP; panic.)

Another possibility, of course is to "fail closed". Instead of ShouldSkipLimiter returning true if there's no IP, instead the request is rejected. The server won't panic, but all requests will be rejected. In my opinion that's not better, since it will probably be harder to notice and diagnose that something's wrong.

Another possibility is to fall back to checking RemoteAddr, which is guaranteed to be non-empty. Except... then the server will end up rate-limiting the reverse proxy. Most-but-not-all requests will fail. And it'll even harder to notice and diagnose (depending on what the server is logging).

Somewhat related to the "fail open" stuff is what RemoteIPFromIPLookup should do if headerIndex ends up negative. You have it using the zeroth index, in that case. But a negative value indicates a configuration failure, so... the same considerations as above apply.

And somewhat related to that... If the user sets HeaderIndexFromRight to like 1000, then they are effectively getting the leftmost XFF IP. Then all the leftmost-ish stuff I said in my post applies, and you should probably be making sure you're getting a valid IP (and not spoofy garbage, and not a private IP).

---

ETA: Regarding configuration failure behaviour, it's also worth mentioning returning an error rather than panicking. You'll still have to decide whether to fail open or close, but at least it won't be silent. (I didn't mention this at first because @didip said in email, "Returning an error is certainly bad from app stability perspective". Which is fair, but it should still be explicitly considered here.)

[didip]

@adam-p If we combine multiple of the same headers into 1 list, then when you say: “give me the first ip from the right”, what does it exactly mean precisely? Just do [array1] + [array2] and then pick the last ip?

[adam-p]

If we combine multiple of the same headers into 1 list, then when you say: “give me the first ip from the right”, what does it exactly mean precisely? Just do [array1] + [array2] and then pick the last ip?

Yes.

Combining the headers the headers like that is specifically allowed by the HTTP RFC, and I've seen AWS ALB do it. The resulting list of them represents the same order that they were added. So counting in from the right becomes easy and makes sense.

[adam-p]

@didip Can you replace the gist disclosure link in your comment with a link to the actual post: https://adam-p.ca/blog/2022/03/x-forwarded-for/

Because I, uh, accidentally deleted the gist. (I forgot for a moment that it was used here. And the post has superseded it.)

[didip]

ok, no problem.

…

On Sat, Mar 5, 2022 at 5:59 AM Adam Pritchard ***@***.***> wrote: @didip <https://github.com/didip> Can you replace the gist disclosure link in your comment with a link to the actual post: https://adam-p.ca/blog/2022/03/x-forwarded-for/ Because I, uh, accidentally deleted the gist. (I forgot for a moment that it was used here. And the post has superseded it.) — Reply to this email directly, view it on GitHub <#99 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAARZVU6F536OA2XHAQHBHDU6NSCRANCNFSM5P4SFZPA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[adam-p]

I wrote a library to help with extracting the "real" client IP from a request. I intended that it be useful in cases like this, so please let me know if it's suitable (and if it's not we'll figure out how to make it better).
https://github.com/realclientip/realclientip-go

[ribtoks]

Has this issue been resolved otherwise (e.g. different branch etc.) or what is the reason changes didn't get merged?

[AidanWelch]

This needs to be merged, this isn't functional as a ratelimiting package by default when it can be circumvented this easily:

req.Header.Add("X-Forwarded-For", strconv.Itoa(rand.Int()))

[AidanWelch]

Thank you!!