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

[asm] IAST security controls #5117

Draft
wants to merge 30 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
d5a52ea
Security controls parser and secure marks for vulnerabilities
iunanua Jan 10, 2025
c4c398c
Use new NOSQL_MONGODB_INJECTION_MARK in nosql-injection-mongodb-analyzer
iunanua Jan 10, 2025
9a1229c
Config
iunanua Jan 13, 2025
dce2f18
first hooks
iunanua Jan 13, 2025
ac1d502
wrap object properties and more tests
iunanua Jan 15, 2025
4cc7895
Use dd-trace:moduleLoad(Start|End) channels
iunanua Jan 15, 2025
6fe83f6
iterate object strings and more tests
iunanua Jan 15, 2025
4e4d69e
fix parameter index, include createNewTainted flag and do not use Plu…
iunanua Jan 16, 2025
90b9ff8
Fix parameter index and include a test with incorrect index
iunanua Jan 16, 2025
2d86aee
Avoid to hook multiple times the same module and config tests
iunanua Jan 16, 2025
5ed8aa2
sql_injection_mark test
iunanua Jan 16, 2025
57548b0
vulnerable ranges tests
iunanua Jan 16, 2025
610e216
fix windows paths
iunanua Jan 16, 2025
ca0bbe5
Merge branch 'master' into igor/iast-security-controls
iunanua Jan 17, 2025
b4a217e
Upgrade taint-tracking to 3.3.0
iunanua Jan 17, 2025
0b0c292
Fix * secure mark
iunanua Jan 20, 2025
af61bf9
add createNewTainted flag to addSecureMark
iunanua Jan 21, 2025
fb1de25
Use existing _isRangeSecure
iunanua Jan 22, 2025
384d526
supressed vulnerabilities metric
iunanua Jan 22, 2025
fce89df
increment supressed vulnerability metric
iunanua Jan 22, 2025
c324c58
typo
iunanua Jan 22, 2025
767d2db
handle esm default export and filenames starting with file://
iunanua Jan 22, 2025
082ac75
esm integration tests
iunanua Jan 22, 2025
4b948ad
clean up
iunanua Jan 23, 2025
d9c1393
secure-marks tests
iunanua Jan 23, 2025
1fdaf86
Merge branch 'master' into igor/iast-security-controls
iunanua Jan 23, 2025
3b697d1
fix secure-marks generator test
iunanua Jan 23, 2025
07dcc02
fix config test
iunanua Jan 23, 2025
5556875
empty
iunanua Jan 24, 2025
3f57dea
check for repeated marks
iunanua Jan 24, 2025
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
52 changes: 52 additions & 0 deletions integration-tests/appsec/esm-security-controls/index.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
'use strict'

import childProcess from 'node:child_process'
import express from 'express'
import { sanitize } from './sanitizer.mjs'
import sanitizeDefault from './sanitizer-default.mjs'
import { validate, validateNotConfigured } from './validator.mjs'

const app = express()
const port = process.env.APP_PORT || 3000

app.get('/cmdi-s-secure', (req, res) => {
const command = sanitize(req.query.command)
try {
childProcess.execSync(command)
} catch (e) {
// ignore
}

res.end()
})

app.get('/cmdi-s-secure-default', (req, res) => {
const command = sanitizeDefault(req.query.command)
try {
childProcess.execSync(command)
} catch (e) {
// ignore
}

res.end()
})

app.get('/cmdi-iv-insecure', (req, res) => {
if (validateNotConfigured(req.query.command)) {
childProcess.execSync(req.query.command)
}

res.end()
})

app.get('/cmdi-iv-secure', (req, res) => {
if (validate(req.query.command)) {
childProcess.execSync(req.query.command)
}

res.end()
})

app.listen(port, () => {
process.send({ port })
})
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
'use strict'

function sanitizeDefault (input) {
return input
}

export default sanitizeDefault
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
'use strict'

export function sanitize (input) {
return input
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
'use strict'

export function validate (input) {
return true
}

export function validateNotConfigured (input) {
return true
}
103 changes: 103 additions & 0 deletions integration-tests/appsec/iast.esm-security-controls.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
'use strict'

const { createSandbox, spawnProc, FakeAgent } = require('../helpers')
const path = require('path')
const getPort = require('get-port')
const Axios = require('axios')
const { assert } = require('chai')

describe('ESM Security controls', () => {
let axios, sandbox, cwd, appPort, appFile, agent, proc

before(async function () {
this.timeout(process.platform === 'win32' ? 90000 : 30000)
sandbox = await createSandbox(['express'])
appPort = await getPort()
cwd = sandbox.folder
appFile = path.join(cwd, 'appsec', 'esm-security-controls', 'index.mjs')

axios = Axios.create({
baseURL: `http://localhost:${appPort}`
})
})

after(async function () {
await sandbox.remove()
})
const nodeOptions = '--import dd-trace/initialize.mjs'

describe('with --import', () => {
beforeEach(async () => {
agent = await new FakeAgent().start()

proc = await spawnProc(appFile, {
cwd,
env: {
DD_TRACE_AGENT_PORT: agent.port,
APP_PORT: appPort,
DD_IAST_ENABLED: 'true',
DD_IAST_REQUEST_SAMPLING: '100',
// eslint-disable-next-line no-multi-str
DD_IAST_SECURITY_CONTROLS_CONFIGURATION: '\
SANITIZER:COMMAND_INJECTION:appsec/esm-security-controls/sanitizer.mjs:sanitize;\
SANITIZER:COMMAND_INJECTION:appsec/esm-security-controls/sanitizer-default.mjs;\
INPUT_VALIDATOR:*:appsec/esm-security-controls/validator.mjs:validate',
NODE_OPTIONS: nodeOptions
}
})
})

afterEach(async () => {
proc.kill()
await agent.stop()
})

it('test endpoint with iv not configured have COMMAND_INJECTION vulnerability', async function () {
await axios.get('/cmdi-iv-insecure?command=ls -la')

await agent.assertMessageReceived(({ payload }) => {
const spans = payload.flatMap(p => p.filter(span => span.name === 'express.request'))
spans.forEach(span => {
assert.property(span.meta, '_dd.iast.json')
assert.include(span.meta['_dd.iast.json'], '"COMMAND_INJECTION"')
})
}, null, 1, true)
})

it('test endpoint sanitizer do not have COMMAND_INJECTION vulnerability', async () => {
await axios.get('/cmdi-s-secure?command=ls -la')

await agent.assertMessageReceived(({ payload }) => {
const spans = payload.flatMap(p => p.filter(span => span.name === 'express.request'))
spans.forEach(span => {
assert.notProperty(span.meta, '_dd.iast.json')
assert.property(span.metrics, '_dd.iast.telemetry.suppressed.vulnerabilities.command_injection')
})
}, null, 1, true)
})

it('test endpoint with default sanitizer do not have COMMAND_INJECTION vulnerability', async () => {
await axios.get('/cmdi-s-secure-default?command=ls -la')

await agent.assertMessageReceived(({ payload }) => {
const spans = payload.flatMap(p => p.filter(span => span.name === 'express.request'))
spans.forEach(span => {
assert.notProperty(span.meta, '_dd.iast.json')
assert.property(span.metrics, '_dd.iast.telemetry.suppressed.vulnerabilities.command_injection')
})
}, null, 1, true)
})

it('test endpoint with iv do not have COMMAND_INJECTION vulnerability', async () => {
await axios.get('/cmdi-iv-secure?command=ls -la')

await agent.assertMessageReceived(({ payload }) => {
const spans = payload.flatMap(p => p.filter(span => span.name === 'express.request'))
spans.forEach(span => {
assert.notProperty(span.meta, '_dd.iast.json')
assert.property(span.metrics, '_dd.iast.telemetry.suppressed.vulnerabilities.command_injection')
})
}, null, 1, true)
})
})
})
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@
"@datadog/libdatadog": "^0.4.0",
"@datadog/native-appsec": "8.4.0",
"@datadog/native-iast-rewriter": "2.6.1",
"@datadog/native-iast-taint-tracking": "3.2.0",
"@datadog/native-iast-taint-tracking": "3.3.0",
"@datadog/native-metrics": "^3.1.0",
"@datadog/pprof": "5.4.1",
"@datadog/sketches-js": "^2.1.0",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,12 @@ const { SQL_ROW_VALUE } = require('../taint-tracking/source-types')

class InjectionAnalyzer extends Analyzer {
_isVulnerable (value, iastContext) {
const ranges = value && getRanges(iastContext, value)
let ranges = value && getRanges(iastContext, value)
if (ranges?.length > 0) {
ranges = this._filterSecureRanges(ranges)
if (!ranges?.length) {
this._incrementSuppressedMetric(iastContext)
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}
}

return this._areRangesVulnerable(ranges)
}

Expand All @@ -21,6 +25,10 @@ class InjectionAnalyzer extends Analyzer {
_areRangesVulnerable (ranges) {
return ranges?.some(range => range.iinfo.type !== SQL_ROW_VALUE)
}

_filterSecureRanges (ranges) {
return ranges?.filter(range => !this._isRangeSecure(range))
}
}

module.exports = InjectionAnalyzer
Original file line number Diff line number Diff line change
Expand Up @@ -4,29 +4,13 @@ const InjectionAnalyzer = require('./injection-analyzer')
const { NOSQL_MONGODB_INJECTION } = require('../vulnerabilities')
const { getRanges, addSecureMark } = require('../taint-tracking/operations')
const { getNodeModulesPaths } = require('../path-line')
const { getNextSecureMark } = require('../taint-tracking/secure-marks-generator')
const { storage } = require('../../../../../datadog-core')
const { getIastContext } = require('../iast-context')
const { HTTP_REQUEST_PARAMETER, HTTP_REQUEST_BODY } = require('../taint-tracking/source-types')

const EXCLUDED_PATHS_FROM_STACK = getNodeModulesPaths('mongodb', 'mongoose', 'mquery')
const MONGODB_NOSQL_SECURE_MARK = getNextSecureMark()

function iterateObjectStrings (target, fn, levelKeys = [], depth = 20, visited = new Set()) {
if (target !== null && typeof target === 'object') {
Object.keys(target).forEach((key) => {
const nextLevelKeys = [...levelKeys, key]
const val = target[key]

if (typeof val === 'string') {
fn(val, nextLevelKeys, target, key)
} else if (depth > 0 && !visited.has(val)) {
iterateObjectStrings(val, fn, nextLevelKeys, depth - 1, visited)
visited.add(val)
}
})
}
}
const { NOSQL_MONGODB_INJECTION_MARK } = require('../taint-tracking/secure-marks')
const { iterateObjectStrings } = require('../utils')

class NosqlInjectionMongodbAnalyzer extends InjectionAnalyzer {
constructor () {
Expand Down Expand Up @@ -88,7 +72,7 @@ class NosqlInjectionMongodbAnalyzer extends InjectionAnalyzer {
const currentLevelKey = levelKeys[i]

if (i === levelsLength - 1) {
parentObj[currentLevelKey] = addSecureMark(iastContext, value, MONGODB_NOSQL_SECURE_MARK)
parentObj[currentLevelKey] = addSecureMark(iastContext, value, NOSQL_MONGODB_INJECTION_MARK)
} else {
parentObj = parentObj[currentLevelKey]
}
Expand All @@ -106,7 +90,7 @@ class NosqlInjectionMongodbAnalyzer extends InjectionAnalyzer {
if (iastContext) { // do nothing if we are not in an iast request
iterateObjectStrings(sanitizedObject, function (value, levelKeys, parent, lastKey) {
try {
parent[lastKey] = addSecureMark(iastContext, value, MONGODB_NOSQL_SECURE_MARK)
parent[lastKey] = addSecureMark(iastContext, value, NOSQL_MONGODB_INJECTION_MARK)
} catch {
// if it is a readonly property, do nothing
}
Expand All @@ -121,8 +105,7 @@ class NosqlInjectionMongodbAnalyzer extends InjectionAnalyzer {

_isVulnerableRange (range) {
const rangeType = range?.iinfo?.type
const isVulnerableType = rangeType === HTTP_REQUEST_PARAMETER || rangeType === HTTP_REQUEST_BODY
return isVulnerableType && (range.secureMarks & MONGODB_NOSQL_SECURE_MARK) !== MONGODB_NOSQL_SECURE_MARK
return rangeType === HTTP_REQUEST_PARAMETER || rangeType === HTTP_REQUEST_BODY
}

_isVulnerable (value, iastContext) {
Expand All @@ -137,10 +120,15 @@ class NosqlInjectionMongodbAnalyzer extends InjectionAnalyzer {
const allRanges = []

iterateObjectStrings(value.filter, (val, nextLevelKeys) => {
const ranges = getRanges(iastContext, val)
let ranges = getRanges(iastContext, val)
if (ranges?.length) {
const filteredRanges = []

ranges = this._filterSecureRanges(ranges)
if (!ranges.length) {
this._incrementSuppressedMetric(iastContext)
}

for (const range of ranges) {
if (this._isVulnerableRange(range)) {
isVulnerable = true
Expand Down Expand Up @@ -175,4 +163,3 @@ class NosqlInjectionMongodbAnalyzer extends InjectionAnalyzer {
}

module.exports = new NosqlInjectionMongodbAnalyzer()
module.exports.MONGODB_NOSQL_SECURE_MARK = MONGODB_NOSQL_SECURE_MARK
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,8 @@ class SqlInjectionAnalyzer extends InjectionAnalyzer {
}
}

_areRangesVulnerable () {
return true
_areRangesVulnerable (ranges) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have this method implemented in more analyzers, like code injection analyzer. I think that this change is necessary for all of them.

return ranges?.length > 0
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,14 @@ const {
getVulnerabilityCallSiteFrames,
replaceCallSiteFromSourceMap
} = require('../vulnerability-reporter')
const { getMarkFromVulnerabilityType } = require('../taint-tracking/secure-marks')
const { SUPPRESSED_VULNERABILITIES } = require('../telemetry/iast-metric')

class Analyzer extends SinkIastPlugin {
constructor (type) {
super()
this._type = type
this._secureMark = getMarkFromVulnerabilityType(type)
}

_isVulnerable (value, context) {
Expand All @@ -25,6 +28,11 @@ class Analyzer extends SinkIastPlugin {
return false
}

_isRangeSecure (range) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does checking the secure marks out of injection-analizer make sense?

const { secureMarks } = range
return (secureMarks & this._secureMark) === this._secureMark
}

_report (value, context, meta) {
const evidence = this._getEvidence(value, context, meta)
this._reportEvidence(value, context, evidence)
Expand Down Expand Up @@ -149,6 +157,17 @@ class Analyzer extends SinkIastPlugin {
return hash
}

_getSuppressedMetricTag () {
if (!this._suppressedMetricTag) {
this._suppressedMetricTag = SUPPRESSED_VULNERABILITIES.formatTags(this._type)[0]
}
return this._suppressedMetricTag
}

_incrementSuppressedMetric (iastContext) {
SUPPRESSED_VULNERABILITIES.inc(iastContext, this._getSuppressedMetricTag())
}

addSub (iastSubOrChannelName, handler) {
const iastSub = typeof iastSubOrChannelName === 'string'
? { channelName: iastSubOrChannelName }
Expand Down
2 changes: 2 additions & 0 deletions packages/dd-trace/src/appsec/iast/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ const {
const { IAST_ENABLED_TAG_KEY } = require('./tags')
const iastTelemetry = require('./telemetry')
const { enable: enableFsPlugin, disable: disableFsPlugin, IAST_MODULE } = require('../rasp/fs-plugin')
const securityControls = require('./security-controls')

// TODO Change to `apm:http:server:request:[start|close]` when the subscription
// order of the callbacks can be enforce
Expand All @@ -35,6 +36,7 @@ function enable (config, _tracer) {
requestClose.subscribe(onIncomingHttpRequestEnd)
overheadController.configure(config.iast)
overheadController.startGlobalContext()
securityControls.configure(config.iast)
vulnerabilityReporter.start(config, _tracer)

isEnabled = true
Expand Down
Loading
Loading