Detect SSRF vulnerabilities #3115

uurien · 2023-05-07T12:55:58Z

What does this PR do?

When it detects than a string that comes from request data (GET parameter, body, ...) is used to make a new request, it identifies a SSRF vulnerability, because an attacker can force a request to an external page.

Motivation

SSRF vulnerability detection is a feature that tracer IAST should implement.

Plugin Checklist

Unit tests.

github-actions · 2023-05-07T12:56:43Z

Overall package size

Self size: 4.2 MB
Deduped: 58.39 MB
No deduping: 58.44 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.2.0	13.38 MB	13.39 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 · 2023-05-07T12:58:48Z

Codecov Report

Merging #3115 (8a88678) into master (d638b4b) will decrease coverage by 0.06%.
The diff coverage is 70.73%.

@@            Coverage Diff             @@
##           master    #3115      +/-   ##
==========================================
- Coverage   86.57%   86.52%   -0.06%     
==========================================
  Files         333      335       +2     
  Lines       11934    11974      +40     
  Branches       33       33              
==========================================
+ Hits        10332    10360      +28     
- Misses       1602     1614      +12

Impacted Files	Coverage Δ
...es/dd-trace/src/appsec/iast/analyzers/analyzers.js	`100.00% <ø> (ø)`
...ckages/dd-trace/src/appsec/iast/vulnerabilities.js	`100.00% <ø> (ø)`
...tion/sensitive-analyzers/url-sensitive-analyzer.js	`50.00% <50.00%> (ø)`
...kages/datadog-instrumentations/src/http2/client.js	`97.22% <66.66%> (-2.78%)`	⬇️
...ckages/datadog-instrumentations/src/http/client.js	`94.80% <100.00%> (+0.06%)`	⬆️
...d-trace/src/appsec/iast/analyzers/ssrf-analyzer.js	`100.00% <100.00%> (ø)`
...-formatter/evidence-redaction/sensitive-handler.js	`100.00% <100.00%> (ø)`

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

pr-commenter · 2023-05-07T13:04:33Z

Benchmarks

Comparing candidate commit 8a88678 in PR branch ugaitz/ssrf-vulnerability-detection with baseline commit d638b4b in branch master.

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

uurien · 2023-05-07T14:07:12Z

packages/datadog-instrumentations/src/http/client.js

@@ -49,7 +49,7 @@ function patch (http, methodName) {
      const asyncResource = new AsyncResource('bound-anonymous-fn')

      return asyncResource.runInAsyncScope(() => {
-        startClientCh.publish({ args, http })
+        startClientCh.publish({ args, http, originalArgs: arguments })


To be able to detect that the arguments are coming from the request, we need the values that the customer is using in the code, and not the normalized args object created by the tracer.

uurien · 2023-05-07T14:08:29Z

packages/datadog-instrumentations/src/http2/client.js

@@ -52,6 +53,9 @@ function createWrapRequest (authority, options) {

 function wrapConnect (connect) {
  return function (authority, options) {
+    if (connectChannel.hasSubscribers) {
+      connectChannel.publish({ args: arguments })


To detect the SSRF vulnerability, we need to check the original parameter of the connect method, we don't need the events and the data that the tracer needs.

My only problem with this is that the goal of args was so that the subscriber doesn't need to know about the structure of the arguments. If we're gonna move this to the subscriber, it should be moved fully. But I would actually prefer if we did the opposite and do both the normalization and denormalization on the publisher side. In any case, any of the two would be fine with me, but let's not have both approaches at the same time.

Ok, make sense. The file which understand about the arguments in the instrumentation file. I've changed it to send more normalized args: { authority }
I've changed it also in packages/datadog-instrumentations/src/http/client.js to send { ..., originalArgs: { wholeUrl, options }}

The new implementation seems to have the same issue and it's duplicating what is passed to the subscriber. The normalizing/denormalizing of arguments should only happen in once place.

I think that you are talking about the changes in packages/datadog-instrumentations/src/http/client.js.

Yes, you are right, we were normalizing it twice. I removed my normalizing method and modify the existing just to add the parameter that we need.

rochdev · 2023-05-09T22:05:35Z

packages/datadog-instrumentations/src/http2/client.js

@@ -52,6 +53,9 @@ function createWrapRequest (authority, options) {

 function wrapConnect (connect) {
  return function (authority, options) {
+    if (connectChannel.hasSubscribers) {
+      connectChannel.publish({ args: arguments })


My only problem with this is that the goal of args was so that the subscriber doesn't need to know about the structure of the arguments. If we're gonna move this to the subscriber, it should be moved fully. But I would actually prefer if we did the opposite and do both the normalization and denormalization on the publisher side. In any case, any of the two would be fine with me, but let's not have both approaches at the same time.

packages/dd-trace/src/appsec/iast/analyzers/ssrf-analyzer.js

packages/datadog-instrumentations/src/http/client.js

* Detect SSRF vulnerabilities * Fix test * Add space * Understand arguments in publisher instead of the subscriber * Redact sensitive information in SSRF vulnerabilities * Tiny style change * Tiny code styles * Use SSRF enum instead of literal * Try to reduce flaky test * Rename originalArgs to originalUrlAndOptions * Fix comment in PR * Do not normalize arguments twice in http/client.js

uurien added the semver-patch label May 7, 2023

uurien force-pushed the ugaitz/ssrf-vulnerability-detection branch from acacd0b to f72302d Compare May 7, 2023 13:21

uurien commented May 7, 2023

View reviewed changes

uurien marked this pull request as ready for review May 7, 2023 14:09

uurien requested review from a team as code owners May 7, 2023 14:09

uurien marked this pull request as draft May 8, 2023 07:52

rochdev requested changes May 9, 2023

View reviewed changes

uurien force-pushed the ugaitz/ssrf-vulnerability-detection branch from b395ccf to dfc5683 Compare May 14, 2023 13:14

uurien marked this pull request as ready for review May 15, 2023 06:23

CarlesDD requested changes May 15, 2023

View reviewed changes

packages/dd-trace/src/appsec/iast/analyzers/ssrf-analyzer.js Outdated Show resolved Hide resolved

uurien requested review from CarlesDD and rochdev May 15, 2023 12:22

CarlesDD previously approved these changes May 15, 2023

View reviewed changes

uurien force-pushed the ugaitz/ssrf-vulnerability-detection branch from 7df429a to d88d801 Compare May 18, 2023 07:19

uurien added the asm-iast label May 18, 2023

uurien dismissed CarlesDD’s stale review via 987269f May 18, 2023 11:53

uurien force-pushed the ugaitz/ssrf-vulnerability-detection branch from 987269f to 2e9a6db Compare May 19, 2023 07:24

iunanua reviewed May 22, 2023

View reviewed changes

packages/dd-trace/src/appsec/iast/analyzers/ssrf-analyzer.js Outdated Show resolved Hide resolved

iunanua reviewed May 22, 2023

View reviewed changes

packages/datadog-instrumentations/src/http/client.js Outdated Show resolved Hide resolved

iunanua previously approved these changes May 23, 2023

View reviewed changes

uurien force-pushed the ugaitz/ssrf-vulnerability-detection branch from 7c57e91 to 4a8f5e5 Compare May 23, 2023 12:42

uurien dismissed iunanua’s stale review via b40735e May 24, 2023 06:45

uurien force-pushed the ugaitz/ssrf-vulnerability-detection branch from b40735e to 8b06a55 Compare May 25, 2023 07:37

uurien added 2 commits May 30, 2023 16:27

Detect SSRF vulnerabilities

f854e91

Fix test

d1d6f63

uurien added 10 commits May 30, 2023 16:27

Add space

67548ab

Understand arguments in publisher instead of the subscriber

028ca45

Redact sensitive information in SSRF vulnerabilities

cd7fde0

Tiny style change

9d20a01

Tiny code styles

f61c2d8

Use SSRF enum instead of literal

6f9b308

Try to reduce flaky test

9644617

Rename originalArgs to originalUrlAndOptions

34f05fb

Fix comment in PR

a399530

Do not normalize arguments twice in http/client.js

8a88678

uurien force-pushed the ugaitz/ssrf-vulnerability-detection branch from 8b06a55 to 8a88678 Compare May 30, 2023 14:28

uurien requested review from iunanua and CarlesDD May 30, 2023 14:28

iunanua approved these changes May 30, 2023

View reviewed changes

CarlesDD approved these changes May 30, 2023

View reviewed changes

rochdev approved these changes Jun 1, 2023

View reviewed changes

uurien merged commit 0c64dfc into master Jun 1, 2023
104 checks passed

uurien deleted the ugaitz/ssrf-vulnerability-detection branch June 1, 2023 06:23

This was referenced Jun 1, 2023

v2.35.0 proposal #3206

Merged

v4.1.0 proposal #3209

Merged

v3.22.0 proposal #3207

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Detect SSRF vulnerabilities #3115

Detect SSRF vulnerabilities #3115

uurien commented May 7, 2023 •

edited

github-actions bot commented May 7, 2023 •

edited

codecov bot commented May 7, 2023 •

edited

pr-commenter bot commented May 7, 2023 •

edited

uurien May 7, 2023

uurien May 7, 2023

rochdev May 9, 2023

uurien May 15, 2023 •

edited

rochdev May 23, 2023

uurien May 24, 2023

rochdev May 9, 2023

Detect SSRF vulnerabilities #3115

Detect SSRF vulnerabilities #3115

Conversation

uurien commented May 7, 2023 • edited

What does this PR do?

Motivation

Plugin Checklist

github-actions bot commented May 7, 2023 • edited

Overall package size

Dependency sizes

codecov bot commented May 7, 2023 • edited

Codecov Report

pr-commenter bot commented May 7, 2023 • edited

Benchmarks

uurien May 7, 2023

Choose a reason for hiding this comment

uurien May 7, 2023

Choose a reason for hiding this comment

rochdev May 9, 2023

Choose a reason for hiding this comment

uurien May 15, 2023 • edited

Choose a reason for hiding this comment

rochdev May 23, 2023

Choose a reason for hiding this comment

uurien May 24, 2023

Choose a reason for hiding this comment

rochdev May 9, 2023

Choose a reason for hiding this comment

uurien commented May 7, 2023 •

edited

github-actions bot commented May 7, 2023 •

edited

codecov bot commented May 7, 2023 •

edited

pr-commenter bot commented May 7, 2023 •

edited

uurien May 15, 2023 •

edited