Skip to content

Commit a8b3ddf

Browse files
CarlesDDtlhunter
CarlesDD
authored and
tlhunter
committed
Taint request URI (#3302)
* Taint request URI * Add check for safe tainted origins on unvalidated redirect analyzer * Change assertion construction for unvalidated redirect analyzer test * Add metric for uri sourcing * Fix PR comments
1 parent decfbaa commit a8b3ddf

File tree

8 files changed

+203
-27
lines changed

8 files changed

+203
-27
lines changed

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

Lines changed: 25 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,11 @@ const InjectionAnalyzer = require('./injection-analyzer')
44
const { UNVALIDATED_REDIRECT } = require('../vulnerabilities')
55
const { getNodeModulesPaths } = require('../path-line')
66
const { getRanges } = require('../taint-tracking/operations')
7-
const { HTTP_REQUEST_HEADER_VALUE } = require('../taint-tracking/source-types')
7+
const {
8+
HTTP_REQUEST_HEADER_VALUE,
9+
HTTP_REQUEST_PATH,
10+
HTTP_REQUEST_PATH_PARAM
11+
} = require('../taint-tracking/source-types')
812

913
const EXCLUDED_PATHS = getNodeModulesPaths('express/lib/response.js')
1014

@@ -17,9 +21,6 @@ class UnvalidatedRedirectAnalyzer extends InjectionAnalyzer {
1721
this.addSub('datadog:http:server:response:set-header:finish', ({ name, value }) => this.analyze(name, value))
1822
}
1923

20-
// TODO: In case the location header value is tainted, this analyzer should check the ranges of the tainted.
21-
// And do not report a vulnerability if source of the ranges (range.iinfo.type) are exclusively url or path params
22-
// to avoid false positives.
2324
analyze (name, value) {
2425
if (!this.isLocationHeader(name) || typeof value !== 'string') return
2526

@@ -34,12 +35,28 @@ class UnvalidatedRedirectAnalyzer extends InjectionAnalyzer {
3435
if (!value) return false
3536

3637
const ranges = getRanges(iastContext, value)
37-
return ranges && ranges.length > 0 && !this._isRefererHeader(ranges)
38+
return ranges && ranges.length > 0 && !this._areSafeRanges(ranges)
3839
}
3940

40-
_isRefererHeader (ranges) {
41-
return ranges && ranges.every(range => range.iinfo.type === HTTP_REQUEST_HEADER_VALUE &&
42-
range.iinfo.parameterName && range.iinfo.parameterName.toLowerCase() === 'referer')
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)
46+
)
47+
}
48+
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_PATH
4360
}
4461

4562
_getExcludedPaths () {

‎packages/dd-trace/src/appsec/iast/index.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ function onIncomingHttpRequestStart (data) {
5454
createTransaction(rootSpan.context().toSpanId(), iastContext)
5555
overheadController.initializeRequestContext(iastContext)
5656
iastTelemetry.onRequestStart(iastContext)
57-
taintTrackingPlugin.taintHeaders(data.req.headers, iastContext)
57+
taintTrackingPlugin.taintRequest(data.req, iastContext)
5858
}
5959
if (rootSpan.addTags) {
6060
rootSpan.addTags({

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

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,14 +3,15 @@
33
const { SourceIastPlugin } = require('../iast-plugin')
44
const { getIastContext } = require('../iast-context')
55
const { storage } = require('../../../../../datadog-core')
6-
const { taintObject } = require('./operations')
6+
const { taintObject, newTaintedString } = require('./operations')
77
const {
88
HTTP_REQUEST_BODY,
99
HTTP_REQUEST_COOKIE_VALUE,
1010
HTTP_REQUEST_COOKIE_NAME,
1111
HTTP_REQUEST_HEADER_VALUE,
1212
HTTP_REQUEST_HEADER_NAME,
1313
HTTP_REQUEST_PARAMETER,
14+
HTTP_REQUEST_PATH,
1415
HTTP_REQUEST_PATH_PARAM
1516
} = require('./source-types')
1617

@@ -89,6 +90,21 @@ class TaintTrackingPlugin extends SourceIastPlugin {
8990
})
9091
}
9192

93+
taintUrl (req, iastContext) {
94+
this.execSource({
95+
handler: function () {
96+
req.url = newTaintedString(iastContext, req.url, 'req.url', HTTP_REQUEST_PATH)
97+
},
98+
tag: [HTTP_REQUEST_PATH],
99+
iastContext
100+
})
101+
}
102+
103+
taintRequest (req, iastContext) {
104+
this.taintHeaders(req.headers, iastContext)
105+
this.taintUrl(req, iastContext)
106+
}
107+
92108
enable () {
93109
this.configure(true)
94110
}

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,5 +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',
1011
HTTP_REQUEST_PATH_PARAM: 'http.request.path.parameter'
1112
}

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

Lines changed: 60 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -3,15 +3,22 @@
33
const { expect } = require('chai')
44
const proxyquire = require('proxyquire')
55
const overheadController = require('../../../../src/appsec/iast/overhead-controller')
6-
const { HTTP_REQUEST_HEADER_VALUE, HTTP_REQUEST_PARAMETER } =
7-
require('../../../../src/appsec/iast/taint-tracking/source-types')
6+
const {
7+
HTTP_REQUEST_HEADER_VALUE,
8+
HTTP_REQUEST_PARAMETER,
9+
HTTP_REQUEST_PATH,
10+
HTTP_REQUEST_PATH_PARAM
11+
} = require('../../../../src/appsec/iast/taint-tracking/source-types')
812

913
describe('unvalidated-redirect-analyzer', () => {
1014
const NOT_TAINTED_LOCATION = 'url.com'
1115
const TAINTED_LOCATION = 'evil.com'
1216

1317
const TAINTED_HEADER_REFERER_ONLY = 'TAINTED_HEADER_REFERER_ONLY'
14-
const TAINTED_HEADER_REFERER_AMONG_OTHERS = 'TAINTED_HEADER_REFERER_ONLY_AMONG_OTHERS'
18+
const TAINTED_PATH_PARAMS_ONLY = 'TAINTED_PATH_PARAMS_ONLY'
19+
const TAINTED_URL_ONLY = 'TAINTED_URL_ONLY'
20+
const TAINTED_SAFE_RANGES = 'TAINTED_SAFE_RANGES'
21+
const TAINTED_SAFE_RANGES_AMONG_OTHERS = 'TAINTED_SAFE_RANGES_AMONG_OTHERS'
1522

1623
const REFERER_RANGE = {
1724
iinfo: {
@@ -31,21 +38,40 @@ describe('unvalidated-redirect-analyzer', () => {
3138
parameterName: 'param2'
3239
}
3340
}
41+
const PATH_PARAM_RANGE = {
42+
iinfo: {
43+
type: HTTP_REQUEST_PATH_PARAM,
44+
parameterName: 'path_param'
45+
}
46+
}
47+
const URL_RANGE = {
48+
iinfo: {
49+
type: HTTP_REQUEST_PATH,
50+
parameterName: 'path'
51+
}
52+
}
3453

3554
const TaintTrackingMock = {
3655
isTainted: (iastContext, string) => {
3756
return string === TAINTED_LOCATION
3857
},
3958

4059
getRanges: (iastContext, value) => {
41-
if (value === NOT_TAINTED_LOCATION) return null
42-
43-
if (value === TAINTED_HEADER_REFERER_ONLY) {
44-
return [REFERER_RANGE]
45-
} else if (value === TAINTED_HEADER_REFERER_AMONG_OTHERS) {
46-
return [REFERER_RANGE, PARAMETER1_RANGE]
47-
} else {
48-
return [PARAMETER1_RANGE, PARAMETER2_RANGE]
60+
switch (value) {
61+
case NOT_TAINTED_LOCATION:
62+
return null
63+
case TAINTED_HEADER_REFERER_ONLY:
64+
return [REFERER_RANGE]
65+
case TAINTED_PATH_PARAMS_ONLY:
66+
return [PATH_PARAM_RANGE]
67+
case TAINTED_URL_ONLY:
68+
return [URL_RANGE]
69+
case TAINTED_SAFE_RANGES:
70+
return [REFERER_RANGE, PATH_PARAM_RANGE, URL_RANGE]
71+
case TAINTED_SAFE_RANGES_AMONG_OTHERS:
72+
return [REFERER_RANGE, PATH_PARAM_RANGE, URL_RANGE, PARAMETER1_RANGE]
73+
default:
74+
return [PARAMETER1_RANGE, PARAMETER2_RANGE]
4975
}
5076
}
5177
}
@@ -78,19 +104,19 @@ describe('unvalidated-redirect-analyzer', () => {
78104
it('should not report headers other than Location', () => {
79105
unvalidatedRedirectAnalyzer.analyze('X-test', NOT_TAINTED_LOCATION)
80106

81-
expect(report).to.not.have.been.called
107+
expect(report).not.to.be.called
82108
})
83109

84110
it('should not report Location header with non string values', () => {
85111
unvalidatedRedirectAnalyzer.analyze('X-test', [TAINTED_LOCATION])
86112

87-
expect(report).to.not.have.been.called
113+
expect(report).not.to.be.called
88114
})
89115

90116
it('should not report Location header with not tainted string value', () => {
91117
unvalidatedRedirectAnalyzer.analyze('Location', NOT_TAINTED_LOCATION)
92118

93-
expect(report).to.not.have.been.called
119+
expect(report).not.to.be.called
94120
})
95121

96122
it('should report Location header with tainted string value', () => {
@@ -102,11 +128,29 @@ describe('unvalidated-redirect-analyzer', () => {
102128
it('should not report if tainted origin is referer header exclusively', () => {
103129
unvalidatedRedirectAnalyzer.analyze('Location', TAINTED_HEADER_REFERER_ONLY)
104130

105-
expect(report).to.not.be.called
131+
expect(report).not.to.be.called
132+
})
133+
134+
it('should not report if tainted origin is path param exclusively', () => {
135+
unvalidatedRedirectAnalyzer.analyze('Location', TAINTED_PATH_PARAMS_ONLY)
136+
137+
expect(report).not.to.be.called
138+
})
139+
140+
it('should not report if tainted origin is url exclusively', () => {
141+
unvalidatedRedirectAnalyzer.analyze('Location', TAINTED_URL_ONLY)
142+
143+
expect(report).not.to.be.called
144+
})
145+
146+
it('should not report if all tainted origin are safe', () => {
147+
unvalidatedRedirectAnalyzer.analyze('Location', TAINTED_SAFE_RANGES)
148+
149+
expect(report).not.to.be.called
106150
})
107151

108152
it('should report if tainted origin contains referer header among others', () => {
109-
unvalidatedRedirectAnalyzer.analyze('Location', TAINTED_HEADER_REFERER_AMONG_OTHERS)
153+
unvalidatedRedirectAnalyzer.analyze('Location', TAINTED_SAFE_RANGES_AMONG_OTHERS)
110154

111155
expect(report).to.be.called
112156
})

‎packages/dd-trace/test/appsec/iast/taint-tracking/plugin.spec.js‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,9 @@ const dc = require('../../../../../diagnostics_channel')
77
const {
88
HTTP_REQUEST_COOKIE_VALUE,
99
HTTP_REQUEST_COOKIE_NAME,
10+
HTTP_REQUEST_HEADER_NAME,
11+
HTTP_REQUEST_HEADER_VALUE,
12+
HTTP_REQUEST_PATH,
1013
HTTP_REQUEST_PATH_PARAM
1114
} = require('../../../../src/appsec/iast/taint-tracking/source-types')
1215

@@ -227,5 +230,30 @@ describe('IAST Taint tracking plugin', () => {
227230
processParamsStartCh.publish({ req })
228231
expect(taintTrackingOperations.taintObject).to.not.be.called
229232
})
233+
234+
it('Should taint headers and uri from request', () => {
235+
const req = {
236+
headers: {
237+
'x-iast-header': 'header-value'
238+
},
239+
url: 'https://testurl'
240+
}
241+
taintTrackingPlugin.taintRequest(req, iastContext)
242+
243+
expect(taintTrackingOperations.taintObject).to.be.calledOnceWith(
244+
iastContext,
245+
req.headers,
246+
HTTP_REQUEST_HEADER_VALUE,
247+
true,
248+
HTTP_REQUEST_HEADER_NAME
249+
)
250+
251+
expect(taintTrackingOperations.newTaintedString).to.be.calledOnceWith(
252+
iastContext,
253+
req.url,
254+
'req.url',
255+
HTTP_REQUEST_PATH
256+
)
257+
})
230258
})
231259
})

‎packages/dd-trace/test/appsec/iast/taint-tracking/sources/taint-tracking.express.plugin.spec.js‎

Lines changed: 61 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,67 @@ const { storage } = require('../../../../../../datadog-core')
99
const iast = require('../../../../../src/appsec/iast')
1010
const iastContextFunctions = require('../../../../../src/appsec/iast/iast-context')
1111
const { isTainted, getRanges } = require('../../../../../src/appsec/iast/taint-tracking/operations')
12-
const { HTTP_REQUEST_PATH_PARAM } = require('../../../../../src/appsec/iast/taint-tracking/source-types')
12+
const {
13+
HTTP_REQUEST_PATH,
14+
HTTP_REQUEST_PATH_PARAM
15+
} = require('../../../../../src/appsec/iast/taint-tracking/source-types')
16+
17+
describe('Path sourcing with express', () => {
18+
let express
19+
let appListener
20+
21+
withVersions('express', 'express', version => {
22+
before(() => {
23+
return agent.load(['http', 'express'], { client: false })
24+
})
25+
26+
after(() => {
27+
return agent.close({ ritmReset: false })
28+
})
29+
30+
beforeEach(() => {
31+
iast.enable(new Config({
32+
experimental: {
33+
iast: {
34+
enabled: true,
35+
requestSampling: 100
36+
}
37+
}
38+
}))
39+
40+
express = require(`../../../../../../../versions/express@${version}`).get()
41+
})
42+
43+
afterEach(() => {
44+
appListener && appListener.close()
45+
appListener = null
46+
47+
iast.disable()
48+
})
49+
50+
it('should taint path', done => {
51+
const app = express()
52+
app.get('/path/*', (req, res) => {
53+
const store = storage.getStore()
54+
const iastContext = iastContextFunctions.getIastContext(store)
55+
const isPathTainted = isTainted(iastContext, req.url)
56+
expect(isPathTainted).to.be.true
57+
const taintedPathValueRanges = getRanges(iastContext, req.url)
58+
expect(taintedPathValueRanges[0].iinfo.type).to.be.equal(HTTP_REQUEST_PATH)
59+
res.status(200).send()
60+
})
61+
62+
getPort().then(port => {
63+
appListener = app.listen(port, 'localhost', () => {
64+
axios
65+
.get(`http://localhost:${port}/path/vulnerable`)
66+
.then(() => done())
67+
.catch(done)
68+
})
69+
})
70+
})
71+
})
72+
})
1373

1474
describe('Path params sourcing with express', () => {
1575
let express

‎packages/dd-trace/test/appsec/iast/telemetry/index.spec.js‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -216,6 +216,16 @@ describe('Telemetry', () => {
216216
}
217217
}).catch(done)
218218
})
219+
220+
it('should have url source execution metric', (done) => {
221+
agent
222+
.use(traces => {
223+
expect(traces[0][0].metrics['_dd.iast.telemetry.executed.source.http_request_path']).to.be.equal(1)
224+
})
225+
.then(done)
226+
.catch(done)
227+
axios.get(`http://localhost:${config.port}/`).catch(done)
228+
})
219229
}
220230
testInRequest(app, tests)
221231
})

0 commit comments

Comments
 (0)