Detect SQL injection with sequelize

[uurien]

What does this PR do?

With sequelize is used as sequelize.query(sql) if the sql comes from the request, the point will be reported as vulnerable.

Motivation

we are already supporting frameworks like pg or mysql, but supporting sequelize we can support all the databases that it supports (oracle, sqlite...) and calculate an accurate vulnerability file and line.

Plugin Checklist

• Unit tests.

Additional Notes

For APM ownership changes:
This PR just adds hooks for sequelize library, to be able to analyze the query method.

[github-actions]

Overall package size

Self size: 4.19 MB
Deduped: 58.32 MB
No deduping: 58.36 MB

Dependency sizes

name	version	self size	total size
@datadog/pprof	2.2.1	14.24 MB	15.12 MB
@datadog/native-iast-taint-tracking	1.4.1	14.85 MB	14.86 MB
@datadog/native-appsec	3.1.0	13.31 MB	13.32 MB
protobufjs	7.1.2	2.76 MB	6.55 MB
@datadog/native-iast-rewriter	2.0.1	2.09 MB	2.1 MB
@datadog/native-metrics	2.0.0	898.77 kB	1.3 MB
opentracing	0.14.7	194.81 kB	194.81 kB
semver	7.3.8	88.2 kB	118.6 kB
@datadog/sketches-js	2.1.0	109.9 kB	109.9 kB
lodash.sortby	4.7.0	75.76 kB	75.76 kB
lru-cache	7.14.0	74.95 kB	74.95 kB
ipaddr.js	2.0.1	59.52 kB	59.52 kB
ignore	5.2.0	48.87 kB	48.87 kB
import-in-the-middle	1.3.5	34.34 kB	38.81 kB
istanbul-lib-coverage	3.2.0	29.34 kB	29.34 kB
retry	0.10.1	27.44 kB	27.44 kB
lodash.uniq	4.5.0	25.01 kB	25.01 kB
limiter	1.1.5	23.17 kB	23.17 kB
lodash.kebabcase	4.1.1	17.75 kB	17.75 kB
lodash.pick	4.4.0	16.33 kB	16.33 kB
node-abort-controller	3.0.1	14.33 kB	14.33 kB
crypto-randomuuid	1.0.0	11.18 kB	11.18 kB
diagnostics_channel	1.1.0	7.07 kB	7.07 kB
path-to-regexp	0.1.7	6.78 kB	6.78 kB
koalas	1.0.2	6.47 kB	6.47 kB
methods	1.1.2	5.29 kB	5.29 kB
module-details-from-path	1.0.3	4.47 kB	4.47 kB

_{🤖 This report was automatically generated by heaviest-objects-in-the-universe}

[codecov]

Codecov Report

Merging #3154 (da59b75) into master (15b2767) will increase coverage by 0.03%.
The diff coverage is 82.50%.

@@            Coverage Diff             @@
##           master    #3154      +/-   ##
==========================================
+ Coverage   86.54%   86.57%   +0.03%     
==========================================
  Files         333      333              
  Lines       11895    11922      +27     
  Branches       33       33              
==========================================
+ Hits        10295    10322      +27     
  Misses       1600     1600              

Impacted Files	Coverage Δ
...rc/appsec/iast/analyzers/sql-injection-analyzer.js	78.94% <53.33%> (-17.35%)	⬇️
...rc/appsec/iast/analyzers/vulnerability-analyzer.js	79.06% <100.00%> (ø)
packages/dd-trace/src/appsec/iast/path-line.js	100.00% <100.00%> (ø)
.../dd-trace/src/appsec/iast/taint-tracking/plugin.js	100.00% <100.00%> (+11.11%)	⬆️
...tion/sensitive-analyzers/sql-sensitive-analyzer.js	96.15% <100.00%> (+4.66%)	⬆️

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[pr-commenter]

Benchmarks

Comparing candidate commit da59b75 in PR branch ugaitz/fix-sequelize-sql-detection with baseline commit 15b2767 in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 441 metrics, 31 unstable metrics.

[Qard]

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

[uurien]

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.

[Qard]

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. 🤔

[uurien]

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.

[simon-id]

VERY weird syntax, i didn't know you could even do that, maybe it's worth to declare the functions above with names instead of putting everything inline

[uurien]

Maybe I can just do something like, that looks easier to read and understand.

shimmer.wrap(Sequelize.prototype, 'query', query => {
  return function (sql) {
    ...
  }
}

(I'm doing this because I've seen it in other files:

dd-trace-js/packages/datadog-instrumentations/src/mysql2.js

Line 15 in d349639

shimmer.wrap(Connection.prototype, 'addCommand', addCommand => function (cmd) {

[rochdev]

This is a fairly common pattern actually, but usually it would all be arrow functions, for example query => sql => {}. The reason for using regular functions originally was to be able to name them, but I'm not sure if that's relevant here.

[uurien]

If we use arrow functions to wrap methods, we will override the this parameter into the methods, because we are not going to be able to get the original this because in the arrow functions, this makes reference to the parent function this.