Skip to content

Commit 8df8c36

Browse files
uurienuurien
authored
Improve iast mongodb nosql detection removing some false positives (#5408)
1 parent 21e0408 commit 8df8c36

File tree

4 files changed

+135
-19
lines changed

4 files changed

+135
-19
lines changed

‎packages/dd-trace/src/appsec/iast/analyzers/nosql-injection-mongodb-analyzer.js

Lines changed: 39 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,9 @@ const EXCLUDED_PATHS_FROM_STACK = getNodeModulesPaths('mongodb', 'mongoose', 'mq
1212
const { NOSQL_MONGODB_INJECTION_MARK } = require('../taint-tracking/secure-marks')
1313
const { iterateObjectStrings } = require('../utils')
1414

15+
const SAFE_OPERATORS = new Set(['$eq', '$gt', '$gte', '$in', '$lt', '$lte', '$ne', '$nin',
16+
'$exists', '$type', '$mod', '$bitsAllClear', '$bitsAllSet', '$bitsAnyClear', '$bitsAnySet'])
17+
1518
class NosqlInjectionMongodbAnalyzer extends InjectionAnalyzer {
1619
constructor () {
1720
super(NOSQL_MONGODB_INJECTION)
@@ -103,7 +106,11 @@ class NosqlInjectionMongodbAnalyzer extends InjectionAnalyzer {
103106
})
104107
}
105108

106-
_isVulnerableRange (range) {
109+
_isVulnerableRange (range, value) {
110+
const rangeIsWholeValue = range.start === 0 && range.end === value?.length
111+
112+
if (!rangeIsWholeValue) return false
113+
107114
const rangeType = range?.iinfo?.type
108115
return rangeType === HTTP_REQUEST_PARAMETER || rangeType === HTTP_REQUEST_BODY
109116
}
@@ -119,29 +126,25 @@ class NosqlInjectionMongodbAnalyzer extends InjectionAnalyzer {
119126
const rangesByKey = {}
120127
const allRanges = []
121128

122-
iterateObjectStrings(value.filter, (val, nextLevelKeys) => {
129+
iterateMongodbQueryStrings(value.filter, (val, nextLevelKeys) => {
123130
let ranges = getRanges(iastContext, val)
124-
if (ranges?.length) {
125-
const filteredRanges = []
126-
131+
if (ranges?.length === 1) {
127132
ranges = this._filterSecureRanges(ranges)
128133
if (!ranges.length) {
129134
this._incrementSuppressedMetric(iastContext)
135+
return
130136
}
131137

132-
for (const range of ranges) {
133-
if (this._isVulnerableRange(range)) {
134-
isVulnerable = true
135-
filteredRanges.push(range)
136-
}
138+
const range = ranges[0]
139+
if (!this._isVulnerableRange(range, val)) {
140+
return
137141
}
142+
isVulnerable = true
138143

139-
if (filteredRanges.length > 0) {
140-
rangesByKey[nextLevelKeys.join('.')] = filteredRanges
141-
allRanges.push(...filteredRanges)
142-
}
144+
rangesByKey[nextLevelKeys.join('.')] = ranges
145+
allRanges.push(range)
143146
}
144-
}, [], 4)
147+
})
145148

146149
if (isVulnerable) {
147150
value.rangesToApply = rangesByKey
@@ -162,4 +165,25 @@ class NosqlInjectionMongodbAnalyzer extends InjectionAnalyzer {
162165
}
163166
}
164167

168+
function iterateMongodbQueryStrings (target, fn, levelKeys = [], depth = 10, visited = new Set()) {
169+
if (target !== null && typeof target === 'object') {
170+
if (visited.has(target)) return
171+
172+
visited.add(target)
173+
174+
Object.keys(target).forEach((key) => {
175+
if (SAFE_OPERATORS.has(key)) return
176+
177+
const nextLevelKeys = [...levelKeys, key]
178+
const val = target[key]
179+
180+
if (typeof val === 'string') {
181+
fn(val, nextLevelKeys, target, key)
182+
} else if (depth > 0) {
183+
iterateMongodbQueryStrings(val, fn, nextLevelKeys, depth - 1, visited)
184+
}
185+
})
186+
}
187+
}
188+
165189
module.exports = new NosqlInjectionMongodbAnalyzer()

‎packages/dd-trace/test/appsec/iast/analyzers/nosql-injection-mongodb-analyzer.express-mongo-sanitize.plugin.spec.js

Lines changed: 90 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,8 @@ const agent = require('../../../plugins/agent')
1010

1111
describe('nosql injection detection in mongodb - whole feature', () => {
1212
// https://github.com/fiznool/express-mongo-sanitize/issues/200
13-
withVersions('mongodb', 'express', '>4.18.0 <5.0.0', expressVersion => {
14-
withVersions('mongodb', 'mongodb', mongodbVersion => {
13+
withVersions('express-mongo-sanitize', 'express', '>4.18.0 <5.0.0', expressVersion => {
14+
withVersions('express-mongo-sanitize', 'mongodb', mongodbVersion => {
1515
const mongodb = require(`../../../../../../versions/mongodb@${mongodbVersion}`)
1616

1717
const satisfiesNodeVersionForMongo3and4 =
@@ -82,6 +82,94 @@ describe('nosql injection detection in mongodb - whole feature', () => {
8282
}
8383
})
8484

85+
testThatRequestHasVulnerability({
86+
testDescription: 'should have NOSQL_MONGODB_INJECTION vulnerability in $or clause',
87+
fn: async (req, res) => {
88+
await collection.find({
89+
key: {
90+
$or: [req.query.key, 'test']
91+
}
92+
})
93+
res.end()
94+
},
95+
vulnerability: 'NOSQL_MONGODB_INJECTION',
96+
makeRequest: (done, config) => {
97+
axios.get(`http://localhost:${config.port}/?key=value`).catch(done)
98+
},
99+
cb: function (vulnerabilities) {
100+
const vulnerability = vulnerabilities[0]
101+
let someRedacted = false
102+
vulnerability.evidence.valueParts.forEach(valuePart => {
103+
if (valuePart.redacted) {
104+
someRedacted = true
105+
}
106+
})
107+
108+
expect(someRedacted).to.be.true
109+
}
110+
})
111+
112+
testThatRequestHasNoVulnerability({
113+
testDescription: 'should not have NOSQL_MONGODB_INJECTION vulnerability using $eq',
114+
fn: async (req, res) => {
115+
await collection.find({
116+
key: {
117+
$eq: req.query.key
118+
}
119+
})
120+
res.end()
121+
},
122+
vulnerability: 'NOSQL_MONGODB_INJECTION',
123+
makeRequest: (done, config) => {
124+
axios.get(`http://localhost:${config.port}/?key=value`).catch(done)
125+
}
126+
})
127+
128+
testThatRequestHasNoVulnerability({
129+
testDescription: 'should not have NOSQL_MONGODB_INJECTION vulnerability with modified tainted string',
130+
fn: async (req, res) => {
131+
const data = req.query.key
132+
// eslint-disable-next-line no-undef
133+
const modifiedData = _ddiast.plusOperator('modified' + data, 'modified', data)
134+
135+
await collection.find({
136+
key: modifiedData
137+
})
138+
139+
res.end()
140+
},
141+
vulnerability: 'NOSQL_MONGODB_INJECTION',
142+
makeRequest: (done, config) => {
143+
axios.get(`http://localhost:${config.port}/?key=value`).catch(done)
144+
}
145+
})
146+
147+
testThatRequestHasNoVulnerability({
148+
testDescription: 'should not have NOSQL_MONGODB_INJECTION vulnerability in too deep property',
149+
fn: async (req, res) => {
150+
const deep = 11
151+
const obj = {}
152+
let next = obj
153+
154+
for (let i = 0; i <= deep; i++) {
155+
if (i === deep) {
156+
next.key = req.query.key
157+
break
158+
}
159+
160+
next.key = {}
161+
next = next.key
162+
}
163+
164+
await collection.find(obj)
165+
res.end()
166+
},
167+
vulnerability: 'NOSQL_MONGODB_INJECTION',
168+
makeRequest: (done, config) => {
169+
axios.get(`http://localhost:${config.port}/?key=value`).catch(done)
170+
}
171+
})
172+
85173
testThatRequestHasNoVulnerability({
86174
testDescription: 'should not have NOSQL_MONGODB_INJECTION vulnerability with path params',
87175
fn: function noop () {},

‎packages/dd-trace/test/appsec/iast/analyzers/nosql-injection-mongodb-analyzer.mquery.plugin.spec.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,8 @@ const fs = require('fs')
1010

1111
describe('nosql injection detection with mquery', () => {
1212
// https://github.com/fiznool/express-mongo-sanitize/issues/200
13-
withVersions('mongodb', 'express', '>4.18.0 <5.0.0', expressVersion => {
14-
withVersions('mongodb', 'mongodb', mongodbVersion => {
13+
withVersions('mquery', 'express', '>4.18.0 <5.0.0', expressVersion => {
14+
withVersions('mquery', 'mongodb', mongodbVersion => {
1515
const mongodb = require(`../../../../../../versions/mongodb@${mongodbVersion}`)
1616

1717
const satisfiesNodeVersionForMongo3and4 =

‎packages/dd-trace/test/plugins/externals.json

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,10 @@
120120
}
121121
],
122122
"mquery": [
123+
{
124+
"name": "express",
125+
"versions": [">=4", ">=4.0.0 <4.3.0", ">=4.0.0 <5.0.0", ">=4.3.0 <5.0.0"]
126+
},
123127
{
124128
"name": "mquery",
125129
"versions": [">=5.0.0"]

0 commit comments

Comments
 (0)