Fix location in mysql vulnerability (#4006)

* Fix location in mysql vulnerability * export logic to report evidence to reuse instead of copying * Fix lint * Improve refactor
DataDog · Feb 12, 2024 · 1ed907c · 1ed907c
1 parent 5f8bd41
commit 1ed907c
Show file tree

Hide file tree

Showing 3 changed files with 25 additions and 39 deletions.
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
@@ -4,8 +4,6 @@ const InjectionAnalyzer = require('./injection-analyzer')
 const { SQL_INJECTION } = require('../vulnerabilities')
 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 = getNodeModulesPaths('mysql', 'mysql2', 'sequelize', 'pg-pool', 'knex')
@@ -16,9 +14,9 @@ class SqlInjectionAnalyzer extends InjectionAnalyzer {
   }
 
   onConfigure () {
-    this.addSub('apm:mysql:query:start', ({ sql }) => this.analyze(sql, 'MYSQL'))
-    this.addSub('apm:mysql2:query:start', ({ sql }) => this.analyze(sql, 'MYSQL'))
-    this.addSub('apm:pg:query:start', ({ query }) => this.analyze(query.text, 'POSTGRES'))
+    this.addSub('apm:mysql:query:start', ({ sql }) => this.analyze(sql, undefined, 'MYSQL'))
+    this.addSub('apm:mysql2:query:start', ({ sql }) => this.analyze(sql, undefined, 'MYSQL'))
+    this.addSub('apm:pg:query:start', ({ query }) => this.analyze(query.text, undefined, 'POSTGRES'))
 
     this.addSub(
       'datadog:sequelize:query:start',
@@ -42,7 +40,7 @@ class SqlInjectionAnalyzer extends InjectionAnalyzer {
   getStoreAndAnalyze (query, dialect) {
     const parentStore = storage.getStore()
     if (parentStore) {
-      this.analyze(query, dialect, parentStore)
+      this.analyze(query, parentStore, dialect)
 
       storage.enterWith({ ...parentStore, sqlAnalyzed: true, sqlParentStore: parentStore })
     }
@@ -60,29 +58,10 @@ class SqlInjectionAnalyzer extends InjectionAnalyzer {
     return { value, ranges, dialect }
   }
 
-  analyze (value, dialect, store = storage.getStore()) {
+  analyze (value, store, dialect) {
+    store = store || storage.getStore()
     if (!(store && store.sqlAnalyzed)) {
-      const iastContext = getIastContext(store)
-      if (this._isInvalidContext(store, iastContext)) return
-      this._reportIfVulnerable(value, iastContext, dialect)
-    }
-  }
-
-  _reportIfVulnerable (value, context, dialect) {
-    if (this._isVulnerable(value, context) && this._checkOCE(context)) {
-      this._report(value, context, dialect)
-      return true
-    }
-    return false
-  }
-
-  _report (value, context, dialect) {
-    const evidence = this._getEvidence(value, context, dialect)
-    const location = this._getLocation()
-    if (!this._isExcluded(location)) {
-      const spanId = context && context.rootSpan && context.rootSpan.context().toSpanId()
-      const vulnerability = this._createVulnerability(this._type, evidence, spanId, location)
-      addVulnerability(context, vulnerability)
+      super.analyze(value, store, dialect)
     }
   }
 

diff --git a/packages/dd-trace/src/appsec/iast/analyzers/vulnerability-analyzer.js b/packages/dd-trace/src/appsec/iast/analyzers/vulnerability-analyzer.js
@@ -22,8 +22,12 @@ class Analyzer extends SinkIastPlugin {
     return false
   }
 
-  _report (value, context) {
-    const evidence = this._getEvidence(value, context)
+  _report (value, context, meta) {
+    const evidence = this._getEvidence(value, context, meta)
+    this._reportEvidence(value, context, evidence)
+  }
+
+  _reportEvidence (value, context, evidence) {
     const location = this._getLocation(value)
     if (!this._isExcluded(location)) {
       const locationSourceMap = this._replaceLocationFromSourceMap(location)
@@ -33,9 +37,9 @@ class Analyzer extends SinkIastPlugin {
     }
   }
 
-  _reportIfVulnerable (value, context) {
+  _reportIfVulnerable (value, context, meta) {
     if (this._isVulnerable(value, context) && this._checkOCE(context, value)) {
-      this._report(value, context)
+      this._report(value, context, meta)
       return true
     }
     return false
@@ -71,11 +75,11 @@ class Analyzer extends SinkIastPlugin {
     return store && !iastContext
   }
 
-  analyze (value, store = storage.getStore()) {
+  analyze (value, store = storage.getStore(), meta) {
     const iastContext = getIastContext(store)
     if (this._isInvalidContext(store, iastContext)) return
 
-    this._reportIfVulnerable(value, iastContext)
+    this._reportIfVulnerable(value, iastContext, meta)
   }
 
   analyzeAll (...values) {

diff --git a/packages/dd-trace/test/appsec/iast/analyzers/sql-injection-analyzer.spec.js b/packages/dd-trace/test/appsec/iast/analyzers/sql-injection-analyzer.spec.js
@@ -78,6 +78,9 @@ describe('sql-injection-analyzer', () => {
       },
       '../overhead-controller': { hasQuota: () => true }
     })
+    sinon.stub(ProxyAnalyzer.prototype, '_reportEvidence')
+    const reportEvidence = ProxyAnalyzer.prototype._reportEvidence
+
     const InjectionAnalyzer = proxyquire('../../../../src/appsec/iast/analyzers/injection-analyzer', {
       '../taint-tracking/operations': TaintTrackingMock,
       './vulnerability-analyzer': ProxyAnalyzer
@@ -91,11 +94,11 @@ describe('sql-injection-analyzer', () => {
         },
         '../vulnerability-reporter': { addVulnerability }
       })
-    proxiedSqlInjectionAnalyzer.analyze(TAINTED_QUERY, dialect)
-    expect(addVulnerability).to.have.been.calledOnce
-    expect(addVulnerability).to.have.been.calledWithMatch({}, {
-      type: 'SQL_INJECTION',
-      evidence: { dialect: dialect }
+    proxiedSqlInjectionAnalyzer.analyze(TAINTED_QUERY, undefined, dialect)
+    expect(reportEvidence).to.have.been.calledOnce
+    expect(reportEvidence).to.have.been.calledWithMatch(TAINTED_QUERY, {}, {
+      value: TAINTED_QUERY,
+      dialect
     })
   })