From 773f80cd6e9c3d3cf0cdc6daba8c78dc2b6c9f86 Mon Sep 17 00:00:00 2001 From: ishabi Date: Fri, 15 Nov 2024 14:56:09 +0100 Subject: [PATCH] Template injection vulnerability detection in handlebars and pug (#4827) * Template injection vulnerability detection in handlebars * template injection vulnerability detection in pug * fix lint and naming issues * create separate job for template injection * add support to registerPartial function * add tests for pug render function --- .github/workflows/appsec.yml | 14 +++ .../src/handlebars.js | 40 +++++++ .../src/helpers/hooks.js | 2 + packages/datadog-instrumentations/src/pug.js | 23 ++++ .../src/appsec/iast/analyzers/analyzers.js | 1 + .../analyzers/template-injection-analyzer.js | 18 ++++ ...tainted-range-based-sensitive-analyzer.js} | 0 .../evidence-redaction/sensitive-handler.js | 5 +- .../src/appsec/iast/vulnerabilities.js | 1 + ...jection-analyzer.handlebars.plugin.spec.js | 79 ++++++++++++++ ...late-injection-analyzer.pug.plugin.spec.js | 100 ++++++++++++++++++ .../vulnerability-formatter/index.spec.js | 3 +- .../resources/evidence-redaction-suite.json | 21 ++-- 13 files changed, 297 insertions(+), 10 deletions(-) create mode 100644 packages/datadog-instrumentations/src/handlebars.js create mode 100644 packages/datadog-instrumentations/src/pug.js create mode 100644 packages/dd-trace/src/appsec/iast/analyzers/template-injection-analyzer.js rename packages/dd-trace/src/appsec/iast/vulnerabilities-formatter/evidence-redaction/sensitive-analyzers/{code-injection-sensitive-analyzer.js => tainted-range-based-sensitive-analyzer.js} (100%) create mode 100644 packages/dd-trace/test/appsec/iast/analyzers/template-injection-analyzer.handlebars.plugin.spec.js create mode 100644 packages/dd-trace/test/appsec/iast/analyzers/template-injection-analyzer.pug.plugin.spec.js diff --git a/.github/workflows/appsec.yml b/.github/workflows/appsec.yml index 5c3ad393c87..ff7b6f88d74 100644 --- a/.github/workflows/appsec.yml +++ b/.github/workflows/appsec.yml @@ -270,3 +270,17 @@ jobs: - uses: ./.github/actions/node/latest - run: yarn test:appsec:plugins:ci - uses: codecov/codecov-action@v3 + + template: + runs-on: ubuntu-latest + env: + PLUGINS: handlebars|pug + steps: + - uses: actions/checkout@v4 + - uses: ./.github/actions/node/setup + - uses: ./.github/actions/install + - uses: ./.github/actions/node/oldest + - run: yarn test:appsec:plugins:ci + - uses: ./.github/actions/node/latest + - run: yarn test:appsec:plugins:ci + - uses: codecov/codecov-action@v3 diff --git a/packages/datadog-instrumentations/src/handlebars.js b/packages/datadog-instrumentations/src/handlebars.js new file mode 100644 index 00000000000..333889db3c6 --- /dev/null +++ b/packages/datadog-instrumentations/src/handlebars.js @@ -0,0 +1,40 @@ +'use strict' + +const shimmer = require('../../datadog-shimmer') +const { channel, addHook } = require('./helpers/instrument') + +const handlebarsCompileCh = channel('datadog:handlebars:compile:start') +const handlebarsRegisterPartialCh = channel('datadog:handlebars:register-partial:start') + +function wrapCompile (compile) { + return function wrappedCompile (source) { + if (handlebarsCompileCh.hasSubscribers) { + handlebarsCompileCh.publish({ source }) + } + + return compile.apply(this, arguments) + } +} + +function wrapRegisterPartial (registerPartial) { + return function wrappedRegisterPartial (name, partial) { + if (handlebarsRegisterPartialCh.hasSubscribers) { + handlebarsRegisterPartialCh.publish({ partial }) + } + + return registerPartial.apply(this, arguments) + } +} + +addHook({ name: 'handlebars', file: 'dist/cjs/handlebars/compiler/compiler.js', versions: ['>=4.0.0'] }, compiler => { + shimmer.wrap(compiler, 'compile', wrapCompile) + shimmer.wrap(compiler, 'precompile', wrapCompile) + + return compiler +}) + +addHook({ name: 'handlebars', file: 'dist/cjs/handlebars/base.js', versions: ['>=4.0.0'] }, base => { + shimmer.wrap(base.HandlebarsEnvironment.prototype, 'registerPartial', wrapRegisterPartial) + + return base +}) diff --git a/packages/datadog-instrumentations/src/helpers/hooks.js b/packages/datadog-instrumentations/src/helpers/hooks.js index 21bdf21298e..948d3c5fe28 100644 --- a/packages/datadog-instrumentations/src/helpers/hooks.js +++ b/packages/datadog-instrumentations/src/helpers/hooks.js @@ -51,6 +51,7 @@ module.exports = { 'generic-pool': () => require('../generic-pool'), graphql: () => require('../graphql'), grpc: () => require('../grpc'), + handlebars: () => require('../handlebars'), hapi: () => require('../hapi'), http: () => require('../http'), http2: () => require('../http2'), @@ -105,6 +106,7 @@ module.exports = { 'promise-js': () => require('../promise-js'), promise: () => require('../promise'), protobufjs: () => require('../protobufjs'), + pug: () => require('../pug'), q: () => require('../q'), qs: () => require('../qs'), redis: () => require('../redis'), diff --git a/packages/datadog-instrumentations/src/pug.js b/packages/datadog-instrumentations/src/pug.js new file mode 100644 index 00000000000..4322ed265cb --- /dev/null +++ b/packages/datadog-instrumentations/src/pug.js @@ -0,0 +1,23 @@ +'use strict' + +const shimmer = require('../../datadog-shimmer') +const { channel, addHook } = require('./helpers/instrument') + +const pugCompileCh = channel('datadog:pug:compile:start') + +function wrapCompile (compile) { + return function wrappedCompile (source) { + if (pugCompileCh.hasSubscribers) { + pugCompileCh.publish({ source }) + } + + return compile.apply(this, arguments) + } +} + +addHook({ name: 'pug', versions: ['>=2.0.4'] }, compiler => { + shimmer.wrap(compiler, 'compile', wrapCompile) + shimmer.wrap(compiler, 'compileClientWithDependenciesTracked', wrapCompile) + + return compiler +}) diff --git a/packages/dd-trace/src/appsec/iast/analyzers/analyzers.js b/packages/dd-trace/src/appsec/iast/analyzers/analyzers.js index 36f6036cf54..c1608ae1261 100644 --- a/packages/dd-trace/src/appsec/iast/analyzers/analyzers.js +++ b/packages/dd-trace/src/appsec/iast/analyzers/analyzers.js @@ -15,6 +15,7 @@ module.exports = { PATH_TRAVERSAL_ANALYZER: require('./path-traversal-analyzer'), SQL_INJECTION_ANALYZER: require('./sql-injection-analyzer'), SSRF: require('./ssrf-analyzer'), + TEMPLATE_INJECTION_ANALYZER: require('./template-injection-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/template-injection-analyzer.js b/packages/dd-trace/src/appsec/iast/analyzers/template-injection-analyzer.js new file mode 100644 index 00000000000..1be35933223 --- /dev/null +++ b/packages/dd-trace/src/appsec/iast/analyzers/template-injection-analyzer.js @@ -0,0 +1,18 @@ +'use strict' + +const InjectionAnalyzer = require('./injection-analyzer') +const { TEMPLATE_INJECTION } = require('../vulnerabilities') + +class TemplateInjectionAnalyzer extends InjectionAnalyzer { + constructor () { + super(TEMPLATE_INJECTION) + } + + onConfigure () { + this.addSub('datadog:handlebars:compile:start', ({ source }) => this.analyze(source)) + this.addSub('datadog:handlebars:register-partial:start', ({ partial }) => this.analyze(partial)) + this.addSub('datadog:pug:compile:start', ({ source }) => this.analyze(source)) + } +} + +module.exports = new TemplateInjectionAnalyzer() diff --git a/packages/dd-trace/src/appsec/iast/vulnerabilities-formatter/evidence-redaction/sensitive-analyzers/code-injection-sensitive-analyzer.js b/packages/dd-trace/src/appsec/iast/vulnerabilities-formatter/evidence-redaction/sensitive-analyzers/tainted-range-based-sensitive-analyzer.js similarity index 100% rename from packages/dd-trace/src/appsec/iast/vulnerabilities-formatter/evidence-redaction/sensitive-analyzers/code-injection-sensitive-analyzer.js rename to packages/dd-trace/src/appsec/iast/vulnerabilities-formatter/evidence-redaction/sensitive-analyzers/tainted-range-based-sensitive-analyzer.js 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 39117dc5a34..13716aea1db 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 @@ -5,13 +5,13 @@ const vulnerabilities = require('../../vulnerabilities') const { contains, intersects, remove } = require('./range-utils') -const codeInjectionSensitiveAnalyzer = require('./sensitive-analyzers/code-injection-sensitive-analyzer') const commandSensitiveAnalyzer = require('./sensitive-analyzers/command-sensitive-analyzer') const hardcodedPasswordAnalyzer = require('./sensitive-analyzers/hardcoded-password-analyzer') const headerSensitiveAnalyzer = require('./sensitive-analyzers/header-sensitive-analyzer') const jsonSensitiveAnalyzer = require('./sensitive-analyzers/json-sensitive-analyzer') const ldapSensitiveAnalyzer = require('./sensitive-analyzers/ldap-sensitive-analyzer') const sqlSensitiveAnalyzer = require('./sensitive-analyzers/sql-sensitive-analyzer') +const taintedRangeBasedSensitiveAnalyzer = require('./sensitive-analyzers/tainted-range-based-sensitive-analyzer') const urlSensitiveAnalyzer = require('./sensitive-analyzers/url-sensitive-analyzer') const { DEFAULT_IAST_REDACTION_NAME_PATTERN, DEFAULT_IAST_REDACTION_VALUE_PATTERN } = require('./sensitive-regex') @@ -24,7 +24,8 @@ class SensitiveHandler { this._valuePattern = new RegExp(DEFAULT_IAST_REDACTION_VALUE_PATTERN, 'gmi') this._sensitiveAnalyzers = new Map() - this._sensitiveAnalyzers.set(vulnerabilities.CODE_INJECTION, codeInjectionSensitiveAnalyzer) + this._sensitiveAnalyzers.set(vulnerabilities.CODE_INJECTION, taintedRangeBasedSensitiveAnalyzer) + this._sensitiveAnalyzers.set(vulnerabilities.TEMPLATE_INJECTION, taintedRangeBasedSensitiveAnalyzer) this._sensitiveAnalyzers.set(vulnerabilities.COMMAND_INJECTION, commandSensitiveAnalyzer) this._sensitiveAnalyzers.set(vulnerabilities.NOSQL_MONGODB_INJECTION, jsonSensitiveAnalyzer) this._sensitiveAnalyzers.set(vulnerabilities.LDAP_INJECTION, ldapSensitiveAnalyzer) diff --git a/packages/dd-trace/src/appsec/iast/vulnerabilities.js b/packages/dd-trace/src/appsec/iast/vulnerabilities.js index 790ec6c5db9..90287c27d91 100644 --- a/packages/dd-trace/src/appsec/iast/vulnerabilities.js +++ b/packages/dd-trace/src/appsec/iast/vulnerabilities.js @@ -13,6 +13,7 @@ module.exports = { PATH_TRAVERSAL: 'PATH_TRAVERSAL', SQL_INJECTION: 'SQL_INJECTION', SSRF: 'SSRF', + TEMPLATE_INJECTION: 'TEMPLATE_INJECTION', UNVALIDATED_REDIRECT: 'UNVALIDATED_REDIRECT', WEAK_CIPHER: 'WEAK_CIPHER', WEAK_HASH: 'WEAK_HASH', diff --git a/packages/dd-trace/test/appsec/iast/analyzers/template-injection-analyzer.handlebars.plugin.spec.js b/packages/dd-trace/test/appsec/iast/analyzers/template-injection-analyzer.handlebars.plugin.spec.js new file mode 100644 index 00000000000..4152f4ab6e9 --- /dev/null +++ b/packages/dd-trace/test/appsec/iast/analyzers/template-injection-analyzer.handlebars.plugin.spec.js @@ -0,0 +1,79 @@ +'use strict' + +const { prepareTestServerForIast } = 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('template-injection-analyzer with handlebars', () => { + withVersions('handlebars', 'handlebars', version => { + let source + before(() => { + source = '

{{name}}

' + }) + + describe('compile', () => { + prepareTestServerForIast('template injection analyzer', + (testThatRequestHasVulnerability, testThatRequestHasNoVulnerability) => { + let lib + beforeEach(() => { + lib = require(`../../../../../../versions/handlebars@${version}`).get() + }) + + testThatRequestHasVulnerability(() => { + const store = storage.getStore() + const iastContext = iastContextFunctions.getIastContext(store) + const template = newTaintedString(iastContext, source, 'param', 'Request') + lib.compile(template) + }, 'TEMPLATE_INJECTION') + + testThatRequestHasNoVulnerability(() => { + lib.compile(source) + }, 'TEMPLATE_INJECTION') + }) + }) + + describe('precompile', () => { + prepareTestServerForIast('template injection analyzer', + (testThatRequestHasVulnerability, testThatRequestHasNoVulnerability) => { + let lib + beforeEach(() => { + lib = require(`../../../../../../versions/handlebars@${version}`).get() + }) + + testThatRequestHasVulnerability(() => { + const store = storage.getStore() + const iastContext = iastContextFunctions.getIastContext(store) + const template = newTaintedString(iastContext, source, 'param', 'Request') + lib.precompile(template) + }, 'TEMPLATE_INJECTION') + + testThatRequestHasNoVulnerability(() => { + lib.precompile(source) + }, 'TEMPLATE_INJECTION') + }) + }) + + describe('registerPartial', () => { + prepareTestServerForIast('template injection analyzer', + (testThatRequestHasVulnerability, testThatRequestHasNoVulnerability) => { + let lib + beforeEach(() => { + lib = require(`../../../../../../versions/handlebars@${version}`).get() + }) + + testThatRequestHasVulnerability(() => { + const store = storage.getStore() + const iastContext = iastContextFunctions.getIastContext(store) + const partial = newTaintedString(iastContext, source, 'param', 'Request') + + lib.registerPartial('vulnerablePartial', partial) + }, 'TEMPLATE_INJECTION') + + testThatRequestHasNoVulnerability(() => { + lib.registerPartial('vulnerablePartial', source) + }, 'TEMPLATE_INJECTION') + }) + }) + }) +}) diff --git a/packages/dd-trace/test/appsec/iast/analyzers/template-injection-analyzer.pug.plugin.spec.js b/packages/dd-trace/test/appsec/iast/analyzers/template-injection-analyzer.pug.plugin.spec.js new file mode 100644 index 00000000000..412da3a62f0 --- /dev/null +++ b/packages/dd-trace/test/appsec/iast/analyzers/template-injection-analyzer.pug.plugin.spec.js @@ -0,0 +1,100 @@ +'use strict' + +const { prepareTestServerForIast } = 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('template-injection-analyzer with pug', () => { + withVersions('pug', 'pug', version => { + let source + before(() => { + source = 'string of pug' + }) + + describe('compile', () => { + prepareTestServerForIast('template injection analyzer', + (testThatRequestHasVulnerability, testThatRequestHasNoVulnerability) => { + let lib + beforeEach(() => { + lib = require(`../../../../../../versions/pug@${version}`).get() + }) + + testThatRequestHasVulnerability(() => { + const store = storage.getStore() + const iastContext = iastContextFunctions.getIastContext(store) + const template = newTaintedString(iastContext, source, 'param', 'Request') + lib.compile(template) + }, 'TEMPLATE_INJECTION') + + testThatRequestHasNoVulnerability(() => { + const template = lib.compile(source) + template() + }, 'TEMPLATE_INJECTION') + }) + }) + + describe('compileClient', () => { + prepareTestServerForIast('template injection analyzer', + (testThatRequestHasVulnerability, testThatRequestHasNoVulnerability) => { + let lib + beforeEach(() => { + lib = require(`../../../../../../versions/pug@${version}`).get() + }) + + testThatRequestHasVulnerability(() => { + const store = storage.getStore() + const iastContext = iastContextFunctions.getIastContext(store) + const template = newTaintedString(iastContext, source, 'param', 'Request') + lib.compileClient(template) + }, 'TEMPLATE_INJECTION') + + testThatRequestHasNoVulnerability(() => { + lib.compileClient(source) + }, 'TEMPLATE_INJECTION') + }) + }) + + describe('compileClientWithDependenciesTracked', () => { + prepareTestServerForIast('template injection analyzer', + (testThatRequestHasVulnerability, testThatRequestHasNoVulnerability) => { + let lib + beforeEach(() => { + lib = require(`../../../../../../versions/pug@${version}`).get() + }) + + testThatRequestHasVulnerability(() => { + const store = storage.getStore() + const iastContext = iastContextFunctions.getIastContext(store) + const template = newTaintedString(iastContext, source, 'param', 'Request') + lib.compileClientWithDependenciesTracked(template, {}) + }, 'TEMPLATE_INJECTION') + + testThatRequestHasNoVulnerability(() => { + lib.compileClient(source) + }, 'TEMPLATE_INJECTION') + }) + }) + + describe('render', () => { + prepareTestServerForIast('template injection analyzer', + (testThatRequestHasVulnerability, testThatRequestHasNoVulnerability) => { + let lib + beforeEach(() => { + lib = require(`../../../../../../versions/pug@${version}`).get() + }) + + testThatRequestHasVulnerability(() => { + const store = storage.getStore() + const iastContext = iastContextFunctions.getIastContext(store) + const str = newTaintedString(iastContext, source, 'param', 'Request') + lib.render(str) + }, 'TEMPLATE_INJECTION') + + testThatRequestHasNoVulnerability(() => { + lib.render(source) + }, 'TEMPLATE_INJECTION') + }) + }) + }) +}) diff --git a/packages/dd-trace/test/appsec/iast/vulnerability-formatter/index.spec.js b/packages/dd-trace/test/appsec/iast/vulnerability-formatter/index.spec.js index d77c5fb8e9b..884df6ebb3d 100644 --- a/packages/dd-trace/test/appsec/iast/vulnerability-formatter/index.spec.js +++ b/packages/dd-trace/test/appsec/iast/vulnerability-formatter/index.spec.js @@ -9,7 +9,8 @@ const excludedVulnerabilityTypes = ['XSS', 'EMAIL_HTML_INJECTION'] const excludedTests = [ 'Query with single quoted string literal and null source', // does not apply 'Redacted source that needs to be truncated', // not implemented yet - 'CODE_INJECTION - Tainted range based redaction - with null source ' // does not apply + 'CODE_INJECTION - Tainted range based redaction - with null source ', // does not apply + 'TEMPLATE_INJECTION - Tainted range based redaction - with null source ' // does not apply ] function doTest (testCase, parameters) { 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 d40546b7328..945c676a688 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 @@ -2911,7 +2911,8 @@ "$1": [ "XSS", "CODE_INJECTION", - "EMAIL_HTML_INJECTION" + "EMAIL_HTML_INJECTION", + "TEMPLATE_INJECTION" ] }, "input": [ @@ -2969,7 +2970,8 @@ "$1": [ "XSS", "CODE_INJECTION", - "EMAIL_HTML_INJECTION" + "EMAIL_HTML_INJECTION", + "TEMPLATE_INJECTION" ] }, "input": [ @@ -3029,7 +3031,8 @@ "$1": [ "XSS", "CODE_INJECTION", - "EMAIL_HTML_INJECTION" + "EMAIL_HTML_INJECTION", + "TEMPLATE_INJECTION" ] }, "input": [ @@ -3083,7 +3086,8 @@ "$1": [ "XSS", "CODE_INJECTION", - "EMAIL_HTML_INJECTION" + "EMAIL_HTML_INJECTION", + "TEMPLATE_INJECTION" ] }, "input": [ @@ -3162,7 +3166,8 @@ "$1": [ "XSS", "CODE_INJECTION", - "EMAIL_HTML_INJECTION" + "EMAIL_HTML_INJECTION", + "TEMPLATE_INJECTION" ] }, "input": [ @@ -3238,7 +3243,8 @@ "$1": [ "XSS", "CODE_INJECTION", - "EMAIL_HTML_INJECTION" + "EMAIL_HTML_INJECTION", + "TEMPLATE_INJECTION" ] }, "input": [ @@ -3311,7 +3317,8 @@ "$1": [ "XSS", "CODE_INJECTION", - "EMAIL_HTML_INJECTION" + "EMAIL_HTML_INJECTION", + "TEMPLATE_INJECTION" ] }, "input": [