Skip to content

Commit 27e1530

Browse files
authored
Taint express route paramaters (#3187)
* Wrap express process_params to taint route params * Add taint tracking plugin test for route params * Change name of express process params channel * Return process_param result * Address taint tracking plugin subscriptions test
1 parent 459ca8d commit 27e1530

File tree

5 files changed

+235
-7
lines changed

5 files changed

+235
-7
lines changed

packages/datadog-instrumentations/src/express.js

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,3 +57,26 @@ addHook({
5757
})
5858
})
5959
})
60+
61+
const processParamsStartCh = channel('datadog:express:process_params:start')
62+
const wrapProcessParamsMethod = (requestPositionInArguments) => {
63+
return (original) => {
64+
return function () {
65+
if (processParamsStartCh.hasSubscribers) {
66+
processParamsStartCh.publish({ req: arguments[requestPositionInArguments] })
67+
}
68+
69+
return original.apply(this, arguments)
70+
}
71+
}
72+
}
73+
74+
addHook({ name: 'express', versions: ['>=4.0.0 <4.3.0'] }, express => {
75+
shimmer.wrap(express.Router, 'process_params', wrapProcessParamsMethod(1))
76+
return express
77+
})
78+
79+
addHook({ name: 'express', versions: ['>=4.3.0'] }, express => {
80+
shimmer.wrap(express.Router, 'process_params', wrapProcessParamsMethod(2))
81+
return express
82+
})

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

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,10 @@
22

33
module.exports = {
44
HTTP_REQUEST_BODY: 'http.request.body',
5-
HTTP_REQUEST_PARAMETER: 'http.request.parameter',
65
HTTP_REQUEST_COOKIE_VALUE: 'http.request.cookie.value',
76
HTTP_REQUEST_COOKIE_NAME: 'http.request.cookie.name',
87
HTTP_REQUEST_HEADER_NAME: 'http.request.header.name',
9-
HTTP_REQUEST_HEADER_VALUE: 'http.request.header'
8+
HTTP_REQUEST_HEADER_VALUE: 'http.request.header',
9+
HTTP_REQUEST_PARAMETER: 'http.request.parameter',
10+
HTTP_REQUEST_PATH_PARAM: 'http.request.path.parameter'
1011
}

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

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,12 +5,14 @@ const { getIastContext } = require('../iast-context')
55
const { storage } = require('../../../../../datadog-core')
66
const { taintObject } = require('./operations')
77
const {
8-
HTTP_REQUEST_PARAMETER,
98
HTTP_REQUEST_BODY,
109
HTTP_REQUEST_COOKIE_VALUE,
1110
HTTP_REQUEST_COOKIE_NAME,
1211
HTTP_REQUEST_HEADER_VALUE,
13-
HTTP_REQUEST_HEADER_NAME
12+
HTTP_REQUEST_HEADER_NAME,
13+
HTTP_REQUEST_PARAMETER,
14+
HTTP_REQUEST_PATH_PARAM
15+
1416
} = require('./origin-types')
1517

1618
class TaintTrackingPlugin extends Plugin {
@@ -44,6 +46,14 @@ class TaintTrackingPlugin extends Plugin {
4446
'datadog:cookie:parse:finish',
4547
({ cookies }) => this._cookiesTaintTrackingHandler(cookies)
4648
)
49+
this.addSub(
50+
'datadog:express:process_params:start',
51+
({ req }) => {
52+
if (req && req.params && typeof req.params === 'object') {
53+
this._taintTrackingHandler(HTTP_REQUEST_PATH_PARAM, req, 'params')
54+
}
55+
}
56+
)
4757
}
4858

4959
_taintTrackingHandler (type, target, property, iastContext = getIastContext(storage.getStore())) {

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

Lines changed: 28 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,13 +6,15 @@ const taintTrackingOperations = require('../../../../src/appsec/iast/taint-track
66
const dc = require('../../../../../diagnostics_channel')
77
const {
88
HTTP_REQUEST_COOKIE_VALUE,
9-
HTTP_REQUEST_COOKIE_NAME
9+
HTTP_REQUEST_COOKIE_NAME,
10+
HTTP_REQUEST_PATH_PARAM
1011
} = require('../../../../src/appsec/iast/taint-tracking/origin-types')
1112

1213
const middlewareNextChannel = dc.channel('apm:express:middleware:next')
1314
const queryParseFinishChannel = dc.channel('datadog:qs:parse:finish')
1415
const bodyParserFinishChannel = dc.channel('datadog:body-parser:read:finish')
1516
const cookieParseFinishCh = dc.channel('datadog:cookie:parse:finish')
17+
const processParamsStartCh = dc.channel('datadog:express:process_params:start')
1618

1719
describe('IAST Taint tracking plugin', () => {
1820
let taintTrackingPlugin
@@ -38,12 +40,13 @@ describe('IAST Taint tracking plugin', () => {
3840
sinon.restore()
3941
})
4042

41-
it('Should subscribe to body parser, qs and cookie channel', () => {
42-
expect(taintTrackingPlugin._subscriptions).to.have.lengthOf(4)
43+
it('Should subscribe to body parser, qs, cookie and process_params channel', () => {
44+
expect(taintTrackingPlugin._subscriptions).to.have.lengthOf(5)
4345
expect(taintTrackingPlugin._subscriptions[0]._channel.name).to.equals('datadog:body-parser:read:finish')
4446
expect(taintTrackingPlugin._subscriptions[1]._channel.name).to.equals('datadog:qs:parse:finish')
4547
expect(taintTrackingPlugin._subscriptions[2]._channel.name).to.equals('apm:express:middleware:next')
4648
expect(taintTrackingPlugin._subscriptions[3]._channel.name).to.equals('datadog:cookie:parse:finish')
49+
expect(taintTrackingPlugin._subscriptions[4]._channel.name).to.equals('datadog:express:process_params:start')
4750
})
4851

4952
describe('taint sources', () => {
@@ -202,5 +205,27 @@ describe('IAST Taint tracking plugin', () => {
202205
HTTP_REQUEST_COOKIE_NAME
203206
)
204207
})
208+
209+
it('Should taint request params when process params event is published', () => {
210+
const req = {
211+
params: {
212+
parameter1: 'tainted1'
213+
}
214+
}
215+
216+
processParamsStartCh.publish({ req })
217+
expect(taintTrackingOperations.taintObject).to.be.calledOnceWith(
218+
iastContext,
219+
req.params,
220+
HTTP_REQUEST_PATH_PARAM
221+
)
222+
})
223+
224+
it('Should not taint request params when process params event is published with non params request', () => {
225+
const req = {}
226+
227+
processParamsStartCh.publish({ req })
228+
expect(taintTrackingOperations.taintObject).to.not.be.called
229+
})
205230
})
206231
})
Lines changed: 169 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,169 @@
1+
'use strict'
2+
3+
const axios = require('axios')
4+
const getPort = require('get-port')
5+
const semver = require('semver')
6+
const agent = require('../../../../plugins/agent')
7+
const Config = require('../../../../../src/config')
8+
const { storage } = require('../../../../../../datadog-core')
9+
const iast = require('../../../../../src/appsec/iast')
10+
const iastContextFunctions = require('../../../../../src/appsec/iast/iast-context')
11+
const { isTainted, getRanges } = require('../../../../../src/appsec/iast/taint-tracking/operations')
12+
const { HTTP_REQUEST_PATH_PARAM } = require('../../../../../src/appsec/iast/taint-tracking/origin-types')
13+
14+
describe('Path params sourcing with express', () => {
15+
let express
16+
let expressVersion
17+
let appListener
18+
19+
withVersions('express', 'express', version => {
20+
const checkParamIsTaintedAndNext = (req, res, next, param) => {
21+
const store = storage.getStore()
22+
const iastContext = iastContextFunctions.getIastContext(store)
23+
24+
const pathParamValue = param
25+
const isParameterTainted = isTainted(iastContext, pathParamValue)
26+
expect(isParameterTainted).to.be.true
27+
const taintedParameterValueRanges = getRanges(iastContext, pathParamValue)
28+
expect(taintedParameterValueRanges[0].iinfo.type).to.be.equal(HTTP_REQUEST_PATH_PARAM)
29+
30+
next()
31+
}
32+
33+
before(() => {
34+
return agent.load(['http', 'express'], { client: false })
35+
})
36+
37+
after(() => {
38+
return agent.close({ ritmReset: false })
39+
})
40+
41+
beforeEach(() => {
42+
iast.enable(new Config({
43+
experimental: {
44+
iast: {
45+
enabled: true,
46+
requestSampling: 100
47+
}
48+
}
49+
}))
50+
51+
const expressRequire = require(`../../../../../../../versions/express@${version}`)
52+
express = expressRequire.get()
53+
expressVersion = expressRequire.version()
54+
})
55+
56+
afterEach(() => {
57+
appListener && appListener.close()
58+
appListener = null
59+
60+
iast.disable()
61+
})
62+
63+
it('should taint path params', function (done) {
64+
const app = express()
65+
app.get('/:parameter1/:parameter2', (req, res) => {
66+
const store = storage.getStore()
67+
const iastContext = iastContextFunctions.getIastContext(store)
68+
69+
for (const pathParamName of ['parameter1', 'parameter2']) {
70+
const pathParamValue = req.params[pathParamName]
71+
const isParameterTainted = isTainted(iastContext, pathParamValue)
72+
expect(isParameterTainted).to.be.true
73+
const taintedParameterValueRanges = getRanges(iastContext, pathParamValue)
74+
expect(taintedParameterValueRanges[0].iinfo.type).to.be.equal(HTTP_REQUEST_PATH_PARAM)
75+
}
76+
77+
res.status(200).send()
78+
})
79+
80+
getPort().then(port => {
81+
appListener = app.listen(port, 'localhost', () => {
82+
axios
83+
.get(`http://localhost:${port}/tainted1/tainted2`)
84+
.then(() => done())
85+
.catch(done)
86+
})
87+
})
88+
})
89+
90+
it('should taint path params in nested routers with merged params', function (done) {
91+
if (!semver.satisfies(expressVersion, '>4.5.0')) {
92+
this.skip()
93+
}
94+
95+
const app = express()
96+
const nestedRouter = express.Router({ mergeParams: true })
97+
98+
nestedRouter.get('/:parameterChild', (req, res) => {
99+
const store = storage.getStore()
100+
const iastContext = iastContextFunctions.getIastContext(store)
101+
102+
for (const pathParamName of ['parameterParent', 'parameterChild']) {
103+
const pathParamValue = req.params[pathParamName]
104+
const isParameterTainted = isTainted(iastContext, pathParamValue)
105+
expect(isParameterTainted).to.be.true
106+
const taintedParameterValueRanges = getRanges(iastContext, pathParamValue)
107+
expect(taintedParameterValueRanges[0].iinfo.type).to.be.equal(HTTP_REQUEST_PATH_PARAM)
108+
}
109+
110+
res.status(200).send()
111+
})
112+
113+
app.use('/:parameterParent', nestedRouter)
114+
115+
getPort().then(port => {
116+
appListener = app.listen(port, 'localhost', () => {
117+
axios
118+
.get(`http://localhost:${port}/tainted1/tainted2`)
119+
.then(() => done())
120+
.catch(done)
121+
})
122+
})
123+
})
124+
125+
it('should taint path param on router.params callback', function (done) {
126+
const app = express()
127+
128+
app.use('/:parameter1/:parameter2', (req, res) => {
129+
res.status(200).send()
130+
})
131+
132+
app.param('parameter1', checkParamIsTaintedAndNext)
133+
app.param('parameter2', checkParamIsTaintedAndNext)
134+
135+
getPort().then(port => {
136+
appListener = app.listen(port, 'localhost', () => {
137+
axios
138+
.get(`http://localhost:${port}/tainted1/tainted2`)
139+
.then(() => done())
140+
.catch(done)
141+
})
142+
})
143+
})
144+
145+
it('should taint path param on router.params callback with custom implementation', function (done) {
146+
const app = express()
147+
148+
app.use('/:parameter1/:parameter2', (req, res) => {
149+
res.status(200).send()
150+
})
151+
152+
app.param((param, option) => {
153+
return checkParamIsTaintedAndNext
154+
})
155+
156+
app.param('parameter1')
157+
app.param('parameter2')
158+
159+
getPort().then(port => {
160+
appListener = app.listen(port, 'localhost', () => {
161+
axios
162+
.get(`http://localhost:${port}/tainted1/tainted2`)
163+
.then(() => done())
164+
.catch(done)
165+
})
166+
})
167+
})
168+
})
169+
})

0 commit comments

Comments
 (0)