Skip to content
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.

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

ClientIP: check every proxy for trustiness #2844

Merged
merged 1 commit into from Oct 9, 2021

Conversation

agmt
Copy link
Contributor

@agmt agmt commented Aug 24, 2021

Fix for #2473

Despite it is marked as fixed by #2632 , it is not.
X-Forwarded-For is a chain of previous remote addresses, appended by each proxy. It means that this list should be parsed in reverse order and parsing should be stopped once untrusted IP found. If we find an untrusted IP in the middle of X-Forwarded-For list, then all IPs to the left may be a fake and the rightmost untrusted IP is a real client IP.

@agmt agmt changed the title ClientIP: check every proxy for trustness ClientIP: check every proxy for trustiness Aug 24, 2021
@agmt
Copy link
Contributor Author

agmt commented Aug 24, 2021

@sorenisanerd May you please help, does this PR fix the issue you raised?

@sorenisanerd
Copy link
Contributor

Honestly, I don't know. I provided a fix. They ignored it for 6 months, then messed it up so that it didn't pass the tests. To address that, they changed the tests. Then I wrote them a library they could use. They ignored that, too. I have no reason to believe that they'll act any differently now. I will not be using Gin. If this is their attitude towards security problems, there's no way I'm exposing it to a network.

Copy link
Contributor

@Bisstocuz Bisstocuz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check out my comments below plz.

@jim3ma
Copy link

jim3ma commented Sep 24, 2021

@appleboy Can you review this PR and try to merge it ?

@@ -779,35 +790,25 @@ func (c *Context) RemoteIP() (net.IP, bool) {
return nil, false
}

if c.engine.trustedCIDRs != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Bisstocuz such check is already in isTrustedProxy(): context.go:769 . Are you suggesting to copy this check to have in both places?

@appleboy
Copy link
Member

need @sorenisanerd to help or confirm the PR.

@appleboy appleboy added the bug label Sep 28, 2021
@appleboy appleboy added this to the v1.7.5 milestone Sep 28, 2021
Copy link
Contributor

@Bisstocuz Bisstocuz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As usual in this project, I think putting (e *Engine) front is more reasonable.

And, #2887 was merged.
Please merge from upstream and handle conflicts.

context.go Outdated
@@ -765,6 +765,17 @@ func (c *Context) ClientIP() string {
return remoteIP.String()
}

func isTrustedProxy(ip net.IP, e *Engine) bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

func (e *Engine) isTrustedProxy(ip net.IP) bool { }

context.go Outdated
}

func validateHeader(header string) (clientIP string, valid bool) {
func validateHeader(header string, e *Engine) (clientIP string, valid bool) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

func (e *Engine) validateHeader(header string) (clientIP string, valid bool) { }

@agmt
Copy link
Contributor Author

agmt commented Oct 7, 2021

@Bisstocuz rebased on master, put (e *Engine) into front.

@Bisstocuz
Copy link
Contributor

@appleboy @thinkerou Hi, could you please run the tests?

@codecov
Copy link

codecov bot commented Oct 9, 2021

Codecov Report

Merging #2844 (0d63d79) into master (21125bb) will increase coverage by 98.72%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #2844       +/-   ##
===========================================
+ Coverage        0   98.72%   +98.72%     
===========================================
  Files           0       41       +41     
  Lines           0     3066     +3066     
===========================================
+ Hits            0     3027     +3027     
- Misses          0       27       +27     
- Partials        0       12       +12     
Flag Coverage Δ
go-1.13 98.72% <100.00%> (?)
go-1.14 98.56% <100.00%> (?)
go-1.15 98.56% <100.00%> (?)
go-1.16 98.56% <100.00%> (?)
go-1.17 98.46% <100.00%> (?)
macos-latest 98.72% <100.00%> (?)
nomsgpack 98.70% <100.00%> (?)
ubuntu-latest 98.72% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
context.go 97.82% <100.00%> (ø)
utils.go 96.80% <0.00%> (ø)
binding/query.go 100.00% <0.00%> (ø)
render/yaml.go 100.00% <0.00%> (ø)
binding/default_validator.go 100.00% <0.00%> (ø)
logger.go 100.00% <0.00%> (ø)
render/json.go 83.14% <0.00%> (ø)
binding/binding_nomsgpack.go 100.00% <0.00%> (ø)
path.go 100.00% <0.00%> (ø)
render/protobuf.go 100.00% <0.00%> (ø)
... and 32 more

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 21125bb...0d63d79. Read the comment docs.

@appleboy
Copy link
Member

appleboy commented Oct 9, 2021

@thinkerou need your review.

Copy link
Member

@thinkerou thinkerou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@appleboy appleboy merged commit 5929d52 into gin-gonic:master Oct 9, 2021
@mrgadgil
Copy link

@appleboy @thinkerou I am using the latest release v1.7.4, our Image vulnerability scanner complains about CVE-2020-28483`` Is the CVE-2020-28483` fixed? From the pull request it looks like the CVE-2020-28483 is fixed. Want to know if it is just a matter of creating a new release?

Bisstocuz pushed a commit to Bisstocuz/gin that referenced this pull request Nov 22, 2021
Bisstocuz pushed a commit to Bisstocuz/gin that referenced this pull request Nov 22, 2021
Bisstocuz pushed a commit to Bisstocuz/gin that referenced this pull request Nov 22, 2021
Bisstocuz pushed a commit to Bisstocuz/gin that referenced this pull request Nov 22, 2021
Bisstocuz pushed a commit to Bisstocuz/gin that referenced this pull request Nov 23, 2021
daheige pushed a commit to daheige/gin that referenced this pull request Apr 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants