From 24394647dfa351c98c34402e3348e6724540d727 Mon Sep 17 00:00:00 2001 From: ishabi Date: Tue, 3 Dec 2024 16:07:48 +0100 Subject: [PATCH 01/11] Exploit prevention command injection --- .../src/child_process.js | 2 +- packages/dd-trace/src/appsec/addresses.js | 1 + .../src/appsec/rasp/command_injection.js | 19 ++- packages/dd-trace/src/appsec/rasp/lfi.js | 4 +- .../dd-trace/src/appsec/rasp/sql_injection.js | 4 +- packages/dd-trace/src/appsec/rasp/ssrf.js | 4 +- .../src/appsec/remote_config/capabilities.js | 3 +- .../src/appsec/remote_config/index.js | 2 + packages/dd-trace/src/appsec/reporter.js | 6 +- packages/dd-trace/src/appsec/telemetry.js | 9 +- packages/dd-trace/src/appsec/waf/index.js | 4 +- .../src/appsec/waf/waf_context_wrapper.js | 4 +- .../command_injection.express.plugin.spec.js | 103 ++++++------ .../command_injection.integration.spec.js | 44 ++++-- .../appsec/rasp/command_injection.spec.js | 149 +++++++++++------- .../dd-trace/test/appsec/rasp/lfi.spec.js | 2 +- .../appsec/rasp/resources/rasp_rules.json | 51 +++++- .../appsec/rasp/resources/shi-app/index.js | 28 ++++ .../test/appsec/rasp/sql_injection.spec.js | 4 +- .../dd-trace/test/appsec/rasp/ssrf.spec.js | 2 +- .../test/appsec/remote_config/index.spec.js | 10 ++ 21 files changed, 308 insertions(+), 147 deletions(-) diff --git a/packages/datadog-instrumentations/src/child_process.js b/packages/datadog-instrumentations/src/child_process.js index f7224953367..6b092aa9947 100644 --- a/packages/datadog-instrumentations/src/child_process.js +++ b/packages/datadog-instrumentations/src/child_process.js @@ -34,7 +34,7 @@ function returnSpawnSyncError (error, context) { pid: 0 } - return context.result + throw error } names.forEach(name => { diff --git a/packages/dd-trace/src/appsec/addresses.js b/packages/dd-trace/src/appsec/addresses.js index cb540bc4e6f..f594c7efd92 100644 --- a/packages/dd-trace/src/appsec/addresses.js +++ b/packages/dd-trace/src/appsec/addresses.js @@ -28,6 +28,7 @@ module.exports = { DB_STATEMENT: 'server.db.statement', DB_SYSTEM: 'server.db.system', + EXEC_COMMAND: 'server.sys.exec.cmd', SHELL_COMMAND: 'server.sys.shell.cmd', LOGIN_SUCCESS: 'server.business_logic.users.login.success', diff --git a/packages/dd-trace/src/appsec/rasp/command_injection.js b/packages/dd-trace/src/appsec/rasp/command_injection.js index 8d6d977aace..8e087086da4 100644 --- a/packages/dd-trace/src/appsec/rasp/command_injection.js +++ b/packages/dd-trace/src/appsec/rasp/command_injection.js @@ -25,19 +25,28 @@ function disable () { } function analyzeCommandInjection ({ file, fileArgs, shell, abortController }) { - if (!file || !shell) return + if (!file) return const store = storage.getStore() const req = store?.req if (!req) return - const commandParams = fileArgs ? [file, ...fileArgs] : file + const persistent = {} + const raspRule = { type: RULE_TYPES.COMMAND_INJECTION } + const params = fileArgs ? [file, ...fileArgs] : file - const persistent = { - [addresses.SHELL_COMMAND]: commandParams + if (shell) { + persistent[addresses.SHELL_COMMAND] = params + raspRule.variant = 'shell' } - const result = waf.run({ persistent }, req, RULE_TYPES.COMMAND_INJECTION) + if (!shell) { + const commandParams = Array.isArray(params) ? params : [params] + persistent[addresses.EXEC_COMMAND] = commandParams + raspRule.variant = 'exec' + } + + const result = waf.run({ persistent }, req, raspRule) const res = store?.res handleResult(result, req, res, abortController, config) diff --git a/packages/dd-trace/src/appsec/rasp/lfi.js b/packages/dd-trace/src/appsec/rasp/lfi.js index 1190734064d..657369ad0fd 100644 --- a/packages/dd-trace/src/appsec/rasp/lfi.js +++ b/packages/dd-trace/src/appsec/rasp/lfi.js @@ -58,7 +58,9 @@ function analyzeLfi (ctx) { [FS_OPERATION_PATH]: path } - const result = waf.run({ persistent }, req, RULE_TYPES.LFI) + const raspRule = { type: RULE_TYPES.LFI } + + const result = waf.run({ persistent }, req, raspRule) handleResult(result, req, res, ctx.abortController, config) }) } diff --git a/packages/dd-trace/src/appsec/rasp/sql_injection.js b/packages/dd-trace/src/appsec/rasp/sql_injection.js index d4a165d8615..157723258f7 100644 --- a/packages/dd-trace/src/appsec/rasp/sql_injection.js +++ b/packages/dd-trace/src/appsec/rasp/sql_injection.js @@ -72,7 +72,9 @@ function analyzeSqlInjection (query, dbSystem, abortController) { [addresses.DB_SYSTEM]: dbSystem } - const result = waf.run({ persistent }, req, RULE_TYPES.SQL_INJECTION) + const raspRule = { type: RULE_TYPES.SQL_INJECTION } + + const result = waf.run({ persistent }, req, raspRule) handleResult(result, req, res, abortController, config) } diff --git a/packages/dd-trace/src/appsec/rasp/ssrf.js b/packages/dd-trace/src/appsec/rasp/ssrf.js index 38a3c150d74..7d429d74549 100644 --- a/packages/dd-trace/src/appsec/rasp/ssrf.js +++ b/packages/dd-trace/src/appsec/rasp/ssrf.js @@ -29,7 +29,9 @@ function analyzeSsrf (ctx) { [addresses.HTTP_OUTGOING_URL]: outgoingUrl } - const result = waf.run({ persistent }, req, RULE_TYPES.SSRF) + const raspRule = { type: RULE_TYPES.SSRF } + + const result = waf.run({ persistent }, req, raspRule) const res = store?.res handleResult(result, req, res, ctx.abortController, config) diff --git a/packages/dd-trace/src/appsec/remote_config/capabilities.js b/packages/dd-trace/src/appsec/remote_config/capabilities.js index bd729cc39cc..f08e71ccfad 100644 --- a/packages/dd-trace/src/appsec/remote_config/capabilities.js +++ b/packages/dd-trace/src/appsec/remote_config/capabilities.js @@ -24,5 +24,6 @@ module.exports = { APM_TRACING_SAMPLE_RULES: 1n << 29n, ASM_ENDPOINT_FINGERPRINT: 1n << 32n, ASM_NETWORK_FINGERPRINT: 1n << 34n, - ASM_HEADER_FINGERPRINT: 1n << 35n + ASM_HEADER_FINGERPRINT: 1n << 35n, + ASM_RASP_CMDI: 1n << 37n } diff --git a/packages/dd-trace/src/appsec/remote_config/index.js b/packages/dd-trace/src/appsec/remote_config/index.js index 90cda5c6f61..ceba8ec498a 100644 --- a/packages/dd-trace/src/appsec/remote_config/index.js +++ b/packages/dd-trace/src/appsec/remote_config/index.js @@ -77,6 +77,7 @@ function enableWafUpdate (appsecConfig) { rc.updateCapabilities(RemoteConfigCapabilities.ASM_RASP_SSRF, true) rc.updateCapabilities(RemoteConfigCapabilities.ASM_RASP_LFI, true) rc.updateCapabilities(RemoteConfigCapabilities.ASM_RASP_SHI, true) + rc.updateCapabilities(RemoteConfigCapabilities.ASM_RASP_CMDI, true) } // TODO: delete noop handlers and kPreUpdate and replace with batched handlers @@ -109,6 +110,7 @@ function disableWafUpdate () { rc.updateCapabilities(RemoteConfigCapabilities.ASM_RASP_SSRF, false) rc.updateCapabilities(RemoteConfigCapabilities.ASM_RASP_LFI, false) rc.updateCapabilities(RemoteConfigCapabilities.ASM_RASP_SHI, false) + rc.updateCapabilities(RemoteConfigCapabilities.ASM_RASP_CMDI, false) rc.removeProductHandler('ASM_DATA') rc.removeProductHandler('ASM_DD') diff --git a/packages/dd-trace/src/appsec/reporter.js b/packages/dd-trace/src/appsec/reporter.js index 57519e5bc79..c2f9bac6cbc 100644 --- a/packages/dd-trace/src/appsec/reporter.js +++ b/packages/dd-trace/src/appsec/reporter.js @@ -101,7 +101,7 @@ function reportWafInit (wafVersion, rulesVersion, diagnosticsRules = {}) { incrementWafInitMetric(wafVersion, rulesVersion) } -function reportMetrics (metrics, raspRuleType) { +function reportMetrics (metrics, raspRule) { const store = storage.getStore() const rootSpan = store?.req && web.root(store.req) if (!rootSpan) return @@ -109,8 +109,8 @@ function reportMetrics (metrics, raspRuleType) { if (metrics.rulesVersion) { rootSpan.setTag('_dd.appsec.event_rules.version', metrics.rulesVersion) } - if (raspRuleType) { - updateRaspRequestsMetricTags(metrics, store.req, raspRuleType) + if (raspRule) { + updateRaspRequestsMetricTags(metrics, store.req, raspRule) } else { updateWafRequestsMetricTags(metrics, store.req) } diff --git a/packages/dd-trace/src/appsec/telemetry.js b/packages/dd-trace/src/appsec/telemetry.js index d96ca77601f..07036f578f4 100644 --- a/packages/dd-trace/src/appsec/telemetry.js +++ b/packages/dd-trace/src/appsec/telemetry.js @@ -79,7 +79,7 @@ function getOrCreateMetricTags (store, versionsTags) { return metricTags } -function updateRaspRequestsMetricTags (metrics, req, raspRuleType) { +function updateRaspRequestsMetricTags (metrics, req, raspRule) { if (!req) return const store = getStore(req) @@ -89,7 +89,12 @@ function updateRaspRequestsMetricTags (metrics, req, raspRuleType) { if (!enabled) return - const tags = { rule_type: raspRuleType, waf_version: metrics.wafVersion } + const tags = { rule_type: raspRule.type, waf_version: metrics.wafVersion } + + if (raspRule.variant) { + tags.rule_variant = raspRule.variant + } + appsecMetrics.count('rasp.rule.eval', tags).inc(1) if (metrics.wafTimeout) { diff --git a/packages/dd-trace/src/appsec/waf/index.js b/packages/dd-trace/src/appsec/waf/index.js index 3b2bc9e2a13..a14a5313a92 100644 --- a/packages/dd-trace/src/appsec/waf/index.js +++ b/packages/dd-trace/src/appsec/waf/index.js @@ -46,7 +46,7 @@ function update (newRules) { } } -function run (data, req, raspRuleType) { +function run (data, req, raspRule) { if (!req) { const store = storage.getStore() if (!store || !store.req) { @@ -59,7 +59,7 @@ function run (data, req, raspRuleType) { const wafContext = waf.wafManager.getWAFContext(req) - return wafContext.run(data, raspRuleType) + return wafContext.run(data, raspRule) } function disposeContext (req) { diff --git a/packages/dd-trace/src/appsec/waf/waf_context_wrapper.js b/packages/dd-trace/src/appsec/waf/waf_context_wrapper.js index 6a90b8f89bb..54dbd16e1be 100644 --- a/packages/dd-trace/src/appsec/waf/waf_context_wrapper.js +++ b/packages/dd-trace/src/appsec/waf/waf_context_wrapper.js @@ -21,7 +21,7 @@ class WAFContextWrapper { this.knownAddresses = knownAddresses } - run ({ persistent, ephemeral }, raspRuleType) { + run ({ persistent, ephemeral }, raspRule) { if (this.ddwafContext.disposed) { log.warn('[ASM] Calling run on a disposed context') return @@ -87,7 +87,7 @@ class WAFContextWrapper { blockTriggered, wafVersion: this.wafVersion, wafTimeout: result.timeout - }, raspRuleType) + }, raspRule) if (ruleTriggered) { Reporter.reportAttack(JSON.stringify(result.events)) diff --git a/packages/dd-trace/test/appsec/rasp/command_injection.express.plugin.spec.js b/packages/dd-trace/test/appsec/rasp/command_injection.express.plugin.spec.js index 3943bd0c3c3..4d34f65bbf7 100644 --- a/packages/dd-trace/test/appsec/rasp/command_injection.express.plugin.spec.js +++ b/packages/dd-trace/test/appsec/rasp/command_injection.express.plugin.spec.js @@ -5,42 +5,25 @@ const appsec = require('../../../src/appsec') const Config = require('../../../src/config') const path = require('path') const Axios = require('axios') -const { getWebSpan, checkRaspExecutedAndHasThreat, checkRaspExecutedAndNotThreat } = require('./utils') +const { checkRaspExecutedAndHasThreat, checkRaspExecutedAndNotThreat } = require('./utils') const { assert } = require('chai') describe('RASP - command_injection', () => { withVersions('express', 'express', expressVersion => { let app, server, axios + function testShellBlockingAndSafeRequests () { + it('should block the threat', async () => { + try { + await axios.get('/?dir=$(cat /etc/passwd 1>%262 ; echo .)') + } catch (e) { + if (!e.response) { + throw e + } - async function testBlockingRequest () { - try { - await axios.get('/?dir=$(cat /etc/passwd 1>%262 ; echo .)') - } catch (e) { - if (!e.response) { - throw e - } - - return checkRaspExecutedAndHasThreat(agent, 'rasp-command_injection-rule-id-3') - } - - assert.fail('Request should be blocked') - } - - function checkRaspNotExecutedAndNotThreat (agent, checkRuleEval = true) { - return agent.use((traces) => { - const span = getWebSpan(traces) - - assert.notProperty(span.meta, '_dd.appsec.json') - assert.notProperty(span.meta_struct || {}, '_dd.stack') - if (checkRuleEval) { - assert.notProperty(span.metrics, '_dd.appsec.rasp.rule.eval') + return checkRaspExecutedAndHasThreat(agent, 'rasp-command_injection-rule-id-3') } - }) - } - function testBlockingAndSafeRequests () { - it('should block the threat', async () => { - await testBlockingRequest() + assert.fail('Request should be blocked') }) it('should not block safe request', async () => { @@ -50,17 +33,25 @@ describe('RASP - command_injection', () => { }) } - function testSafeInNonShell () { - it('should not block the threat', async () => { - await axios.get('/?dir=$(cat /etc/passwd 1>%262 ; echo .)') + function testNonShellBlockingAndSafeRequests () { + it('should block the threat', async () => { + try { + await axios.get('/?dir=cat /etc/passwd 1>&2 ; echo .') + } catch (e) { + if (!e.response) { + throw e + } - return checkRaspNotExecutedAndNotThreat(agent) + return checkRaspExecutedAndHasThreat(agent, 'rasp-command_injection-rule-id-4') + } + + assert.fail('Request should be blocked') }) it('should not block safe request', async () => { await axios.get('/?dir=.') - return checkRaspNotExecutedAndNotThreat(agent) + return checkRaspExecutedAndNotThreat(agent) }) } @@ -116,7 +107,7 @@ describe('RASP - command_injection', () => { } }) - testBlockingAndSafeRequests() + testShellBlockingAndSafeRequests() }) describe('with promise', () => { @@ -137,7 +128,7 @@ describe('RASP - command_injection', () => { } }) - testBlockingAndSafeRequests() + testShellBlockingAndSafeRequests() }) describe('with event emitter', () => { @@ -158,7 +149,7 @@ describe('RASP - command_injection', () => { } }) - testBlockingAndSafeRequests() + testShellBlockingAndSafeRequests() }) describe('execSync', () => { @@ -178,7 +169,7 @@ describe('RASP - command_injection', () => { } }) - testBlockingAndSafeRequests() + testShellBlockingAndSafeRequests() }) }) @@ -199,7 +190,7 @@ describe('RASP - command_injection', () => { } }) - testBlockingAndSafeRequests() + testShellBlockingAndSafeRequests() }) describe('with promise', () => { @@ -220,7 +211,7 @@ describe('RASP - command_injection', () => { } }) - testBlockingAndSafeRequests() + testShellBlockingAndSafeRequests() }) describe('with event emitter', () => { @@ -241,7 +232,7 @@ describe('RASP - command_injection', () => { } }) - testBlockingAndSafeRequests() + testShellBlockingAndSafeRequests() }) describe('execFileSync', () => { @@ -261,7 +252,7 @@ describe('RASP - command_injection', () => { } }) - testBlockingAndSafeRequests() + testShellBlockingAndSafeRequests() }) }) @@ -271,7 +262,7 @@ describe('RASP - command_injection', () => { app = (req, res) => { const childProcess = require('child_process') - childProcess.execFile('ls', [req.query.dir], function (e) { + childProcess.execFile('sh', ['-c', req.query.dir], function (e) { if (e?.name === 'DatadogRaspAbortError') { res.writeHead(500) } @@ -281,7 +272,7 @@ describe('RASP - command_injection', () => { } }) - testSafeInNonShell() + testNonShellBlockingAndSafeRequests() }) describe('with promise', () => { @@ -291,7 +282,7 @@ describe('RASP - command_injection', () => { const execFile = util.promisify(require('child_process').execFile) try { - await execFile('ls', [req.query.dir]) + await execFile('sh', ['-c', req.query.dir]) } catch (e) { if (e.name === 'DatadogRaspAbortError') { res.writeHead(500) @@ -302,15 +293,14 @@ describe('RASP - command_injection', () => { } }) - testSafeInNonShell() + testNonShellBlockingAndSafeRequests() }) describe('with event emitter', () => { beforeEach(() => { app = (req, res) => { const childProcess = require('child_process') - - const child = childProcess.execFile('ls', [req.query.dir]) + const child = childProcess.execFile('sh', ['-c', req.query.dir]) child.on('error', (e) => { if (e.name === 'DatadogRaspAbortError') { res.writeHead(500) @@ -323,7 +313,7 @@ describe('RASP - command_injection', () => { } }) - testSafeInNonShell() + testNonShellBlockingAndSafeRequests() }) describe('execFileSync', () => { @@ -332,7 +322,7 @@ describe('RASP - command_injection', () => { const childProcess = require('child_process') try { - childProcess.execFileSync('ls', [req.query.dir]) + childProcess.execFileSync('sh', ['-c', req.query.dir]) } catch (e) { if (e.name === 'DatadogRaspAbortError') { res.writeHead(500) @@ -343,7 +333,7 @@ describe('RASP - command_injection', () => { } }) - testSafeInNonShell() + testNonShellBlockingAndSafeRequests() }) }) }) @@ -368,7 +358,7 @@ describe('RASP - command_injection', () => { } }) - testBlockingAndSafeRequests() + testShellBlockingAndSafeRequests() }) describe('spawnSync', () => { @@ -385,7 +375,7 @@ describe('RASP - command_injection', () => { } }) - testBlockingAndSafeRequests() + testShellBlockingAndSafeRequests() }) }) @@ -394,8 +384,7 @@ describe('RASP - command_injection', () => { beforeEach(() => { app = (req, res) => { const childProcess = require('child_process') - - const child = childProcess.spawn('ls', [req.query.dir]) + const child = childProcess.spawn('sh', ['-c', req.query.dir]) child.on('error', (e) => { if (e.name === 'DatadogRaspAbortError') { res.writeHead(500) @@ -408,7 +397,7 @@ describe('RASP - command_injection', () => { } }) - testSafeInNonShell() + testNonShellBlockingAndSafeRequests() }) describe('spawnSync', () => { @@ -416,7 +405,7 @@ describe('RASP - command_injection', () => { app = (req, res) => { const childProcess = require('child_process') - const child = childProcess.spawnSync('ls', [req.query.dir]) + const child = childProcess.spawnSync('sh', ['-c', req.query.dir]) if (child.error?.name === 'DatadogRaspAbortError') { res.writeHead(500) } @@ -425,7 +414,7 @@ describe('RASP - command_injection', () => { } }) - testSafeInNonShell() + testNonShellBlockingAndSafeRequests() }) }) }) diff --git a/packages/dd-trace/test/appsec/rasp/command_injection.integration.spec.js b/packages/dd-trace/test/appsec/rasp/command_injection.integration.spec.js index 4ebb8c4910a..5741809de40 100644 --- a/packages/dd-trace/test/appsec/rasp/command_injection.integration.spec.js +++ b/packages/dd-trace/test/appsec/rasp/command_injection.integration.spec.js @@ -52,7 +52,7 @@ describe('RASP - command_injection - integration', () => { await agent.stop() }) - async function testRequestBlocked (url) { + async function testRequestBlocked (url, id = 3) { try { await axios.get(url) } catch (e) { @@ -63,26 +63,46 @@ describe('RASP - command_injection - integration', () => { assert.strictEqual(e.response.status, 403) return await agent.assertMessageReceived(({ headers, payload }) => { assert.property(payload[0][0].meta, '_dd.appsec.json') - assert.include(payload[0][0].meta['_dd.appsec.json'], '"rasp-command_injection-rule-id-3"') + assert.include(payload[0][0].meta['_dd.appsec.json'], `"rasp-command_injection-rule-id-${id}"`) }) } throw new Error('Request should be blocked') } - it('should block using execFileSync and exception handled by express', async () => { - await testRequestBlocked('/shi/execFileSync?dir=$(cat /etc/passwd 1>%262 ; echo .)') - }) + describe('with shell', () => { + it('should block using execFileSync and exception handled by express', async () => { + await testRequestBlocked('/shi/execFileSync?dir=$(cat /etc/passwd 1>%262 ; echo .)') + }) - it('should block using execFileSync and unhandled exception', async () => { - await testRequestBlocked('/shi/execFileSync/out-of-express-scope?dir=$(cat /etc/passwd 1>%262 ; echo .)') - }) + it('should block using execFileSync and unhandled exception', async () => { + await testRequestBlocked('/shi/execFileSync/out-of-express-scope?dir=$(cat /etc/passwd 1>%262 ; echo .)') + }) - it('should block using execSync and exception handled by express', async () => { - await testRequestBlocked('/shi/execSync?dir=$(cat /etc/passwd 1>%262 ; echo .)') + it('should block using execSync and exception handled by express', async () => { + await testRequestBlocked('/shi/execSync?dir=$(cat /etc/passwd 1>%262 ; echo .)') + }) + + it('should block using execSync and unhandled exception', async () => { + await testRequestBlocked('/shi/execSync/out-of-express-scope?dir=$(cat /etc/passwd 1>%262 ; echo .)') + }) }) - it('should block using execSync and unhandled exception', async () => { - await testRequestBlocked('/shi/execSync/out-of-express-scope?dir=$(cat /etc/passwd 1>%262 ; echo .)') + describe('without shell', () => { + it('should block using execFileSync and exception handled by express', async () => { + await testRequestBlocked('/cmdi/execFileSync?dir=cat /etc/passwd 1>&2 ; echo .', 4) + }) + + it('should block using execFileSync and unhandled exception', async () => { + await testRequestBlocked('/cmdi/execFileSync/out-of-express-scope?dir=cat /etc/passwd 1>&2 ; echo .', 4) + }) + + it('should block using spawnSync and exception handled by express', async () => { + await testRequestBlocked('/cmdi/spawnSync?dir=cat /etc/passwd 1>&2 ; echo .', 4) + }) + + it('should block using spawnSync and exception handled', async () => { + await testRequestBlocked('/cmdi/spawnSync/out-of-express-scope?dir=cat /etc/passwd 1>&2 ; echo .', 4) + }) }) }) diff --git a/packages/dd-trace/test/appsec/rasp/command_injection.spec.js b/packages/dd-trace/test/appsec/rasp/command_injection.spec.js index 785b155a113..bf920940c7a 100644 --- a/packages/dd-trace/test/appsec/rasp/command_injection.spec.js +++ b/packages/dd-trace/test/appsec/rasp/command_injection.spec.js @@ -49,49 +49,6 @@ describe('RASP - command_injection.js', () => { }) describe('analyzeCommandInjection', () => { - it('should analyze command_injection without arguments', () => { - const ctx = { - file: 'cmd', - shell: true - } - const req = {} - datadogCore.storage.getStore.returns({ req }) - - start.publish(ctx) - - const persistent = { [addresses.SHELL_COMMAND]: 'cmd' } - sinon.assert.calledOnceWithExactly(waf.run, { persistent }, req, 'command_injection') - }) - - it('should analyze command_injection with arguments', () => { - const ctx = { - file: 'cmd', - fileArgs: ['arg0', 'arg1'], - shell: true - } - const req = {} - datadogCore.storage.getStore.returns({ req }) - - start.publish(ctx) - - const persistent = { [addresses.SHELL_COMMAND]: ['cmd', 'arg0', 'arg1'] } - sinon.assert.calledOnceWithExactly(waf.run, { persistent }, req, 'command_injection') - }) - - it('should not analyze command_injection when it is not shell', () => { - const ctx = { - file: 'cmd', - fileArgs: ['arg0', 'arg1'], - shell: false - } - const req = {} - datadogCore.storage.getStore.returns({ req }) - - start.publish(ctx) - - sinon.assert.notCalled(waf.run) - }) - it('should not analyze command_injection if rasp is disabled', () => { commandInjection.disable() const ctx = { @@ -139,18 +96,102 @@ describe('RASP - command_injection.js', () => { sinon.assert.notCalled(waf.run) }) - it('should call handleResult', () => { - const abortController = { abort: 'abort' } - const ctx = { file: 'cmd', abortController, shell: true } - const wafResult = { waf: 'waf' } - const req = { req: 'req' } - const res = { res: 'res' } - waf.run.returns(wafResult) - datadogCore.storage.getStore.returns({ req, res }) - - start.publish(ctx) + describe('command_injection with shell', () => { + it('should analyze command_injection without arguments', () => { + const ctx = { + file: 'cmd', + shell: true + } + const req = {} + datadogCore.storage.getStore.returns({ req }) + + start.publish(ctx) + + const persistent = { [addresses.SHELL_COMMAND]: 'cmd' } + sinon.assert.calledOnceWithExactly( + waf.run, { persistent }, req, { type: 'command_injection', variant: 'shell' } + ) + }) + + it('should analyze command_injection with arguments', () => { + const ctx = { + file: 'cmd', + fileArgs: ['arg0', 'arg1'], + shell: true + } + const req = {} + datadogCore.storage.getStore.returns({ req }) + + start.publish(ctx) + + const persistent = { [addresses.SHELL_COMMAND]: ['cmd', 'arg0', 'arg1'] } + sinon.assert.calledOnceWithExactly( + waf.run, { persistent }, req, { type: 'command_injection', variant: 'shell' } + ) + }) + + it('should call handleResult', () => { + const abortController = { abort: 'abort' } + const ctx = { file: 'cmd', abortController, shell: true } + const wafResult = { waf: 'waf' } + const req = { req: 'req' } + const res = { res: 'res' } + waf.run.returns(wafResult) + datadogCore.storage.getStore.returns({ req, res }) + + start.publish(ctx) + + sinon.assert.calledOnceWithExactly(utils.handleResult, wafResult, req, res, abortController, config) + }) + }) - sinon.assert.calledOnceWithExactly(utils.handleResult, wafResult, req, res, abortController, config) + describe('command_injection without shell', () => { + it('should analyze command injection without arguments', () => { + const ctx = { + file: 'ls', + shell: false + } + const req = {} + datadogCore.storage.getStore.returns({ req }) + + start.publish(ctx) + + const persistent = { [addresses.EXEC_COMMAND]: ['ls'] } + sinon.assert.calledOnceWithExactly( + waf.run, { persistent }, req, { type: 'command_injection', variant: 'exec' } + ) + }) + + it('should analyze command injection with arguments', () => { + const ctx = { + file: 'ls', + fileArgs: ['-la', '/tmp'], + shell: false + } + const req = {} + datadogCore.storage.getStore.returns({ req }) + + start.publish(ctx) + + const persistent = { [addresses.EXEC_COMMAND]: ['ls', '-la', '/tmp'] } + sinon.assert.calledOnceWithExactly( + waf.run, { persistent }, req, { type: 'command_injection', variant: 'exec' } + ) + }) + + it('should call handleResult', () => { + const abortController = { abort: 'abort' } + const ctx = { file: 'cmd', abortController, shell: false } + const wafResult = { waf: 'waf' } + const req = { req: 'req' } + const res = { res: 'res' } + waf.run.returns(wafResult) + datadogCore.storage.getStore.returns({ req, res }) + + start.publish(ctx) + + sinon.assert.calledOnceWithExactly(utils.handleResult, wafResult, req, res, abortController, config) + }) }) }) }) diff --git a/packages/dd-trace/test/appsec/rasp/lfi.spec.js b/packages/dd-trace/test/appsec/rasp/lfi.spec.js index 405311ae0d3..0a1328e2c52 100644 --- a/packages/dd-trace/test/appsec/rasp/lfi.spec.js +++ b/packages/dd-trace/test/appsec/rasp/lfi.spec.js @@ -111,7 +111,7 @@ describe('RASP - lfi.js', () => { fsOperationStart.publish(ctx) const persistent = { [FS_OPERATION_PATH]: path } - sinon.assert.calledOnceWithExactly(waf.run, { persistent }, req, 'lfi') + sinon.assert.calledOnceWithExactly(waf.run, { persistent }, req, { type: 'lfi' }) }) it('should NOT analyze lfi for child fs operations', () => { diff --git a/packages/dd-trace/test/appsec/rasp/resources/rasp_rules.json b/packages/dd-trace/test/appsec/rasp/resources/rasp_rules.json index daca47d8d20..c0396bd9871 100644 --- a/packages/dd-trace/test/appsec/rasp/resources/rasp_rules.json +++ b/packages/dd-trace/test/appsec/rasp/resources/rasp_rules.json @@ -110,7 +110,7 @@ }, { "id": "rasp-command_injection-rule-id-3", - "name": "Command injection exploit", + "name": "Shell command injection exploit", "tags": { "type": "command_injection", "category": "vulnerability_trigger", @@ -156,6 +156,55 @@ "block", "stack_trace" ] + }, + { + "id": "rasp-command_injection-rule-id-4", + "name": "OS command injection exploit", + "tags": { + "type": "command_injection", + "category": "vulnerability_trigger", + "cwe": "77", + "capec": "1000/152/248/88", + "confidence": "0", + "module": "rasp" + }, + "conditions": [ + { + "parameters": { + "resource": [ + { + "address": "server.sys.exec.cmd" + } + ], + "params": [ + { + "address": "server.request.query" + }, + { + "address": "server.request.body" + }, + { + "address": "server.request.path_params" + }, + { + "address": "grpc.server.request.message" + }, + { + "address": "graphql.server.all_resolvers" + }, + { + "address": "graphql.server.resolver" + } + ] + }, + "operator": "cmdi_detector" + } + ], + "transformers": [], + "on_match": [ + "block", + "stack_trace" + ] } ] } diff --git a/packages/dd-trace/test/appsec/rasp/resources/shi-app/index.js b/packages/dd-trace/test/appsec/rasp/resources/shi-app/index.js index a6714bd2148..ead83d03d44 100644 --- a/packages/dd-trace/test/appsec/rasp/resources/shi-app/index.js +++ b/packages/dd-trace/test/appsec/rasp/resources/shi-app/index.js @@ -39,6 +39,34 @@ app.get('/shi/execSync/out-of-express-scope', async (req, res) => { }) }) +app.get('/cmdi/execFileSync', async (req, res) => { + childProcess.execFileSync('sh', ['-c', req.query.dir]) + + res.end('OK') +}) + +app.get('/cmdi/execFileSync/out-of-express-scope', async (req, res) => { + process.nextTick(() => { + childProcess.execFileSync('sh', ['-c', req.query.dir]) + + res.end('OK') + }) +}) + +app.get('/cmdi/spawnSync', async (req, res) => { + childProcess.spawnSync('sh', ['-c', req.query.dir]) + + res.end('OK') +}) + +app.get('/cmdi/spawnSync/out-of-express-scope', async (req, res) => { + process.nextTick(() => { + childProcess.spawnSync('sh', ['-c', req.query.dir]) + + res.end('OK') + }) +}) + app.listen(port, () => { process.send({ port }) }) diff --git a/packages/dd-trace/test/appsec/rasp/sql_injection.spec.js b/packages/dd-trace/test/appsec/rasp/sql_injection.spec.js index d713521e986..fe7c9af082d 100644 --- a/packages/dd-trace/test/appsec/rasp/sql_injection.spec.js +++ b/packages/dd-trace/test/appsec/rasp/sql_injection.spec.js @@ -57,7 +57,7 @@ describe('RASP - sql_injection', () => { [addresses.DB_STATEMENT]: 'SELECT 1', [addresses.DB_SYSTEM]: 'postgresql' } - sinon.assert.calledOnceWithExactly(waf.run, { persistent }, req, 'sql_injection') + sinon.assert.calledOnceWithExactly(waf.run, { persistent }, req, { type: 'sql_injection' }) }) it('should not analyze sql injection if rasp is disabled', () => { @@ -128,7 +128,7 @@ describe('RASP - sql_injection', () => { [addresses.DB_STATEMENT]: 'SELECT 1', [addresses.DB_SYSTEM]: 'mysql' } - sinon.assert.calledOnceWithExactly(waf.run, { persistent }, req, 'sql_injection') + sinon.assert.calledOnceWithExactly(waf.run, { persistent }, req, { type: 'sql_injection' }) }) it('should not analyze sql injection if rasp is disabled', () => { diff --git a/packages/dd-trace/test/appsec/rasp/ssrf.spec.js b/packages/dd-trace/test/appsec/rasp/ssrf.spec.js index c40867ea254..98d5c8a0104 100644 --- a/packages/dd-trace/test/appsec/rasp/ssrf.spec.js +++ b/packages/dd-trace/test/appsec/rasp/ssrf.spec.js @@ -54,7 +54,7 @@ describe('RASP - ssrf.js', () => { httpClientRequestStart.publish(ctx) const persistent = { [addresses.HTTP_OUTGOING_URL]: 'http://example.com' } - sinon.assert.calledOnceWithExactly(waf.run, { persistent }, req, 'ssrf') + sinon.assert.calledOnceWithExactly(waf.run, { persistent }, req, { type: 'ssrf' }) }) it('should not analyze ssrf if rasp is disabled', () => { diff --git a/packages/dd-trace/test/appsec/remote_config/index.spec.js b/packages/dd-trace/test/appsec/remote_config/index.spec.js index 67447cf7a69..a79b5955701 100644 --- a/packages/dd-trace/test/appsec/remote_config/index.spec.js +++ b/packages/dd-trace/test/appsec/remote_config/index.spec.js @@ -171,6 +171,8 @@ describe('Remote Config index', () => { .to.have.been.calledWithExactly(RemoteConfigCapabilities.ASM_RASP_LFI, true) expect(rc.updateCapabilities) .to.have.been.calledWithExactly(RemoteConfigCapabilities.ASM_RASP_SHI, true) + expect(rc.updateCapabilities) + .to.have.been.calledWithExactly(RemoteConfigCapabilities.ASM_RASP_CMDI, true) expect(rc.setProductHandler).to.have.been.calledWith('ASM_DATA') expect(rc.setProductHandler).to.have.been.calledWith('ASM_DD') @@ -215,6 +217,8 @@ describe('Remote Config index', () => { .to.have.been.calledWithExactly(RemoteConfigCapabilities.ASM_RASP_LFI, true) expect(rc.updateCapabilities) .to.have.been.calledWithExactly(RemoteConfigCapabilities.ASM_RASP_SHI, true) + expect(rc.updateCapabilities) + .to.have.been.calledWithExactly(RemoteConfigCapabilities.ASM_RASP_CMDI, true) expect(rc.setProductHandler).to.have.been.calledWith('ASM_DATA') expect(rc.setProductHandler).to.have.been.calledWith('ASM_DD') @@ -261,6 +265,8 @@ describe('Remote Config index', () => { .to.have.been.calledWithExactly(RemoteConfigCapabilities.ASM_RASP_LFI, true) expect(rc.updateCapabilities) .to.have.been.calledWithExactly(RemoteConfigCapabilities.ASM_RASP_SHI, true) + expect(rc.updateCapabilities) + .to.have.been.calledWithExactly(RemoteConfigCapabilities.ASM_RASP_CMDI, true) }) it('should not activate rasp capabilities if rasp is disabled', () => { @@ -302,6 +308,8 @@ describe('Remote Config index', () => { .to.not.have.been.calledWithExactly(RemoteConfigCapabilities.ASM_RASP_LFI) expect(rc.updateCapabilities) .to.not.have.been.calledWithExactly(RemoteConfigCapabilities.ASM_RASP_SHI) + expect(rc.updateCapabilities) + .to.not.have.been.calledWithExactly(RemoteConfigCapabilities.ASM_RASP_CMDI) }) }) @@ -343,6 +351,8 @@ describe('Remote Config index', () => { .to.have.been.calledWithExactly(RemoteConfigCapabilities.ASM_RASP_LFI, false) expect(rc.updateCapabilities) .to.have.been.calledWithExactly(RemoteConfigCapabilities.ASM_RASP_SHI, false) + expect(rc.updateCapabilities) + .to.have.been.calledWithExactly(RemoteConfigCapabilities.ASM_RASP_CMDI, false) expect(rc.removeProductHandler).to.have.been.calledWith('ASM_DATA') expect(rc.removeProductHandler).to.have.been.calledWith('ASM_DD') From 8810d34cebfc01050305afd98852dc9090386d0b Mon Sep 17 00:00:00 2001 From: ishabi Date: Wed, 4 Dec 2024 15:10:10 +0100 Subject: [PATCH 02/11] fix spawnSync abort error test --- .../src/child_process.js | 29 ++++--------------- .../test/child_process.spec.js | 18 +----------- 2 files changed, 6 insertions(+), 41 deletions(-) diff --git a/packages/datadog-instrumentations/src/child_process.js b/packages/datadog-instrumentations/src/child_process.js index 6b092aa9947..bbf613f430c 100644 --- a/packages/datadog-instrumentations/src/child_process.js +++ b/packages/datadog-instrumentations/src/child_process.js @@ -19,32 +19,14 @@ const names = ['child_process', 'node:child_process'] // child_process and node:child_process returns the same object instance, we only want to add hooks once let patched = false -function throwSyncError (error) { - throw error -} - -function returnSpawnSyncError (error, context) { - context.result = { - error, - status: null, - signal: null, - output: null, - stdout: null, - stderr: null, - pid: 0 - } - - throw error -} - names.forEach(name => { addHook({ name }, childProcess => { if (!patched) { patched = true shimmer.massWrap(childProcess, execAsyncMethods, wrapChildProcessAsyncMethod(childProcess.ChildProcess)) - shimmer.wrap(childProcess, 'execSync', wrapChildProcessSyncMethod(throwSyncError, true)) - shimmer.wrap(childProcess, 'execFileSync', wrapChildProcessSyncMethod(throwSyncError)) - shimmer.wrap(childProcess, 'spawnSync', wrapChildProcessSyncMethod(returnSpawnSyncError)) + shimmer.wrap(childProcess, 'execSync', wrapChildProcessSyncMethod(true)) + shimmer.wrap(childProcess, 'execFileSync', wrapChildProcessSyncMethod()) + shimmer.wrap(childProcess, 'spawnSync', wrapChildProcessSyncMethod()) } return childProcess @@ -89,7 +71,7 @@ function createContextFromChildProcessInfo (childProcessInfo) { return context } -function wrapChildProcessSyncMethod (returnError, shell = false) { +function wrapChildProcessSyncMethod (shell = false) { return function wrapMethod (childProcessMethod) { return function () { if (!childProcessChannel.start.hasSubscribers || arguments.length === 0) { @@ -108,8 +90,7 @@ function wrapChildProcessSyncMethod (returnError, shell = false) { try { if (abortController.signal.aborted) { const error = abortController.signal.reason || new Error('Aborted') - // expected behaviors on error are different - return returnError(error, context) + throw error } const result = childProcessMethod.apply(this, arguments) diff --git a/packages/datadog-instrumentations/test/child_process.spec.js b/packages/datadog-instrumentations/test/child_process.spec.js index f6d19423797..8adc11c02af 100644 --- a/packages/datadog-instrumentations/test/child_process.spec.js +++ b/packages/datadog-instrumentations/test/child_process.spec.js @@ -614,7 +614,7 @@ describe('child process', () => { childProcessChannel.unsubscribe({ start: abort }) }) - ;['execFileSync', 'execSync'].forEach((methodName) => { + ;['execFileSync', 'execSync', 'spawnSync'].forEach((methodName) => { describe(`method ${methodName}`, () => { it('should throw the expected error', () => { try { @@ -629,22 +629,6 @@ describe('child process', () => { }) }) }) - - describe('method spawnSync', () => { - it('should return error field', () => { - const result = childProcess.spawnSync('aborted_command') - - expect(result).to.be.deep.equal({ - error: abortError, - status: null, - signal: null, - output: null, - stdout: null, - stderr: null, - pid: 0 - }) - }) - }) }) }) }) From 46762c4cba4c890b3729674699ef09ef9e81fe74 Mon Sep 17 00:00:00 2001 From: ishabi Date: Mon, 16 Dec 2024 13:58:29 +0100 Subject: [PATCH 03/11] add telemetry tests --- integration-tests/helpers/fake-agent.js | 8 ++-- .../src/appsec/rasp/command_injection.js | 4 +- .../command_injection.express.plugin.spec.js | 1 + .../command_injection.integration.spec.js | 37 +++++++++++++++---- 4 files changed, 35 insertions(+), 15 deletions(-) diff --git a/integration-tests/helpers/fake-agent.js b/integration-tests/helpers/fake-agent.js index 4902c80d9a1..58aaa7c3b35 100644 --- a/integration-tests/helpers/fake-agent.js +++ b/integration-tests/helpers/fake-agent.js @@ -146,7 +146,7 @@ module.exports = class FakeAgent extends EventEmitter { return resultPromise } - assertTelemetryReceived (fn, timeout, requestType, expectedMessageCount = 1) { + assertTelemetryReceived (fn, timeout, requestType, expectedMessageCount = 1, resolveAtFirstSuccess) { timeout = timeout || 30000 let resultResolve let resultReject @@ -174,15 +174,13 @@ module.exports = class FakeAgent extends EventEmitter { msgCount += 1 try { fn(msg) - if (msgCount === expectedMessageCount) { + if (resolveAtFirstSuccess || msgCount === expectedMessageCount) { resultResolve() + this.removeListener('message', messageHandler) } } catch (e) { errors.push(e) } - if (msgCount === expectedMessageCount) { - this.removeListener('telemetry', messageHandler) - } } this.on('telemetry', messageHandler) diff --git a/packages/dd-trace/src/appsec/rasp/command_injection.js b/packages/dd-trace/src/appsec/rasp/command_injection.js index 8e087086da4..62546e2b6a6 100644 --- a/packages/dd-trace/src/appsec/rasp/command_injection.js +++ b/packages/dd-trace/src/appsec/rasp/command_injection.js @@ -38,9 +38,7 @@ function analyzeCommandInjection ({ file, fileArgs, shell, abortController }) { if (shell) { persistent[addresses.SHELL_COMMAND] = params raspRule.variant = 'shell' - } - - if (!shell) { + } else { const commandParams = Array.isArray(params) ? params : [params] persistent[addresses.EXEC_COMMAND] = commandParams raspRule.variant = 'exec' diff --git a/packages/dd-trace/test/appsec/rasp/command_injection.express.plugin.spec.js b/packages/dd-trace/test/appsec/rasp/command_injection.express.plugin.spec.js index 4d34f65bbf7..a39b6fc0369 100644 --- a/packages/dd-trace/test/appsec/rasp/command_injection.express.plugin.spec.js +++ b/packages/dd-trace/test/appsec/rasp/command_injection.express.plugin.spec.js @@ -384,6 +384,7 @@ describe('RASP - command_injection', () => { beforeEach(() => { app = (req, res) => { const childProcess = require('child_process') + const child = childProcess.spawn('sh', ['-c', req.query.dir]) child.on('error', (e) => { if (e.name === 'DatadogRaspAbortError') { diff --git a/packages/dd-trace/test/appsec/rasp/command_injection.integration.spec.js b/packages/dd-trace/test/appsec/rasp/command_injection.integration.spec.js index 5741809de40..5b4d9ffcc32 100644 --- a/packages/dd-trace/test/appsec/rasp/command_injection.integration.spec.js +++ b/packages/dd-trace/test/appsec/rasp/command_injection.integration.spec.js @@ -42,6 +42,7 @@ describe('RASP - command_injection - integration', () => { APP_PORT: appPort, DD_APPSEC_ENABLED: 'true', DD_APPSEC_RASP_ENABLED: 'true', + DD_TELEMETRY_HEARTBEAT_INTERVAL: 1, DD_APPSEC_RULES: path.join(cwd, 'resources', 'rasp_rules.json') } }) @@ -52,7 +53,7 @@ describe('RASP - command_injection - integration', () => { await agent.stop() }) - async function testRequestBlocked (url, id = 3) { + async function testRequestBlocked (url, ruleId = 3, variant = 'shell') { try { await axios.get(url) } catch (e) { @@ -61,10 +62,32 @@ describe('RASP - command_injection - integration', () => { } assert.strictEqual(e.response.status, 403) - return await agent.assertMessageReceived(({ headers, payload }) => { + + const checkMessages = await agent.assertMessageReceived(({ headers, payload }) => { assert.property(payload[0][0].meta, '_dd.appsec.json') - assert.include(payload[0][0].meta['_dd.appsec.json'], `"rasp-command_injection-rule-id-${id}"`) + assert.include(payload[0][0].meta['_dd.appsec.json'], `"rasp-command_injection-rule-id-${ruleId}"`) }) + + const checkTelemetry = await agent.assertTelemetryReceived(({ headers, payload }) => { + const namespace = payload.payload.namespace + assert.equal(namespace, 'appsec') + + const series = payload.payload.series + const evalSerie = series.find(s => s.metric === 'rasp.rule.eval') + const matchSerie = series.find(s => s.metric === 'rasp.rule.match') + + assert.exists(evalSerie, 'eval serie should exist') + assert.include(evalSerie.tags, 'rule_type:command_injection') + assert.include(evalSerie.tags, `rule_variant:${variant}`) + assert.strictEqual(evalSerie.type, 'count') + + assert.exists(matchSerie, 'match serie should exist') + assert.include(matchSerie.tags, 'rule_type:command_injection') + assert.include(matchSerie.tags, `rule_variant:${variant}`) + assert.strictEqual(matchSerie.type, 'count') + }, 30_000, 'generate-metrics', 1, true) + + return Promise.all([checkMessages, checkTelemetry]) } throw new Error('Request should be blocked') @@ -90,19 +113,19 @@ describe('RASP - command_injection - integration', () => { describe('without shell', () => { it('should block using execFileSync and exception handled by express', async () => { - await testRequestBlocked('/cmdi/execFileSync?dir=cat /etc/passwd 1>&2 ; echo .', 4) + await testRequestBlocked('/cmdi/execFileSync?dir=cat /etc/passwd 1>&2 ; echo .', 4, 'exec') }) it('should block using execFileSync and unhandled exception', async () => { - await testRequestBlocked('/cmdi/execFileSync/out-of-express-scope?dir=cat /etc/passwd 1>&2 ; echo .', 4) + await testRequestBlocked('/cmdi/execFileSync/out-of-express-scope?dir=cat /etc/passwd 1>&2 ; echo .', 4, 'exec') }) it('should block using spawnSync and exception handled by express', async () => { - await testRequestBlocked('/cmdi/spawnSync?dir=cat /etc/passwd 1>&2 ; echo .', 4) + await testRequestBlocked('/cmdi/spawnSync?dir=cat /etc/passwd 1>&2 ; echo .', 4, 'exec') }) it('should block using spawnSync and exception handled', async () => { - await testRequestBlocked('/cmdi/spawnSync/out-of-express-scope?dir=cat /etc/passwd 1>&2 ; echo .', 4) + await testRequestBlocked('/cmdi/spawnSync/out-of-express-scope?dir=cat /etc/passwd 1>&2 ; echo .', 4, 'exec') }) }) }) From 755a5b4646aedc3143048b856a4a8265f08917be Mon Sep 17 00:00:00 2001 From: ishabi Date: Mon, 16 Dec 2024 16:09:46 +0100 Subject: [PATCH 04/11] fix sql injection tests on postgres --- .../test/appsec/rasp/sql_injection.pg.plugin.spec.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/dd-trace/test/appsec/rasp/sql_injection.pg.plugin.spec.js b/packages/dd-trace/test/appsec/rasp/sql_injection.pg.plugin.spec.js index 8f05158c22d..2d4dd779c17 100644 --- a/packages/dd-trace/test/appsec/rasp/sql_injection.pg.plugin.spec.js +++ b/packages/dd-trace/test/appsec/rasp/sql_injection.pg.plugin.spec.js @@ -219,7 +219,7 @@ describe('RASP - sql_injection', () => { await axios.get('/') - assert.equal(run.args.filter(arg => arg[1] === 'sql_injection').length, 1) + assert.equal(run.args.filter(arg => arg[1]?.type === 'sql_injection').length, 1) }) it('should call to waf twice for sql injection with two different queries in pg Pool', async () => { @@ -232,7 +232,7 @@ describe('RASP - sql_injection', () => { await axios.get('/') - assert.equal(run.args.filter(arg => arg[1] === 'sql_injection').length, 2) + assert.equal(run.args.filter(arg => arg[1]?.type === 'sql_injection').length, 2) }) it('should call to waf twice for sql injection and same query when input address is updated', async () => { @@ -254,7 +254,7 @@ describe('RASP - sql_injection', () => { await axios.get('/') - assert.equal(run.args.filter(arg => arg[1] === 'sql_injection').length, 2) + assert.equal(run.args.filter(arg => arg[1]?.type === 'sql_injection').length, 2) }) it('should call to waf once for sql injection and same query when input address is updated', async () => { @@ -276,7 +276,7 @@ describe('RASP - sql_injection', () => { await axios.get('/') - assert.equal(run.args.filter(arg => arg[1] === 'sql_injection').length, 1) + assert.equal(run.args.filter(arg => arg[1]?.type === 'sql_injection').length, 1) }) }) }) From b0bb29a36e23a19a05a81f96765adca959b37d38 Mon Sep 17 00:00:00 2001 From: ishabi Date: Tue, 17 Dec 2024 11:55:18 +0100 Subject: [PATCH 05/11] add different test --- .../rasp/command_injection.express.plugin.spec.js | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/packages/dd-trace/test/appsec/rasp/command_injection.express.plugin.spec.js b/packages/dd-trace/test/appsec/rasp/command_injection.express.plugin.spec.js index a39b6fc0369..2bc0b4c34ce 100644 --- a/packages/dd-trace/test/appsec/rasp/command_injection.express.plugin.spec.js +++ b/packages/dd-trace/test/appsec/rasp/command_injection.express.plugin.spec.js @@ -36,7 +36,7 @@ describe('RASP - command_injection', () => { function testNonShellBlockingAndSafeRequests () { it('should block the threat', async () => { try { - await axios.get('/?dir=cat /etc/passwd 1>&2 ; echo .') + await axios.get('/?dir=/usr/bin/reboot') } catch (e) { if (!e.response) { throw e @@ -262,7 +262,7 @@ describe('RASP - command_injection', () => { app = (req, res) => { const childProcess = require('child_process') - childProcess.execFile('sh', ['-c', req.query.dir], function (e) { + childProcess.execFile(req.query.dir, function (e) { if (e?.name === 'DatadogRaspAbortError') { res.writeHead(500) } @@ -282,7 +282,7 @@ describe('RASP - command_injection', () => { const execFile = util.promisify(require('child_process').execFile) try { - await execFile('sh', ['-c', req.query.dir]) + await execFile([req.query.dir]) } catch (e) { if (e.name === 'DatadogRaspAbortError') { res.writeHead(500) @@ -300,7 +300,7 @@ describe('RASP - command_injection', () => { beforeEach(() => { app = (req, res) => { const childProcess = require('child_process') - const child = childProcess.execFile('sh', ['-c', req.query.dir]) + const child = childProcess.execFile(req.query.dir) child.on('error', (e) => { if (e.name === 'DatadogRaspAbortError') { res.writeHead(500) @@ -322,7 +322,7 @@ describe('RASP - command_injection', () => { const childProcess = require('child_process') try { - childProcess.execFileSync('sh', ['-c', req.query.dir]) + childProcess.execFileSync([req.query.dir]) } catch (e) { if (e.name === 'DatadogRaspAbortError') { res.writeHead(500) @@ -385,7 +385,7 @@ describe('RASP - command_injection', () => { app = (req, res) => { const childProcess = require('child_process') - const child = childProcess.spawn('sh', ['-c', req.query.dir]) + const child = childProcess.spawn(req.query.dir) child.on('error', (e) => { if (e.name === 'DatadogRaspAbortError') { res.writeHead(500) @@ -406,7 +406,7 @@ describe('RASP - command_injection', () => { app = (req, res) => { const childProcess = require('child_process') - const child = childProcess.spawnSync('sh', ['-c', req.query.dir]) + const child = childProcess.spawnSync(req.query.dir) if (child.error?.name === 'DatadogRaspAbortError') { res.writeHead(500) } From 7238de09a016540584c0aebb7df6cfd1751a4ee4 Mon Sep 17 00:00:00 2001 From: ishabi Date: Tue, 17 Dec 2024 21:45:08 +0100 Subject: [PATCH 06/11] revert spawnSync changes --- integration-tests/helpers/fake-agent.js | 8 +-- .../src/child_process.js | 29 +++++++++-- .../test/child_process.spec.js | 18 ++++++- .../command_injection.express.plugin.spec.js | 16 +++--- .../command_injection.integration.spec.js | 49 ++++++++++--------- .../appsec/rasp/resources/shi-app/index.js | 18 +------ .../dd-trace/test/appsec/reporter.spec.js | 8 +-- 7 files changed, 86 insertions(+), 60 deletions(-) diff --git a/integration-tests/helpers/fake-agent.js b/integration-tests/helpers/fake-agent.js index 58aaa7c3b35..4902c80d9a1 100644 --- a/integration-tests/helpers/fake-agent.js +++ b/integration-tests/helpers/fake-agent.js @@ -146,7 +146,7 @@ module.exports = class FakeAgent extends EventEmitter { return resultPromise } - assertTelemetryReceived (fn, timeout, requestType, expectedMessageCount = 1, resolveAtFirstSuccess) { + assertTelemetryReceived (fn, timeout, requestType, expectedMessageCount = 1) { timeout = timeout || 30000 let resultResolve let resultReject @@ -174,13 +174,15 @@ module.exports = class FakeAgent extends EventEmitter { msgCount += 1 try { fn(msg) - if (resolveAtFirstSuccess || msgCount === expectedMessageCount) { + if (msgCount === expectedMessageCount) { resultResolve() - this.removeListener('message', messageHandler) } } catch (e) { errors.push(e) } + if (msgCount === expectedMessageCount) { + this.removeListener('telemetry', messageHandler) + } } this.on('telemetry', messageHandler) diff --git a/packages/datadog-instrumentations/src/child_process.js b/packages/datadog-instrumentations/src/child_process.js index bbf613f430c..f7224953367 100644 --- a/packages/datadog-instrumentations/src/child_process.js +++ b/packages/datadog-instrumentations/src/child_process.js @@ -19,14 +19,32 @@ const names = ['child_process', 'node:child_process'] // child_process and node:child_process returns the same object instance, we only want to add hooks once let patched = false +function throwSyncError (error) { + throw error +} + +function returnSpawnSyncError (error, context) { + context.result = { + error, + status: null, + signal: null, + output: null, + stdout: null, + stderr: null, + pid: 0 + } + + return context.result +} + names.forEach(name => { addHook({ name }, childProcess => { if (!patched) { patched = true shimmer.massWrap(childProcess, execAsyncMethods, wrapChildProcessAsyncMethod(childProcess.ChildProcess)) - shimmer.wrap(childProcess, 'execSync', wrapChildProcessSyncMethod(true)) - shimmer.wrap(childProcess, 'execFileSync', wrapChildProcessSyncMethod()) - shimmer.wrap(childProcess, 'spawnSync', wrapChildProcessSyncMethod()) + shimmer.wrap(childProcess, 'execSync', wrapChildProcessSyncMethod(throwSyncError, true)) + shimmer.wrap(childProcess, 'execFileSync', wrapChildProcessSyncMethod(throwSyncError)) + shimmer.wrap(childProcess, 'spawnSync', wrapChildProcessSyncMethod(returnSpawnSyncError)) } return childProcess @@ -71,7 +89,7 @@ function createContextFromChildProcessInfo (childProcessInfo) { return context } -function wrapChildProcessSyncMethod (shell = false) { +function wrapChildProcessSyncMethod (returnError, shell = false) { return function wrapMethod (childProcessMethod) { return function () { if (!childProcessChannel.start.hasSubscribers || arguments.length === 0) { @@ -90,7 +108,8 @@ function wrapChildProcessSyncMethod (shell = false) { try { if (abortController.signal.aborted) { const error = abortController.signal.reason || new Error('Aborted') - throw error + // expected behaviors on error are different + return returnError(error, context) } const result = childProcessMethod.apply(this, arguments) diff --git a/packages/datadog-instrumentations/test/child_process.spec.js b/packages/datadog-instrumentations/test/child_process.spec.js index 8adc11c02af..f6d19423797 100644 --- a/packages/datadog-instrumentations/test/child_process.spec.js +++ b/packages/datadog-instrumentations/test/child_process.spec.js @@ -614,7 +614,7 @@ describe('child process', () => { childProcessChannel.unsubscribe({ start: abort }) }) - ;['execFileSync', 'execSync', 'spawnSync'].forEach((methodName) => { + ;['execFileSync', 'execSync'].forEach((methodName) => { describe(`method ${methodName}`, () => { it('should throw the expected error', () => { try { @@ -629,6 +629,22 @@ describe('child process', () => { }) }) }) + + describe('method spawnSync', () => { + it('should return error field', () => { + const result = childProcess.spawnSync('aborted_command') + + expect(result).to.be.deep.equal({ + error: abortError, + status: null, + signal: null, + output: null, + stdout: null, + stderr: null, + pid: 0 + }) + }) + }) }) }) }) diff --git a/packages/dd-trace/test/appsec/rasp/command_injection.express.plugin.spec.js b/packages/dd-trace/test/appsec/rasp/command_injection.express.plugin.spec.js index 2bc0b4c34ce..d7609367ab9 100644 --- a/packages/dd-trace/test/appsec/rasp/command_injection.express.plugin.spec.js +++ b/packages/dd-trace/test/appsec/rasp/command_injection.express.plugin.spec.js @@ -36,7 +36,7 @@ describe('RASP - command_injection', () => { function testNonShellBlockingAndSafeRequests () { it('should block the threat', async () => { try { - await axios.get('/?dir=/usr/bin/reboot') + await axios.get('/?command=/usr/bin/reboot') } catch (e) { if (!e.response) { throw e @@ -49,7 +49,7 @@ describe('RASP - command_injection', () => { }) it('should not block safe request', async () => { - await axios.get('/?dir=.') + await axios.get('/?command=.') return checkRaspExecutedAndNotThreat(agent) }) @@ -262,7 +262,7 @@ describe('RASP - command_injection', () => { app = (req, res) => { const childProcess = require('child_process') - childProcess.execFile(req.query.dir, function (e) { + childProcess.execFile(req.query.command, function (e) { if (e?.name === 'DatadogRaspAbortError') { res.writeHead(500) } @@ -282,7 +282,7 @@ describe('RASP - command_injection', () => { const execFile = util.promisify(require('child_process').execFile) try { - await execFile([req.query.dir]) + await execFile([req.query.command]) } catch (e) { if (e.name === 'DatadogRaspAbortError') { res.writeHead(500) @@ -300,7 +300,7 @@ describe('RASP - command_injection', () => { beforeEach(() => { app = (req, res) => { const childProcess = require('child_process') - const child = childProcess.execFile(req.query.dir) + const child = childProcess.execFile(req.query.command) child.on('error', (e) => { if (e.name === 'DatadogRaspAbortError') { res.writeHead(500) @@ -322,7 +322,7 @@ describe('RASP - command_injection', () => { const childProcess = require('child_process') try { - childProcess.execFileSync([req.query.dir]) + childProcess.execFileSync([req.query.command]) } catch (e) { if (e.name === 'DatadogRaspAbortError') { res.writeHead(500) @@ -385,7 +385,7 @@ describe('RASP - command_injection', () => { app = (req, res) => { const childProcess = require('child_process') - const child = childProcess.spawn(req.query.dir) + const child = childProcess.spawn(req.query.command) child.on('error', (e) => { if (e.name === 'DatadogRaspAbortError') { res.writeHead(500) @@ -406,7 +406,7 @@ describe('RASP - command_injection', () => { app = (req, res) => { const childProcess = require('child_process') - const child = childProcess.spawnSync(req.query.dir) + const child = childProcess.spawnSync(req.query.command) if (child.error?.name === 'DatadogRaspAbortError') { res.writeHead(500) } diff --git a/packages/dd-trace/test/appsec/rasp/command_injection.integration.spec.js b/packages/dd-trace/test/appsec/rasp/command_injection.integration.spec.js index 5b4d9ffcc32..c4db0ef73bc 100644 --- a/packages/dd-trace/test/appsec/rasp/command_injection.integration.spec.js +++ b/packages/dd-trace/test/appsec/rasp/command_injection.integration.spec.js @@ -63,6 +63,8 @@ describe('RASP - command_injection - integration', () => { assert.strictEqual(e.response.status, 403) + let appsecTelemetryReceived = false + const checkMessages = await agent.assertMessageReceived(({ headers, payload }) => { assert.property(payload[0][0].meta, '_dd.appsec.json') assert.include(payload[0][0].meta['_dd.appsec.json'], `"rasp-command_injection-rule-id-${ruleId}"`) @@ -70,24 +72,31 @@ describe('RASP - command_injection - integration', () => { const checkTelemetry = await agent.assertTelemetryReceived(({ headers, payload }) => { const namespace = payload.payload.namespace - assert.equal(namespace, 'appsec') - const series = payload.payload.series - const evalSerie = series.find(s => s.metric === 'rasp.rule.eval') - const matchSerie = series.find(s => s.metric === 'rasp.rule.match') + // Only check telemetry received in appsec namespace and ignore others + if (namespace, 'appsec') { + appsecTelemetryReceived = true + const series = payload.payload.series + const evalSerie = series.find(s => s.metric === 'rasp.rule.eval') + const matchSerie = series.find(s => s.metric === 'rasp.rule.match') + + assert.exists(evalSerie, 'eval serie should exist') + assert.include(evalSerie.tags, 'rule_type:command_injection') + assert.include(evalSerie.tags, `rule_variant:${variant}`) + assert.strictEqual(evalSerie.type, 'count') - assert.exists(evalSerie, 'eval serie should exist') - assert.include(evalSerie.tags, 'rule_type:command_injection') - assert.include(evalSerie.tags, `rule_variant:${variant}`) - assert.strictEqual(evalSerie.type, 'count') + assert.exists(matchSerie, 'match serie should exist') + assert.include(matchSerie.tags, 'rule_type:command_injection') + assert.include(matchSerie.tags, `rule_variant:${variant}`) + assert.strictEqual(matchSerie.type, 'count') + } - assert.exists(matchSerie, 'match serie should exist') - assert.include(matchSerie.tags, 'rule_type:command_injection') - assert.include(matchSerie.tags, `rule_variant:${variant}`) - assert.strictEqual(matchSerie.type, 'count') - }, 30_000, 'generate-metrics', 1, true) + }, 30_000, 'generate-metrics', 2) - return Promise.all([checkMessages, checkTelemetry]) + const checks = await Promise.all([checkMessages, checkTelemetry]) + assert.equal(appsecTelemetryReceived, true) + + return checks } throw new Error('Request should be blocked') @@ -113,19 +122,11 @@ describe('RASP - command_injection - integration', () => { describe('without shell', () => { it('should block using execFileSync and exception handled by express', async () => { - await testRequestBlocked('/cmdi/execFileSync?dir=cat /etc/passwd 1>&2 ; echo .', 4, 'exec') + await testRequestBlocked('/cmdi/execFileSync?command=cat /etc/passwd 1>&2 ; echo .', 4, 'exec') }) it('should block using execFileSync and unhandled exception', async () => { - await testRequestBlocked('/cmdi/execFileSync/out-of-express-scope?dir=cat /etc/passwd 1>&2 ; echo .', 4, 'exec') - }) - - it('should block using spawnSync and exception handled by express', async () => { - await testRequestBlocked('/cmdi/spawnSync?dir=cat /etc/passwd 1>&2 ; echo .', 4, 'exec') - }) - - it('should block using spawnSync and exception handled', async () => { - await testRequestBlocked('/cmdi/spawnSync/out-of-express-scope?dir=cat /etc/passwd 1>&2 ; echo .', 4, 'exec') + await testRequestBlocked('/cmdi/execFileSync/out-of-express-scope?command=cat /etc/passwd 1>&2 ; echo .', 4, 'exec') }) }) }) diff --git a/packages/dd-trace/test/appsec/rasp/resources/shi-app/index.js b/packages/dd-trace/test/appsec/rasp/resources/shi-app/index.js index ead83d03d44..133c57dfb2b 100644 --- a/packages/dd-trace/test/appsec/rasp/resources/shi-app/index.js +++ b/packages/dd-trace/test/appsec/rasp/resources/shi-app/index.js @@ -40,28 +40,14 @@ app.get('/shi/execSync/out-of-express-scope', async (req, res) => { }) app.get('/cmdi/execFileSync', async (req, res) => { - childProcess.execFileSync('sh', ['-c', req.query.dir]) + childProcess.execFileSync('sh', ['-c', req.query.command]) res.end('OK') }) app.get('/cmdi/execFileSync/out-of-express-scope', async (req, res) => { process.nextTick(() => { - childProcess.execFileSync('sh', ['-c', req.query.dir]) - - res.end('OK') - }) -}) - -app.get('/cmdi/spawnSync', async (req, res) => { - childProcess.spawnSync('sh', ['-c', req.query.dir]) - - res.end('OK') -}) - -app.get('/cmdi/spawnSync/out-of-express-scope', async (req, res) => { - process.nextTick(() => { - childProcess.spawnSync('sh', ['-c', req.query.dir]) + childProcess.execFileSync('sh', ['-c', req.query.command]) res.end('OK') }) diff --git a/packages/dd-trace/test/appsec/reporter.spec.js b/packages/dd-trace/test/appsec/reporter.spec.js index cd7cc9a1581..a38092e728d 100644 --- a/packages/dd-trace/test/appsec/reporter.spec.js +++ b/packages/dd-trace/test/appsec/reporter.spec.js @@ -192,13 +192,15 @@ describe('reporter', () => { expect(telemetry.updateRaspRequestsMetricTags).to.not.have.been.called }) - it('should call updateRaspRequestsMetricTags when ruleType if provided', () => { + it('should call updateRaspRequestsMetricTags when raspRule is provided', () => { const metrics = { rulesVersion: '1.2.3' } const store = storage.getStore() - Reporter.reportMetrics(metrics, 'rule_type') + const raspRule = { type: 'rule_type', variant: 'rule_variant' } - expect(telemetry.updateRaspRequestsMetricTags).to.have.been.calledOnceWithExactly(metrics, store.req, 'rule_type') + Reporter.reportMetrics(metrics, raspRule) + + expect(telemetry.updateRaspRequestsMetricTags).to.have.been.calledOnceWithExactly(metrics, store.req, raspRule) expect(telemetry.updateWafRequestsMetricTags).to.not.have.been.called }) }) From 283c88245f368376ef258d7795a093b9aa95a53b Mon Sep 17 00:00:00 2001 From: ishabi Date: Tue, 17 Dec 2024 21:53:01 +0100 Subject: [PATCH 07/11] fix linter --- .../test/appsec/rasp/command_injection.integration.spec.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/dd-trace/test/appsec/rasp/command_injection.integration.spec.js b/packages/dd-trace/test/appsec/rasp/command_injection.integration.spec.js index c4db0ef73bc..d6fe4015202 100644 --- a/packages/dd-trace/test/appsec/rasp/command_injection.integration.spec.js +++ b/packages/dd-trace/test/appsec/rasp/command_injection.integration.spec.js @@ -74,7 +74,7 @@ describe('RASP - command_injection - integration', () => { const namespace = payload.payload.namespace // Only check telemetry received in appsec namespace and ignore others - if (namespace, 'appsec') { + if (namespace === 'appsec') { appsecTelemetryReceived = true const series = payload.payload.series const evalSerie = series.find(s => s.metric === 'rasp.rule.eval') @@ -90,7 +90,6 @@ describe('RASP - command_injection - integration', () => { assert.include(matchSerie.tags, `rule_variant:${variant}`) assert.strictEqual(matchSerie.type, 'count') } - }, 30_000, 'generate-metrics', 2) const checks = await Promise.all([checkMessages, checkTelemetry]) @@ -126,7 +125,9 @@ describe('RASP - command_injection - integration', () => { }) it('should block using execFileSync and unhandled exception', async () => { - await testRequestBlocked('/cmdi/execFileSync/out-of-express-scope?command=cat /etc/passwd 1>&2 ; echo .', 4, 'exec') + await testRequestBlocked( + '/cmdi/execFileSync/out-of-express-scope?command=cat /etc/passwd 1>&2 ; echo .', 4, 'exec' + ) }) }) }) From 5b8385f0cbce89d788b09ae1a315e9da341466c1 Mon Sep 17 00:00:00 2001 From: ishabi Date: Wed, 18 Dec 2024 11:18:49 +0100 Subject: [PATCH 08/11] add spawnSync tests --- .../command_injection.integration.spec.js | 12 ++++++++ .../appsec/rasp/resources/shi-app/index.js | 28 +++++++++++++++++++ 2 files changed, 40 insertions(+) diff --git a/packages/dd-trace/test/appsec/rasp/command_injection.integration.spec.js b/packages/dd-trace/test/appsec/rasp/command_injection.integration.spec.js index d6fe4015202..cf08f1d9404 100644 --- a/packages/dd-trace/test/appsec/rasp/command_injection.integration.spec.js +++ b/packages/dd-trace/test/appsec/rasp/command_injection.integration.spec.js @@ -129,5 +129,17 @@ describe('RASP - command_injection - integration', () => { '/cmdi/execFileSync/out-of-express-scope?command=cat /etc/passwd 1>&2 ; echo .', 4, 'exec' ) }) + + it('should abort spawnSync operation handled by express', async () => { + await testRequestBlocked( + '/cmdi/spawnSync?command=/sbin/reboot', 4, 'exec' + ) + }) + + it('should abort spawnSync operation outside of express scope', async () => { + await testRequestBlocked( + '/cmdi/spawnSync/out-of-express-scope?command=/sbin/reboot', 4, 'exec' + ) + }) }) }) diff --git a/packages/dd-trace/test/appsec/rasp/resources/shi-app/index.js b/packages/dd-trace/test/appsec/rasp/resources/shi-app/index.js index 133c57dfb2b..72c1e4e0e44 100644 --- a/packages/dd-trace/test/appsec/rasp/resources/shi-app/index.js +++ b/packages/dd-trace/test/appsec/rasp/resources/shi-app/index.js @@ -53,6 +53,34 @@ app.get('/cmdi/execFileSync/out-of-express-scope', async (req, res) => { }) }) +app.get('/cmdi/spawnSync', async (req, res) => { + const result = childProcess.spawnSync(req.query.command) + + if (result.error?.name === 'DatadogRaspAbortError') { + res.status(403).json({ + error: 'DatadogRaspAbortError' + }) + return + } + + res.end('OK') +}) + +app.get('/cmdi/spawnSync/out-of-express-scope', async (req, res) => { + process.nextTick(() => { + const result = childProcess.spawnSync(req.query.command) + + if (result.error?.name === 'DatadogRaspAbortError') { + res.status(403).json({ + error: 'DatadogRaspAbortError' + }) + return + } + + res.end('OK') + }) +}) + app.listen(port, () => { process.send({ port }) }) From 709d0c30ab15661b71951fae5a1e9c967d12b35b Mon Sep 17 00:00:00 2001 From: ishabi Date: Wed, 18 Dec 2024 13:27:54 +0100 Subject: [PATCH 09/11] remove spawnSync not needed test --- .../command_injection.integration.spec.js | 12 -------- .../appsec/rasp/resources/shi-app/index.js | 28 ------------------- 2 files changed, 40 deletions(-) diff --git a/packages/dd-trace/test/appsec/rasp/command_injection.integration.spec.js b/packages/dd-trace/test/appsec/rasp/command_injection.integration.spec.js index cf08f1d9404..d6fe4015202 100644 --- a/packages/dd-trace/test/appsec/rasp/command_injection.integration.spec.js +++ b/packages/dd-trace/test/appsec/rasp/command_injection.integration.spec.js @@ -129,17 +129,5 @@ describe('RASP - command_injection - integration', () => { '/cmdi/execFileSync/out-of-express-scope?command=cat /etc/passwd 1>&2 ; echo .', 4, 'exec' ) }) - - it('should abort spawnSync operation handled by express', async () => { - await testRequestBlocked( - '/cmdi/spawnSync?command=/sbin/reboot', 4, 'exec' - ) - }) - - it('should abort spawnSync operation outside of express scope', async () => { - await testRequestBlocked( - '/cmdi/spawnSync/out-of-express-scope?command=/sbin/reboot', 4, 'exec' - ) - }) }) }) diff --git a/packages/dd-trace/test/appsec/rasp/resources/shi-app/index.js b/packages/dd-trace/test/appsec/rasp/resources/shi-app/index.js index 72c1e4e0e44..133c57dfb2b 100644 --- a/packages/dd-trace/test/appsec/rasp/resources/shi-app/index.js +++ b/packages/dd-trace/test/appsec/rasp/resources/shi-app/index.js @@ -53,34 +53,6 @@ app.get('/cmdi/execFileSync/out-of-express-scope', async (req, res) => { }) }) -app.get('/cmdi/spawnSync', async (req, res) => { - const result = childProcess.spawnSync(req.query.command) - - if (result.error?.name === 'DatadogRaspAbortError') { - res.status(403).json({ - error: 'DatadogRaspAbortError' - }) - return - } - - res.end('OK') -}) - -app.get('/cmdi/spawnSync/out-of-express-scope', async (req, res) => { - process.nextTick(() => { - const result = childProcess.spawnSync(req.query.command) - - if (result.error?.name === 'DatadogRaspAbortError') { - res.status(403).json({ - error: 'DatadogRaspAbortError' - }) - return - } - - res.end('OK') - }) -}) - app.listen(port, () => { process.send({ port }) }) From 4a3d76657d7ced365268c60d5146e86833eafb19 Mon Sep 17 00:00:00 2001 From: ishabi Date: Thu, 19 Dec 2024 11:08:25 +0100 Subject: [PATCH 10/11] fix cmdi params --- packages/dd-trace/src/appsec/rasp/command_injection.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/dd-trace/src/appsec/rasp/command_injection.js b/packages/dd-trace/src/appsec/rasp/command_injection.js index 62546e2b6a6..4fe8312dac7 100644 --- a/packages/dd-trace/src/appsec/rasp/command_injection.js +++ b/packages/dd-trace/src/appsec/rasp/command_injection.js @@ -33,14 +33,13 @@ function analyzeCommandInjection ({ file, fileArgs, shell, abortController }) { const persistent = {} const raspRule = { type: RULE_TYPES.COMMAND_INJECTION } - const params = fileArgs ? [file, ...fileArgs] : file + const params = fileArgs ? [file, ...fileArgs] : [file] if (shell) { persistent[addresses.SHELL_COMMAND] = params raspRule.variant = 'shell' } else { - const commandParams = Array.isArray(params) ? params : [params] - persistent[addresses.EXEC_COMMAND] = commandParams + persistent[addresses.EXEC_COMMAND] = params raspRule.variant = 'exec' } From 02f9b1f979759da0c1fcc9edd504f663c31ad355 Mon Sep 17 00:00:00 2001 From: ishabi Date: Thu, 19 Dec 2024 11:22:52 +0100 Subject: [PATCH 11/11] Revert "fix cmdi params" This reverts commit 4a3d76657d7ced365268c60d5146e86833eafb19. --- packages/dd-trace/src/appsec/rasp/command_injection.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/dd-trace/src/appsec/rasp/command_injection.js b/packages/dd-trace/src/appsec/rasp/command_injection.js index 4fe8312dac7..62546e2b6a6 100644 --- a/packages/dd-trace/src/appsec/rasp/command_injection.js +++ b/packages/dd-trace/src/appsec/rasp/command_injection.js @@ -33,13 +33,14 @@ function analyzeCommandInjection ({ file, fileArgs, shell, abortController }) { const persistent = {} const raspRule = { type: RULE_TYPES.COMMAND_INJECTION } - const params = fileArgs ? [file, ...fileArgs] : [file] + const params = fileArgs ? [file, ...fileArgs] : file if (shell) { persistent[addresses.SHELL_COMMAND] = params raspRule.variant = 'shell' } else { - persistent[addresses.EXEC_COMMAND] = params + const commandParams = Array.isArray(params) ? params : [params] + persistent[addresses.EXEC_COMMAND] = commandParams raspRule.variant = 'exec' }