From 7ef5e136a4300798fcd51956ec558da9fe49f139 Mon Sep 17 00:00:00 2001 From: Igor Unanua Date: Tue, 30 May 2023 17:51:33 +0200 Subject: [PATCH 1/3] Unvalidated redirect analyzer --- .../src/appsec/iast/analyzers/analyzers.js | 1 + .../analyzers/insecure-cookie-analyzer.js | 3 +- .../iast/analyzers/sql-injection-analyzer.js | 4 +- .../unvalidated-redirect-analyzer.js | 34 +++ .../dd-trace/src/appsec/iast/path-line.js | 13 + .../evidence-redaction/sensitive-handler.js | 4 +- .../src/appsec/iast/vulnerabilities.js | 1 + .../resources/redirect-express-functions.js | 19 ++ ...d-redirect-analyzer.express.plugin.spec.js | 77 ++++++ .../unvalidated-redirect-analyzer.spec.js | 63 +++++ .../test/appsec/iast/path-line.spec.js | 46 ++++ packages/dd-trace/test/appsec/iast/utils.js | 6 +- .../resources/evidence-redaction-suite.json | 260 +++++------------- 13 files changed, 341 insertions(+), 190 deletions(-) create mode 100644 packages/dd-trace/src/appsec/iast/analyzers/unvalidated-redirect-analyzer.js create mode 100644 packages/dd-trace/test/appsec/iast/analyzers/resources/redirect-express-functions.js create mode 100644 packages/dd-trace/test/appsec/iast/analyzers/unvalidated-redirect-analyzer.express.plugin.spec.js create mode 100644 packages/dd-trace/test/appsec/iast/analyzers/unvalidated-redirect-analyzer.spec.js diff --git a/packages/dd-trace/src/appsec/iast/analyzers/analyzers.js b/packages/dd-trace/src/appsec/iast/analyzers/analyzers.js index 1ddba972d91..147eaead473 100644 --- a/packages/dd-trace/src/appsec/iast/analyzers/analyzers.js +++ b/packages/dd-trace/src/appsec/iast/analyzers/analyzers.js @@ -7,6 +7,7 @@ module.exports = { 'PATH_TRAVERSAL_ANALYZER': require('./path-traversal-analyzer'), 'SQL_INJECTION_ANALYZER': require('./sql-injection-analyzer'), 'SSRF': require('./ssrf-analyzer'), + 'UNVALIDATED_REDIRECT_ANALYZER': require('./unvalidated-redirect-analyzer'), 'WEAK_CIPHER_ANALYZER': require('./weak-cipher-analyzer'), 'WEAK_HASH_ANALYZER': require('./weak-hash-analyzer') } diff --git a/packages/dd-trace/src/appsec/iast/analyzers/insecure-cookie-analyzer.js b/packages/dd-trace/src/appsec/iast/analyzers/insecure-cookie-analyzer.js index 02bc01fd471..7630dc27990 100644 --- a/packages/dd-trace/src/appsec/iast/analyzers/insecure-cookie-analyzer.js +++ b/packages/dd-trace/src/appsec/iast/analyzers/insecure-cookie-analyzer.js @@ -2,8 +2,9 @@ const Analyzer = require('./vulnerability-analyzer') const { INSECURE_COOKIE } = require('../vulnerabilities') +const { getNodeModulesPaths } = require('../path-line') -const EXCLUDED_PATHS = ['node_modules/express/lib/response.js', 'node_modules\\express\\lib\\response.js'] +const EXCLUDED_PATHS = getNodeModulesPaths('express/lib/response.js') class InsecureCookieAnalyzer extends Analyzer { constructor () { diff --git a/packages/dd-trace/src/appsec/iast/analyzers/sql-injection-analyzer.js b/packages/dd-trace/src/appsec/iast/analyzers/sql-injection-analyzer.js index 006ca53b6ad..f769961c478 100644 --- a/packages/dd-trace/src/appsec/iast/analyzers/sql-injection-analyzer.js +++ b/packages/dd-trace/src/appsec/iast/analyzers/sql-injection-analyzer.js @@ -6,9 +6,9 @@ const { getRanges } = require('../taint-tracking/operations') const { storage } = require('../../../../../datadog-core') const { getIastContext } = require('../iast-context') const { addVulnerability } = require('../vulnerability-reporter') +const { getNodeModulesPaths } = require('../path-line') -const EXCLUDED_PATHS = ['node_modules/mysql2', 'node_modules/sequelize', 'node_modules\\mysql2', - 'node_modules\\sequelize'] +const EXCLUDED_PATHS = getNodeModulesPaths('mysql2', 'sequelize') class SqlInjectionAnalyzer extends InjectionAnalyzer { constructor () { diff --git a/packages/dd-trace/src/appsec/iast/analyzers/unvalidated-redirect-analyzer.js b/packages/dd-trace/src/appsec/iast/analyzers/unvalidated-redirect-analyzer.js new file mode 100644 index 00000000000..03132336512 --- /dev/null +++ b/packages/dd-trace/src/appsec/iast/analyzers/unvalidated-redirect-analyzer.js @@ -0,0 +1,34 @@ +'use strict' + +const InjectionAnalyzer = require('./injection-analyzer') +const { UNVALIDATED_REDIRECT } = require('../vulnerabilities') +const { getNodeModulesPaths } = require('../path-line') + +const EXCLUDED_PATHS = getNodeModulesPaths('express/lib/response.js') + +class UnvalidatedRedirectAnalyzer extends InjectionAnalyzer { + constructor () { + super(UNVALIDATED_REDIRECT) + + this.addSub('datadog:http:server:response:set-header:finish', ({ name, value }) => this.analyze(name, value)) + } + + // TODO: In case the location header value is tainted, this analyzer should check the ranges of the tainted. + // And do not report a vulnerability if source of the ranges (range.iinfo.type) are exclusively url or path params + // to avoid false positives. + analyze (name, value) { + if (!this.isLocationHeader(name) || typeof value !== 'string') return + + super.analyze(value) + } + + isLocationHeader (name) { + return name && name.trim().toLowerCase() === 'location' + } + + _getExcludedPaths () { + return EXCLUDED_PATHS + } +} + +module.exports = new UnvalidatedRedirectAnalyzer() diff --git a/packages/dd-trace/src/appsec/iast/path-line.js b/packages/dd-trace/src/appsec/iast/path-line.js index 4207034c7ef..1d08ac4d9dc 100644 --- a/packages/dd-trace/src/appsec/iast/path-line.js +++ b/packages/dd-trace/src/appsec/iast/path-line.js @@ -5,6 +5,7 @@ const process = require('process') const { calculateDDBasePath } = require('../../util') const pathLine = { getFirstNonDDPathAndLine, + getNodeModulesPaths, getFirstNonDDPathAndLineFromCallsites, // Exported only for test purposes calculateDDBasePath, // Exported only for test purposes ddBasePath: calculateDDBasePath(__dirname) // Only for test purposes @@ -83,4 +84,16 @@ function isExcluded (callsite, externallyExcludedPaths) { function getFirstNonDDPathAndLine (externallyExcludedPaths) { return getFirstNonDDPathAndLineFromCallsites(getCallSiteInfo(), externallyExcludedPaths) } + +function getNodeModulesPaths (...paths) { + const nodeModulesPaths = [] + + paths.forEach(p => { + const pathParts = p.split('/') + nodeModulesPaths.push(path.join('node_modules', ...pathParts)) + }) + + return nodeModulesPaths +} + module.exports = pathLine diff --git a/packages/dd-trace/src/appsec/iast/vulnerabilities-formatter/evidence-redaction/sensitive-handler.js b/packages/dd-trace/src/appsec/iast/vulnerabilities-formatter/evidence-redaction/sensitive-handler.js index fbeb3178e5c..22ae54aa68a 100644 --- a/packages/dd-trace/src/appsec/iast/vulnerabilities-formatter/evidence-redaction/sensitive-handler.js +++ b/packages/dd-trace/src/appsec/iast/vulnerabilities-formatter/evidence-redaction/sensitive-handler.js @@ -23,7 +23,9 @@ class SensitiveHandler { this._sensitiveAnalyzers.set(vulnerabilities.COMMAND_INJECTION, new CommandSensitiveAnalyzer()) this._sensitiveAnalyzers.set(vulnerabilities.LDAP_INJECTION, new LdapSensitiveAnalyzer()) this._sensitiveAnalyzers.set(vulnerabilities.SQL_INJECTION, new SqlSensitiveAnalyzer()) - this._sensitiveAnalyzers.set(vulnerabilities.SSRF, new UrlSensitiveAnalyzer()) + const urlSensitiveAnalyzer = new UrlSensitiveAnalyzer() + this._sensitiveAnalyzers.set(vulnerabilities.SSRF, urlSensitiveAnalyzer) + this._sensitiveAnalyzers.set(vulnerabilities.UNVALIDATED_REDIRECT, urlSensitiveAnalyzer) } isSensibleName (name) { diff --git a/packages/dd-trace/src/appsec/iast/vulnerabilities.js b/packages/dd-trace/src/appsec/iast/vulnerabilities.js index 4bf761a2d20..24ad3bb666c 100644 --- a/packages/dd-trace/src/appsec/iast/vulnerabilities.js +++ b/packages/dd-trace/src/appsec/iast/vulnerabilities.js @@ -5,6 +5,7 @@ module.exports = { PATH_TRAVERSAL: 'PATH_TRAVERSAL', SQL_INJECTION: 'SQL_INJECTION', SSRF: 'SSRF', + UNVALIDATED_REDIRECT: 'UNVALIDATED_REDIRECT', WEAK_CIPHER: 'WEAK_CIPHER', WEAK_HASH: 'WEAK_HASH' } diff --git a/packages/dd-trace/test/appsec/iast/analyzers/resources/redirect-express-functions.js b/packages/dd-trace/test/appsec/iast/analyzers/resources/redirect-express-functions.js new file mode 100644 index 00000000000..5666ba4e7b3 --- /dev/null +++ b/packages/dd-trace/test/appsec/iast/analyzers/resources/redirect-express-functions.js @@ -0,0 +1,19 @@ +'use strict' + +function insecureWithResHeaderMethod (headerName, value, res) { + res.header(headerName, value) +} + +function insecureWithResRedirectMethod (value, res) { + res.redirect(200, value) +} + +function insecureWithResLocationMethod (value, res) { + res.location(value) +} + +module.exports = { + insecureWithResHeaderMethod, + insecureWithResRedirectMethod, + insecureWithResLocationMethod +} diff --git a/packages/dd-trace/test/appsec/iast/analyzers/unvalidated-redirect-analyzer.express.plugin.spec.js b/packages/dd-trace/test/appsec/iast/analyzers/unvalidated-redirect-analyzer.express.plugin.spec.js new file mode 100644 index 00000000000..238af6d0210 --- /dev/null +++ b/packages/dd-trace/test/appsec/iast/analyzers/unvalidated-redirect-analyzer.express.plugin.spec.js @@ -0,0 +1,77 @@ +'use strict' + +const fs = require('fs') +const os = require('os') +const path = require('path') + +const { UNVALIDATED_REDIRECT } = require('../../../../src/appsec/iast/vulnerabilities') +const { prepareTestServerForIastInExpress } = require('../utils') +const { storage } = require('../../../../../datadog-core') +const iastContextFunctions = require('../../../../src/appsec/iast/iast-context') +const { newTaintedString } = require('../../../../src/appsec/iast/taint-tracking/operations') + +describe('Unvalidated Redirect vulnerability', () => { + let redirectFunctions + const redirectFunctionsFilename = 'redirect-express-functions.js' + const redirectFunctionsPath = path.join(os.tmpdir(), redirectFunctionsFilename) + + before(() => { + fs.copyFileSync(path.join(__dirname, 'resources', redirectFunctionsFilename), redirectFunctionsPath) + redirectFunctions = require(redirectFunctionsPath) + }) + + after(() => { + fs.unlinkSync(redirectFunctionsPath) + }) + + withVersions('express', 'express', version => { + prepareTestServerForIastInExpress('in express', version, + (testThatRequestHasVulnerability, testThatRequestHasNoVulnerability) => { + testThatRequestHasVulnerability((req, res) => { + const store = storage.getStore() + const iastCtx = iastContextFunctions.getIastContext(store) + const location = newTaintedString(iastCtx, 'https://app.com?id=tron', 'param', 'Request') + redirectFunctions.insecureWithResHeaderMethod('location', location, res) + }, UNVALIDATED_REDIRECT, { + occurrences: 1, + location: { + path: redirectFunctionsFilename, + line: 4 + } + }) + + testThatRequestHasVulnerability((req, res) => { + const store = storage.getStore() + const iastCtx = iastContextFunctions.getIastContext(store) + const location = newTaintedString(iastCtx, 'http://user@app.com/', 'param', 'Request') + redirectFunctions.insecureWithResRedirectMethod(location, res) + }, UNVALIDATED_REDIRECT, { + occurrences: 1, + location: { + path: redirectFunctionsFilename, + line: 8 + } + }) + + testThatRequestHasVulnerability((req, res) => { + const store = storage.getStore() + const iastCtx = iastContextFunctions.getIastContext(store) + const location = newTaintedString(iastCtx, 'http://user@app.com/', 'param', 'Request') + redirectFunctions.insecureWithResLocationMethod(location, res) + }, UNVALIDATED_REDIRECT, { + occurrences: 1, + location: { + path: redirectFunctionsFilename, + line: 12 + } + }) + + testThatRequestHasNoVulnerability((req, res) => { + const store = storage.getStore() + const iastCtx = iastContextFunctions.getIastContext(store) + const location = newTaintedString(iastCtx, 'http://user@app.com/', 'pathParam', 'Request') + res.header('X-test', location) + }, UNVALIDATED_REDIRECT) + }) + }) +}) diff --git a/packages/dd-trace/test/appsec/iast/analyzers/unvalidated-redirect-analyzer.spec.js b/packages/dd-trace/test/appsec/iast/analyzers/unvalidated-redirect-analyzer.spec.js new file mode 100644 index 00000000000..f27bed59974 --- /dev/null +++ b/packages/dd-trace/test/appsec/iast/analyzers/unvalidated-redirect-analyzer.spec.js @@ -0,0 +1,63 @@ +'use strict' + +const { expect } = require('chai') +const proxyquire = require('proxyquire') +const overheadController = require('../../../../src/appsec/iast/overhead-controller') + +describe('unvalidated-redirect-analyzer', () => { + const NOT_TAINTED_LOCATION = 'url.com' + const TAINTED_LOCATION = 'evil.com' + + const TaintTrackingMock = { + isTainted: (iastContext, string) => { + return string === TAINTED_LOCATION + } + } + + let report + beforeEach(() => { + report = sinon.stub(unvalidatedRedirectAnalyzer, '_report') + }) + + afterEach(sinon.restore) + + const InjectionAnalyzer = proxyquire('../../../../src/appsec/iast/analyzers/injection-analyzer', { + '../taint-tracking/operations': TaintTrackingMock + }) + const unvalidatedRedirectAnalyzer = + proxyquire('../../../../src/appsec/iast/analyzers/unvalidated-redirect-analyzer', { + './injection-analyzer': InjectionAnalyzer + }) + + it('should subscribe to set-header:finish channel', () => { + expect(unvalidatedRedirectAnalyzer._subscriptions).to.have.lengthOf(1) + expect(unvalidatedRedirectAnalyzer._subscriptions[0]._channel.name).to + .equals('datadog:http:server:response:set-header:finish') + }) + + it('should not report headers other than Location', () => { + unvalidatedRedirectAnalyzer.analyze('X-test', NOT_TAINTED_LOCATION) + + expect(report).to.not.have.been.called + }) + + it('should not report Location header with non string values', () => { + unvalidatedRedirectAnalyzer.analyze('X-test', [TAINTED_LOCATION]) + + expect(report).to.not.have.been.called + }) + + it('should not report Location header with not tainted string value', () => { + unvalidatedRedirectAnalyzer.analyze('Location', NOT_TAINTED_LOCATION) + + expect(report).to.not.have.been.called + }) + + it('should report Location header with tainted string value', () => { + sinon.stub(overheadController, 'hasQuota').returns(1) + + unvalidatedRedirectAnalyzer.analyze('Location', TAINTED_LOCATION) + + expect(report).to.be.called + }) +}) diff --git a/packages/dd-trace/test/appsec/iast/path-line.spec.js b/packages/dd-trace/test/appsec/iast/path-line.spec.js index 1721d6eeffb..aa016004cf6 100644 --- a/packages/dd-trace/test/appsec/iast/path-line.spec.js +++ b/packages/dd-trace/test/appsec/iast/path-line.spec.js @@ -1,6 +1,7 @@ const proxyquire = require('proxyquire') const path = require('path') const os = require('os') +const { expect } = require('chai') class CallSiteMock { constructor (fileName, lineNumber) { @@ -192,4 +193,49 @@ describe('path-line', function () { }) }) }) + + describe('getNodeModulesPaths', () => { + function getCallSiteInfo () { + const previousPrepareStackTrace = Error.prepareStackTrace + const previousStackTraceLimit = Error.stackTraceLimit + let callsiteList + Error.stackTraceLimit = 100 + Error.prepareStackTrace = function (_, callsites) { + callsiteList = callsites + } + const e = new Error() + e.stack + Error.prepareStackTrace = previousPrepareStackTrace + Error.stackTraceLimit = previousStackTraceLimit + return callsiteList + } + + it('should handle windows paths correctly', () => { + const basePath = pathLine.ddBasePath + pathLine.ddBasePath = path.join('test', 'base', 'path') + + const list = getCallSiteInfo() + const firstNonDDPath = pathLine.getFirstNonDDPathAndLineFromCallsites(list) + + const nodeModulesPaths = pathLine.getNodeModulesPaths(__filename) + expect(nodeModulesPaths[0]).to.eq(path.join('node_modules', process.cwd(), firstNonDDPath.path)) + + pathLine.ddBasePath = basePath + }) + + it('should convert / to \\ in windows platforms', () => { + const dirname = __dirname + const dirParts = dirname.split(path.sep) + const paths = pathLine.getNodeModulesPaths(dirParts.join('/')) + + expect(paths[0]).to.equals(path.join('node_modules', dirname)) + }) + + it('should return multiple paths', () => { + const paths = pathLine.getNodeModulesPaths('/this/is/a/path', '/another/path') + + expect(paths.length).to.equals(2) + expect(paths[0].startsWith('node_modules')).to.true + }) + }) }) diff --git a/packages/dd-trace/test/appsec/iast/utils.js b/packages/dd-trace/test/appsec/iast/utils.js index b0607e5a8bf..d3cce6a62af 100644 --- a/packages/dd-trace/test/appsec/iast/utils.js +++ b/packages/dd-trace/test/appsec/iast/utils.js @@ -123,8 +123,10 @@ function endResponse (res, appResult) { res.end() }) } else { - res.writeHead(200) - res.end() + if (!res.headersSent) { + res.writeHead(200) + res.end() + } } } diff --git a/packages/dd-trace/test/appsec/iast/vulnerability-formatter/resources/evidence-redaction-suite.json b/packages/dd-trace/test/appsec/iast/vulnerability-formatter/resources/evidence-redaction-suite.json index 49a62fb7009..18c9186d5cc 100644 --- a/packages/dd-trace/test/appsec/iast/vulnerability-formatter/resources/evidence-redaction-suite.json +++ b/packages/dd-trace/test/appsec/iast/vulnerability-formatter/resources/evidence-redaction-suite.json @@ -6,44 +6,44 @@ "description": "Source with sensitive name \"$1\"", "parameters": { "$1": [ - "password", - "passwd", - "pwd", - "pass", - "pass_phrase", - "passPhrase", - "secret", - "api_key", - "apikey", - "secret_key", - "secretKey", "access_key_id", "accessKeyId", - "secret_access_key", - "secretAccessKey", - "private_key", - "privateKey", - "public_key", - "publicKey", - "token", - "api_token", + "apikey", + "api_key", "apiToken", - "expiration_token", - "expirationToken", - "refresh_token", - "refreshToken", - "consumer_id", + "api_token", + "auth", + "authentication", + "authorization", "consumerId", - "consumer_key", + "consumer_id", "consumerKey", - "consumer_secret", + "consumer_key", "consumerSecret", + "consumer_secret", + "expirationToken", + "expiration_token", + "pass", + "passwd", + "password", + "passPhrase", + "pass_phrase", + "private_key", + "privateKey", + "publicKey", + "public_key", + "pwd", + "refreshToken", + "refresh_token", + "secret", + "secretAccessKey", + "secret_access_key", + "secretKey", + "secret_key", "sign", "signature", "signed", - "auth", - "authorization", - "authentication" + "token" ] }, "input": [ @@ -1594,6 +1594,13 @@ } ], "expected": { + "sources": [ + { + "origin": "http.request.parameter", + "name": "oid", + "value": "2.4.6.8.10" + } + ], "vulnerabilities": [ { "type": "LDAP_INJECTION", @@ -2030,10 +2037,16 @@ }, { "type": "VULNERABILITIES", - "description": "SSRF without authority or query params", + "description": "$1 without authority or query params", + "parameters": { + "$1": [ + "SSRF", + "UNVALIDATED_REDIRECT" + ] + }, "input": [ { - "type": "SSRF", + "type": "$1", "evidence": { "value": "http://datadoghq.com", "ranges": [ @@ -2060,7 +2073,7 @@ ], "vulnerabilities": [ { - "type": "SSRF", + "type": "$1", "evidence": { "valueParts": [ { @@ -2078,9 +2091,13 @@ }, { "type": "VULNERABILITIES", - "description": "SSRF with authority $1", + "description": "$1 with authority $2", "parameters": { "$1": [ + "SSRF", + "UNVALIDATED_REDIRECT" + ], + "$2": [ "user", ":", "user:", @@ -2090,9 +2107,9 @@ }, "input": [ { - "type": "SSRF", + "type": "$1", "evidence": { - "value": "http://$1@datadoghq.com", + "value": "http://$2@datadoghq.com", "ranges": [ { "start": 0, @@ -2117,7 +2134,7 @@ ], "vulnerabilities": [ { - "type": "SSRF", + "type": "$1", "evidence": { "valueParts": [ { @@ -2141,18 +2158,22 @@ }, { "type": "VULNERABILITIES", - "description": "SSRF with query parameters or fragment", + "description": "$1 with query parameters or fragment", "parameters": { "$1": [ + "SSRF", + "UNVALIDATED_REDIRECT" + ], + "$2": [ "?", "#" ] }, "input": [ { - "type": "SSRF", + "type": "$1", "evidence": { - "value": "http://datadoghq.com$1first_name=john&last_name=doe", + "value": "http://datadoghq.com$2first_name=john&last_name=doe", "ranges": [ { "start": 7, @@ -2191,7 +2212,7 @@ ], "vulnerabilities": [ { - "type": "SSRF", + "type": "$1", "evidence": { "valueParts": [ { @@ -2202,7 +2223,7 @@ "value": "datadoghq.com" }, { - "value": "$1first_name=" + "value": "$2first_name=" }, { "source": 1, @@ -2222,10 +2243,16 @@ }, { "type": "VULNERABILITIES", - "description": "SSRF authority query and fragment", + "description": "$1 authority query and fragment", + "parameters": { + "$1": [ + "SSRF", + "UNVALIDATED_REDIRECT" + ] + }, "input": [ { - "type": "SSRF", + "type": "$1", "evidence": { "value": "https://user:password@datadoghq.com:443/api/v1/test/123/?param1=pone¶m2=ptwo#fragment1=fone&fragment2=ftwo", "ranges": [ @@ -2252,7 +2279,7 @@ ], "vulnerabilities": [ { - "type": "SSRF", + "type": "$1", "evidence": { "valueParts": [ { @@ -2300,156 +2327,21 @@ }, { "type": "VULNERABILITIES", - "description": "SSRF without authority or query params", + "description": "Weak randomness should not be redacted", "input": [ { - "type": "SSRF", + "type": "WEAK_RANDOMNESS", "evidence": { - "value": "http://datadoghq.com", - "ranges": [ - { "start" : 7, "end" : 20, "iinfo": { "type": "http.request.parameter", "parameterName": "host", "parameterValue": "datadoghq.com" } } - ] + "value": "java.util.Math" } } ], "expected": { - "sources": [ - { "origin": "http.request.parameter", "name": "host", "value": "datadoghq.com" } - ], "vulnerabilities": [ { - "type": "SSRF", + "type": "WEAK_RANDOMNESS", "evidence": { - "valueParts": [ - { "value": "http://" }, - { "source": 0, "value": "datadoghq.com"} - ] - } - } - ] - } - }, - { - "type": "VULNERABILITIES", - "description": "SSRF with authority $1", - "parameters": { - "$1": [ - "user", - ":", - "user:", - ":password", - "user:password" - ] - }, - "input": [ - { - "type": "SSRF", - "evidence": { - "value": "http://$1@datadoghq.com", - "ranges": [ - { "start" : 0, "end" : 4, "iinfo": { "type": "http.request.parameter", "parameterName": "protocol", "parameterValue": "http" } } - ] - } - } - ], - "expected": { - "sources": [ - { "origin": "http.request.parameter", "name": "protocol", "value": "http" } - ], - "vulnerabilities": [ - { - "type": "SSRF", - "evidence": { - "valueParts": [ - { "source": 0, "value": "http" }, - { "value": "://" }, - { "redacted": true }, - { "value": "@datadoghq.com" } - ] - } - } - ] - } - }, - { - "type": "VULNERABILITIES", - "description": "SSRF with query parameters or fragment", - "parameters": { - "$1": [ - "?", - "#" - ] - }, - "input": [ - { - "type": "SSRF", - "evidence": { - "value": "http://datadoghq.com$1first_name=john&last_name=doe", - "ranges": [ - { "start" : 7, "end" : 20, "iinfo": { "type": "http.request.parameter", "parameterName": "host", "parameterValue": "datadoghq.com" } }, - { "start" : 32, "end" : 36, "iinfo": { "type": "http.request.parameter", "parameterName": "fist_name", "parameterValue": "john" } } - ] - } - } - ], - "expected": { - "sources": [ - { "origin": "http.request.parameter", "name": "host", "value": "datadoghq.com" }, - { "origin": "http.request.parameter", "name": "fist_name", "redacted": true } - ], - "vulnerabilities": [ - { - "type": "SSRF", - "evidence": { - "valueParts": [ - { "value": "http://" }, - { "source": 0, "value": "datadoghq.com" }, - { "value": "$1first_name=" }, - { "source": 1, "redacted": true }, - { "value": "&last_name=" }, - { "redacted": true } - ] - } - } - ] - } - }, - { - "type": "VULNERABILITIES", - "description": "SSRF authority query and fragment", - "input": [ - { - "type": "SSRF", - "evidence": { - "value": "https://user:password@datadoghq.com:443/api/v1/test/123/?param1=pone¶m2=ptwo#fragment1=fone&fragment2=ftwo", - "ranges": [ - { "start" : 22, "end" : 35, "iinfo": { "type": "http.request.parameter", "parameterName": "host", "parameterValue": "datadoghq.com" } } - ] - } - } - ], - "expected": { - "sources": [ - { "origin": "http.request.parameter", "name": "host", "value": "datadoghq.com" } - ], - "vulnerabilities": [ - { - "type": "SSRF", - "evidence": { - "valueParts": [ - { "value": "https://" }, - { "redacted": true }, - { "value": "@" }, - { "source": 0, "value": "datadoghq.com" }, - { "value": ":443/api/v1/test/123/?param1=" }, - { "redacted": true }, - { "value": "¶m2=" }, - { "redacted": true }, - { "value": "#fragment1=" }, - { "redacted": true }, - { "value": "&fragment2=" }, - { "redacted": true } - ] + "value": "java.util.Math" } } ] From 9dcb293b61c4d10c19cc371603c20e2fd1debfeb Mon Sep 17 00:00:00 2001 From: Igor Unanua Date: Fri, 9 Jun 2023 10:57:08 +0200 Subject: [PATCH 2/3] Ignore tainteds from Referer header --- .../unvalidated-redirect-analyzer.js | 14 +++++ .../iast/taint-tracking/origin-types.js | 3 +- .../unvalidated-redirect-analyzer.spec.js | 55 ++++++++++++++++++- 3 files changed, 70 insertions(+), 2 deletions(-) diff --git a/packages/dd-trace/src/appsec/iast/analyzers/unvalidated-redirect-analyzer.js b/packages/dd-trace/src/appsec/iast/analyzers/unvalidated-redirect-analyzer.js index 03132336512..f05144543f3 100644 --- a/packages/dd-trace/src/appsec/iast/analyzers/unvalidated-redirect-analyzer.js +++ b/packages/dd-trace/src/appsec/iast/analyzers/unvalidated-redirect-analyzer.js @@ -3,6 +3,8 @@ const InjectionAnalyzer = require('./injection-analyzer') const { UNVALIDATED_REDIRECT } = require('../vulnerabilities') const { getNodeModulesPaths } = require('../path-line') +const { getRanges } = require('../taint-tracking/operations') +const { HTTP_REQUEST_HEADER_VALUE } = require('../taint-tracking/origin-types') const EXCLUDED_PATHS = getNodeModulesPaths('express/lib/response.js') @@ -26,6 +28,18 @@ class UnvalidatedRedirectAnalyzer extends InjectionAnalyzer { return name && name.trim().toLowerCase() === 'location' } + _isVulnerable (value, iastContext) { + if (!value) return false + + const ranges = getRanges(iastContext, value) + return !this._isRefererHeader(ranges) + } + + _isRefererHeader (ranges) { + return ranges && ranges.every(range => range.iinfo.type === HTTP_REQUEST_HEADER_VALUE && + range.iinfo.parameterName && range.iinfo.parameterName.toLowerCase() === 'referer') + } + _getExcludedPaths () { return EXCLUDED_PATHS } diff --git a/packages/dd-trace/src/appsec/iast/taint-tracking/origin-types.js b/packages/dd-trace/src/appsec/iast/taint-tracking/origin-types.js index 4aff7550bee..45e14bcb313 100644 --- a/packages/dd-trace/src/appsec/iast/taint-tracking/origin-types.js +++ b/packages/dd-trace/src/appsec/iast/taint-tracking/origin-types.js @@ -2,5 +2,6 @@ module.exports = { HTTP_REQUEST_BODY: 'http.request.body', - HTTP_REQUEST_PARAMETER: 'http.request.parameter' + HTTP_REQUEST_PARAMETER: 'http.request.parameter', + HTTP_REQUEST_HEADER_VALUE: 'http.request.header' } diff --git a/packages/dd-trace/test/appsec/iast/analyzers/unvalidated-redirect-analyzer.spec.js b/packages/dd-trace/test/appsec/iast/analyzers/unvalidated-redirect-analyzer.spec.js index f27bed59974..c18f8cbbee5 100644 --- a/packages/dd-trace/test/appsec/iast/analyzers/unvalidated-redirect-analyzer.spec.js +++ b/packages/dd-trace/test/appsec/iast/analyzers/unvalidated-redirect-analyzer.spec.js @@ -3,14 +3,50 @@ const { expect } = require('chai') const proxyquire = require('proxyquire') const overheadController = require('../../../../src/appsec/iast/overhead-controller') +const { HTTP_REQUEST_HEADER_VALUE, HTTP_REQUEST_PARAMETER } = + require('../../../../src/appsec/iast/taint-tracking/origin-types') describe('unvalidated-redirect-analyzer', () => { const NOT_TAINTED_LOCATION = 'url.com' const TAINTED_LOCATION = 'evil.com' + const TAINTED_HEADER_REFERER_ONLY = 'TAINTED_HEADER_REFERER_ONLY' + const TAINTED_HEADER_REFERER_AMONG_OTHERS = 'TAINTED_HEADER_REFERER_ONLY_AMONG_OTHERS' + + const REFERER_RANGE = { + iinfo: { + type: HTTP_REQUEST_HEADER_VALUE, + parameterName: 'Referer' + } + } + const PARAMETER1_RANGE = { + iinfo: { + type: HTTP_REQUEST_PARAMETER, + parameterName: 'param1' + } + } + const PARAMETER2_RANGE = { + iinfo: { + type: HTTP_REQUEST_PARAMETER, + parameterName: 'param2' + } + } + const TaintTrackingMock = { isTainted: (iastContext, string) => { return string === TAINTED_LOCATION + }, + + getRanges: (iastContext, value) => { + if (value === NOT_TAINTED_LOCATION) return [] + + if (value === TAINTED_HEADER_REFERER_ONLY) { + return [REFERER_RANGE] + } else if (value === TAINTED_HEADER_REFERER_AMONG_OTHERS) { + return [REFERER_RANGE, PARAMETER1_RANGE] + } else { + return [PARAMETER1_RANGE, PARAMETER2_RANGE] + } } } @@ -26,7 +62,8 @@ describe('unvalidated-redirect-analyzer', () => { }) const unvalidatedRedirectAnalyzer = proxyquire('../../../../src/appsec/iast/analyzers/unvalidated-redirect-analyzer', { - './injection-analyzer': InjectionAnalyzer + './injection-analyzer': InjectionAnalyzer, + '../taint-tracking/operations': TaintTrackingMock }) it('should subscribe to set-header:finish channel', () => { @@ -60,4 +97,20 @@ describe('unvalidated-redirect-analyzer', () => { expect(report).to.be.called }) + + it('should not report if tainted origin is referer header exclusively', () => { + sinon.stub(overheadController, 'hasQuota').returns(1) + + unvalidatedRedirectAnalyzer.analyze('Location', TAINTED_HEADER_REFERER_ONLY) + + expect(report).to.not.be.called + }) + + it('should report if tainted origin contains referer header among others', () => { + sinon.stub(overheadController, 'hasQuota').returns(1) + + unvalidatedRedirectAnalyzer.analyze('Location', TAINTED_HEADER_REFERER_AMONG_OTHERS) + + expect(report).to.be.called + }) }) From 262f78687dca23ab1fc03a8a62746bed8e1c7696 Mon Sep 17 00:00:00 2001 From: Igor Unanua Date: Fri, 9 Jun 2023 12:03:18 +0200 Subject: [PATCH 3/3] Move res.end() --- packages/dd-trace/test/appsec/iast/utils.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/dd-trace/test/appsec/iast/utils.js b/packages/dd-trace/test/appsec/iast/utils.js index d3cce6a62af..c3f5b652e3a 100644 --- a/packages/dd-trace/test/appsec/iast/utils.js +++ b/packages/dd-trace/test/appsec/iast/utils.js @@ -119,14 +119,16 @@ function beforeEachIastTest () { function endResponse (res, appResult) { if (appResult && typeof appResult.then === 'function') { appResult.then(() => { - res.writeHead(200) + if (!res.headersSent) { + res.writeHead(200) + } res.end() }) } else { if (!res.headersSent) { res.writeHead(200) - res.end() } + res.end() } }