Detect SSRF vulnerabilities#3115
Conversation
Overall package sizeSelf size: 4.2 MB Dependency sizes
🤖 This report was automatically generated by heaviest-objects-in-the-universe |
Codecov Report
@@ 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
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
acacd0b to
f72302d
Compare
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 }}
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
b395ccf to
dfc5683
Compare
7df429a to
d88d801
Compare
987269f to
2e9a6db
Compare
7c57e91 to
4a8f5e5
Compare
b40735e to
8b06a55
Compare
8b06a55 to
8a88678
Compare
* 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
* 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
* 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
* 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
* 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
* 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
4 participants
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