-
Notifications
You must be signed in to change notification settings - Fork 43
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
'use strict'; | ||
|
||
const utils = require('./lib/utils'); | ||
|
||
module.exports = agent => { | ||
utils.processSSRFConfig(agent.config.security.ssrf); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,13 @@ | ||
'use strict'; | ||
|
||
const safeRedirect = require('./lib/safe_redirect'); | ||
const utils = require('./lib/utils'); | ||
|
||
module.exports = app => { | ||
app.config.coreMiddleware.push('securities'); | ||
|
||
// patch response.redirect | ||
safeRedirect(app); | ||
|
||
utils.processSSRFConfig(app.config.security.ssrf); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
'use strict'; | ||
|
||
module.exports = { | ||
/** | ||
* safe curl with ssrf protect | ||
* @param {String} url request url | ||
* @param {Object} options request options | ||
* @return {Promise} response | ||
*/ | ||
safeCurl(url, options = {}) { | ||
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'); | ||
} | ||
|
||
return this.curl(url, options); | ||
}, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
'use strict'; | ||
|
||
const normalize = require('path').normalize; | ||
const IP = require('ip'); | ||
|
||
exports.isSafeDomain = function isSafeDomain(domain, domain_white_list) { | ||
// add prefix `.`, because all domains in white list are start with `.` | ||
|
@@ -80,3 +81,24 @@ exports.merge = function merge(origin, opts) { | |
} | ||
return res; | ||
}; | ||
|
||
exports.processSSRFConfig = function(config) { | ||
// transfor ssrf.ipBlackList to ssrf.checkAddress | ||
// checkAddress has higher priority than ipBlackList | ||
if (config && config.ipBlackList && !config.checkAddress) { | ||
const containsList = config.ipBlackList.map(getContains); | ||
config.checkAddress = ip => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 加个 benchmark There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 加个 lru?活跃的 ip 应该是有限的。 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 先不加 lru 吧,相较于做一次 http 请求来说,这个性能算是可以接受的,现在也不会对 curl 造成影响。 |
||
for (const contains of containsList) { | ||
if (contains(ip)) return false; | ||
} | ||
return true; | ||
}; | ||
} | ||
}; | ||
|
||
function getContains(ip) { | ||
if (IP.isV4Format(ip) || IP.isV6Format(ip)) { | ||
return _ip => ip === _ip; | ||
} | ||
return IP.cidrSubnet(ip).contains; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
'use strict'; | ||
|
||
const ip = require('ip'); | ||
const Benchmark = require('benchmark'); | ||
const benchmarks = require('beautify-benchmark'); | ||
const suite = new Benchmark.Suite(); | ||
|
||
const parsed1 = ip.cidrSubnet('10.0.0.0/8'); | ||
const parsed2 = ip.cidrSubnet('0.0.0.0/32'); | ||
|
||
console.log('10.0.0.0/8 contains 10.255.168.1', parsed1.contains('10.255.168.1')); | ||
console.log('10.0.0.0/8 contains 11.255.168.1', parsed1.contains('11.255.168.1')); | ||
console.log('0.0.0.0/32 contains 0.0.0.0', parsed2.contains('0.0.0.0')); | ||
console.log('0.0.0.0/32 contains 0.0.0.1', parsed2.contains('0.0.0.1')); | ||
|
||
suite | ||
|
||
.add('10.0.0/8 match', () => { | ||
parsed1.contains('10.255.168.1'); | ||
}) | ||
.add('10.0.0/8 not match', () => { | ||
parsed1.contains('11.255.168.1'); | ||
}) | ||
.add('0.0.0/32 match', () => { | ||
parsed1.contains('0.0.0.0'); | ||
}) | ||
.add('0.0.0/32 not match', () => { | ||
parsed1.contains('0.0.0.1'); | ||
}) | ||
.on('cycle', event => { | ||
benchmarks.add(event.target); | ||
}) | ||
.on('start', event => { | ||
console.log('\n ip.cidrsubnet().contains() Benchmark\n node version: %s, date: %s\n Starting...', | ||
process.version, Date()); | ||
}) | ||
.on('complete', () => { | ||
benchmarks.log(); | ||
}) | ||
.run({ 'async': false }); | ||
|
||
// ip.cidrsubnet().contains() Benchmark | ||
// node version: v8.9.1, date: Tue Mar 27 2018 12:04:41 GMT+0800 (CST) | ||
// Starting... | ||
// 4 tests completed. | ||
|
||
// 10.0.0/8 match x 338,567 ops/sec ±2.98% (84 runs sampled) | ||
// 10.0.0/8 not match x 315,822 ops/sec ±5.29% (81 runs sampled) | ||
// 0.0.0/32 match x 366,250 ops/sec ±4.47% (78 runs sampled) | ||
// 0.0.0/32 not match x 370,959 ops/sec ±4.23% (82 runs sampled) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
'use strict'; | ||
|
||
exports.security = { | ||
ssrf: { | ||
ipBlackList: [ | ||
'10.0.0.0/8', | ||
'127.0.0.1', | ||
'0.0.0.0/32', | ||
], | ||
checkAddress(ip) { | ||
return ip !== '127.0.0.2'; | ||
}, | ||
}, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
{ | ||
"name": "ssrf-ip-check-address" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
'use strict'; | ||
|
||
exports.security = { | ||
ssrf: { | ||
ipBlackList: [ | ||
'10.0.0.0/8', | ||
'127.0.0.1', | ||
'0.0.0.0/32', | ||
], | ||
}, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
{ | ||
"name": "ssrf-ip-black-list" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,98 @@ | ||
'use strict'; | ||
|
||
const mm = require('egg-mock'); | ||
const dns = require('dns'); | ||
const assert = require('assert'); | ||
|
||
let app; | ||
describe('test/ssrf.test.js', function() { | ||
afterEach(mm.restore); | ||
|
||
describe('no ssrf config', () => { | ||
before(() => { | ||
app = mm.app({ baseDir: 'apps/csrf' }); | ||
return app.ready(); | ||
}); | ||
|
||
it('should safeCurl work', async () => { | ||
const ctx = app.createAnonymousContext(); | ||
const url = 'https://127.0.0.1'; | ||
mm.data(app, 'curl', 'response'); | ||
mm.data(app.agent, 'curl', 'response'); | ||
mm.data(ctx, 'curl', 'response'); | ||
|
||
let count = 0; | ||
function mockWarn(msg) { | ||
count++; | ||
assert(msg === '[egg-security] please configure `config.security.ssrf` first'); | ||
} | ||
|
||
mm(app.logger, 'warn', mockWarn); | ||
mm(app.agent.logger, 'warn', mockWarn); | ||
mm(ctx.logger, 'warn', mockWarn); | ||
|
||
const r1 = await app.safeCurl(url); | ||
const r2 = await app.agent.safeCurl(url); | ||
const r3 = await ctx.safeCurl(url); | ||
assert(r1 === 'response'); | ||
assert(r2 === 'response'); | ||
assert(r3 === 'response'); | ||
assert(count === 3); | ||
}); | ||
}); | ||
|
||
|
||
describe('ipBlackList', () => { | ||
before(() => { | ||
app = mm.app({ baseDir: 'apps/ssrf-ip-black-list' }); | ||
return app.ready(); | ||
}); | ||
|
||
it('should safeCurl work', async () => { | ||
const urls = [ | ||
'https://127.0.0.1/foo', | ||
'http://10.1.2.3/foo?bar=1', | ||
'https://0.0.0.0/', | ||
'https://www.google.com/', | ||
]; | ||
mm.data(dns, 'lookup', '127.0.0.1'); | ||
const ctx = app.createAnonymousContext(); | ||
|
||
for (const url of urls) { | ||
await checkIllegalAddressError(app, url); | ||
await checkIllegalAddressError(app.agent, url); | ||
await checkIllegalAddressError(ctx, url); | ||
} | ||
}); | ||
}); | ||
|
||
describe('checkAddress', () => { | ||
before(() => { | ||
app = mm.app({ baseDir: 'apps/ssrf-check-address' }); | ||
return app.ready(); | ||
}); | ||
|
||
it('should safeCurl work', async () => { | ||
const urls = [ | ||
'https://127.0.0.2/foo', | ||
'https://www.google.com/foo', | ||
]; | ||
mm.data(dns, 'lookup', '127.0.0.2'); | ||
const ctx = app.createAnonymousContext(); | ||
for (const url of urls) { | ||
await checkIllegalAddressError(app, url); | ||
await checkIllegalAddressError(app.agent, url); | ||
await checkIllegalAddressError(ctx, url); | ||
} | ||
}); | ||
}); | ||
}); | ||
|
||
async function checkIllegalAddressError(instance, url) { | ||
try { | ||
await instance.safeCurl(url); | ||
throw new Error('should not execute'); | ||
} catch (err) { | ||
assert(err.name === 'IllegalAddressError'); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里不是应该提示配置 config.security.ssrf. checkAddress?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agent 和 application 的重复代码,抽到 lib 里面去吧
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
还可以配置 ipBlackList
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
从逻辑看只要没配 checkAddress 就 warn 了,其实有配置 ipBlackList 但没配 checkAddress 的情况。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
恩,前面 app.js 里面处理了一下,把 ipBlackList 转换成了一个 checkAdderss 的方法配置