Implement common taint tracking operations (#4239)

* Lodash trim implementation * Implement CSI for toLowerCase and toUpperCase * Fix lint * Implement Array.join CSI * Instrument lodash string case methods to propagate taint valuesh * Instrument lodash array join method to propagate taint valuesh * Refactor * Move test to a plugin test * Sort methods alphabetically * Reformat lodash taint tracking handler + test * Fix this loosing in lodash instrumentation * Add blankline * Prevent tainting empty strings for lodash * Bump iast-rewriter to 2.3.1 with array expression test * Sort fn alphabetically + fix lint * Fix tainted instance replacement + test * Appsec propagation test with node 21 instead of latest * Change job title for lodash plugin test Co-authored-by: Ugaitz Urien <[email protected]> * Add use strict in lodash fn file Co-authored-by: Ugaitz Urien <[email protected]> --------- Co-authored-by: Ugaitz Urien <[email protected]>
DataDog · May 14, 2024 · 0c4cf8c · 0c4cf8c
1 parent 8137237
commit 0c4cf8c
Show file tree

Hide file tree

Showing 13 changed files with 330 additions and 31 deletions.
diff --git a/.github/workflows/appsec.yml b/.github/workflows/appsec.yml
@@ -217,3 +217,17 @@ jobs:
       - if: always()
         uses: ./.github/actions/testagent/logs
       - uses: codecov/codecov-action@v3
+
+  lodash:
+    runs-on: ubuntu-latest
+    env:
+      PLUGINS: lodash
+    steps:
+      - uses: actions/checkout@v4
+      - uses: ./.github/actions/node/setup
+      - run: yarn install
+      - uses: ./.github/actions/node/oldest
+      - run: yarn test:appsec:plugins:ci
+      - uses: ./.github/actions/node/21
+      - run: yarn test:appsec:plugins:ci
+      - uses: codecov/codecov-action@v3
diff --git a/package.json b/package.json
@@ -71,7 +71,7 @@
   },
   "dependencies": {
     "@datadog/native-appsec": "7.1.1",
-    "@datadog/native-iast-rewriter": "2.3.0",
+    "@datadog/native-iast-rewriter": "2.3.1",
     "@datadog/native-iast-taint-tracking": "2.1.0",
     "@datadog/native-metrics": "^2.0.0",
     "@datadog/pprof": "5.2.0",

diff --git a/packages/datadog-instrumentations/src/helpers/hooks.js b/packages/datadog-instrumentations/src/helpers/hooks.js
@@ -66,6 +66,7 @@ module.exports = {
   kafkajs: () => require('../kafkajs'),
   ldapjs: () => require('../ldapjs'),
   'limitd-client': () => require('../limitd-client'),
+  lodash: () => require('../lodash'),
   mariadb: () => require('../mariadb'),
   memcached: () => require('../memcached'),
   'microgateway-core': () => require('../microgateway-core'),

diff --git a/packages/datadog-instrumentations/src/lodash.js b/packages/datadog-instrumentations/src/lodash.js
@@ -0,0 +1,31 @@
+'use strict'
+
+const { channel, addHook } = require('./helpers/instrument')
+
+const shimmer = require('../../datadog-shimmer')
+
+addHook({ name: 'lodash', versions: ['>=4'] }, lodash => {
+  const lodashOperationCh = channel('datadog:lodash:operation')
+
+  const instrumentedLodashFn = ['trim', 'trimStart', 'trimEnd', 'toLower', 'toUpper', 'join']
+
+  shimmer.massWrap(
+    lodash,
+    instrumentedLodashFn,
+    lodashFn => {
+      return function () {
+        if (!lodashOperationCh.hasSubscribers) {
+          return lodashFn.apply(this, arguments)
+        }
+
+        const result = lodashFn.apply(this, arguments)
+        const message = { operation: lodashFn.name, arguments, result }
+        lodashOperationCh.publish(message)
+
+        return message.result
+      }
+    }
+  )
+
+  return lodash
+})
diff --git a/packages/dd-trace/src/appsec/iast/taint-tracking/csi-methods.js b/packages/dd-trace/src/appsec/iast/taint-tracking/csi-methods.js
@@ -2,13 +2,16 @@
 
 const csiMethods = [
   { src: 'concat' },
+  { src: 'join' },
   { src: 'parse' },
   { src: 'plusOperator', operator: true },
   { src: 'random' },
   { src: 'replace' },
   { src: 'slice' },
   { src: 'substr' },
   { src: 'substring' },
+  { src: 'toLowerCase', dst: 'stringCase' },
+  { src: 'toUpperCase', dst: 'stringCase' },
   { src: 'trim' },
   { src: 'trimEnd' },
   { src: 'trimStart', dst: 'trim' }

diff --git a/packages/dd-trace/src/appsec/iast/taint-tracking/operations.js b/packages/dd-trace/src/appsec/iast/taint-tracking/operations.js
@@ -1,13 +1,20 @@
 'use strict'
 
+const dc = require('dc-polyfill')
 const TaintedUtils = require('@datadog/native-iast-taint-tracking')
 const { IAST_TRANSACTION_ID } = require('../iast-context')
 const iastTelemetry = require('../telemetry')
 const { REQUEST_TAINTED } = require('../telemetry/iast-metric')
 const { isInfoAllowed } = require('../telemetry/verbosity')
-const { getTaintTrackingImpl, getTaintTrackingNoop } = require('./taint-tracking-impl')
+const {
+  getTaintTrackingImpl,
+  getTaintTrackingNoop,
+  lodashTaintTrackingHandler
+} = require('./taint-tracking-impl')
 const { taintObject } = require('./operations-taint-object')
 
+const lodashOperationCh = dc.channel('datadog:lodash:operation')
+
 function createTransaction (id, iastContext) {
   if (id && iastContext) {
     iastContext[IAST_TRANSACTION_ID] = TaintedUtils.createTransaction(id)
@@ -92,10 +99,12 @@ function enableTaintOperations (telemetryVerbosity) {
   }
 
   global._ddiast = getTaintTrackingImpl(telemetryVerbosity)
+  lodashOperationCh.subscribe(lodashTaintTrackingHandler)
 }
 
 function disableTaintOperations () {
   global._ddiast = getTaintTrackingNoop()
+  lodashOperationCh.unsubscribe(lodashTaintTrackingHandler)
 }
 
 function setMaxTransactions (transactions) {

diff --git a/packages/dd-trace/src/appsec/iast/taint-tracking/taint-tracking-impl.js b/packages/dd-trace/src/appsec/iast/taint-tracking/taint-tracking-impl.js
@@ -18,13 +18,15 @@ function noop (res) { return res }
 // Otherwise you may end up rewriting a method and not providing its rewritten implementation
 const TaintTrackingNoop = {
   concat: noop,
+  join: noop,
   parse: noop,
   plusOperator: noop,
   random: noop,
   replace: noop,
   slice: noop,
   substr: noop,
   substring: noop,
+  stringCase: noop,
   trim: noop,
   trimEnd: noop
 }
@@ -113,6 +115,13 @@ function csiMethodsOverrides (getContext) {
       return res
     },
 
+    stringCase: getCsiFn(
+      (transactionId, res, target) => TaintedUtils.stringCase(transactionId, res, target),
+      getContext,
+      String.prototype.toLowerCase,
+      String.prototype.toUpperCase
+    ),
+
     trim: getCsiFn(
       (transactionId, res, target) => TaintedUtils.trim(transactionId, res, target),
       getContext,
@@ -147,6 +156,22 @@ function csiMethodsOverrides (getContext) {
         }
       }
 
+      return res
+    },
+
+    join: function (res, fn, target, separator) {
+      if (fn === Array.prototype.join) {
+        try {
+          const iastContext = getContext()
+          const transactionId = getTransactionId(iastContext)
+          if (transactionId) {
+            res = TaintedUtils.arrayJoin(transactionId, res, target, separator)
+          }
+        } catch (e) {
+          iastLog.error(e)
+        }
+      }
+
       return res
     }
   }
@@ -176,7 +201,36 @@ function getTaintTrackingNoop () {
   return getTaintTrackingImpl(null, true)
 }
 
+const lodashFns = {
+  join: TaintedUtils.arrayJoin,
+  toLower: TaintedUtils.stringCase,
+  toUpper: TaintedUtils.stringCase,
+  trim: TaintedUtils.trim,
+  trimEnd: TaintedUtils.trimEnd,
+  trimStart: TaintedUtils.trim
+
+}
+
+function getLodashTaintedUtilFn (lodashFn) {
+  return lodashFns[lodashFn] || ((transactionId, result) => result)
+}
+
+function lodashTaintTrackingHandler (message) {
+  try {
+    if (!message.result) return
+    const context = getContextDefault()
+    const transactionId = getTransactionId(context)
+    if (transactionId) {
+      message.result = getLodashTaintedUtilFn(message.operation)(transactionId, message.result, ...message.arguments)
+    }
+  } catch (e) {
+    iastLog.error(`Error invoking CSI lodash ${message.operation}`)
+      .errorAndPublish(e)
+  }
+}
+
 module.exports = {
   getTaintTrackingImpl,
-  getTaintTrackingNoop
+  getTaintTrackingNoop,
+  lodashTaintTrackingHandler
 }
diff --git a/packages/dd-trace/test/appsec/iast/taint-tracking/resources/propagationFunctions.js b/packages/dd-trace/test/appsec/iast/taint-tracking/resources/propagationFunctions.js
@@ -52,6 +52,14 @@ function sliceStr (str) {
   return str.slice(1, 4)
 }
 
+function toLowerCaseStr (str) {
+  return str.toLowerCase()
+}
+
+function toUpperCaseStr (str) {
+  return str.toUpperCase()
+}
+
 function replaceStr (str) {
   return str.replace('ls', 'sl')
 }
@@ -64,21 +72,39 @@ function jsonParseStr (str) {
   return JSON.parse(str)
 }
 
+function arrayJoin (str) {
+  return [str, str].join(str)
+}
+
+function arrayInVariableJoin (str) {
+  const testArr = [str, str]
+  return testArr.join(',')
+}
+
+function arrayProtoJoin (str) {
+  return Array.prototype.join.call([str, str], ',')
+}
+
 module.exports = {
-  concatSuffix,
-  insertStr,
   appendStr,
-  trimStr,
-  trimStartStr,
-  trimEndStr,
-  trimProtoStr,
+  arrayInVariableJoin,
+  arrayJoin,
+  arrayProtoJoin,
+  concatProtoStr,
   concatStr,
+  concatSuffix,
   concatTaintedStr,
-  concatProtoStr,
-  substringStr,
-  substrStr,
-  sliceStr,
-  replaceStr,
+  insertStr,
+  jsonParseStr,
   replaceRegexStr,
-  jsonParseStr
+  replaceStr,
+  sliceStr,
+  substrStr,
+  substringStr,
+  toLowerCaseStr,
+  toUpperCaseStr,
+  trimEndStr,
+  trimProtoStr,
+  trimStartStr,
+  trimStr
 }
diff --git a/packages/dd-trace/test/appsec/iast/taint-tracking/resources/propagationLodashFunctions.js b/packages/dd-trace/test/appsec/iast/taint-tracking/resources/propagationLodashFunctions.js
@@ -0,0 +1,44 @@
+'use strict'
+
+function trimLodash (_, str) {
+  return _.trim(str)
+}
+
+function trimStartLodash (_, str) {
+  return _.trimStart(str)
+}
+
+function trimEndLodash (_, str) {
+  return _.trimEnd(str)
+}
+
+function toLowerLodash (_, str) {
+  return _.toLower(str)
+}
+
+function toUpperLodash (_, str) {
+  return _.toUpper(str)
+}
+
+function arrayJoinLodashWithoutSeparator (_, str) {
+  return _.join([str, str])
+}
+
+function arrayJoinLodashWithSeparator (_, str) {
+  return _.join([str, str], str)
+}
+
+function startCaseLodash (_, str) {
+  return _.startCase(str)
+}
+
+module.exports = {
+  arrayJoinLodashWithoutSeparator,
+  arrayJoinLodashWithSeparator,
+  toLowerLodash,
+  toUpperLodash,
+  startCaseLodash,
+  trimEndLodash,
+  trimLodash,
+  trimStartLodash
+}
diff --git a/packages/dd-trace/test/appsec/iast/taint-tracking/taint-tracking-impl.spec.js b/packages/dd-trace/test/appsec/iast/taint-tracking/taint-tracking-impl.spec.js
@@ -11,21 +11,26 @@ const { clearCache } = require('../../../../src/appsec/iast/vulnerability-report
 const { expect } = require('chai')
 
 const propagationFns = [
-  'concatSuffix',
-  'insertStr',
   'appendStr',
-  'trimStr',
-  'trimStartStr',
-  'trimEndStr',
-  'trimProtoStr',
+  'arrayInVariableJoin',
+  'arrayJoin',
+  'arrayProtoJoin',
+  'concatProtoStr',
   'concatStr',
+  'concatSuffix',
   'concatTaintedStr',
-  'concatProtoStr',
-  'substringStr',
-  'substrStr',
-  'sliceStr',
+  'insertStr',
+  'replaceRegexStr',
   'replaceStr',
-  'replaceRegexStr'
+  'sliceStr',
+  'substrStr',
+  'substringStr',
+  'toLowerCaseStr',
+  'toUpperCaseStr',
+  'trimEndStr',
+  'trimProtoStr',
+  'trimStartStr',
+  'trimStr'
 ]
 
 const commands = [
@@ -122,7 +127,15 @@ describe('TaintTracking', () => {
   })
 
   describe('should not catch original Error', () => {
-    const filtered = ['concatSuffix', 'insertStr', 'appendStr', 'concatTaintedStr']
+    const filtered = [
+      'appendStr',
+      'arrayInVariableJoin',
+      'arrayJoin',
+      'arrayProtoJoin',
+      'concatSuffix',
+      'concatTaintedStr',
+      'insertStr'
+    ]
     propagationFns.forEach((propFn) => {
       if (filtered.includes(propFn)) return
       it(`invoking ${propFn} with null argument`, () => {