diff --git a/packages/datadog-instrumentations/src/router.js b/packages/datadog-instrumentations/src/router.js index 44b275ad10..7b6c9e0f07 100644 --- a/packages/datadog-instrumentations/src/router.js +++ b/packages/datadog-instrumentations/src/router.js @@ -179,10 +179,9 @@ addHook({ name: 'router', versions: ['>=1'] }, Router => { return Router }) -function createWrapLayerMethod () { - // TODO: create a dedicate channel for this - const processParamsStartCh = channel('datadog:express:process_params:start') +const processParamsStartCh = channel('datadog:express:process_params:start') +function createWrapLayerMethod () { return function wrapMethod (original) { return function wrappedHandleRequest (req, res, next) { if (processParamsStartCh.hasSubscribers) { @@ -191,8 +190,7 @@ function createWrapLayerMethod () { processParamsStartCh.publish({ req, res, - abortController, - params: req?.params + abortController }) if (abortController.signal.aborted) return } @@ -209,4 +207,33 @@ addHook({ return Layer }) +function wrapProcessParamsMethod (original) { + return function wrappedProcessParams (name, fn) { + const wrappedFn = function wrappedParamCallback (req, res, next, param) { + if (processParamsStartCh.hasSubscribers) { + const abortController = new AbortController() + + processParamsStartCh.publish({ + req, + res, + abortController + }) + + if (abortController.signal.aborted) return + } + + return fn(req, res, next, param, name) + } + + return original.call(this, name, wrappedFn) + } +} + +addHook({ + name: 'router', versions: ['>=2'] +}, router => { + shimmer.wrap(router.prototype, 'param', wrapProcessParamsMethod) + return router +}) + module.exports = { createWrapRouterMethod } diff --git a/packages/dd-trace/src/appsec/channels.js b/packages/dd-trace/src/appsec/channels.js index 8e7f27211c..1368e937dc 100644 --- a/packages/dd-trace/src/appsec/channels.js +++ b/packages/dd-trace/src/appsec/channels.js @@ -19,6 +19,7 @@ module.exports = { nextBodyParsed: dc.channel('apm:next:body-parsed'), nextQueryParsed: dc.channel('apm:next:query-parsed'), expressProcessParams: dc.channel('datadog:express:process_params:start'), + routerParam: dc.channel('datadog:router:param:start'), responseBody: dc.channel('datadog:express:response:json:start'), responseWriteHead: dc.channel('apm:http:server:response:writeHead:start'), httpClientRequestStart: dc.channel('apm:http:client:request:start'), diff --git a/packages/dd-trace/src/appsec/iast/taint-tracking/plugin.js b/packages/dd-trace/src/appsec/iast/taint-tracking/plugin.js index 71aba6101e..025234c3c5 100644 --- a/packages/dd-trace/src/appsec/iast/taint-tracking/plugin.js +++ b/packages/dd-trace/src/appsec/iast/taint-tracking/plugin.js @@ -78,6 +78,15 @@ class TaintTrackingPlugin extends SourceIastPlugin { } ) + this.addSub( + { channelName: 'datadog:router:param:start', tag: HTTP_REQUEST_PATH_PARAM }, + ({ req }) => { + if (req && req.params !== null && typeof req.params === 'object') { + this._taintTrackingHandler(HTTP_REQUEST_PATH_PARAM, req, 'params') + } + } + ) + this.addSub( { channelName: 'datadog:query:read:finish', tag: HTTP_REQUEST_QUERY }, ({ query }) => { diff --git a/packages/dd-trace/src/appsec/index.js b/packages/dd-trace/src/appsec/index.js index 3f0513d51c..35cb31fe5e 100644 --- a/packages/dd-trace/src/appsec/index.js +++ b/packages/dd-trace/src/appsec/index.js @@ -16,7 +16,8 @@ const { expressProcessParams, responseBody, responseWriteHead, - responseSetHeader + responseSetHeader, + routerParam } = require('./channels') const waf = require('./waf') const addresses = require('./addresses') @@ -67,6 +68,7 @@ function enable (_config) { nextBodyParsed.subscribe(onRequestBodyParsed) nextQueryParsed.subscribe(onRequestQueryParsed) expressProcessParams.subscribe(onRequestProcessParams) + routerParam.subscribe(onRequestProcessParams) responseBody.subscribe(onResponseBody) responseWriteHead.subscribe(onResponseWriteHead) responseSetHeader.subscribe(onResponseSetHeader) @@ -311,6 +313,7 @@ function disable () { if (nextBodyParsed.hasSubscribers) nextBodyParsed.unsubscribe(onRequestBodyParsed) if (nextQueryParsed.hasSubscribers) nextQueryParsed.unsubscribe(onRequestQueryParsed) if (expressProcessParams.hasSubscribers) expressProcessParams.unsubscribe(onRequestProcessParams) + if (routerParam.hasSubscribers) routerParam.unsubscribe(onRequestProcessParams) if (responseBody.hasSubscribers) responseBody.unsubscribe(onResponseBody) if (responseWriteHead.hasSubscribers) responseWriteHead.unsubscribe(onResponseWriteHead) if (responseSetHeader.hasSubscribers) responseSetHeader.unsubscribe(onResponseSetHeader) diff --git a/packages/dd-trace/test/appsec/iast/taint-tracking/plugin.spec.js b/packages/dd-trace/test/appsec/iast/taint-tracking/plugin.spec.js index 1a21b0a5b0..14cc58e635 100644 --- a/packages/dd-trace/test/appsec/iast/taint-tracking/plugin.spec.js +++ b/packages/dd-trace/test/appsec/iast/taint-tracking/plugin.spec.js @@ -16,6 +16,7 @@ const queryParseFinishChannel = dc.channel('datadog:qs:parse:finish') const bodyParserFinishChannel = dc.channel('datadog:body-parser:read:finish') const cookieParseFinishCh = dc.channel('datadog:cookie:parse:finish') const processParamsStartCh = dc.channel('datadog:express:process_params:start') +const routerParamStartCh = dc.channel('datadog:router:param:start') describe('IAST Taint tracking plugin', () => { let taintTrackingPlugin @@ -42,16 +43,18 @@ describe('IAST Taint tracking plugin', () => { }) it('Should subscribe to body parser, qs, cookie and process_params channel', () => { - expect(taintTrackingPlugin._subscriptions).to.have.lengthOf(9) + expect(taintTrackingPlugin._subscriptions).to.have.lengthOf(11) expect(taintTrackingPlugin._subscriptions[0]._channel.name).to.equals('datadog:body-parser:read:finish') expect(taintTrackingPlugin._subscriptions[1]._channel.name).to.equals('datadog:multer:read:finish') expect(taintTrackingPlugin._subscriptions[2]._channel.name).to.equals('datadog:qs:parse:finish') expect(taintTrackingPlugin._subscriptions[3]._channel.name).to.equals('apm:express:middleware:next') expect(taintTrackingPlugin._subscriptions[4]._channel.name).to.equals('datadog:cookie:parse:finish') expect(taintTrackingPlugin._subscriptions[5]._channel.name).to.equals('datadog:express:process_params:start') - expect(taintTrackingPlugin._subscriptions[6]._channel.name).to.equals('apm:graphql:resolve:start') - expect(taintTrackingPlugin._subscriptions[7]._channel.name).to.equals('datadog:url:parse:finish') - expect(taintTrackingPlugin._subscriptions[8]._channel.name).to.equals('datadog:url:getter:finish') + expect(taintTrackingPlugin._subscriptions[6]._channel.name).to.equals('datadog:router:param:start') + expect(taintTrackingPlugin._subscriptions[7]._channel.name).to.equals('datadog:query:read:finish') + expect(taintTrackingPlugin._subscriptions[8]._channel.name).to.equals('apm:graphql:resolve:start') + expect(taintTrackingPlugin._subscriptions[9]._channel.name).to.equals('datadog:url:parse:finish') + expect(taintTrackingPlugin._subscriptions[10]._channel.name).to.equals('datadog:url:getter:finish') }) describe('taint sources', () => { @@ -209,7 +212,7 @@ describe('IAST Taint tracking plugin', () => { ) }) - it('Should taint request params when process params event is published', () => { + it('Should taint request params when process params event is published with processParamsStartCh', () => { const req = { params: { parameter1: 'tainted1' @@ -224,6 +227,21 @@ describe('IAST Taint tracking plugin', () => { ) }) + it('Should taint request params when process params event is published with routerParamStartCh', () => { + const req = { + params: { + parameter1: 'tainted1' + } + } + + routerParamStartCh.publish({ req }) + expect(taintTrackingOperations.taintObject).to.be.calledOnceWith( + iastContext, + req.params, + HTTP_REQUEST_PATH_PARAM + ) + }) + it('Should not taint request params when process params event is published with non params request', () => { const req = {} diff --git a/packages/dd-trace/test/appsec/iast/taint-tracking/sources/taint-tracking.express.plugin.spec.js b/packages/dd-trace/test/appsec/iast/taint-tracking/sources/taint-tracking.express.plugin.spec.js index a2f7b935ae..ce348382a2 100644 --- a/packages/dd-trace/test/appsec/iast/taint-tracking/sources/taint-tracking.express.plugin.spec.js +++ b/packages/dd-trace/test/appsec/iast/taint-tracking/sources/taint-tracking.express.plugin.spec.js @@ -80,11 +80,11 @@ describe('Path params sourcing with express', () => { let appListener withVersions('express', 'express', version => { - const checkParamIsTaintedAndNext = (req, res, next, param) => { + const checkParamIsTaintedAndNext = (req, res, next, param, name) => { const store = storage.getStore() const iastContext = iastContextFunctions.getIastContext(store) - const pathParamValue = param + const pathParamValue = name ? req.params[name] : req.params const isParameterTainted = isTainted(iastContext, pathParamValue) expect(isParameterTainted).to.be.true const taintedParameterValueRanges = getRanges(iastContext, pathParamValue) @@ -185,8 +185,7 @@ describe('Path params sourcing with express', () => { }) }) - // to be fixed - it.skip('should taint path param on router.params callback', function (done) { + it('should taint path param on router.params callback', function (done) { const app = express() app.use('/:parameter1/:parameter2', (req, res) => { @@ -206,20 +205,15 @@ describe('Path params sourcing with express', () => { }) }) - // to be fixed - it.skip('should taint path param on router.params callback with custom implementation', function (done) { + it('should taint path param on router.params callback with custom implementation', function (done) { const app = express() app.use('/:parameter1/:parameter2', (req, res) => { res.status(200).send() }) - app.param((param, option) => { - return checkParamIsTaintedAndNext - }) - - app.param('parameter1') - app.param('parameter2') + app.param('parameter1', checkParamIsTaintedAndNext) + app.param('parameter2', checkParamIsTaintedAndNext) appListener = app.listen(0, 'localhost', () => { const port = appListener.address().port diff --git a/packages/dd-trace/test/appsec/index.spec.js b/packages/dd-trace/test/appsec/index.spec.js index 4b8c6c0438..26d8c36e68 100644 --- a/packages/dd-trace/test/appsec/index.spec.js +++ b/packages/dd-trace/test/appsec/index.spec.js @@ -15,6 +15,7 @@ const { nextBodyParsed, nextQueryParsed, expressProcessParams, + routerParam, responseBody, responseWriteHead, responseSetHeader @@ -175,6 +176,7 @@ describe('AppSec Index', function () { expect(nextBodyParsed.hasSubscribers).to.be.false expect(nextQueryParsed.hasSubscribers).to.be.false expect(expressProcessParams.hasSubscribers).to.be.false + expect(routerParam.hasSubscribers).to.be.false expect(responseWriteHead.hasSubscribers).to.be.false expect(responseSetHeader.hasSubscribers).to.be.false @@ -187,6 +189,7 @@ describe('AppSec Index', function () { expect(nextBodyParsed.hasSubscribers).to.be.true expect(nextQueryParsed.hasSubscribers).to.be.true expect(expressProcessParams.hasSubscribers).to.be.true + expect(routerParam.hasSubscribers).to.be.true expect(responseWriteHead.hasSubscribers).to.be.true expect(responseSetHeader.hasSubscribers).to.be.true }) @@ -268,6 +271,7 @@ describe('AppSec Index', function () { expect(nextBodyParsed.hasSubscribers).to.be.false expect(nextQueryParsed.hasSubscribers).to.be.false expect(expressProcessParams.hasSubscribers).to.be.false + expect(routerParam.hasSubscribers).to.be.false expect(responseWriteHead.hasSubscribers).to.be.false expect(responseSetHeader.hasSubscribers).to.be.false })