Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/appsec.yml
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ jobs:
ports:
- 3306:3306
env:
PLUGINS: mysql|mysql2
PLUGINS: mysql|mysql2|sequelize
SERVICES: mysql
steps:
- uses: actions/checkout@v2
Expand Down
1 change: 1 addition & 0 deletions packages/datadog-instrumentations/src/helpers/hooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ module.exports = {
'rhea': () => require('../rhea'),
'router': () => require('../router'),
'sharedb': () => require('../sharedb'),
'sequelize': () => require('../sequelize'),
'tedious': () => require('../tedious'),
'when': () => require('../when'),
'winston': () => require('../winston')
Expand Down
51 changes: 51 additions & 0 deletions packages/datadog-instrumentations/src/sequelize.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
'use strict'

const {
channel,
addHook,
AsyncResource
} = require('./helpers/instrument')

const shimmer = require('../../datadog-shimmer')

addHook({ name: 'sequelize', versions: ['>=4'] }, Sequelize => {
const startCh = channel('datadog:sequelize:query:start')
const finishCh = channel('datadog:sequelize:query:finish')

shimmer.wrap(Sequelize.prototype, 'query', query => {
return function (sql) {
if (!startCh.hasSubscribers) {
return query.apply(this, arguments)
}

const asyncResource = new AsyncResource('bound-anonymous-fn')

let dialect
if (this.options && this.options.dialect) {
dialect = this.options.dialect
} else if (this.dialect && this.dialect.name) {
dialect = this.dialect.name
}

function onFinish () {
asyncResource.bind(function () {
finishCh.publish()
}, this).apply(this)
}

return asyncResource.bind(function () {
startCh.publish({
sql,
dialect
})

const promise = query.apply(this, arguments)
promise.then(onFinish, onFinish)

return promise
}, this).apply(this, arguments)
}
})

return Sequelize
})
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,22 @@ class SqlInjectionAnalyzer extends InjectionAnalyzer {
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('datadog:sequelize:query:start', ({ sql, dialect }) => {
const parentStore = storage.getStore()
if (parentStore) {
this.analyze(sql, dialect.toUpperCase())

storage.enterWith({ ...parentStore, sqlAnalyzed: true, sequelizeParentStore: parentStore })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This data doesn't seem to ever get cleared from the store so it may have some unintended side-effects. 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is restored on datadog:sequelize:query:finish

The purpose of this lines is prevent double detections of the same vulnerability. As sequelize library uses pg or mysql2 libraries, we need a way to set in store that we are already in the query. For that reason we are entering with new store in :start and restoring the old one in :finish.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, true. Do you know if sequelize could trigger these in a nested way? Setting on the store rather than storing on the span could make things leak beyond where it should. 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I've reviewed it after your comment. The answer is not, sequelize.query method does not call internally again to the same query method.
sequelize.query method calls to the query method of the select dialect (pg, sqlite, mysql...)

With sequelize we are not creating new span, so current span is the parent span, and the parent span can have multiple calls to sequelize.query method, that the reason that we should save the data in AsyncResource (that is new asyncresource, created in the instrumentation point) and not in the parent span.

}
})

this.addSub('datadog:sequelize:query:finish', () => {
const store = storage.getStore()
if (store.sequelizeParentStore) {
storage.enterWith(store.sequelizeParentStore)
}
})
}

_getEvidence (value, iastContext, dialect) {
Expand All @@ -22,9 +38,12 @@ class SqlInjectionAnalyzer extends InjectionAnalyzer {

analyze (value, dialect) {
const store = storage.getStore()
const iastContext = getIastContext(store)
if (store && !iastContext) return
this._reportIfVulnerable(value, iastContext, dialect)

if (!(store && store.sqlAnalyzed)) {
const iastContext = getIastContext(store)
if (store && !iastContext) return
this._reportIfVulnerable(value, iastContext, dialect)
}
}

_reportIfVulnerable (value, context, dialect) {
Expand All @@ -37,13 +56,17 @@ class SqlInjectionAnalyzer extends InjectionAnalyzer {

_report (value, context, dialect) {
const evidence = this._getEvidence(value, context, dialect)
const location = this._getLocation()
const location = this._getLocation(this._getExcludedLocations())
if (!this._isExcluded(location)) {
const spanId = context && context.rootSpan && context.rootSpan.context().toSpanId()
const vulnerability = createVulnerability(this._type, evidence, spanId, location)
addVulnerability(context, vulnerability)
}
}

_getExcludedLocations () {
return ['node_modules/mysql2', 'node_modules/sequelize']
}
}

module.exports = new SqlInjectionAnalyzer()
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,8 @@ class Analyzer extends Plugin {
return { value }
}

_getLocation () {
return getFirstNonDDPathAndLine()
_getLocation (externallyExcludedLocations) {
return getFirstNonDDPathAndLine(externallyExcludedLocations)
}

analyze (value) {
Expand Down
18 changes: 11 additions & 7 deletions packages/dd-trace/src/appsec/iast/path-line.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,12 @@ function getCallSiteInfo () {
return callsiteList
}

function getFirstNonDDPathAndLineFromCallsites (callsites) {
function getFirstNonDDPathAndLineFromCallsites (callsites, externallyExcludedLocations) {
if (callsites) {
for (let i = 0; i < callsites.length; i++) {
const callsite = callsites[i]
const filepath = callsite.getFileName()
if (!isExcluded(callsite) && filepath.indexOf(pathLine.ddBasePath) === -1) {
if (!isExcluded(callsite, externallyExcludedLocations) && filepath.indexOf(pathLine.ddBasePath) === -1) {
return {
path: path.relative(process.cwd(), filepath),
line: callsite.getLineNumber(),
Expand All @@ -54,14 +54,18 @@ function getFirstNonDDPathAndLineFromCallsites (callsites) {
return null
}

function isExcluded (callsite) {
function isExcluded (callsite, externallyExcludedPaths) {
if (callsite.isNative()) return true
const filename = callsite.getFileName()
if (!filename) {
return true
}
for (let i = 0; i < EXCLUDED_PATHS.length; i++) {
if (filename.indexOf(EXCLUDED_PATHS[i]) > -1) {
let excludedPaths = EXCLUDED_PATHS
if (externallyExcludedPaths) {
excludedPaths = [...excludedPaths, ...externallyExcludedPaths]
}
for (let i = 0; i < excludedPaths.length; i++) {
if (filename.indexOf(excludedPaths[i]) > -1) {
return true
}
}
Expand All @@ -73,7 +77,7 @@ function isExcluded (callsite) {
return false
}

function getFirstNonDDPathAndLine () {
return getFirstNonDDPathAndLineFromCallsites(getCallSiteInfo())
function getFirstNonDDPathAndLine (externallyExcludedLocations) {
return getFirstNonDDPathAndLineFromCallsites(getCallSiteInfo(), externallyExcludedLocations)
}
module.exports = pathLine
23 changes: 19 additions & 4 deletions packages/dd-trace/src/appsec/iast/taint-tracking/plugin.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,17 +12,32 @@ class TaintTrackingPlugin extends Plugin {
this._type = 'taint-tracking'
this.addSub(
'datadog:body-parser:read:finish',
({ req }) => this._taintTrackingHandler(HTTP_REQUEST_BODY, req, 'body')
({ req }) => {
const iastContext = getIastContext(storage.getStore())
if (iastContext && iastContext['body'] !== req.body) {
this._taintTrackingHandler(HTTP_REQUEST_BODY, req, 'body', iastContext)
iastContext['body'] = req.body
}
}
)
this.addSub(
'datadog:qs:parse:finish',
({ qs }) => this._taintTrackingHandler(HTTP_REQUEST_PARAMETER, qs))

this.addSub('apm:express:middleware:next', ({ req }) => {
if (req && req.body && typeof req.body === 'object') {
const iastContext = getIastContext(storage.getStore())
if (iastContext && iastContext['body'] !== req.body) {
this._taintTrackingHandler(HTTP_REQUEST_BODY, req, 'body', iastContext)
iastContext['body'] = req.body
}
}
})
}

_taintTrackingHandler (type, target, property) {
const iastContext = getIastContext(storage.getStore())
_taintTrackingHandler (type, target, property, iastContext = getIastContext(storage.getStore())) {
if (!property) {
target = taintObject(iastContext, target, type)
taintObject(iastContext, target, type)
} else {
target[property] = taintObject(iastContext, target[property], type)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,20 @@ const NUMERIC_LITERAL =
INTEGER_NUMBER + EXPONENT
].join('|')
})`
const ORACLE_ESCAPED_LITERAL = 'q\'<.*?>\'|q\'\\(.*?\\)\'|q\'\\{.*?\\}\'|q\'\\[.*?\\]\'|q\'(?<ESCAPE>.).*?\\k<ESCAPE>\''

class SqlSensitiveAnalyzer {
constructor () {
this._patterns = {
ANSI: new RegExp( // Default
[
NUMERIC_LITERAL,
STRING_LITERAL,
LINE_COMMENT,
BLOCK_COMMENT
].join('|'),
'gmi'
),
MYSQL: new RegExp(
[
NUMERIC_LITERAL,
Expand All @@ -43,13 +53,26 @@ class SqlSensitiveAnalyzer {
BLOCK_COMMENT
].join('|'),
'gmi'
)
),
ORACLE: new RegExp([
NUMERIC_LITERAL,
ORACLE_ESCAPED_LITERAL,
STRING_LITERAL,
LINE_COMMENT,
BLOCK_COMMENT
].join('|'),
'gmi')
}
this._patterns.SQLITE = this._patterns.MYSQL
this._patterns.MARIADB = this._patterns.MYSQL
}

extractSensitiveRanges (evidence) {
try {
const pattern = this._patterns[evidence.dialect]
let pattern = this._patterns[evidence.dialect]
if (!pattern) {
pattern = this._patterns['ANSI']
}
pattern.lastIndex = 0
const tokens = []

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ prepareTestServerForIast('integration test', (testThatRequestHasVulnerability, t
callArgs[vulnerableIndex] = newTaintedString(iastCtx, callArgs[vulnerableIndex], 'param', 'Request')
}
return fn(callArgs)
}, 'PATH_TRAVERSAL', 1)
}, 'PATH_TRAVERSAL', { occurrences: 1 })
})
describe('no vulnerable', () => {
testThatRequestHasNoVulnerability(function () {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
'use strict'

const fs = require('fs')
const os = require('os')
const path = require('path')
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')
const vulnerabilityReporter = require('../../../../src/appsec/iast/vulnerability-reporter')

describe('sql-injection-analyzer with sequelize', () => {
withVersions('sequelize', 'sequelize', sequelizeVersion => {
withVersions('mysql2', 'mysql2', (mysqlVersion) => {
let sequelize

prepareTestServerForIast('sequelize + mysql2',
(testThatRequestHasVulnerability, testThatRequestHasNoVulnerability) => {
beforeEach(() => {
const Sequelize = require(`../../../../../../versions/sequelize@${sequelizeVersion}`).get()
sequelize = new Sequelize('db', 'root', '', {
host: '127.0.0.1',
dialect: 'mysql'
})
vulnerabilityReporter.clearCache()
return sequelize.authenticate()
})

testThatRequestHasVulnerability(() => {
const store = storage.getStore()
const iastCtx = iastContextFunctions.getIastContext(store)

let sql = 'SELECT 1'
sql = newTaintedString(iastCtx, sql, 'param', 'Request')

return sequelize.query(sql)
}, 'SQL_INJECTION', { occurrences: 1 })

const externalFileContent = `'use strict'

module.exports = function (sequelize, sql) {
return sequelize.query(sql)
}
`
testThatRequestHasVulnerability(() => {
const filepath = path.join(os.tmpdir(), 'test-sequelize-sqli.js')
fs.writeFileSync(filepath, externalFileContent)

const store = storage.getStore()
const iastCtx = iastContextFunctions.getIastContext(store)

let sql = 'SELECT 1'
sql = newTaintedString(iastCtx, sql, 'param', 'Request')

return require(filepath)(sequelize, sql)
}, 'SQL_INJECTION', {
occurrences: 1,
location: {
path: 'test-sequelize-sqli.js',
line: 4
}
})

testThatRequestHasNoVulnerability(() => {
return sequelize.query('SELECT 1')
}, 'SQL_INJECTION')
})
})
})
})
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,12 @@ describe('sql-injection-analyzer', () => {
})

it('should subscribe to mysql, mysql2 and pg start query channel', () => {
expect(sqlInjectionAnalyzer._subscriptions).to.have.lengthOf(3)
expect(sqlInjectionAnalyzer._subscriptions).to.have.lengthOf(5)
expect(sqlInjectionAnalyzer._subscriptions[0]._channel.name).to.equals('apm:mysql:query:start')
expect(sqlInjectionAnalyzer._subscriptions[1]._channel.name).to.equals('apm:mysql2:query:start')
expect(sqlInjectionAnalyzer._subscriptions[2]._channel.name).to.equals('apm:pg:query:start')
expect(sqlInjectionAnalyzer._subscriptions[3]._channel.name).to.equals('datadog:sequelize:query:start')
expect(sqlInjectionAnalyzer._subscriptions[4]._channel.name).to.equals('datadog:sequelize:query:finish')
})

it('should not detect vulnerability when no query', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,6 @@ describe('weak-cipher-analyzer', () => {
const key = '1111111111111111'
const iv = 'abcdefgh'
crypto.createCipheriv(VULNERABLE_CIPHER, key, iv)
}, 'WEAK_CIPHER')
}, 'WEAK_CIPHER', { occurrences: 1 })
})
})
Loading