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

feat: support safeCurl for SSRF protection #32

Merged
merged 3 commits into from Mar 27, 2018
Merged

feat: support safeCurl for SSRF protection #32

merged 3 commits into from Mar 27, 2018

Conversation

dead-horse
Copy link
Member

No description provided.

@dead-horse
Copy link
Member Author

base on node-modules/urllib#279

@jtyjty99999
Copy link
Member

jtyjty99999 commented Mar 26, 2018

个人觉得ip这块够了,但是几个可能的问题或需要实现的地方,因为ip限制比较容易被绕过,参考 http://www.freebuf.com/articles/web/135342.html:

  • host为内网的域名
  • 多重跳转,例如向服务器请求访问www.eval.com服务,服务默认重定向到www.alibaba-inxxxxxx.com
  • 解析域名有开销,有可能被ddos

@dead-horse
Copy link
Member Author

@jtyjty99999

可以看 urllib 的具体实现。

  1. 302 之后的 ip 也会经过 urllib 的 checkAddress 逻辑。
  2. DNS 解析通过包装了一层 dns.lookup 来实现,和之前相比没有性能损耗,可能的性能损耗在对 ip 段做匹配的部分。

内网域名这种无法全部过滤干净,不如不支持为好,都通过 ip 段来屏蔽。

@codecov
Copy link

codecov bot commented Mar 26, 2018

Codecov Report

Merging #32 into master will increase coverage by 0.03%.
The diff coverage is 96.29%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #32      +/-   ##
==========================================
+ Coverage   95.75%   95.78%   +0.03%     
==========================================
  Files          27       30       +3     
  Lines         448      475      +27     
==========================================
+ Hits          429      455      +26     
- Misses         19       20       +1
Impacted Files Coverage Δ
config/config.default.js 100% <ø> (ø) ⬆️
app/extend/application.js 100% <100%> (ø) ⬆️
app/extend/agent.js 100% <100%> (ø)
app/extend/context.js 100% <100%> (ø) ⬆️
app.js 100% <100%> (ø) ⬆️
lib/extend/safe_curl.js 100% <100%> (ø)
agent.js 100% <100%> (ø)
lib/utils.js 78.57% <90.9%> (+3.01%) ⬆️
... and 1 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 119725d...86e8545. Read the comment docs.

if (this.config.security.ssrf && this.config.security.ssrf.checkAddress) {
options.checkAddress = this.config.security.ssrf.checkAddress;
} else {
this.logger.warn('[egg-security] please configure `config.security.ssrf` first');
Copy link
Member

Choose a reason for hiding this comment

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

这里不是应该提示配置 config.security.ssrf. checkAddress?

Copy link
Member

Choose a reason for hiding this comment

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

agent 和 application 的重复代码,抽到 lib 里面去吧

Copy link
Member Author

Choose a reason for hiding this comment

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

这里不是应该提示配置 config.security.ssrf. checkAddress?

还可以配置 ipBlackList

Copy link
Member

Choose a reason for hiding this comment

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

从逻辑看只要没配 checkAddress 就 warn 了,其实有配置 ipBlackList 但没配 checkAddress 的情况。

Copy link
Member Author

Choose a reason for hiding this comment

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

恩,前面 app.js 里面处理了一下,把 ipBlackList 转换成了一个 checkAdderss 的方法配置

// checkAddress has higher priority than ipBlackList
if (config && config.ipBlackList && !config.checkAddress) {
const containsList = config.ipBlackList.map(getContains);
config.checkAddress = ip => {
Copy link
Member

Choose a reason for hiding this comment

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

加个 benchmark

Copy link
Member

Choose a reason for hiding this comment

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

加个 lru?活跃的 ip 应该是有限的。

Copy link
Member Author

Choose a reason for hiding this comment

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

先不加 lru 吧,相较于做一次 http 请求来说,这个性能算是可以接受的,现在也不会对 curl 造成影响。

@dead-horse
Copy link
Member Author

我处理一下重复代码再合并

@dead-horse dead-horse merged commit eba4555 into master Mar 27, 2018
@dead-horse dead-horse deleted the ssrf branch March 27, 2018 06:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants