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

fix: ClientIP handling is unsafe #401

Merged
merged 1 commit into from
Feb 15, 2023

Conversation

BaiZe1998
Copy link
Contributor

@BaiZe1998 BaiZe1998 commented Nov 20, 2022

What type of PR is this?

fix

Check the PR title.

  • This PR title match the format: <type>(optional scope): <description>
  • The description of this PR title is user-oriented and clear enough for others to understand.

(Optional) Translate the PR title into Chinese.

修复使用 ClientIP() 获取客户端 IP 时存在的安全问题

(Optional) More detail description for this PR(en: English/zh: Chinese).

en:
Problems.

  1. The current ClientIP gets the client IP only by getting the "X-Real-IP" and "X-Forwarded-For" fields in the request header, which can be forged directly. The server has no ability to discern this.
  2. The "X-Forwarded-For" header is intended to be used to append the IP address of the proxy server in order when a proxy server is forwarded, and when requesting a client IP, it should be resolved in reverse until the first IP is resolved as the initial client IP (why not just get the first IP?). Because the proxy forwarding process is not necessarily completely secure, if an illegal proxy server px is added in the middle, such as client -> p0 -> p1 -> server becomes client -> p0 -> px -> p1 -> server then for security reasons, the ClientIP method should be called to get the IP of px instead of the client (which is why it is resolved in reverse order, rather than directly to the first IP in the "X-Forwarded-For" list).

Solution.
Referring to gin's fix idea, add TrustedProxies list to store a list of trusted proxy server addresses for the server, by default "0.0.0.0", the logic within the ClientIP method is as follows.

  1. need to distinguish the difference between client accessing server directly and client accessing server through a proxy.
  2. ctx.RemoteAddr() gets the IP address of the requesting party and determines whether the IP of the requesting party is in the trusted list, and returns the IP if the ClientIP is not trusted.
  3. If it is in the trusted list, it gets the headers of "X-Real-IP" and "X-Forwarded-For", and parses them in reverse order (the difference between the two is that X-Forwarded-For records the append result of the IP at the time of forwarding, but X-Real-IP only stores the IP of the requesting party of the previous hop, and under normal circumstances the last IP of X-Forwarded-For is the same as the IP of the requesting party. Forwarded-For last IP is the same as X-Real-IP), and returns the IP if it encounters a non-trusted IP address, or if it resolves to the first IP address.
  4. The difference with gin is that gin writes the TrustedProxies configuration into the Engine structure, whereas hertz allows user-defined implementations of the ClientIP function, so the fix refers to the resolution process, but does not dump it into the hertz Engine structure.

zh(optional):
问题:

  1. 当前 ClientIP 获取客户端 IP 只通过获取请求头中的 "X-Real-IP" 和 "X-Forwarded-For" 字段,而这个字段是可以直接伪造的。服务端没有判别能力。
  2. 而且 "X-Forwarded-For" 头本意是用于当出现代理服务器转发时,可以在该请求头中依次 append 代理服务器的 IP 地址,在请求获取客户端 IP 时,应该反向解析,直到解析得到第一个 IP 作为最初的客户端的 IP(为什么不直接获取第一个 IP ?因为代理转发过程并不一定是完全安全的,如果中间有一个非法的代理服务器 px 加入,如client -> p0 -> p1 -> server 变为 client -> p0 -> px -> p1 -> server 则处于安全性考虑,调用 ClientIP 方法应该获取 px 的 IP 而非 client 的 IP,这也是为什么要逆序解析,而非直接解析 "X-Forwarded-For" 列表的首个 IP)。

解决方案:
参考了 gin 的修复思路,增加 TrustedProxies 列表存放服务器可信的代理服务器地址列表,默认情况下为 "0.0.0.0",ClientIP 方法内逻辑如下:

  1. 需要区分 client 直接访问 server 与 client 通过代理访问 server 的不同。
  2. ctx.RemoteAddr() 获取请求方 IP 地址,判断请求来源的 IP 是否在可信列表中,不可信 ClientIP 返回该 IP。
  3. 如果在可信列表中,则依次获取 "X-Real-IP" 和 "X-Forwarded-For" 的头部信息,逆序解析(二者区别在于 X-Forwarded-For 记录了转发时 IP 的 append 结果,但是 X-Real-IP 只是存放了前一跳的请求方的 IP,正常情况下 X-Forwarded-For 最后一个 IP 与 X-Real-IP 相同),如果遇到非可信的 IP 地址,或者解析到了第一个 IP 地址,则返回该 IP。
  4. 与 gin 的不同在于,gin 将 TrustedProxies 相关配置写入了 Engine 结构,而 hertz 允许用户自定义 ClientIP 函数的实现,因此修复时参考了解析流程,但未侵入 hertz 的 Engine 结构。

Which issue(s) this PR fixes:

Fixes #161

@codecov
Copy link

codecov bot commented Nov 20, 2022

Codecov Report

Base: 69.42% // Head: 69.95% // Increases project coverage by +0.52% 🎉

Coverage data is based on head (ee0e6d0) compared to base (4d7af18).
Patch coverage: 73.91% of modified lines in pull request are covered.

❗ Current head ee0e6d0 differs from pull request most recent head 4fe85da. Consider uploading reports for the commit 4fe85da to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #401      +/-   ##
===========================================
+ Coverage    69.42%   69.95%   +0.52%     
===========================================
  Files           93       93              
  Lines         8874     8907      +33     
===========================================
+ Hits          6161     6231      +70     
+ Misses        2348     2304      -44     
- Partials       365      372       +7     
Impacted Files Coverage Δ
pkg/common/config/option.go 100.00% <ø> (ø)
pkg/network/netpoll/connection.go 73.33% <0.00%> (-7.16%) ⬇️
pkg/network/netpoll/transport.go 0.00% <ø> (ø)
pkg/network/standard/connection.go 80.06% <0.00%> (ø)
pkg/network/standard/transport.go 0.00% <0.00%> (ø)
pkg/protocol/http1/ext/stream.go 0.00% <0.00%> (ø)
pkg/app/context.go 85.15% <82.75%> (+0.85%) ⬆️
pkg/app/server/option.go 95.77% <100.00%> (ø)
pkg/protocol/header.go 75.59% <100.00%> (+0.65%) ⬆️
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@li-jin-gou li-jin-gou requested review from welkeyever and li-jin-gou and removed request for welkeyever November 21, 2022 04:28
@CLAassistant
Copy link

CLAassistant commented Nov 22, 2022

CLA assistant check
All committers have signed the CLA.

@welkeyever
Copy link
Member

Please resign the CLA🙏

@BaiZe1998
Copy link
Contributor Author

Please resign the CLA🙏

got it.

pkg/app/context.go Outdated Show resolved Hide resolved
pkg/app/context.go Outdated Show resolved Hide resolved
pkg/app/context.go Outdated Show resolved Hide resolved
pkg/app/context.go Outdated Show resolved Hide resolved
pkg/app/context.go Outdated Show resolved Hide resolved
@li-jin-gou
Copy link
Member

li-jin-gou commented Jan 29, 2023

any process? @BaiZe1998

Duslia
Duslia previously approved these changes Feb 10, 2023
pkg/app/context.go Outdated Show resolved Hide resolved
@li-jin-gou
Copy link
Member

LGTM

@li-jin-gou li-jin-gou merged commit c679431 into cloudwego:develop Feb 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

"ClientIP" handling is unsafe
8 participants