X-Forwarded-For handling is unsafe #2474

sorenisanerd · 2020-08-20T22:00:47Z

Allow specifying which headers to use for deducing client IP.

Allow specifying which proxies you trust to provide those headers.

Set defaults to match current behaviour.

codecov · 2020-08-20T22:06:22Z

Codecov Report

Merging #2474 (d0bf406) into master (b01605b) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2474      +/-   ##
==========================================
+ Coverage   98.64%   98.66%   +0.02%     
==========================================
  Files          41       41              
  Lines        1989     2026      +37     
==========================================
+ Hits         1962     1999      +37     
  Misses         15       15              
  Partials       12       12

Impacted Files	Coverage Δ
context.go	`97.72% <100.00%> (+0.21%)`	⬆️
gin.go	`99.02% <100.00%> (+0.01%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b01605b...d0bf406. Read the comment docs.

sorenisanerd · 2020-08-22T14:04:59Z

Squashed according to contribution guidelines.

Breaks API, but immensely improves security Fixes gin-gonic#2473

rubensayshi · 2020-10-01T14:17:17Z

looks good, @thinkerou this really should be merged, it's somewhat dangerous with the current implementation.

and scary to realize this has been going wrong for 6yrs, since it was simplied;
18880f9

billyplus · 2020-11-09T07:23:20Z

If you trust the proxy, the code would be a bit slower to get clientIp than before.

rubensayshi · 2020-11-09T08:37:21Z

no, anyone can add a X-Forwarded-For header to any request and it will be accepted as the clientIp, it's a vulnerability !

billyplus · 2020-11-09T09:05:25Z

If app is not behind proxies, "ForwardedByClientIP" should be set to false, so "X-Forwarded-For" will be ignored.

rubensayshi · 2020-11-09T09:42:37Z

but when it is behind a proxy it's very important that it will parse X-Forwarded-For based on a trusted proxies list, without that it's vulnerable to IP spoofing and that is a rather serious vulnerability!

I guess ForwardedByClientIP could be false by default... or maybe just make TrustedProxies nil by default and if it is then skip parsing X-Forwarded-For?

I don't think the default TrustedProxies should be []string{"0.0.0.0/0"},as this puts us right back at a insecure default and if ClientIP() "just works", then users will never even consider looking into how to configure TrustedProxies ...
From personal experience, testing this stuff is always a bit of a pain, specially when behind multiple proxies (cloud loadbalancer, or cloudflare proxy etc), gin really shouldn't provide an insecure default !

billyplus · 2020-11-09T12:19:19Z

Gin is suposed to be fast.
Mostly app behind proxies should be protected by firewall rules rather than gin itself.
If what you want is truested list, maybe middleware with configuration would fit your need anyway.

sorenisanerd · 2020-11-17T19:00:49Z

You can't solve this problem with firewalls.

I don't want to spend more time on this. Without this patch, spoofing client IP's is a cakewalk and there's no way way to prevent it.

It's only a matter of time before it blows up in someone's face. Not mine, because if the Gin maintainers really don't think this sort of thing is important, who knows what other security problems are left around because speed is considered more important?

rubensayshi · 2020-11-18T14:01:30Z

I agree, this is basically a CVE worthy vulnerability...

hadasbloom · 2020-11-19T14:15:01Z

Hi @sorenh and @rubensayshi,
As part of the Snyk Security team, we contacted the maintainers to urge for their acknowledgement (@sorenh you are CC'd). We see this as a security issue and are willing to assign it a CVE.
Will keep you updated. Thanks for helping us keep open-source safe!

rubensayshi · 2021-01-11T08:38:11Z

@thinkerou this really should get some priority!

billyplus · 2021-01-11T13:15:19Z

Maybe you can define a client IP extractor.
#2608

rubensayshi · 2021-01-11T13:46:34Z

that's not the problem, the problem is that the current implementation for handling X-Forwarded-For is unsafe

cpl · 2021-01-11T14:00:54Z

@rubensayshi

no, anyone can add a X-Forwarded-For header to any request and it will be accepted as the clientIp, it's a vulnerability !

that's not the problem, the problem is that the current implementation for handling X-Forwarded-For is unsafe

I disagree. The issues is not with the current implementation but with the general understanding of the community, for ClientIP(). I for one only expose services behind a Firewall and Proxy (or Loadbalancer). The proxy/LB in my case does a hard Set on a custom Header (X-Foobar-Ip) and then my services only read that one; An attacker cat set XFF or RealIP to whatever they want. The code before was doing an OK job of parsing the possible "Client IP" headers, people should understand their own setups and come with solutions (such as proper Proxy configuration). Calling this a "vulnerability" is a bit of a stretch.

I find this PR to be a complicated and complex "fix" for something that can be managed and avoided with a simple config and general good "hygine".

rubensayshi · 2021-01-11T15:02:54Z

you're both making an great argument that gin should either not handle X-Forwarded-For at all, or do it the right way...

either it needs to completely remove X-Forwarded-For handling (and let the middleware live in a different contrib repo), or it needs to do it properly.

I'm of the opinion it should do it right, but not doing it at all would be acceptable as well to me.

sorenisanerd · 2021-01-11T18:05:05Z

Not doing it at all sounds like a terrible idea to me. If the authors of a framework like Gin can't get this stuff right, expecting application developers to get it right isn't likely to get better results.

sorenisanerd · 2021-01-11T18:07:19Z

that's not the problem, the problem is that the current implementation for handling X-Forwarded-For is unsafe

Indeed.

It's unsafe because you put gin serve directly to your clients. We don't do things like this. we will filter the requests, block proxieds and then send request to gin.

Good for you. Not everyone does and nowhere in the documentation tells you do to this. Also, your setup is not the correct setup for many, many use cases.

sorenisanerd · 2021-01-11T18:09:28Z

This is a complete no-brainer. There's a proposed fix that doesn't even change the default, terrible behavior. I thought leaving the defaults as-is would get this merged in a jiffy.

I'd be more than happy to change the defaults to something that's actually secure, but come on, people. Unless you see actual problems with the patch, what possible reason could there be for not merging it? It solves real problems and doesn't change anything for people who don't care.

sorenisanerd · 2021-01-19T19:32:10Z

@hadasbloom I think a CVE for this issue is perfectly reasonable, whether it gets fixed right now today or not. It's an existing vulnerability. Whether it ever gets fixed is anybody's guess at this point, but anything to alert users of this problem would be good.

hadasbloom · 2021-01-20T17:14:08Z

@sorenh I agree and hope it will be fixed soon.

This was assigned the cve - CVE-2020-28483.
You will be able to see it soon also on the Snyk website here: https://snyk.io/vuln/SNYK-GOLANG-GITHUBCOMGINGONICGIN-1041736

ichiro999 · 2021-01-23T07:08:58Z

@sorenh I agree and hope it will be fixed soon.

appleboy · 2021-01-23T14:53:14Z

@thinkerou @javierprovecho We need to take a look.

crashdump · 2021-01-24T14:53:04Z

This was flagged up by our vulnerability management tools, and I agree with the above, this behaviour needs to either be documented clearly or fixed.

Great to see the maintainers jumping in and acknowledging that something needs to be done to fix this security issue. A shame we had to wait 6 month though, especially since someone spent some time to fix this at the time. It will raise question on how serious the team is about security.

manucorporat · 2021-02-07T21:29:15Z

context.go

+}
+
+func isTrustedProxy(ip net.IP, e *Engine) bool {
+	for _, trustedProxy := range e.TrustedProxies {


might be worth it to precompute the array of CIDR on engine.Run() and save it to: engine.trustedCIDR so we don't need to parse the list of trusted proxies every time, the ClientIP() is called

manucorporat · 2021-02-07T21:40:47Z

Original maintainer here! happy to jump and help to get this merged asap!

javierprovecho · 2021-02-07T23:54:20Z

After reviewing this PR we had some feedback about implementation to apply. Therefore @manucorporat opened a new DRAFT PR here, keeping the commit history: #2632

javierprovecho

Do not merge, we’re reviewing this PR.

sorenisanerd · 2021-02-09T14:28:26Z

I don't really understand why you can ignore this for almost 6 months and then suddenly hijack it. 🤦 Extremely bad form, people.

manucorporat · 2021-02-09T14:58:55Z

We didn't hijack it, but I offered a different solution, also I forked your PR as a way you ensure you will be a Co-Author. Nothing has been merged yet, both PRs are open!

While I am the original creator of Gin I have been out of the project for years already, just jumped to offer some help :)

sorenisanerd · 2021-02-09T20:30:06Z

That sounds like textbook hijacking to me. I've been doing this stuff for decades, so idgaf, but this kind of thing drives new contributors away.

I put time and effort into fixing this security issue. Then it gets ignored for 6 months and then, instead of providing constructive feedback, you just create another PR to fix it up. Especially when it's been sitting there for 6 months and then all of a sudden it can't wait another few days for me to address your concerns?

Why even bother trying?

Again, I've grown a lot of thick skin over the years, so I personally don't care, but if you care at all about future contributors, don't do this again.

mbana · 2021-02-18T16:53:54Z

@sorenh I agree and hope it will be fixed soon.

This was assigned the cve - CVE-2020-28483.
You will be able to see it soon also on the Snyk website here: https://snyk.io/vuln/SNYK-GOLANG-GITHUBCOMGINGONICGIN-1041736

I'm in the same boat as above.

Any updates on this pull request? For example, when a new version with a "fix" will be released?

sorenisanerd · 2021-02-19T22:05:06Z

Any updates on this pull request? For example, when a new version with a "fix" will be released?

This pull request is for the 6 month old, functional patch that succesfully addresses a CVE, but the maintainers don't want.

#2632 is the one that might have a chance of getting merged someday.

Also: Seriously reconsider if you can use something other than gin. Since this is the attitude towards security problems, I'm out.

sorenisanerd · 2021-03-30T01:53:08Z

Wow. Just wow.

sorenisanerd changed the title ~~X-Forwarded-For handling is unsafe~~ WIP: X-Forwarded-For handling is unsafe Aug 21, 2020

sorenisanerd changed the title ~~WIP: X-Forwarded-For handling is unsafe~~ X-Forwarded-For handling is unsafe Aug 21, 2020

sorenisanerd mentioned this pull request Aug 21, 2020

ClientIP() using X-Forwarded-For and X-Real-Ip should be opt-in #1684

Closed

sorenisanerd force-pushed the sorenh/issue2473 branch from 0ca3c51 to 12da875 Compare August 22, 2020 14:03

X-Forwarded-For handling is unsafe

de0f9eb

Breaks API, but immensely improves security Fixes gin-gonic#2473

sorenisanerd force-pushed the sorenh/issue2473 branch from 12da875 to de0f9eb Compare August 22, 2020 14:06

saiya mentioned this pull request Dec 14, 2020

gin -> net/http + httprouter saiya/dsps#34

Merged

appleboy added bug enhancement labels Jan 4, 2021

appleboy requested review from appleboy and thinkerou January 4, 2021 08:25

appleboy added this to the v1.8 milestone Jan 4, 2021

appleboy and others added 2 commits January 4, 2021 16:27

Merge branch 'master' into sorenh/issue2473

b343e7e

Merge branch 'master' into sorenh/issue2473

b7a35bf

billyplus mentioned this pull request Jan 11, 2021

customizable client ip extractor #2608

Closed

Merge branch 'master' into sorenh/issue2473

d0bf406

buixor mentioned this pull request Feb 2, 2021

Fix default configurations crowdsecurity/crowdsec#597

Merged

manucorporat reviewed Feb 7, 2021

View reviewed changes

javierprovecho assigned manucorporat and javierprovecho Feb 7, 2021

manucorporat requested review from austinheap, javierprovecho and manucorporat February 7, 2021 21:51

javierprovecho requested changes Feb 8, 2021

View reviewed changes

sorenisanerd closed this Feb 9, 2021

melnikovio mentioned this pull request Sep 7, 2021

fix: 🎨 updated gin to 1.7.4 to fix CVE-2020-28483 rs/cors#119

Closed

rwos mentioned this pull request Sep 7, 2021

X-Forwarded-For handling is still unsafe, CVE-2020-28483 is NOT fixed #2862

Closed

yuina1056 mentioned this pull request Dec 22, 2021

fix CVE-2020-28483 rs/cors#124

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

X-Forwarded-For handling is unsafe #2474

X-Forwarded-For handling is unsafe #2474

sorenisanerd commented Aug 20, 2020 •

edited

codecov bot commented Aug 20, 2020 •

edited

sorenisanerd commented Aug 22, 2020

rubensayshi commented Oct 1, 2020

billyplus commented Nov 9, 2020

rubensayshi commented Nov 9, 2020 •

edited

billyplus commented Nov 9, 2020

rubensayshi commented Nov 9, 2020 •

edited

billyplus commented Nov 9, 2020

sorenisanerd commented Nov 17, 2020

rubensayshi commented Nov 18, 2020

hadasbloom commented Nov 19, 2020

rubensayshi commented Jan 11, 2021

billyplus commented Jan 11, 2021

rubensayshi commented Jan 11, 2021

cpl commented Jan 11, 2021

rubensayshi commented Jan 11, 2021 •

edited

sorenisanerd commented Jan 11, 2021

sorenisanerd commented Jan 11, 2021

sorenisanerd commented Jan 11, 2021

sorenisanerd commented Jan 19, 2021

hadasbloom commented Jan 20, 2021

ichiro999 commented Jan 23, 2021

appleboy commented Jan 23, 2021

crashdump commented Jan 24, 2021

manucorporat Feb 7, 2021

manucorporat commented Feb 7, 2021

javierprovecho commented Feb 7, 2021

javierprovecho left a comment

sorenisanerd commented Feb 9, 2021

manucorporat commented Feb 9, 2021

sorenisanerd commented Feb 9, 2021

mbana commented Feb 18, 2021 •

edited

sorenisanerd commented Feb 19, 2021

sorenisanerd commented Mar 30, 2021

X-Forwarded-For handling is unsafe #2474

X-Forwarded-For handling is unsafe #2474

Conversation

sorenisanerd commented Aug 20, 2020 • edited

codecov bot commented Aug 20, 2020 • edited

Codecov Report

sorenisanerd commented Aug 22, 2020

rubensayshi commented Oct 1, 2020

billyplus commented Nov 9, 2020

rubensayshi commented Nov 9, 2020 • edited

billyplus commented Nov 9, 2020

rubensayshi commented Nov 9, 2020 • edited

billyplus commented Nov 9, 2020

sorenisanerd commented Nov 17, 2020

rubensayshi commented Nov 18, 2020

hadasbloom commented Nov 19, 2020

rubensayshi commented Jan 11, 2021

billyplus commented Jan 11, 2021

rubensayshi commented Jan 11, 2021

cpl commented Jan 11, 2021

rubensayshi commented Jan 11, 2021 • edited

sorenisanerd commented Jan 11, 2021

sorenisanerd commented Jan 11, 2021

sorenisanerd commented Jan 11, 2021

sorenisanerd commented Jan 19, 2021

hadasbloom commented Jan 20, 2021

ichiro999 commented Jan 23, 2021

appleboy commented Jan 23, 2021

crashdump commented Jan 24, 2021

manucorporat Feb 7, 2021

Choose a reason for hiding this comment

manucorporat commented Feb 7, 2021

javierprovecho commented Feb 7, 2021

javierprovecho left a comment

Choose a reason for hiding this comment

sorenisanerd commented Feb 9, 2021

manucorporat commented Feb 9, 2021

sorenisanerd commented Feb 9, 2021

mbana commented Feb 18, 2021 • edited

sorenisanerd commented Feb 19, 2021

sorenisanerd commented Mar 30, 2021

sorenisanerd commented Aug 20, 2020 •

edited

codecov bot commented Aug 20, 2020 •

edited

rubensayshi commented Nov 9, 2020 •

edited

rubensayshi commented Nov 9, 2020 •

edited

rubensayshi commented Jan 11, 2021 •

edited

mbana commented Feb 18, 2021 •

edited