Skip to content

Commit b249c2e

Browse files
uurienuurien
authored
Reduce false positives when unvalidated redirect vulnerability is detected (#5880)
1 parent 601885d commit b249c2e

File tree

4 files changed

+64
-51
lines changed

4 files changed

+64
-51
lines changed

‎packages/dd-trace/src/appsec/iast/analyzers/unvalidated-redirect-analyzer.js

Lines changed: 13 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -5,13 +5,17 @@ const { UNVALIDATED_REDIRECT } = require('../vulnerabilities')
55
const { getNodeModulesPaths } = require('../path-line')
66
const { getRanges } = require('../taint-tracking/operations')
77
const {
8-
HTTP_REQUEST_HEADER_VALUE,
9-
HTTP_REQUEST_PATH_PARAM,
10-
HTTP_REQUEST_URI
8+
HTTP_REQUEST_BODY,
9+
HTTP_REQUEST_PARAMETER
1110
} = require('../taint-tracking/source-types')
1211

1312
const EXCLUDED_PATHS = getNodeModulesPaths('express/lib/response.js')
1413

14+
const VULNERABLE_SOURCE_TYPES = new Set([
15+
HTTP_REQUEST_BODY,
16+
HTTP_REQUEST_PARAMETER
17+
])
18+
1519
class UnvalidatedRedirectAnalyzer extends InjectionAnalyzer {
1620
constructor () {
1721
super(UNVALIDATED_REDIRECT)
@@ -35,28 +39,17 @@ class UnvalidatedRedirectAnalyzer extends InjectionAnalyzer {
3539
if (!value) return false
3640

3741
const ranges = getRanges(iastContext, value)
38-
return ranges && ranges.length > 0 && !this._areSafeRanges(ranges)
42+
return ranges?.length > 0 && this._hasUnsafeRange(ranges)
3943
}
4044

41-
// Do not report vulnerability if ranges sources are exclusively url,
42-
// path params or referer header to avoid false positives.
43-
_areSafeRanges (ranges) {
44-
return ranges && ranges.every(
45-
range => this._isPathParam(range) || this._isUrl(range) || this._isRefererHeader(range)
45+
_hasUnsafeRange (ranges) {
46+
return ranges.some(
47+
range => this._isVulnerableRange(range)
4648
)
4749
}
4850

49-
_isRefererHeader (range) {
50-
return range.iinfo.type === HTTP_REQUEST_HEADER_VALUE &&
51-
range.iinfo.parameterName && range.iinfo.parameterName.toLowerCase() === 'referer'
52-
}
53-
54-
_isPathParam (range) {
55-
return range.iinfo.type === HTTP_REQUEST_PATH_PARAM
56-
}
57-
58-
_isUrl (range) {
59-
return range.iinfo.type === HTTP_REQUEST_URI
51+
_isVulnerableRange (range) {
52+
return VULNERABLE_SOURCE_TYPES.has(range.iinfo.type)
6053
}
6154

6255
_getExcludedPaths () {

‎packages/dd-trace/src/appsec/iast/taint-tracking/source-types.js

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ module.exports = {
77
HTTP_REQUEST_HEADER_NAME: 'http.request.header.name',
88
HTTP_REQUEST_HEADER_VALUE: 'http.request.header',
99
HTTP_REQUEST_PARAMETER: 'http.request.parameter',
10-
HTTP_REQUEST_PATH: 'http.request.path',
1110
HTTP_REQUEST_PATH_PARAM: 'http.request.path.parameter',
1211
HTTP_REQUEST_URI: 'http.request.uri',
1312
KAFKA_MESSAGE_KEY: 'kafka.message.key',

‎packages/dd-trace/test/appsec/iast/analyzers/unvalidated-redirect-analyzer.express.plugin.spec.js

Lines changed: 44 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,7 @@ const path = require('path')
66

77
const { UNVALIDATED_REDIRECT } = require('../../../../src/appsec/iast/vulnerabilities')
88
const { prepareTestServerForIastInExpress } = require('../utils')
9-
const { storage } = require('../../../../../datadog-core')
10-
const iastContextFunctions = require('../../../../src/appsec/iast/iast-context')
11-
const { newTaintedString } = require('../../../../src/appsec/iast/taint-tracking/operations')
9+
const Axios = require('axios')
1210

1311
describe('Unvalidated Redirect vulnerability', () => {
1412
let redirectFunctions
@@ -24,58 +22,85 @@ describe('Unvalidated Redirect vulnerability', () => {
2422
fs.unlinkSync(redirectFunctionsPath)
2523
})
2624

25+
function getAxiosInstance (config) {
26+
return Axios.create({
27+
baseURL: `http://localhost:${config.port}`
28+
})
29+
}
30+
2731
withVersions('express', 'express', version => {
2832
prepareTestServerForIastInExpress('in express', version,
2933
(testThatRequestHasVulnerability, testThatRequestHasNoVulnerability) => {
3034
testThatRequestHasVulnerability((req, res) => {
31-
const store = storage('legacy').getStore()
32-
const iastCtx = iastContextFunctions.getIastContext(store)
33-
const location = newTaintedString(iastCtx, 'https://app.com?id=tron', 'param', 'Request')
35+
const location = req.query.location
3436
redirectFunctions.insecureWithResHeaderMethod('location', location, res)
3537
}, UNVALIDATED_REDIRECT, {
3638
occurrences: 1,
3739
location: {
3840
path: redirectFunctionsFilename,
3941
line: 4
4042
}
43+
}, null, (done, config) => {
44+
getAxiosInstance(config).get('/?location=https://app.com?id=tron').catch(done)
4145
})
4246

4347
testThatRequestHasVulnerability((req, res) => {
44-
const store = storage('legacy').getStore()
45-
const iastCtx = iastContextFunctions.getIastContext(store)
46-
const location = newTaintedString(iastCtx, 'http://user@app.com/', 'param', 'Request')
47-
redirectFunctions.insecureWithResRedirectMethod(location, res)
48+
redirectFunctions.insecureWithResRedirectMethod(req.query.location, res)
4849
}, UNVALIDATED_REDIRECT, {
4950
occurrences: 1,
5051
location: {
5152
path: redirectFunctionsFilename,
5253
line: 8
5354
}
55+
}, null, (done, config) => {
56+
getAxiosInstance(config).get('/?location=http://user@app.com/').catch(done)
5457
})
5558

5659
testThatRequestHasVulnerability((req, res) => {
57-
const store = storage('legacy').getStore()
58-
const iastCtx = iastContextFunctions.getIastContext(store)
59-
const location = newTaintedString(iastCtx, 'http://user@app.com/', 'param', 'Request')
60-
redirectFunctions.insecureWithResLocationMethod(location, res)
60+
redirectFunctions.insecureWithResLocationMethod(req.query.location, res)
6161
}, UNVALIDATED_REDIRECT, {
6262
occurrences: 1,
6363
location: {
6464
path: redirectFunctionsFilename,
6565
line: 12
6666
}
67+
}, null, (done, config) => {
68+
getAxiosInstance(config).get('/?location=http://user@app.com/').catch(done)
69+
})
70+
71+
testThatRequestHasVulnerability((req, res) => {
72+
redirectFunctions.insecureWithResLocationMethod(req.body.location, res)
73+
}, UNVALIDATED_REDIRECT, {
74+
occurrences: 1,
75+
location: {
76+
path: redirectFunctionsFilename,
77+
line: 12
78+
}
79+
}, null, (done, config) => {
80+
getAxiosInstance(config).post('', {
81+
location: 'http://user@app.com/'
82+
}).catch(done)
6783
})
6884

6985
testThatRequestHasNoVulnerability((req, res) => {
70-
const store = storage('legacy').getStore()
71-
const iastCtx = iastContextFunctions.getIastContext(store)
72-
const location = newTaintedString(iastCtx, 'http://user@app.com/', 'pathParam', 'Request')
73-
res.header('X-test', location)
74-
}, UNVALIDATED_REDIRECT)
86+
res.header('X-test', req.query.location)
87+
}, UNVALIDATED_REDIRECT, (done, config) => {
88+
getAxiosInstance(config).get('/?location=http://user@app.com/').catch(done)
89+
})
7590

7691
testThatRequestHasNoVulnerability((req, res) => {
7792
redirectFunctions.insecureWithResHeaderMethod('location', 'http://user@app.com/', res)
7893
}, UNVALIDATED_REDIRECT)
94+
95+
testThatRequestHasNoVulnerability((req, res) => {
96+
redirectFunctions.insecureWithResLocationMethod(req.headers.redirectlocation, res)
97+
}, UNVALIDATED_REDIRECT, (done, config) => {
98+
getAxiosInstance(config).get('', {
99+
headers: {
100+
redirectlocation: 'http://user@app.com/'
101+
}
102+
}).catch(done)
103+
})
79104
})
80105
})
81106
})

‎packages/dd-trace/test/appsec/iast/analyzers/vulnerability-analyzer.express.plugin.spec.js

Lines changed: 7 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,7 @@ const path = require('path')
66

77
const { UNVALIDATED_REDIRECT } = require('../../../../src/appsec/iast/vulnerabilities')
88
const { prepareTestServerForIastInExpress } = require('../utils')
9-
const { storage } = require('../../../../../datadog-core')
10-
const iastContextFunctions = require('../../../../src/appsec/iast/iast-context')
11-
const { newTaintedString } = require('../../../../src/appsec/iast/taint-tracking/operations')
9+
const axios = require('axios')
1210

1311
describe('Vulnerability Analyzer plugin', () => {
1412
let redirectFunctions
@@ -45,29 +43,27 @@ describe('Vulnerability Analyzer plugin', () => {
4543
prepareTestServerForIastInExpress('should find original source line minified or not', version,
4644
(testThatRequestHasVulnerability, testThatRequestHasNoVulnerability) => {
4745
testThatRequestHasVulnerability((req, res) => {
48-
const store = storage('legacy').getStore()
49-
const iastCtx = iastContextFunctions.getIastContext(store)
50-
const location = newTaintedString(iastCtx, 'https://app.com?id=tron', 'param', 'Request')
51-
redirectMinFunctions.insecureWithResHeaderMethod('location', location, res)
46+
redirectMinFunctions.insecureWithResHeaderMethod('location', req.query.location, res)
5247
}, UNVALIDATED_REDIRECT, {
5348
occurrences: 1,
5449
location: {
5550
path: redirectFunctionsFilename, // original source code file indicated in sourceMappingURL
5651
line: 4 // line in not minified source file
5752
}
53+
}, null, (done, config) => {
54+
axios.get(`http://localhost:${config.port}/?location=https://app.com?id=tron`).catch(done)
5855
})
5956

6057
testThatRequestHasVulnerability((req, res) => {
61-
const store = storage('legacy').getStore()
62-
const iastCtx = iastContextFunctions.getIastContext(store)
63-
const location = newTaintedString(iastCtx, 'https://app.com?id=tron', 'param', 'Request')
64-
redirectFunctions.insecureWithResHeaderMethod('location', location, res)
58+
redirectFunctions.insecureWithResHeaderMethod('location', req.query.location, res)
6559
}, UNVALIDATED_REDIRECT, {
6660
occurrences: 1,
6761
location: {
6862
path: redirectFunctionsFilename,
6963
line: 4
7064
}
65+
}, null, (done, config) => {
66+
axios.get(`http://localhost:${config.port}/?location=https://app.com?id=tron`).catch(done)
7167
})
7268
})
7369
})

0 commit comments

Comments
 (0)