diff --git a/.github/workflows/appsec.yml b/.github/workflows/appsec.yml index e95f82307fc..13f82b8afd2 100644 --- a/.github/workflows/appsec.yml +++ b/.github/workflows/appsec.yml @@ -130,18 +130,3 @@ 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/packages/datadog-instrumentations/src/cookie.js b/packages/datadog-instrumentations/src/cookie.js deleted file mode 100644 index 291045d49ab..00000000000 --- a/packages/datadog-instrumentations/src/cookie.js +++ /dev/null @@ -1,21 +0,0 @@ -'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 ed63465ac40..45762c94a8e 100644 --- a/packages/datadog-instrumentations/src/helpers/hooks.js +++ b/packages/datadog-instrumentations/src/helpers/hooks.js @@ -25,7 +25,6 @@ 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 4524391b0d7..2b1c2725195 100644 --- a/packages/dd-trace/src/appsec/iast/index.js +++ b/packages/dd-trace/src/appsec/iast/index.js @@ -5,13 +5,7 @@ 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, - taintTrackingPlugin -} = require('./taint-tracking') +const { enableTaintTracking, disableTaintTracking, createTransaction, removeTransaction } = require('./taint-tracking') const telemetryLogs = require('./telemetry/logs') const IAST_ENABLED_TAG_KEY = '_dd.iast.enabled' @@ -54,7 +48,6 @@ 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 137b5c4fe60..3d5da490725 100644 --- a/packages/dd-trace/src/appsec/iast/taint-tracking/index.js +++ b/packages/dd-trace/src/appsec/iast/taint-tracking/index.js @@ -23,6 +23,5 @@ module.exports = { }, setMaxTransactions: setMaxTransactions, createTransaction: createTransaction, - removeTransaction: removeTransaction, - taintTrackingPlugin + removeTransaction: removeTransaction } 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 7724c4edbda..48a424823c7 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, keyTainting, keyType) { +function taintObject (iastContext, object, type) { 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, key } = queue.pop() + const { parent, property, value } = queue.pop() if (value === null) { continue } @@ -47,23 +47,14 @@ function taintObject (iastContext, object, type, keyTainting, keyType) { if (!parent) { result = tainted } else { - if (keyTainting && key) { - const taintedProperty = TaintedUtils.newTaintedString(transactionId, key, property, keyType) - parent[taintedProperty] = tainted - } else { - parent[property] = tainted - } + 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], key }) - } - if (parent && keyTainting && key) { - const taintedProperty = TaintedUtils.newTaintedString(transactionId, key, property, keyType) - parent[taintedProperty] = value + queue.push({ parent: value, property: property ? `${property}.${key}` : key, value: value[key] }) } } } 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 6285a4d2d79..4aff7550bee 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 @@ -2,9 +2,5 @@ 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' + HTTP_REQUEST_PARAMETER: 'http.request.parameter' } 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 1edb2b7c200..56599db6f16 100644 --- a/packages/dd-trace/src/appsec/iast/taint-tracking/plugin.js +++ b/packages/dd-trace/src/appsec/iast/taint-tracking/plugin.js @@ -3,15 +3,8 @@ 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 () { @@ -29,8 +22,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()) @@ -40,29 +33,16 @@ 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 if (target[property]) { + } else { 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 8c63d21ead4..aaf1b3133be 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,15 +4,10 @@ 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 @@ -38,12 +33,11 @@ describe('IAST Taint tracking plugin', () => { sinon.restore() }) - it('Should subscribe to body parser, qs and cookie channel', () => { - expect(taintTrackingPlugin._subscriptions).to.have.lengthOf(4) + it('Should subscribe to body parser and qs channel', () => { + expect(taintTrackingPlugin._subscriptions).to.have.lengthOf(3) 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', () => { @@ -61,10 +55,6 @@ describe('IAST Taint tracking plugin', () => { ) }) - afterEach(() => { - taintTrackingOperations.removeTransaction(iastContext) - }) - it('Should taint full object', () => { const originType = 'ORIGIN_TYPE' const objToBeTainted = { @@ -118,7 +108,11 @@ describe('IAST Taint tracking plugin', () => { } taintTrackingPlugin._taintTrackingHandler(originType, objToBeTainted, propertyToBeTainted) - expect(taintTrackingOperations.taintObject).to.not.be.called + expect(taintTrackingOperations.taintObject).to.be.calledOnceWith( + iastContext, + objToBeTainted[propertyToBeTainted], + originType + ) }) it('Should taint request parameter when qs event is published', () => { @@ -186,21 +180,5 @@ 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 deleted file mode 100644 index c0c7bea232d..00000000000 --- a/packages/dd-trace/test/appsec/iast/taint-tracking/sources/taint-tracking.cookie.plugin.spec.js +++ /dev/null @@ -1,64 +0,0 @@ -'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 deleted file mode 100644 index 12959cfd10a..00000000000 --- a/packages/dd-trace/test/appsec/iast/taint-tracking/sources/taint-tracking.headers.spec.js +++ /dev/null @@ -1,66 +0,0 @@ -'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 2044114983c..1a91fdfcdfa 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,75 +125,6 @@ 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'