Fix fs and rasp/iast problems (#6088)

Author: uurien

packages/dd-trace/src/appsec/rasp/fs-plugin.js

@@ -14,25 +14,31 @@ const enabledFor = {
 
 let fsPlugin
 
-function enterWith (fsProps, store = storage('legacy').getStore()) {
+function getStoreToStart (fsProps, store = storage('legacy').getStore()) {
   if (store && !store.fs?.opExcluded) {
-    storage('legacy').enterWith({
+    return {
       ...store,
       fs: {
         ...store.fs,
         ...fsProps,
         parentStore: store
       }
-    })
+    }
   }
+
+  return store
 }
 
 class AppsecFsPlugin extends Plugin {
   enable () {
-    this.addSub('apm:fs:operation:start', this._onFsOperationStart)
-    this.addSub('apm:fs:operation:finish', this._onFsOperationFinishOrRenderEnd)
-    this.addSub('tracing:datadog:express:response:render:start', this._onResponseRenderStart)
-    this.addSub('tracing:datadog:express:response:render:end', this._onFsOperationFinishOrRenderEnd)
+    this.addBind('apm:fs:operation:start', this._onFsOperationStart)
+    this.addBind('apm:fs:operation:finish', this._onFsOperationFinishOrRenderEnd)
+    this.addBind('tracing:datadog:express:response:render:start', this._onResponseRenderStart)
+    this.addBind('tracing:datadog:express:response:render:end', this._onFsOperationFinishOrRenderEnd)
+    // TODO Remove this when dc-polyfill is fixed&updated
+    //  hack to node 18 and early 20.x
+    //  with dc-polyfill addBind is not enough to force a channel.hasSubscribers === true
+    this.addSub('tracing:datadog:express:response:render:start', () => {})
 
     super.configure(true)
   }
@@ -44,19 +50,20 @@ class AppsecFsPlugin extends Plugin {
   _onFsOperationStart () {
     const store = storage('legacy').getStore()
     if (store) {
-      enterWith({ root: store.fs?.root === undefined }, store)
+      return getStoreToStart({ root: store.fs?.root === undefined }, store)
     }
   }
 
   _onResponseRenderStart () {
-    enterWith({ opExcluded: true })
+    return getStoreToStart({ opExcluded: true })
   }
 
   _onFsOperationFinishOrRenderEnd () {
     const store = storage('legacy').getStore()
-    if (store?.fs?.parentStore) {
-      storage('legacy').enterWith(store.fs.parentStore)
+    if (store?.fs) {
+      return store.fs.parentStore
     }
+    return store
   }
 }
 

packages/dd-trace/test/appsec/iast/analyzers/path-traversal-analyzer.spec.js

@@ -477,6 +477,26 @@ prepareTestServerForIast('integration test', (testThatRequestHasVulnerability, t
 
   describe('test stat', () => {
     runFsMethodTestThreeWay('stat', 0, null, __filename)
+
+    describe('with two calls to async method without waiting to the callback', () => {
+      const fsAsyncWayMethodPath = path.join(os.tmpdir(), 'fs-async-way-method.js')
+
+      before(() => {
+        fs.copyFileSync(path.join(__dirname, 'resources', 'fs-async-way-method.js'), fsAsyncWayMethodPath)
+      })
+
+      after(() => {
+        fs.unlinkSync(fsAsyncWayMethodPath)
+      })
+
+      testThatRequestHasVulnerability(function () {
+        const store = storage('legacy').getStore()
+        const iastCtx = iastContextFunctions.getIastContext(store)
+        const callArgs = [fsAsyncWayMethodPath]
+        callArgs[0] = newTaintedString(iastCtx, callArgs[0], 'param', 'Request')
+        return require(fsAsyncWayMethodPath).doubleCallIgnoringCb('stat', callArgs)
+      }, 'PATH_TRAVERSAL', { occurrences: 2 })
+    })
   })
 
   describe('test symlink', () => {

packages/dd-trace/test/appsec/iast/analyzers/resources/fs-async-way-method.js

@@ -2,10 +2,21 @@
 
 const fs = require('fs')
 
-module.exports = function (methodName, args, cb) {
+function main (methodName, args, cb) {
   return new Promise((resolve, reject) => {
     fs[methodName](...args, (err, res) => {
       resolve(cb(res))
     })
   })
 }
+
+main.doubleCallIgnoringCb = function (methodName, args) {
+  return new Promise((resolve) => {
+    fs[methodName](...args, () => {})
+    fs[methodName](...args, () => {
+      resolve()
+    })
+  })
+}
+
+module.exports = main

packages/dd-trace/test/appsec/rasp/fs-plugin.spec.js

@@ -27,6 +27,7 @@ describe('AppsecFsPlugin', () => {
     beforeEach(() => {
       configure = sinon.stub()
       class PluginClass {
+        addBind (channelName, handler) {}
         addSub (channelName, handler) {}
 
         configure (config) {
@@ -93,20 +94,18 @@ describe('AppsecFsPlugin', () => {
   })
 
   describe('_onFsOperationStart', () => {
-    it('should mark fs root', () => {
+    it('should return fs root', () => {
       const origStore = {}
       storage('legacy').enterWith(origStore)
 
-      appsecFsPlugin._onFsOperationStart()
+      let store = appsecFsPlugin._onFsOperationStart()
 
-      let store = storage('legacy').getStore()
       assert.property(store, 'fs')
       assert.propertyVal(store.fs, 'parentStore', origStore)
       assert.propertyVal(store.fs, 'root', true)
 
-      appsecFsPlugin._onFsOperationFinishOrRenderEnd()
+      store = appsecFsPlugin._onFsOperationFinishOrRenderEnd()
 
-      store = storage('legacy').getStore()
       assert.equal(store, origStore)
       assert.notProperty(store, 'fs')
     })
@@ -115,28 +114,30 @@ describe('AppsecFsPlugin', () => {
       const origStore = { orig: true }
       storage('legacy').enterWith(origStore)
 
-      appsecFsPlugin._onFsOperationStart()
+      const rootStore = appsecFsPlugin._onFsOperationStart()
 
-      const rootStore = storage('legacy').getStore()
       assert.property(rootStore, 'fs')
       assert.propertyVal(rootStore.fs, 'parentStore', origStore)
       assert.propertyVal(rootStore.fs, 'root', true)
 
-      appsecFsPlugin._onFsOperationStart()
+      storage('legacy').enterWith(rootStore)
+
+      let store = appsecFsPlugin._onFsOperationStart()
 
-      let store = storage('legacy').getStore()
       assert.property(store, 'fs')
       assert.propertyVal(store.fs, 'parentStore', rootStore)
       assert.propertyVal(store.fs, 'root', false)
       assert.propertyVal(store, 'orig', true)
 
-      appsecFsPlugin._onFsOperationFinishOrRenderEnd()
+      storage('legacy').enterWith(store)
+
+      store = appsecFsPlugin._onFsOperationFinishOrRenderEnd()
 
-      store = storage('legacy').getStore()
       assert.equal(store, rootStore)
 
-      appsecFsPlugin._onFsOperationFinishOrRenderEnd()
-      store = storage('legacy').getStore()
+      storage('legacy').enterWith(store)
+
+      store = appsecFsPlugin._onFsOperationFinishOrRenderEnd()
       assert.equal(store, origStore)
     })
   })
@@ -148,16 +149,16 @@ describe('AppsecFsPlugin', () => {
       const origStore = {}
       storage('legacy').enterWith(origStore)
 
-      appsecFsPlugin._onResponseRenderStart()
+      let store = appsecFsPlugin._onResponseRenderStart()
 
-      let store = storage('legacy').getStore()
       assert.property(store, 'fs')
       assert.propertyVal(store.fs, 'parentStore', origStore)
       assert.propertyVal(store.fs, 'opExcluded', true)
 
-      appsecFsPlugin._onFsOperationFinishOrRenderEnd()
+      storage('legacy').enterWith(store)
+
+      store = appsecFsPlugin._onFsOperationFinishOrRenderEnd()
 
-      store = storage('legacy').getStore()
       assert.equal(store, origStore)
       assert.notProperty(store, 'fs')
     })
@@ -225,6 +226,12 @@ describe('AppsecFsPlugin', () => {
 
       it('should clean up store when finishing op', () => {
         let count = 4
+        // TODO Remove this when node 18 is unsupported or dc-polyfill is fixed&updated
+        //  hack to node 18 and early 20.x
+        //  with dc-polyfill addBind is not enough to force a channel.hasSubscribers === true
+        const onStart = () => {}
+        opStartCh.subscribe(onStart)
+
         const onFinish = () => {
           const store = storage('legacy').getStore()
           count--
@@ -244,6 +251,7 @@ describe('AppsecFsPlugin', () => {
           assert.strictEqual(count, 0)
         } finally {
           opFinishCh.unsubscribe(onFinish)
+          opStartCh.unsubscribe(onStart)
         }
       })
     })