From 1ea43eb757a5e13866a339e5b46c20dac5ce06d7 Mon Sep 17 00:00:00 2001 From: Ugaitz Urien Date: Tue, 29 Oct 2024 20:03:34 +0100 Subject: [PATCH] Support url.parse, url.URL.parse and new url.URL for taint tracking --- .github/workflows/plugins.yml | 13 +- .../src/helpers/hooks.js | 2 + packages/datadog-instrumentations/src/url.js | 85 +++++++++++++ .../datadog-instrumentations/test/url.spec.js | 114 ++++++++++++++++++ .../src/appsec/iast/taint-tracking/plugin.js | 41 +++++++ .../plugin.express.plugin.spec.js | 90 ++++++++++++++ .../appsec/iast/taint-tracking/plugin.spec.js | 4 +- 7 files changed, 347 insertions(+), 2 deletions(-) create mode 100644 packages/datadog-instrumentations/src/url.js create mode 100644 packages/datadog-instrumentations/test/url.spec.js diff --git a/.github/workflows/plugins.yml b/.github/workflows/plugins.yml index c71ff2a2441..7f5db15c165 100644 --- a/.github/workflows/plugins.yml +++ b/.github/workflows/plugins.yml @@ -484,6 +484,17 @@ jobs: uses: ./.github/actions/testagent/logs - uses: codecov/codecov-action@v3 + url: + strategy: + matrix: + node-version: ['18', '20', 'latest'] + runs-on: ubuntu-latest + env: + PLUGINS: url + steps: + - uses: actions/checkout@v4 + - uses: ./.github/actions/plugins/test + http2: runs-on: ubuntu-latest env: @@ -578,7 +589,7 @@ jobs: steps: - uses: actions/checkout@v4 - uses: ./.github/actions/plugins/test - + mariadb: runs-on: ubuntu-latest services: diff --git a/packages/datadog-instrumentations/src/helpers/hooks.js b/packages/datadog-instrumentations/src/helpers/hooks.js index dbcd55a0b86..21bdf21298e 100644 --- a/packages/datadog-instrumentations/src/helpers/hooks.js +++ b/packages/datadog-instrumentations/src/helpers/hooks.js @@ -91,6 +91,7 @@ module.exports = { 'node:http2': () => require('../http2'), 'node:https': () => require('../http'), 'node:net': () => require('../net'), + 'node:url': () => require('../url'), nyc: () => require('../nyc'), oracledb: () => require('../oracledb'), openai: () => require('../openai'), @@ -115,6 +116,7 @@ module.exports = { sharedb: () => require('../sharedb'), tedious: () => require('../tedious'), undici: () => require('../undici'), + url: () => require('../url'), vitest: { esmFirst: true, fn: () => require('../vitest') }, when: () => require('../when'), winston: () => require('../winston'), diff --git a/packages/datadog-instrumentations/src/url.js b/packages/datadog-instrumentations/src/url.js new file mode 100644 index 00000000000..8bbe3e3b02a --- /dev/null +++ b/packages/datadog-instrumentations/src/url.js @@ -0,0 +1,85 @@ +'use strict' + +const { addHook, channel } = require('./helpers/instrument') +const shimmer = require('../../datadog-shimmer') +const names = ['url', 'node:url'] + +const parseFinishedChannel = channel('datadog:url:parse:finish') +const urlGetterChannel = channel('datadog:url:getter:finish') +const instrumentedGetters = ['host', 'origin', 'hostname'] + +addHook({ name: names }, function (url) { + shimmer.wrap(url, 'parse', (parse) => { + return function wrappedParse () { + const parsedValue = parse.apply(this, arguments) + if (!parseFinishedChannel.hasSubscribers) return parsedValue + + parseFinishedChannel.publish({ + input: arguments[0], + parsed: parsedValue, + isURL: false + }) + + return parsedValue + } + }) + + const URLPrototype = url.URL.prototype.constructor.prototype + instrumentedGetters.forEach(property => { + const originalDescriptor = Object.getOwnPropertyDescriptor(URLPrototype, property) + + if (originalDescriptor?.get) { + const newDescriptor = { + ...originalDescriptor, + get: function () { + const result = originalDescriptor.get.apply(this, arguments) + if (!urlGetterChannel.hasSubscribers) return result + + const context = { urlObject: this, result, property } + urlGetterChannel.publish(context) + + return context.result + } + } + + Object.defineProperty(URLPrototype, property, newDescriptor) + } + }) + + shimmer.wrap(url, 'URL', (URL) => { + return class extends URL { + constructor () { + super(...arguments) + + if (!parseFinishedChannel.hasSubscribers) return + + parseFinishedChannel.publish({ + input: arguments[0], + base: arguments[1], + parsed: this, + isURL: true + }) + } + } + }) + + if (url.URL.parse) { + shimmer.wrap(url.URL, 'parse', (parse) => { + return function wrappedParse () { + const parsedValue = parse.apply(this, arguments) + if (!parseFinishedChannel.hasSubscribers) return parsedValue + + parseFinishedChannel.publish({ + input: arguments[0], + base: arguments[1], + parsed: parsedValue, + isURL: true + }) + + return parsedValue + } + }) + } + + return url +}) diff --git a/packages/datadog-instrumentations/test/url.spec.js b/packages/datadog-instrumentations/test/url.spec.js new file mode 100644 index 00000000000..defb8f08193 --- /dev/null +++ b/packages/datadog-instrumentations/test/url.spec.js @@ -0,0 +1,114 @@ +'use strict' + +const agent = require('../../dd-trace/test/plugins/agent') +const { channel } = require('../src/helpers/instrument') +const names = ['url', 'node:url'] + +names.forEach(name => { + describe(name, () => { + const url = require(name) + const parseFinishedChannel = channel('datadog:url:parse:finish') + const urlGetterChannel = channel('datadog:url:getter:finish') + let parseFinishedChannelCb, urlGetterChannelCb + + before(async () => { + await agent.load('url') + }) + + after(() => { + return agent.close() + }) + + beforeEach(() => { + parseFinishedChannelCb = sinon.stub() + urlGetterChannelCb = sinon.stub() + parseFinishedChannel.subscribe(parseFinishedChannelCb) + urlGetterChannel.subscribe(urlGetterChannelCb) + }) + + afterEach(() => { + parseFinishedChannel.unsubscribe(parseFinishedChannelCb) + urlGetterChannel.unsubscribe(urlGetterChannelCb) + }) + + describe('url.parse', () => { + it('should publish', () => { + // eslint-disable-next-line n/no-deprecated-api + const result = url.parse('https://www.datadoghq.com') + + sinon.assert.calledOnceWithExactly(parseFinishedChannelCb, { + input: 'https://www.datadoghq.com', + parsed: result, + isURL: false + }, sinon.match.any) + }) + }) + + describe('url.URL', () => { + describe('new URL', () => { + it('should publish with input', () => { + const result = new url.URL('https://www.datadoghq.com') + + sinon.assert.calledOnceWithExactly(parseFinishedChannelCb, { + input: 'https://www.datadoghq.com', + base: undefined, + parsed: result, + isURL: true + }, sinon.match.any) + }) + + it('should publish with base and input', () => { + const result = new url.URL('/path', 'https://www.datadoghq.com') + + sinon.assert.calledOnceWithExactly(parseFinishedChannelCb, { + base: 'https://www.datadoghq.com', + input: '/path', + parsed: result, + isURL: true + }, sinon.match.any) + }) + + ;['host', 'origin', 'hostname'].forEach(property => { + it(`should publish on get ${property}`, () => { + const urlObject = new url.URL('/path', 'https://www.datadoghq.com') + + const result = urlObject[property] + + sinon.assert.calledWithExactly(urlGetterChannelCb, { + urlObject, + result, + property + }, sinon.match.any) + }) + }) + }) + }) + + if (url.URL.parse) { // added in v22.1.0 + describe('url.URL.parse', () => { + it('should publish with input', () => { + const input = 'https://www.datadoghq.com' + const parsed = url.URL.parse(input) + + sinon.assert.calledOnceWithExactly(parseFinishedChannelCb, { + input, + parsed, + base: undefined, + isURL: true + }, sinon.match.any) + }) + + it('should publish with base and input', () => { + const result = new url.URL('/path', 'https://www.datadoghq.com') + + sinon.assert.calledOnceWithExactly(parseFinishedChannelCb, { + base: 'https://www.datadoghq.com', + input: '/path', + parsed: result, + isURL: true + }, sinon.match.any) + }) + }) + } + }) +}) 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 67e99ff7fb0..ed46cbe5f2e 100644 --- a/packages/dd-trace/src/appsec/iast/taint-tracking/plugin.js +++ b/packages/dd-trace/src/appsec/iast/taint-tracking/plugin.js @@ -23,6 +23,7 @@ class TaintTrackingPlugin extends SourceIastPlugin { constructor () { super() this._type = 'taint-tracking' + this._taintedURLs = new WeakMap() } onConfigure () { @@ -88,6 +89,46 @@ class TaintTrackingPlugin extends SourceIastPlugin { } ) + const urlResultTaintedProperties = ['host', 'origin', 'hostname'] + this.addSub( + { channelName: 'datadog:url:parse:finish' }, + ({ input, base, parsed, isURL }) => { + const iastContext = getIastContext(storage.getStore()) + let ranges + + if (base) { + ranges = getRanges(iastContext, base) + } else { + ranges = getRanges(iastContext, input) + } + + if (ranges?.length) { + if (isURL) { + this._taintedURLs.set(parsed, ranges[0]) + } else { + urlResultTaintedProperties.forEach(param => { + this._taintTrackingHandler(ranges[0].iinfo.type, parsed, param, iastContext) + }) + } + } + } + ) + + this.addSub( + { channelName: 'datadog:url:getter:finish' }, + (context) => { + if (!urlResultTaintedProperties.includes(context.property)) return + + const origRange = this._taintedURLs.get(context.urlObject) + if (!origRange) return + + const iastContext = getIastContext(storage.getStore()) + if (!iastContext) return + + context.result = + newTaintedString(iastContext, context.result, origRange.iinfo.parameterName, origRange.iinfo.type) + }) + // this is a special case to increment INSTRUMENTED_SOURCE metric for header this.addInstrumentedSource('http', [HTTP_REQUEST_HEADER_VALUE, HTTP_REQUEST_HEADER_NAME]) } diff --git a/packages/dd-trace/test/appsec/iast/taint-tracking/plugin.express.plugin.spec.js b/packages/dd-trace/test/appsec/iast/taint-tracking/plugin.express.plugin.spec.js index f2a8193d1be..a9a995783f1 100644 --- a/packages/dd-trace/test/appsec/iast/taint-tracking/plugin.express.plugin.spec.js +++ b/packages/dd-trace/test/appsec/iast/taint-tracking/plugin.express.plugin.spec.js @@ -2,6 +2,7 @@ const { prepareTestServerForIastInExpress } = require('../utils') const axios = require('axios') +const { URL } = require('url') function noop () {} @@ -47,6 +48,95 @@ describe('Taint tracking plugin sources express tests', () => { childProcess.exec(req.headers['x-iast-test-command'], noop) }, 'COMMAND_INJECTION', 1, noop, makeRequestWithHeader) }) + + describe('url parse taint tracking', () => { + function makePostRequest (done) { + axios.post(`http://localhost:${config.port}/`, { + url: 'http://www.datadoghq.com/' + }).catch(done) + } + + testThatRequestHasVulnerability( + { + fn: (req) => { + // eslint-disable-next-line n/no-deprecated-api + const { parse } = require('url') + const url = parse(req.body.url) + + const childProcess = require('child_process') + childProcess.exec(url.host, noop) + }, + vulnerability: 'COMMAND_INJECTION', + occurrences: 1, + cb: noop, + makeRequest: makePostRequest, + testDescription: 'should detect vulnerability when tainted is coming from url.parse' + }) + + testThatRequestHasVulnerability( + { + fn: (req) => { + const { URL } = require('url') + const url = new URL(req.body.url) + + const childProcess = require('child_process') + childProcess.exec(url.host, noop) + }, + vulnerability: 'COMMAND_INJECTION', + occurrences: 1, + cb: noop, + makeRequest: makePostRequest, + testDescription: 'should detect vulnerability when tainted is coming from new url.URL input' + }) + + testThatRequestHasVulnerability( + { + fn: (req) => { + const { URL } = require('url') + const url = new URL('/path', req.body.url) + + const childProcess = require('child_process') + childProcess.exec(url.host, noop) + }, + vulnerability: 'COMMAND_INJECTION', + occurrences: 1, + cb: noop, + makeRequest: makePostRequest, + testDescription: 'should detect vulnerability when tainted is coming from new url.URL base' + }) + + if (URL.parse) { + testThatRequestHasVulnerability( + { + fn: (req) => { + const { URL } = require('url') + const url = URL.parse(req.body.url) + const childProcess = require('child_process') + childProcess.exec(url.host, noop) + }, + vulnerability: 'COMMAND_INJECTION', + occurrences: 1, + cb: noop, + makeRequest: makePostRequest, + testDescription: 'should detect vulnerability when tainted is coming from url.URL.parse input' + }) + + testThatRequestHasVulnerability( + { + fn: (req) => { + const { URL } = require('url') + const url = URL.parse('/path', req.body.url) + const childProcess = require('child_process') + childProcess.exec(url.host, noop) + }, + vulnerability: 'COMMAND_INJECTION', + occurrences: 1, + cb: noop, + makeRequest: makePostRequest, + testDescription: 'should detect vulnerability when tainted is coming from url.URL.parse base' + }) + } + }) } ) }) 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 f4bab360663..1a21b0a5b08 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 @@ -42,7 +42,7 @@ describe('IAST Taint tracking plugin', () => { }) it('Should subscribe to body parser, qs, cookie and process_params channel', () => { - expect(taintTrackingPlugin._subscriptions).to.have.lengthOf(7) + expect(taintTrackingPlugin._subscriptions).to.have.lengthOf(9) 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') @@ -50,6 +50,8 @@ describe('IAST Taint tracking plugin', () => { 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') }) describe('taint sources', () => {