ClientIP: check every proxy for trustiness #2844
Conversation
agmt
changed the title
|
@sorenisanerd May you please help, does this PR fix the issue you raised? |
|
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. |
|
@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 { | |||
There was a problem hiding this comment.
@Bisstocuz such check is already in isTrustedProxy(): context.go:769 . Are you suggesting to copy this check to have in both places?
|
need @sorenisanerd to help or confirm the PR. |
context.go
Outdated
| @@ -765,6 +765,17 @@ func (c *Context) ClientIP() string { | |||
| return remoteIP.String() | |||
| } | |||
|
|
|||
| func isTrustedProxy(ip net.IP, e *Engine) bool { | |||
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
func (e *Engine) validateHeader(header string) (clientIP string, valid bool) { }
agmt
force-pushed
the
fix/fix-client-ip-cve
branch
from
0112747 to
dd71c72
Compare
agmt
force-pushed
the
fix/fix-client-ip-cve
branch
from
dd71c72 to
0d63d79
Compare
|
@Bisstocuz rebased on master, put |
|
@appleboy @thinkerou Hi, could you please run the tests? |
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
@thinkerou need your review. |
|
@appleboy @thinkerou I am using the latest release v1.7.4, our Image vulnerability scanner complains about |
(cherry picked from commit 5929d52)
(cherry picked from commit 5929d52)
(cherry picked from commit 5929d52)
(cherry picked from commit 5929d52)
(cherry picked from commit 5929d52)
Fix for #2473
Despite it is marked as fixed by #2632 , it is not.
X-Forwarded-Foris 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 ofX-Forwarded-Forlist, then all IPs to the left may be a fake and the rightmost untrusted IP is a real client IP.