Skip to content

Commit

Permalink
Use static vulnerability hash source when the cookie name is too long (
Browse files Browse the repository at this point in the history
  • Loading branch information
uurien authored Oct 9, 2024
1 parent 5eea208 commit 6052944
Show file tree
Hide file tree
Showing 11 changed files with 193 additions and 8 deletions.
2 changes: 2 additions & 0 deletions docs/test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ tracer.init({
},
iast: {
enabled: true,
cookieFilterPattern: '.*',
requestSampling: 50,
maxConcurrentRequests: 4,
maxContextOperations: 30,
Expand All @@ -143,6 +144,7 @@ tracer.init({
experimental: {
iast: {
enabled: true,
cookieFilterPattern: '.*',
requestSampling: 50,
maxConcurrentRequests: 4,
maxContextOperations: 30,
Expand Down
7 changes: 6 additions & 1 deletion index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1739,7 +1739,6 @@ declare namespace tracer {
* on the tracer.
*/
interface pino extends Integration {}

/**
* This plugin automatically patches the [protobufjs](https://protobufjs.github.io/protobuf.js/)
* to collect protobuf message schemas when Datastreams Monitoring is enabled.
Expand Down Expand Up @@ -2160,6 +2159,12 @@ declare namespace tracer {
*/
maxContextOperations?: number,

/**
* Defines the pattern to ignore cookie names in the vulnerability hash calculation
* @default ".{32,}"
*/
cookieFilterPattern?: string,

/**
* Whether to enable vulnerability deduplication
*/
Expand Down
14 changes: 13 additions & 1 deletion packages/dd-trace/src/appsec/iast/analyzers/cookie-analyzer.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

const Analyzer = require('./vulnerability-analyzer')
const { getNodeModulesPaths } = require('../path-line')
const iastLog = require('../iast-log')

const EXCLUDED_PATHS = getNodeModulesPaths('express/lib/response.js')

Expand All @@ -11,7 +12,14 @@ class CookieAnalyzer extends Analyzer {
this.propertyToBeSafe = propertyToBeSafe.toLowerCase()
}

onConfigure () {
onConfigure (config) {
try {
this.cookieFilterRegExp = new RegExp(config.iast.cookieFilterPattern)
} catch {
iastLog.error('Invalid regex in cookieFilterPattern')
this.cookieFilterRegExp = /.{32,}/
}

this.addSub(
{ channelName: 'datadog:iast:set-cookie', moduleName: 'http' },
(cookieInfo) => this.analyze(cookieInfo)
Expand All @@ -28,6 +36,10 @@ class CookieAnalyzer extends Analyzer {
}

_createHashSource (type, evidence, location) {
if (typeof evidence.value === 'string' && evidence.value.match(this.cookieFilterRegExp)) {
return 'FILTERED_' + this._type
}

return `${type}:${evidence.value}`
}

Expand Down
2 changes: 1 addition & 1 deletion packages/dd-trace/src/appsec/iast/iast-plugin.js
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ class IastPlugin extends Plugin {
config = { enabled: config }
}
if (config.enabled && !this.configured) {
this.onConfigure()
this.onConfigure(config.tracerConfig)
this.configured = true
}

Expand Down
4 changes: 4 additions & 0 deletions packages/dd-trace/src/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -478,6 +478,7 @@ class Config {
this._setValue(defaults, 'gitMetadataEnabled', true)
this._setValue(defaults, 'headerTags', [])
this._setValue(defaults, 'hostname', '127.0.0.1')
this._setValue(defaults, 'iast.cookieFilterPattern', '.{32,}')
this._setValue(defaults, 'iast.deduplicationEnabled', true)
this._setValue(defaults, 'iast.enabled', false)
this._setValue(defaults, 'iast.maxConcurrentRequests', 2)
Expand Down Expand Up @@ -582,6 +583,7 @@ class Config {
DD_EXPERIMENTAL_APPSEC_STANDALONE_ENABLED,
DD_EXPERIMENTAL_PROFILING_ENABLED,
JEST_WORKER_ID,
DD_IAST_COOKIE_FILTER_PATTERN,
DD_IAST_DEDUPLICATION_ENABLED,
DD_IAST_ENABLED,
DD_IAST_MAX_CONCURRENT_REQUESTS,
Expand Down Expand Up @@ -717,6 +719,7 @@ class Config {
this._setBoolean(env, 'gitMetadataEnabled', DD_TRACE_GIT_METADATA_ENABLED)
this._setArray(env, 'headerTags', DD_TRACE_HEADER_TAGS)
this._setString(env, 'hostname', coalesce(DD_AGENT_HOST, DD_TRACE_AGENT_HOSTNAME))
this._setString(env, 'iast.cookieFilterPattern', DD_IAST_COOKIE_FILTER_PATTERN)
this._setBoolean(env, 'iast.deduplicationEnabled', DD_IAST_DEDUPLICATION_ENABLED)
this._setBoolean(env, 'iast.enabled', DD_IAST_ENABLED)
this._setValue(env, 'iast.maxConcurrentRequests', maybeInt(DD_IAST_MAX_CONCURRENT_REQUESTS))
Expand Down Expand Up @@ -885,6 +888,7 @@ class Config {
this._optsUnprocessed.flushMinSpans = options.flushMinSpans
this._setArray(opts, 'headerTags', options.headerTags)
this._setString(opts, 'hostname', options.hostname)
this._setString(opts, 'iast.cookieFilterPattern', options.iast?.cookieFilterPattern)
this._setBoolean(opts, 'iast.deduplicationEnabled', options.iast && options.iast.deduplicationEnabled)
this._setBoolean(opts, 'iast.enabled',
options.iast && (options.iast === true || options.iast.enabled === true))
Expand Down
110 changes: 110 additions & 0 deletions packages/dd-trace/test/appsec/iast/analyzers/cookie-analyzer.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
'use strict'

const { assert } = require('chai')
const CookieAnalyzer = require('../../../../src/appsec/iast/analyzers/cookie-analyzer')
const Analyzer = require('../../../../src/appsec/iast/analyzers/vulnerability-analyzer')
const Config = require('../../../../src/config')

describe('CookieAnalyzer', () => {
const VULNERABILITY_TYPE = 'VULN_TYPE'

it('should extends Analyzer', () => {
assert.isTrue(Analyzer.isPrototypeOf(CookieAnalyzer))
})

describe('_createHashSource', () => {
let cookieAnalyzer

beforeEach(() => {
cookieAnalyzer = new CookieAnalyzer(VULNERABILITY_TYPE, 'prop')
})

describe('default config', () => {
beforeEach(() => {
cookieAnalyzer.onConfigure(new Config({ iast: true }))
})

it('should create hash from vulnerability type and not long enough evidence value', () => {
const evidence = {
value: '0'.repeat(31)
}

const vulnerability = cookieAnalyzer._createVulnerability(VULNERABILITY_TYPE, evidence, null, {})

assert.equal(vulnerability.hash, cookieAnalyzer._createHash(`${VULNERABILITY_TYPE}:${evidence.value}`))
})

it('should create different hash from vulnerability type and long evidence value', () => {
const evidence = {
value: '0'.repeat(32)
}

const vulnerability = cookieAnalyzer._createVulnerability(VULNERABILITY_TYPE, evidence, null, {})

assert.equal(vulnerability.hash, cookieAnalyzer._createHash(`FILTERED_${VULNERABILITY_TYPE}`))
})
})

describe('custom cookieFilterPattern', () => {
beforeEach(() => {
cookieAnalyzer.onConfigure(new Config({
iast: {
enabled: true,
cookieFilterPattern: '^filtered$'
}
}))
})

it('should create hash from vulnerability with the default pattern', () => {
const evidence = {
value: 'notfiltered'
}

const vulnerability = cookieAnalyzer._createVulnerability(VULNERABILITY_TYPE, evidence, null, {})

assert.equal(vulnerability.hash, cookieAnalyzer._createHash(`${VULNERABILITY_TYPE}:${evidence.value}`))
})

it('should create different hash from vulnerability type and long evidence value', () => {
const evidence = {
value: 'filtered'
}

const vulnerability = cookieAnalyzer._createVulnerability(VULNERABILITY_TYPE, evidence, null, {})

assert.equal(vulnerability.hash, cookieAnalyzer._createHash(`FILTERED_${VULNERABILITY_TYPE}`))
})
})

describe('invalid cookieFilterPattern maintains default behaviour', () => {
beforeEach(() => {
cookieAnalyzer.onConfigure(new Config({
iast: {
enabled: true,
cookieFilterPattern: '('
}
}))
})

it('should create hash from vulnerability type and not long enough evidence value', () => {
const evidence = {
value: '0'.repeat(31)
}

const vulnerability = cookieAnalyzer._createVulnerability(VULNERABILITY_TYPE, evidence, null, {})

assert.equal(vulnerability.hash, cookieAnalyzer._createHash(`${VULNERABILITY_TYPE}:${evidence.value}`))
})

it('should create different hash from vulnerability type and long evidence value', () => {
const evidence = {
value: '0'.repeat(32)
}

const vulnerability = cookieAnalyzer._createVulnerability(VULNERABILITY_TYPE, evidence, null, {})

assert.equal(vulnerability.hash, cookieAnalyzer._createHash(`FILTERED_${VULNERABILITY_TYPE}`))
})
})
})
})
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,20 @@
const { prepareTestServerForIast } = require('../utils')
const Analyzer = require('../../../../src/appsec/iast/analyzers/vulnerability-analyzer')
const { INSECURE_COOKIE } = require('../../../../src/appsec/iast/vulnerabilities')
const insecureCookieAnalyzer = require('../../../../src/appsec/iast/analyzers/insecure-cookie-analyzer')
const CookieAnalyzer = require('../../../../src/appsec/iast/analyzers/cookie-analyzer')

const analyzer = new Analyzer()

describe('insecure cookie analyzer', () => {
it('Expected vulnerability identifier', () => {
expect(INSECURE_COOKIE).to.be.equals('INSECURE_COOKIE')
})

it('InsecureCookieAnalyzer extends CookieAnalyzer', () => {
expect(CookieAnalyzer.isPrototypeOf(insecureCookieAnalyzer.constructor)).to.be.true
})

// In these test, even when we are having multiple vulnerabilities, all the vulnerabilities
// are in the same cookies method, and it is expected to detect both even when the max operations is 1
const iastConfig = {
Expand Down Expand Up @@ -43,6 +51,12 @@ describe('insecure cookie analyzer', () => {
res.setHeader('set-cookie', ['key=value; HttpOnly', 'key2=value2; Secure'])
}, INSECURE_COOKIE, 1)

testThatRequestHasVulnerability((req, res) => {
const cookieNamePrefix = '0'.repeat(32)
res.setHeader('set-cookie', [cookieNamePrefix + 'key1=value', cookieNamePrefix + 'key2=value2'])
}, INSECURE_COOKIE, 1, undefined, undefined,
'Should be detected as the same INSECURE_COOKIE vulnerability when the cookie name is long')

testThatRequestHasNoVulnerability((req, res) => {
res.setHeader('set-cookie', 'key=value; Secure')
}, INSECURE_COOKIE)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,20 @@
const { prepareTestServerForIast } = require('../utils')
const Analyzer = require('../../../../src/appsec/iast/analyzers/vulnerability-analyzer')
const { NO_HTTPONLY_COOKIE } = require('../../../../src/appsec/iast/vulnerabilities')
const CookieAnalyzer = require('../../../../src/appsec/iast/analyzers/cookie-analyzer')
const noHttponlyCookieAnalyzer = require('../../../../src/appsec/iast/analyzers/no-httponly-cookie-analyzer')

const analyzer = new Analyzer()

describe('no HttpOnly cookie analyzer', () => {
it('Expected vulnerability identifier', () => {
expect(NO_HTTPONLY_COOKIE).to.be.equals('NO_HTTPONLY_COOKIE')
})

it('NoHttponlyCookieAnalyzer extends CookieAnalyzer', () => {
expect(CookieAnalyzer.isPrototypeOf(noHttponlyCookieAnalyzer.constructor)).to.be.true
})

// In these test, even when we are having multiple vulnerabilities, all the vulnerabilities
// are in the same cookies method, and it is expected to detect both even when the max operations is 1
const iastConfig = {
Expand All @@ -18,6 +25,7 @@ describe('no HttpOnly cookie analyzer', () => {
maxConcurrentRequests: 1,
maxContextOperations: 1
}

prepareTestServerForIast('no HttpOnly cookie analyzer',
(testThatRequestHasVulnerability, testThatRequestHasNoVulnerability) => {
testThatRequestHasVulnerability((req, res) => {
Expand Down Expand Up @@ -47,6 +55,12 @@ describe('no HttpOnly cookie analyzer', () => {
res.setHeader('set-cookie', ['key=value; HttpOnly', 'key2=value2; Secure'])
}, NO_HTTPONLY_COOKIE, 1)

testThatRequestHasVulnerability((req, res) => {
const cookieNamePrefix = '0'.repeat(32)
res.setHeader('set-cookie', [cookieNamePrefix + 'key1=value', cookieNamePrefix + 'key2=value2'])
}, NO_HTTPONLY_COOKIE, 1, undefined, undefined,
'Should be detected as the same NO_HTTPONLY_COOKIE vulnerability when the cookie name is long')

testThatRequestHasNoVulnerability((req, res) => {
res.setHeader('set-cookie', 'key=value; HttpOnly')
}, NO_HTTPONLY_COOKIE)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,20 @@
const { prepareTestServerForIast } = require('../utils')
const Analyzer = require('../../../../src/appsec/iast/analyzers/vulnerability-analyzer')
const { NO_SAMESITE_COOKIE } = require('../../../../src/appsec/iast/vulnerabilities')
const CookieAnalyzer = require('../../../../src/appsec/iast/analyzers/cookie-analyzer')
const noSamesiteCookieAnalyzer = require('../../../../src/appsec/iast/analyzers/no-samesite-cookie-analyzer')

const analyzer = new Analyzer()

describe('no SameSite cookie analyzer', () => {
it('Expected vulnerability identifier', () => {
expect(NO_SAMESITE_COOKIE).to.be.equals('NO_SAMESITE_COOKIE')
})

it('NoSamesiteCookieAnalyzer extends CookieAnalyzer', () => {
expect(CookieAnalyzer.isPrototypeOf(noSamesiteCookieAnalyzer.constructor)).to.be.true
})

// In these test, even when we are having multiple vulnerabilities, all the vulnerabilities
// are in the same cookies method, and it is expected to detect both even when the max operations is 1
const iastConfig = {
Expand Down Expand Up @@ -59,6 +66,12 @@ describe('no SameSite cookie analyzer', () => {
res.setHeader('set-cookie', 'key=value; SameSite=strict')
}, NO_SAMESITE_COOKIE)

testThatRequestHasVulnerability((req, res) => {
const cookieNamePrefix = '0'.repeat(32)
res.setHeader('set-cookie', [cookieNamePrefix + 'key1=value', cookieNamePrefix + 'key2=value2'])
}, NO_SAMESITE_COOKIE, 1, undefined, undefined,
'Should be detected as the same NO_SAMESITE_COOKIE vulnerability when the cookie name is long')

testThatRequestHasNoVulnerability((req, res) => {
res.setHeader('set-cookie', 'key=')
}, NO_SAMESITE_COOKIE)
Expand Down
8 changes: 3 additions & 5 deletions packages/dd-trace/test/appsec/iast/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -112,9 +112,7 @@ function beforeEachIastTest (iastConfig) {
beforeEach(() => {
vulnerabilityReporter.clearCache()
iast.enable(new Config({
experimental: {
iast: iastConfig
}
iast: iastConfig
}))
})
}
Expand Down Expand Up @@ -249,8 +247,8 @@ function prepareTestServerForIast (description, tests, iastConfig) {
return agent.close({ ritmReset: false })
})

function testThatRequestHasVulnerability (fn, vulnerability, occurrences, cb, makeRequest) {
it(`should have ${vulnerability} vulnerability`, function (done) {
function testThatRequestHasVulnerability (fn, vulnerability, occurrences, cb, makeRequest, description) {
it(description || `should have ${vulnerability} vulnerability`, function (done) {
this.timeout(5000)
app = fn
checkVulnerabilityInRequest(vulnerability, occurrences, cb, makeRequest, config, done)
Expand Down
Loading

0 comments on commit 6052944

Please sign in to comment.