Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use static vulnerability hash source when the cookie name is too long #4764

Merged
merged 3 commits into from
Oct 9, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
12 changes: 11 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 @@ -11,7 +11,13 @@ class CookieAnalyzer extends Analyzer {
this.propertyToBeSafe = propertyToBeSafe.toLowerCase()
}

onConfigure () {
onConfigure (config) {
try {
this.cookieFilterRegExp = new RegExp(config.iast.cookieFilterPattern)
} catch {
this.cookieFilterRegExp = /.{32,}/
CarlesDD marked this conversation as resolved.
Show resolved Hide resolved
}

this.addSub(
{ channelName: 'datadog:iast:set-cookie', moduleName: 'http' },
(cookieInfo) => this.analyze(cookieInfo)
Expand All @@ -28,6 +34,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 @@ -581,6 +582,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 @@ -716,6 +718,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 @@ -884,6 +887,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}`))
})
})
})
})
CarlesDD marked this conversation as resolved.
Show resolved Hide resolved
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(50)
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 = (new Array(50)).join('0')
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 = (new Array(50)).join('0')
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
Loading