Skip to content

Commit

Permalink
Support url.parse, url.URL.parse and new url.URL for IAST taint track…
Browse files Browse the repository at this point in the history
…ing (#4836)

* Support url.parse, url.URL.parse and new url.URL for taint tracking

* Address PR comments

* Use shimmer.wrap instead of doing it manually
  • Loading branch information
uurien authored Nov 5, 2024
1 parent 5028d30 commit 497ff72
Show file tree
Hide file tree
Showing 7 changed files with 343 additions and 2 deletions.
10 changes: 9 additions & 1 deletion .github/workflows/plugins.yml
Original file line number Diff line number Diff line change
Expand Up @@ -578,7 +578,7 @@ jobs:
steps:
- uses: actions/checkout@v4
- uses: ./.github/actions/plugins/test

mariadb:
runs-on: ubuntu-latest
services:
Expand Down Expand Up @@ -999,6 +999,14 @@ jobs:
- uses: actions/checkout@v4
- uses: ./.github/actions/plugins/test

url:
runs-on: ubuntu-latest
env:
PLUGINS: url
steps:
- uses: actions/checkout@v4
- uses: ./.github/actions/plugins/test

when:
runs-on: ubuntu-latest
env:
Expand Down
2 changes: 2 additions & 0 deletions packages/datadog-instrumentations/src/helpers/hooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -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'),
Expand All @@ -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'),
Expand Down
84 changes: 84 additions & 0 deletions packages/datadog-instrumentations/src/url.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
'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 (input) {
const parsedValue = parse.apply(this, arguments)
if (!parseFinishedChannel.hasSubscribers) return parsedValue

parseFinishedChannel.publish({
input,
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 = shimmer.wrap(originalDescriptor, 'get', function (originalGet) {
return function get () {
const result = originalGet.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 (input, base) {
super(...arguments)

if (!parseFinishedChannel.hasSubscribers) return

parseFinishedChannel.publish({
input,
base,
parsed: this,
isURL: true
})
}
}
})

if (url.URL.parse) {
shimmer.wrap(url.URL, 'parse', (parse) => {
return function wrappedParse (input, base) {
const parsedValue = parse.apply(this, arguments)
if (!parseFinishedChannel.hasSubscribers) return parsedValue

parseFinishedChannel.publish({
input,
base,
parsed: parsedValue,
isURL: true
})

return parsedValue
}
})
}

return url
})
114 changes: 114 additions & 0 deletions packages/datadog-instrumentations/test/url.spec.js
Original file line number Diff line number Diff line change
@@ -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)
})
})
}
})
})
41 changes: 41 additions & 0 deletions packages/dd-trace/src/appsec/iast/taint-tracking/plugin.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ class TaintTrackingPlugin extends SourceIastPlugin {
constructor () {
super()
this._type = 'taint-tracking'
this._taintedURLs = new WeakMap()
}

onConfigure () {
Expand Down Expand Up @@ -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])
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

const { prepareTestServerForIastInExpress } = require('../utils')
const axios = require('axios')
const { URL } = require('url')

function noop () {}

Expand Down Expand Up @@ -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'
})
}
})
}
)
})
Expand Down
Loading

0 comments on commit 497ff72

Please sign in to comment.