diff --git a/.github/workflows/appsec.yml b/.github/workflows/appsec.yml index 13f82b8afd2..e95f82307fc 100644 --- a/.github/workflows/appsec.yml +++ b/.github/workflows/appsec.yml @@ -130,3 +130,18 @@ jobs: - uses: ./.github/actions/node/latest - run: yarn test:appsec:plugins:ci - uses: codecov/codecov-action@v2 + + sourcing: + runs-on: ubuntu-latest + env: + PLUGINS: cookie + steps: + - uses: actions/checkout@v2 + - uses: ./.github/actions/node/setup + - run: yarn install + - uses: ./.github/actions/node/16 + - run: yarn test:appsec:plugins:ci + - uses: ./.github/actions/node/18 + - run: yarn test:appsec:plugins:ci + - uses: ./.github/actions/node/latest + - run: yarn test:appsec:plugins:ci diff --git a/package.json b/package.json index 2c98739e936..21467726898 100644 --- a/package.json +++ b/package.json @@ -68,7 +68,7 @@ "dependencies": { "@datadog/native-appsec": "^3.2.0", "@datadog/native-iast-rewriter": "2.0.1", - "@datadog/native-iast-taint-tracking": "^1.4.1", + "@datadog/native-iast-taint-tracking": "^1.5.0", "@datadog/native-metrics": "^2.0.0", "@datadog/pprof": "^2.2.1", "@datadog/sketches-js": "^2.1.0", diff --git a/packages/datadog-instrumentations/src/cookie.js b/packages/datadog-instrumentations/src/cookie.js new file mode 100644 index 00000000000..291045d49ab --- /dev/null +++ b/packages/datadog-instrumentations/src/cookie.js @@ -0,0 +1,21 @@ +'use strict' + +const shimmer = require('../../datadog-shimmer') +const { channel, addHook } = require('./helpers/instrument') + +const cookieParseCh = channel('datadog:cookie:parse:finish') + +function wrapParse (originalParse) { + return function () { + const cookies = originalParse.apply(this, arguments) + if (cookieParseCh.hasSubscribers && cookies) { + cookieParseCh.publish({ cookies }) + } + return cookies + } +} + +addHook({ name: 'cookie', versions: ['>=0.4'] }, cookie => { + shimmer.wrap(cookie, 'parse', wrapParse) + return cookie +}) diff --git a/packages/datadog-instrumentations/src/helpers/hooks.js b/packages/datadog-instrumentations/src/helpers/hooks.js index 45762c94a8e..ed63465ac40 100644 --- a/packages/datadog-instrumentations/src/helpers/hooks.js +++ b/packages/datadog-instrumentations/src/helpers/hooks.js @@ -25,6 +25,7 @@ module.exports = { 'child_process': () => require('../child-process'), 'node:child_process': () => require('../child-process'), 'connect': () => require('../connect'), + 'cookie': () => require('../cookie'), 'couchbase': () => require('../couchbase'), 'crypto': () => require('../crypto'), 'cypress': () => require('../cypress'), diff --git a/packages/dd-trace/src/appsec/iast/index.js b/packages/dd-trace/src/appsec/iast/index.js index 03d03097973..205f16a349d 100644 --- a/packages/dd-trace/src/appsec/iast/index.js +++ b/packages/dd-trace/src/appsec/iast/index.js @@ -5,7 +5,13 @@ const { storage } = require('../../../../datadog-core') const overheadController = require('./overhead-controller') const dc = require('../../../../diagnostics_channel') const iastContextFunctions = require('./iast-context') -const { enableTaintTracking, disableTaintTracking, createTransaction, removeTransaction } = require('./taint-tracking') +const { + enableTaintTracking, + disableTaintTracking, + createTransaction, + removeTransaction, + taintTrackingPlugin +} = require('./taint-tracking') const { IAST_ENABLED_TAG_KEY } = require('./tags') const telemetryLogs = require('./telemetry/logs') @@ -48,6 +54,7 @@ function onIncomingHttpRequestStart (data) { const iastContext = iastContextFunctions.saveIastContext(store, topContext, { rootSpan, req: data.req }) createTransaction(rootSpan.context().toSpanId(), iastContext) overheadController.initializeRequestContext(iastContext) + taintTrackingPlugin.taintHeaders(data.req.headers, iastContext) } if (rootSpan.addTags) { rootSpan.addTags({ diff --git a/packages/dd-trace/src/appsec/iast/taint-tracking/index.js b/packages/dd-trace/src/appsec/iast/taint-tracking/index.js index 3d5da490725..137b5c4fe60 100644 --- a/packages/dd-trace/src/appsec/iast/taint-tracking/index.js +++ b/packages/dd-trace/src/appsec/iast/taint-tracking/index.js @@ -23,5 +23,6 @@ module.exports = { }, setMaxTransactions: setMaxTransactions, createTransaction: createTransaction, - removeTransaction: removeTransaction + removeTransaction: removeTransaction, + taintTrackingPlugin } diff --git a/packages/dd-trace/src/appsec/iast/taint-tracking/operations.js b/packages/dd-trace/src/appsec/iast/taint-tracking/operations.js index 48a424823c7..7724c4edbda 100644 --- a/packages/dd-trace/src/appsec/iast/taint-tracking/operations.js +++ b/packages/dd-trace/src/appsec/iast/taint-tracking/operations.js @@ -30,14 +30,14 @@ function newTaintedString (iastContext, string, name, type) { return result } -function taintObject (iastContext, object, type) { +function taintObject (iastContext, object, type, keyTainting, keyType) { let result = object if (iastContext && iastContext[IAST_TRANSACTION_ID]) { const transactionId = iastContext[IAST_TRANSACTION_ID] const queue = [{ parent: null, property: null, value: object }] const visited = new WeakSet() while (queue.length > 0) { - const { parent, property, value } = queue.pop() + const { parent, property, value, key } = queue.pop() if (value === null) { continue } @@ -47,14 +47,23 @@ function taintObject (iastContext, object, type) { if (!parent) { result = tainted } else { - parent[property] = tainted + if (keyTainting && key) { + const taintedProperty = TaintedUtils.newTaintedString(transactionId, key, property, keyType) + parent[taintedProperty] = tainted + } else { + parent[property] = tainted + } } } else if (typeof value === 'object' && !visited.has(value)) { visited.add(value) const keys = Object.keys(value) for (let i = 0; i < keys.length; i++) { const key = keys[i] - queue.push({ parent: value, property: property ? `${property}.${key}` : key, value: value[key] }) + queue.push({ parent: value, property: property ? `${property}.${key}` : key, value: value[key], key }) + } + if (parent && keyTainting && key) { + const taintedProperty = TaintedUtils.newTaintedString(transactionId, key, property, keyType) + parent[taintedProperty] = value } } } catch (e) { diff --git a/packages/dd-trace/src/appsec/iast/taint-tracking/origin-types.js b/packages/dd-trace/src/appsec/iast/taint-tracking/origin-types.js index 45e14bcb313..6285a4d2d79 100644 --- a/packages/dd-trace/src/appsec/iast/taint-tracking/origin-types.js +++ b/packages/dd-trace/src/appsec/iast/taint-tracking/origin-types.js @@ -3,5 +3,8 @@ module.exports = { HTTP_REQUEST_BODY: 'http.request.body', HTTP_REQUEST_PARAMETER: 'http.request.parameter', + HTTP_REQUEST_COOKIE_VALUE: 'http.request.cookie.value', + HTTP_REQUEST_COOKIE_NAME: 'http.request.cookie.name', + HTTP_REQUEST_HEADER_NAME: 'http.request.header.name', HTTP_REQUEST_HEADER_VALUE: 'http.request.header' } 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 56599db6f16..1edb2b7c200 100644 --- a/packages/dd-trace/src/appsec/iast/taint-tracking/plugin.js +++ b/packages/dd-trace/src/appsec/iast/taint-tracking/plugin.js @@ -3,8 +3,15 @@ const Plugin = require('../../../plugins/plugin') const { getIastContext } = require('../iast-context') const { storage } = require('../../../../../datadog-core') -const { HTTP_REQUEST_PARAMETER, HTTP_REQUEST_BODY } = require('./origin-types') const { taintObject } = require('./operations') +const { + HTTP_REQUEST_PARAMETER, + HTTP_REQUEST_BODY, + HTTP_REQUEST_COOKIE_VALUE, + HTTP_REQUEST_COOKIE_NAME, + HTTP_REQUEST_HEADER_VALUE, + HTTP_REQUEST_HEADER_NAME +} = require('./origin-types') class TaintTrackingPlugin extends Plugin { constructor () { @@ -22,8 +29,8 @@ class TaintTrackingPlugin extends Plugin { ) this.addSub( 'datadog:qs:parse:finish', - ({ qs }) => this._taintTrackingHandler(HTTP_REQUEST_PARAMETER, qs)) - + ({ qs }) => this._taintTrackingHandler(HTTP_REQUEST_PARAMETER, qs) + ) this.addSub('apm:express:middleware:next', ({ req }) => { if (req && req.body && typeof req.body === 'object') { const iastContext = getIastContext(storage.getStore()) @@ -33,16 +40,29 @@ class TaintTrackingPlugin extends Plugin { } } }) + this.addSub( + 'datadog:cookie:parse:finish', + ({ cookies }) => this._cookiesTaintTrackingHandler(cookies) + ) } _taintTrackingHandler (type, target, property, iastContext = getIastContext(storage.getStore())) { if (!property) { taintObject(iastContext, target, type) - } else { + } else if (target[property]) { target[property] = taintObject(iastContext, target[property], type) } } + _cookiesTaintTrackingHandler (target) { + const iastContext = getIastContext(storage.getStore()) + taintObject(iastContext, target, HTTP_REQUEST_COOKIE_VALUE, true, HTTP_REQUEST_COOKIE_NAME) + } + + taintHeaders (headers, iastContext) { + taintObject(iastContext, headers, HTTP_REQUEST_HEADER_VALUE, true, HTTP_REQUEST_HEADER_NAME) + } + enable () { this.configure(true) } 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 aaf1b3133be..aae7db2ae24 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 @@ -4,10 +4,15 @@ const proxyquire = require('proxyquire') const iastContextFunctions = require('../../../../src/appsec/iast/iast-context') const taintTrackingOperations = require('../../../../src/appsec/iast/taint-tracking/operations') const dc = require('../../../../../diagnostics_channel') +const { + HTTP_REQUEST_COOKIE_VALUE, + HTTP_REQUEST_COOKIE_NAME +} = require('../../../../src/appsec/iast/taint-tracking/origin-types') const middlewareNextChannel = dc.channel('apm:express:middleware:next') 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') describe('IAST Taint tracking plugin', () => { let taintTrackingPlugin @@ -33,11 +38,12 @@ describe('IAST Taint tracking plugin', () => { sinon.restore() }) - it('Should subscribe to body parser and qs channel', () => { - expect(taintTrackingPlugin._subscriptions).to.have.lengthOf(3) + it('Should subscribe to body parser, qs and cookie channel', () => { + expect(taintTrackingPlugin._subscriptions).to.have.lengthOf(4) expect(taintTrackingPlugin._subscriptions[0]._channel.name).to.equals('datadog:body-parser:read:finish') expect(taintTrackingPlugin._subscriptions[1]._channel.name).to.equals('datadog:qs:parse:finish') expect(taintTrackingPlugin._subscriptions[2]._channel.name).to.equals('apm:express:middleware:next') + expect(taintTrackingPlugin._subscriptions[3]._channel.name).to.equals('datadog:cookie:parse:finish') }) describe('taint sources', () => { @@ -55,6 +61,10 @@ describe('IAST Taint tracking plugin', () => { ) }) + afterEach(() => { + taintTrackingOperations.removeTransaction(iastContext) + }) + it('Should taint full object', () => { const originType = 'ORIGIN_TYPE' const objToBeTainted = { @@ -108,11 +118,7 @@ describe('IAST Taint tracking plugin', () => { } taintTrackingPlugin._taintTrackingHandler(originType, objToBeTainted, propertyToBeTainted) - expect(taintTrackingOperations.taintObject).to.be.calledOnceWith( - iastContext, - objToBeTainted[propertyToBeTainted], - originType - ) + expect(taintTrackingOperations.taintObject).not.to.be.called }) it('Should taint request parameter when qs event is published', () => { @@ -180,5 +186,21 @@ describe('IAST Taint tracking plugin', () => { 'http.request.body' ) }) + + it('Should taint cookies when cookie parser event is published', () => { + const cookies = { + cookie1: 'tainted_cookie' + } + + cookieParseFinishCh.publish({ cookies }) + + expect(taintTrackingOperations.taintObject).to.be.calledOnceWith( + iastContext, + cookies, + HTTP_REQUEST_COOKIE_VALUE, + true, + HTTP_REQUEST_COOKIE_NAME + ) + }) }) }) diff --git a/packages/dd-trace/test/appsec/iast/taint-tracking/sources/taint-tracking.cookie.plugin.spec.js b/packages/dd-trace/test/appsec/iast/taint-tracking/sources/taint-tracking.cookie.plugin.spec.js new file mode 100644 index 00000000000..c0c7bea232d --- /dev/null +++ b/packages/dd-trace/test/appsec/iast/taint-tracking/sources/taint-tracking.cookie.plugin.spec.js @@ -0,0 +1,64 @@ +'use strict' + +const axios = require('axios') +const Config = require('../../../../../src/config') +const { storage } = require('../../../../../../datadog-core') +const iast = require('../../../../../src/appsec/iast') +const iastContextFunctions = require('../../../../../src/appsec/iast/iast-context') +const { isTainted, getRanges } = require('../../../../../src/appsec/iast/taint-tracking/operations') +const { + HTTP_REQUEST_COOKIE_NAME, + HTTP_REQUEST_COOKIE_VALUE +} = require('../../../../../src/appsec/iast/taint-tracking/origin-types') +const { testInRequest } = require('../../utils') + +describe('Cookies sourcing with cookies', () => { + let cookie + withVersions('cookie', 'cookie', version => { + function app () { + const store = storage.getStore() + const iastContext = iastContextFunctions.getIastContext(store) + + const rawCookies = 'cookie=value' + const parsedCookies = cookie.parse(rawCookies) + Object.getOwnPropertySymbols(parsedCookies).forEach(cookieName => { + const cookieValue = parsedCookies[cookieName] + const isCookieValueTainted = isTainted(iastContext, cookieValue) + expect(isCookieValueTainted).to.be.true + const taintedCookieValueRanges = getRanges(iastContext, cookieValue) + expect(taintedCookieValueRanges[0].iinfo.type).to.be.equal(HTTP_REQUEST_COOKIE_VALUE) + const isCookieNameTainted = isTainted(iastContext, cookieName) + expect(isCookieNameTainted).to.be.true + const taintedCookieNameRanges = getRanges(iastContext, cookieName) + expect(taintedCookieNameRanges[0].iinfo.type).to.be.equal(HTTP_REQUEST_COOKIE_NAME) + }) + } + + function tests (config) { + beforeEach(() => { + iast.enable(new Config({ + experimental: { + iast: { + enabled: true, + requestSampling: 100 + } + } + })) + + cookie = require(`../../../../../../../versions/cookie@${version}`).get() + }) + + afterEach(() => { + iast.disable() + }) + + it('should taint cookies', (done) => { + axios.get(`http://localhost:${config.port}/`) + .then(() => done()) + .catch(done) + }) + } + + testInRequest(app, tests) + }) +}) diff --git a/packages/dd-trace/test/appsec/iast/taint-tracking/sources/taint-tracking.headers.spec.js b/packages/dd-trace/test/appsec/iast/taint-tracking/sources/taint-tracking.headers.spec.js new file mode 100644 index 00000000000..12959cfd10a --- /dev/null +++ b/packages/dd-trace/test/appsec/iast/taint-tracking/sources/taint-tracking.headers.spec.js @@ -0,0 +1,66 @@ +'use strict' + +const axios = require('axios') +const Config = require('../../../../../src/config') +const { storage } = require('../../../../../../datadog-core') +const iast = require('../../../../../src/appsec/iast') +const iastContextFunctions = require('../../../../../src/appsec/iast/iast-context') +const { isTainted, getRanges } = require('../../../../../src/appsec/iast/taint-tracking/operations') +const { + HTTP_REQUEST_HEADER_NAME, + HTTP_REQUEST_HEADER_VALUE +} = require('../../../../../src/appsec/iast/taint-tracking/origin-types') +const { testInRequest } = require('../../utils') + +describe('Headers sourcing', () => { + function app (req) { + const store = storage.getStore() + const iastContext = iastContextFunctions.getIastContext(store) + + Object.keys(req.headers).forEach(headerName => { + const headerValue = req.headers[headerName] + const isHeaderValueTainted = isTainted(iastContext, headerValue) + expect(isHeaderValueTainted).to.be.true + const taintedHeaderValueRanges = getRanges(iastContext, headerValue) + expect(taintedHeaderValueRanges[0].iinfo.type).to.be.equal(HTTP_REQUEST_HEADER_VALUE) + // @see packages/dd-trace/test/appsec/iast/taint-tracking/taint-tracking-operations.spec.js + if (headerName.length >= 10) { + const isHeaderNameTainted = isTainted(iastContext, headerName) + expect(isHeaderNameTainted).to.be.true + const taintedHeaderNameRanges = getRanges(iastContext, headerName) + expect(taintedHeaderNameRanges[0].iinfo.type).to.be.equal(HTTP_REQUEST_HEADER_NAME) + } + }) + } + + function tests (config) { + beforeEach(() => { + iast.enable(new Config({ + experimental: { + iast: { + enabled: true, + requestSampling: 100 + } + } + })) + }) + + afterEach(() => { + iast.disable() + }) + + it('should taint headers', (done) => { + axios.get( + `http://localhost:${config.port}/`, + { + headers: { + 'x-iast-test-header': 'value to be tainted' + } + }) + .then(() => done()) + .catch(done) + }) + } + + testInRequest(app, tests) +}) diff --git a/packages/dd-trace/test/appsec/iast/taint-tracking/taint-tracking-operations.spec.js b/packages/dd-trace/test/appsec/iast/taint-tracking/taint-tracking-operations.spec.js index 1a91fdfcdfa..2044114983c 100644 --- a/packages/dd-trace/test/appsec/iast/taint-tracking/taint-tracking-operations.spec.js +++ b/packages/dd-trace/test/appsec/iast/taint-tracking/taint-tracking-operations.spec.js @@ -125,6 +125,75 @@ describe('IAST TaintTracking Operations', () => { expect(result).to.equal(obj) }) + it('Should call newTaintedString in object keys when keyTainting is true', () => { + const iastContext = {} + const transactionId = 'id' + taintTrackingOperations.createTransaction(transactionId, iastContext) + + const VALUE_TYPE = 'value.type' + const KEY_TYPE = 'key.type' + + const obj = { + key1: 'parent', + key2: { + key3: 'child' + } + } + + const result = taintTrackingOperations.taintObject(iastContext, obj, VALUE_TYPE, true, KEY_TYPE) + expect(taintedUtilsMock.newTaintedString.getCall(0)).to.have.been + .calledWithExactly(transactionId, 'key2', 'key2', KEY_TYPE) + expect(taintedUtilsMock.newTaintedString.getCall(1)).to.have.been + .calledWithExactly(transactionId, 'child', 'key2.key3', VALUE_TYPE) + expect(taintedUtilsMock.newTaintedString.getCall(2)).to.have.been + .calledWithExactly(transactionId, 'key3', 'key2.key3', KEY_TYPE) + expect(taintedUtilsMock.newTaintedString.getCall(3)).to.have.been + .calledWithExactly(transactionId, 'parent', 'key1', VALUE_TYPE) + expect(taintedUtilsMock.newTaintedString.getCall(4)).to.have.been + .calledWithExactly(transactionId, 'key1', 'key1', KEY_TYPE) + expect(result).to.equal(obj) + + taintTrackingOperations.removeTransaction() + }) + + it('Should taint object keys when taintingKeys is true', () => { + const taintTrackingOperations = require('../../../../src/appsec/iast/taint-tracking/operations') + const iastContext = {} + const transactionId = 'id' + taintTrackingOperations.createTransaction(transactionId, iastContext) + + const VALUE_TYPE = 'value.type' + const KEY_TYPE = 'key.type' + + const obj = { + keyLargerThan10Chars: 'parent', + anotherKeyLargerThan10Chars: { + shortKey: 'child' + } + } + + const checkValueAndKeyAreTainted = (target, key) => { + // Strings shorter than 10 characters are not tainted directly, but a new instance of the string is created + // in dd-native-iast-taint-tracking. This leads to object keys that meet this condition not being detected + // as tainted + if (key && key.length >= 10) { + const isKeyTainted = taintTrackingOperations.isTainted(iastContext, key) + expect(isKeyTainted).to.be.true + } + + const obj = key ? target[key] : target + if (!key || typeof obj === 'object') { + Object.keys(obj).forEach(k => checkValueAndKeyAreTainted(obj, k)) + } else if (typeof obj === 'string') { + const isValueTainted = taintTrackingOperations.isTainted(iastContext, obj) + expect(isValueTainted).to.be.true + } + } + + taintTrackingOperations.taintObject(iastContext, obj, VALUE_TYPE, true, KEY_TYPE) + checkValueAndKeyAreTainted(obj, null) + }) + it('Should handle the exception', () => { const iastContext = {} const transactionId = 'id' diff --git a/yarn.lock b/yarn.lock index 114933c0eab..2eb74c0f91b 100644 --- a/yarn.lock +++ b/yarn.lock @@ -394,10 +394,10 @@ dependencies: node-gyp-build "^4.5.0" -"@datadog/native-iast-taint-tracking@^1.4.1": - version "1.4.1" - resolved "https://registry.yarnpkg.com/@datadog/native-iast-taint-tracking/-/native-iast-taint-tracking-1.4.1.tgz#e84198607230e0193d0d88f06c1f56d5938daf9e" - integrity sha512-wWJebnK5fADXGGwmoHi9ElMsvR/M4IZpRxBxzAfKU2WI1GRkCvSxQBhbIFUTQEuO7l6ZOpASWQ9yUXK3cx8n+w== +"@datadog/native-iast-taint-tracking@^1.5.0": + version "1.5.0" + resolved "https://registry.yarnpkg.com/@datadog/native-iast-taint-tracking/-/native-iast-taint-tracking-1.5.0.tgz#1a55eca6692079ac6167696682acb972aa0b0181" + integrity sha512-SOWIk1M6PZH0osNB191Voz2rKBPoF5hISWVSK9GiJPrD40+xjib1Z/bFDV7EkDn3kjOyordSBdNPG5zOqZJdyg== dependencies: node-gyp-build "^3.9.0"