-
Notifications
You must be signed in to change notification settings - Fork 291
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added support for SQLi exploit prevention (detection only) #7051
Conversation
65f7400
to
4f59176
Compare
d450504
to
1fc0cc5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing configuration flag, and possibly RC capability announcement.
RequestContext ctx = span.getRequestContext(); | ||
if (ctx != null) { | ||
String dbType = dbInfo.getType(); | ||
if (dbType != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the type is always non-null. In case of errors parsing the jdbc url, it takes the value database
as default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm convinced that is possible to have null
type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For instance here HibernateDecorator.dbType()
or here DatanucleusDecorator.dbType()
* The method used to provide raw sql to prevent SQL-injection attacks SQL query should never be | ||
* exposed because it may contain sensitive data. | ||
*/ | ||
public AgentSpan onStatementRaw(AgentSpan span, String sql) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we move this into datadog.trace.bootstrap.instrumentation.decorator.DatabaseClientDecorator
it can be used by other clients like vertx_sql_client
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch, moved to DatabaseClientDecorator
6de0ff7
to
7ba14a8
Compare
...src/main/java/datadog/trace/bootstrap/instrumentation/decorator/DatabaseClientDecorator.java
Outdated
Show resolved
Hide resolved
...src/main/java/datadog/trace/bootstrap/instrumentation/decorator/DatabaseClientDecorator.java
Outdated
Show resolved
Hide resolved
dd-trace-api/src/main/java/datadog/trace/api/ConfigDefaults.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a future PR, we are very close to have it also available for vertx sql.
9a1a62b
to
4321a50
Compare
@@ -113,6 +113,7 @@ public static AgentScope onEnter( | |||
appendComment); | |||
} | |||
DECORATE.onStatement(span, DBQueryInfo.ofStatement(copy)); | |||
DECORATE.onStatementRaw(span, sql); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would use copy
here because the original might be modified by dbm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well spotted! Fixed
* The method used to provide raw sql to prevent SQL-injection attacks SQL query should never be | ||
* exposed because it may contain sensitive data. | ||
*/ | ||
public AgentSpan onStatementRaw(AgentSpan span, String sql) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would just have one method to call: either onStatement
for prepared ones either onRawStatement
for raw ones. onRawStatement
may call onStatement
upon return
This will leave the orchestration responsability to the decorator and not have to call 2 methods on the instrumentation itself
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored as suggested
BenchmarksStartupParameters
See matching parameters
SummaryFound 0 performance improvements and 2 performance regressions! Performance is the same for 45 metrics, 16 unstable metrics.
Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.35.0-SNAPSHOT~967969c98f, baseline=1.35.0-SNAPSHOT~489de243b6
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.058 s) : 0, 1058211
Total [baseline] (10.339 s) : 0, 10339226
Agent [candidate] (1.057 s) : 0, 1057233
Total [candidate] (10.346 s) : 0, 10345791
section appsec
Agent [baseline] (1.176 s) : 0, 1175916
Total [baseline] (10.518 s) : 0, 10517897
Agent [candidate] (1.177 s) : 0, 1176586
Total [candidate] (10.494 s) : 0, 10494376
section iast
Agent [baseline] (1.164 s) : 0, 1163773
Total [baseline] (10.767 s) : 0, 10766693
Agent [candidate] (1.163 s) : 0, 1163234
Total [candidate] (10.756 s) : 0, 10755874
section profiling
Agent [baseline] (1.258 s) : 0, 1258135
Total [baseline] (10.593 s) : 0, 10592998
Agent [candidate] (1.263 s) : 0, 1263449
Total [candidate] (10.484 s) : 0, 10483778
gantt
title petclinic - break down per module: candidate=1.35.0-SNAPSHOT~967969c98f, baseline=1.35.0-SNAPSHOT~489de243b6
dateFormat X
axisFormat %s
section tracing
BytebuddyAgent [baseline] (652.731 ms) : 0, 652731
BytebuddyAgent [candidate] (651.638 ms) : 0, 651638
GlobalTracer [baseline] (312.922 ms) : 0, 312922
GlobalTracer [candidate] (312.614 ms) : 0, 312614
AppSec [baseline] (49.948 ms) : 0, 49948
AppSec [candidate] (50.25 ms) : 0, 50250
Remote Config [baseline] (668.462 µs) : 0, 668
Remote Config [candidate] (736.325 µs) : 0, 736
Telemetry [baseline] (7.601 ms) : 0, 7601
Telemetry [candidate] (7.543 ms) : 0, 7543
section appsec
BytebuddyAgent [baseline] (674.745 ms) : 0, 674745
BytebuddyAgent [candidate] (674.755 ms) : 0, 674755
GlobalTracer [baseline] (296.14 ms) : 0, 296140
GlobalTracer [candidate] (295.394 ms) : 0, 295394
AppSec [baseline] (152.536 ms) : 0, 152536
AppSec [candidate] (153.271 ms) : 0, 153271
Remote Config [baseline] (621.62 µs) : 0, 622
Remote Config [candidate] (620.103 µs) : 0, 620
Telemetry [baseline] (8.112 ms) : 0, 8112
Telemetry [candidate] (8.707 ms) : 0, 8707
IAST [baseline] (18.732 ms) : 0, 18732
IAST [candidate] (18.723 ms) : 0, 18723
section iast
BytebuddyAgent [baseline] (777.276 ms) : 0, 777276
BytebuddyAgent [candidate] (776.686 ms) : 0, 776686
GlobalTracer [baseline] (291.812 ms) : 0, 291812
GlobalTracer [candidate] (291.989 ms) : 0, 291989
AppSec [baseline] (47.803 ms) : 0, 47803
AppSec [candidate] (49.026 ms) : 0, 49026
Remote Config [baseline] (598.643 µs) : 0, 599
Remote Config [candidate] (587.778 µs) : 0, 588
Telemetry [baseline] (8.248 ms) : 0, 8248
Telemetry [candidate] (8.492 ms) : 0, 8492
IAST [baseline] (24.814 ms) : 0, 24814
IAST [candidate] (23.214 ms) : 0, 23214
section profiling
BytebuddyAgent [baseline] (661.298 ms) : 0, 661298
BytebuddyAgent [candidate] (664.089 ms) : 0, 664089
GlobalTracer [baseline] (385.045 ms) : 0, 385045
GlobalTracer [candidate] (386.6 ms) : 0, 386600
AppSec [baseline] (50.425 ms) : 0, 50425
AppSec [candidate] (51.237 ms) : 0, 51237
Remote Config [baseline] (835.047 µs) : 0, 835
Remote Config [candidate] (749.868 µs) : 0, 750
Telemetry [baseline] (7.495 ms) : 0, 7495
Telemetry [candidate] (7.409 ms) : 0, 7409
ProfilingAgent [baseline] (96.499 ms) : 0, 96499
ProfilingAgent [candidate] (96.572 ms) : 0, 96572
Profiling [baseline] (96.524 ms) : 0, 96524
Profiling [candidate] (96.596 ms) : 0, 96596
Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.35.0-SNAPSHOT~967969c98f, baseline=1.35.0-SNAPSHOT~489de243b6
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.063 s) : 0, 1062899
Total [baseline] (8.541 s) : 0, 8540919
Agent [candidate] (1.059 s) : 0, 1058802
Total [candidate] (8.567 s) : 0, 8566744
section iast
Agent [baseline] (1.165 s) : 0, 1165209
Total [baseline] (9.022 s) : 0, 9021601
Agent [candidate] (1.175 s) : 0, 1174768
Total [candidate] (9.047 s) : 0, 9046830
section iast_HARDCODED_SECRET_DISABLED
Agent [baseline] (1.172 s) : 0, 1171606
Total [baseline] (8.955 s) : 0, 8954826
Agent [candidate] (1.164 s) : 0, 1164073
Total [candidate] (8.981 s) : 0, 8981148
section iast_TELEMETRY_OFF
Agent [baseline] (1.169 s) : 0, 1169000
Total [baseline] (9.05 s) : 0, 9049532
Agent [candidate] (1.167 s) : 0, 1167337
Total [candidate] (8.977 s) : 0, 8976567
gantt
title insecure-bank - break down per module: candidate=1.35.0-SNAPSHOT~967969c98f, baseline=1.35.0-SNAPSHOT~489de243b6
dateFormat X
axisFormat %s
section tracing
BytebuddyAgent [baseline] (655.635 ms) : 0, 655635
BytebuddyAgent [candidate] (652.432 ms) : 0, 652432
GlobalTracer [baseline] (314.393 ms) : 0, 314393
GlobalTracer [candidate] (313.04 ms) : 0, 313040
AppSec [baseline] (49.979 ms) : 0, 49979
AppSec [candidate] (50.595 ms) : 0, 50595
Remote Config [baseline] (670.693 µs) : 0, 671
Remote Config [candidate] (724.482 µs) : 0, 724
Telemetry [baseline] (7.575 ms) : 0, 7575
Telemetry [candidate] (7.588 ms) : 0, 7588
section iast
BytebuddyAgent [baseline] (777.677 ms) : 0, 777677
BytebuddyAgent [candidate] (783.658 ms) : 0, 783658
GlobalTracer [baseline] (291.884 ms) : 0, 291884
GlobalTracer [candidate] (294.176 ms) : 0, 294176
AppSec [baseline] (50.196 ms) : 0, 50196
AppSec [candidate] (51.277 ms) : 0, 51277
IAST [baseline] (23.962 ms) : 0, 23962
IAST [candidate] (23.277 ms) : 0, 23277
Remote Config [baseline] (607.722 µs) : 0, 608
Remote Config [candidate] (600.066 µs) : 0, 600
Telemetry [baseline] (7.653 ms) : 0, 7653
Telemetry [candidate] (8.406 ms) : 0, 8406
section iast_HARDCODED_SECRET_DISABLED
BytebuddyAgent [baseline] (781.455 ms) : 0, 781455
BytebuddyAgent [candidate] (776.647 ms) : 0, 776647
GlobalTracer [baseline] (293.935 ms) : 0, 293935
GlobalTracer [candidate] (292.198 ms) : 0, 292198
AppSec [baseline] (50.931 ms) : 0, 50931
AppSec [candidate] (50.448 ms) : 0, 50448
IAST [baseline] (23.809 ms) : 0, 23809
IAST [candidate] (23.192 ms) : 0, 23192
Remote Config [baseline] (587.41 µs) : 0, 587
Remote Config [candidate] (586.8 µs) : 0, 587
Telemetry [baseline] (7.56 ms) : 0, 7560
Telemetry [candidate] (7.715 ms) : 0, 7715
section iast_TELEMETRY_OFF
BytebuddyAgent [baseline] (780.82 ms) : 0, 780820
BytebuddyAgent [candidate] (777.924 ms) : 0, 777924
GlobalTracer [baseline] (293.446 ms) : 0, 293446
GlobalTracer [candidate] (293.013 ms) : 0, 293013
AppSec [baseline] (49.587 ms) : 0, 49587
AppSec [candidate] (51.291 ms) : 0, 51291
IAST [baseline] (23.652 ms) : 0, 23652
IAST [candidate] (23.651 ms) : 0, 23651
Remote Config [baseline] (602.27 µs) : 0, 602
Remote Config [candidate] (612.543 µs) : 0, 613
Telemetry [baseline] (7.56 ms) : 0, 7560
Telemetry [candidate] (7.516 ms) : 0, 7516
LoadParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 6 metrics, 22 unstable metrics. Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.35.0-SNAPSHOT~967969c98f, baseline=1.35.0-SNAPSHOT~489de243b6
dateFormat X
axisFormat %s
section baseline
no_agent (452.559 µs) : 424, 481
. : milestone, 453,
iast (586.244 µs) : 555, 618
. : milestone, 586,
iast_FULL (697.308 µs) : 665, 730
. : milestone, 697,
iast_GLOBAL (615.488 µs) : 584, 647
. : milestone, 615,
iast_HARDCODED_SECRET_DISABLED (585.08 µs) : 554, 616
. : milestone, 585,
iast_INACTIVE (556.647 µs) : 525, 588
. : milestone, 557,
iast_TELEMETRY_OFF (571.71 µs) : 541, 602
. : milestone, 572,
tracing (547.794 µs) : 518, 578
. : milestone, 548,
section candidate
no_agent (453.28 µs) : 424, 482
. : milestone, 453,
iast (586.654 µs) : 555, 619
. : milestone, 587,
iast_FULL (685.523 µs) : 653, 718
. : milestone, 686,
iast_GLOBAL (621.034 µs) : 589, 653
. : milestone, 621,
iast_HARDCODED_SECRET_DISABLED (587.054 µs) : 556, 618
. : milestone, 587,
iast_INACTIVE (559.102 µs) : 528, 590
. : milestone, 559,
iast_TELEMETRY_OFF (573.783 µs) : 543, 605
. : milestone, 574,
tracing (541.456 µs) : 512, 571
. : milestone, 541,
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.35.0-SNAPSHOT~967969c98f, baseline=1.35.0-SNAPSHOT~489de243b6
dateFormat X
axisFormat %s
section baseline
no_agent (1.71 ms) : 1685, 1734
. : milestone, 1710,
appsec (2.214 ms) : 2183, 2246
. : milestone, 2214,
appsec_no_iast (2.184 ms) : 2152, 2216
. : milestone, 2184,
iast (1.9 ms) : 1870, 1929
. : milestone, 1900,
profiling (1.942 ms) : 1908, 1975
. : milestone, 1942,
tracing (1.853 ms) : 1823, 1883
. : milestone, 1853,
section candidate
no_agent (1.703 ms) : 1678, 1727
. : milestone, 1703,
appsec (2.18 ms) : 2147, 2212
. : milestone, 2180,
appsec_no_iast (2.19 ms) : 2157, 2223
. : milestone, 2190,
iast (1.857 ms) : 1825, 1890
. : milestone, 1857,
profiling (1.877 ms) : 1846, 1908
. : milestone, 1877,
tracing (1.855 ms) : 1822, 1887
. : milestone, 1855,
DacapoParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 12 metrics, 0 unstable metrics. Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.35.0-SNAPSHOT~967969c98f, baseline=1.35.0-SNAPSHOT~489de243b6
dateFormat X
axisFormat %s
section baseline
no_agent (1.457 ms) : 1446, 1469
. : milestone, 1457,
appsec (2.193 ms) : 2159, 2227
. : milestone, 2193,
iast (1.96 ms) : 1919, 2001
. : milestone, 1960,
iast_GLOBAL (2.013 ms) : 1972, 2055
. : milestone, 2013,
profiling (1.838 ms) : 1804, 1872
. : milestone, 1838,
tracing (1.82 ms) : 1788, 1851
. : milestone, 1820,
section candidate
no_agent (1.464 ms) : 1452, 1475
. : milestone, 1464,
appsec (2.214 ms) : 2179, 2248
. : milestone, 2214,
iast (1.962 ms) : 1921, 2003
. : milestone, 1962,
iast_GLOBAL (1.987 ms) : 1946, 2027
. : milestone, 1987,
profiling (1.857 ms) : 1824, 1890
. : milestone, 1857,
tracing (1.829 ms) : 1797, 1861
. : milestone, 1829,
Execution time for biojavagantt
title biojava - execution time [CI 0.99] : candidate=1.35.0-SNAPSHOT~967969c98f, baseline=1.35.0-SNAPSHOT~489de243b6
dateFormat X
axisFormat %s
section baseline
no_agent (15.566 s) : 15566000, 15566000
. : milestone, 15566000,
appsec (14.918 s) : 14918000, 14918000
. : milestone, 14918000,
iast (18.875 s) : 18875000, 18875000
. : milestone, 18875000,
iast_GLOBAL (17.994 s) : 17994000, 17994000
. : milestone, 17994000,
profiling (15.392 s) : 15392000, 15392000
. : milestone, 15392000,
tracing (15.01 s) : 15010000, 15010000
. : milestone, 15010000,
section candidate
no_agent (14.943 s) : 14943000, 14943000
. : milestone, 14943000,
appsec (15.179 s) : 15179000, 15179000
. : milestone, 15179000,
iast (18.669 s) : 18669000, 18669000
. : milestone, 18669000,
iast_GLOBAL (17.883 s) : 17883000, 17883000
. : milestone, 17883000,
profiling (15.302 s) : 15302000, 15302000
. : milestone, 15302000,
tracing (14.94 s) : 14940000, 14940000
. : milestone, 14940000,
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks ok from instrumentations pov
73e362e
to
6ef6fc8
Compare
} | ||
} | ||
} | ||
return span; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like there's no need to return span
b/o it's never modified inside of this method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
@@ -109,6 +109,10 @@ public interface KnownAddresses { | |||
|
|||
Address<String> USER_ID = new Address<>("usr.id"); | |||
|
|||
Address<String> DB_TYPE = new Address<>("server.db.system"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps add a short javadoc for these new fields, similar to most other fields in this class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
2e8685e
to
967969c
Compare
@@ -96,7 +97,8 @@ private void subscribeConfigurationPoller() { | |||
| CAPABILITY_ASM_USER_BLOCKING | |||
| CAPABILITY_ASM_CUSTOM_RULES | |||
| CAPABILITY_ASM_CUSTOM_BLOCKING_RESPONSE | |||
| CAPABILITY_ASM_TRUSTED_IPS); | |||
| CAPABILITY_ASM_TRUSTED_IPS | |||
| CAPABILITY_ASM_RASP_SQLI); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably not announce the capability if RASP is disabled.
967969c
to
0020d24
Compare
What Does This Do
This PR introduces support for SQL capturing which can be used for detecting SQL injection attacks, e.i. added ability to collect data for
server.db.system
andserver.db.statement
addresses via instrumentation gateway with subsequent forwarding to RASP (ex-WAF).server.db.system
- sql dialect (mysql, postgresql, etc.)server.db.statement
- raw sql-query to check for sql-injection attacksMotivation
This PR is part of ASM Exploit Prevention initiative (Runtime application self-protection).
Additional Notes
The current set of dialects supported is:
Jira ticket: APPSEC-47228