[APPSEC-10967] ASM parse response body by GustavoCaso · Pull Request #3153 · DataDog/dd-trace-rb

GustavoCaso · 2023-09-25T12:38:04Z

What does this PR do?

Parsed the response body and passed it to the waf as server.response.body.

We only parse bodies that are either an Array or a Rack::BodyProxy. Since we might not be the last middleware in the customer application, we can not consume the response body directly by calling each. To circumvent that, we make a copy of the body.

Parsing the response body could lead to performance implications for our customers. Since we have yet to learn how this would impact our customers, I added a configuration entry for them to skip the response body parsing altogether.

This documentation would remain undocumented, and we would only mention it to customers if they experience any performance degradation.

Motivation:

Additional Notes:

How to test the change?

For Datadog employees:

If this PR touches code that signs or publishes builds or packages, or handles
credentials of any kind, I've requested a review from @DataDog/security-design-and-guidance.
This PR doesn't touch any of that.

Unsure? Have a question? Request a review!

codecov-commenter · 2023-09-25T13:01:19Z

Codecov Report

Merging #3153 (22106fc) into master (1511f99) will increase coverage by 0.00%.
Report is 44 commits behind head on master.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3153   +/-   ##
=======================================
  Coverage   98.16%   98.16%           
=======================================
  Files        1283     1283           
  Lines       73915    74000   +85     
  Branches     3425     3433    +8     
=======================================
+ Hits        72559    72645   +86     
+ Misses       1356     1355    -1

Files Changed	Coverage Δ
...ib/datadog/appsec/contrib/rack/gateway/response.rb	`100.00% <100.00%> (ø)`
...b/datadog/appsec/contrib/rack/reactive/response.rb	`100.00% <100.00%> (ø)`
...tadog/appsec/contrib/rack/gateway/response_spec.rb	`100.00% <100.00%> (ø)`
...adog/appsec/contrib/rack/reactive/response_spec.rb	`100.00% <100.00%> (ø)`

... and 1 file with indirect coverage changes

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

lloeki

Overall ok, but I'm concerned about the fuzziness around content type conditions.

lloeki · 2023-09-25T13:59:23Z

Very good move! I was going to suggest doing that but you outran me!

lloeki · 2023-09-25T14:00:21Z

Hmm, that doesn't seem like the intended log message there.

lloeki · 2023-09-25T14:01:19Z

For Array this is not needed, so is that for Rack::BodyProxy?

Yes.

Since Rack::BodyProxy acts as a proxy. If we call each directly, we might iterate over the body and consume it.

https://github.com/rack/rack/blob/main/lib/rack/body_proxy.rb#L45-L58

If we call to to_ary we also close the body, and we do not want that, since we might not be the last middleware. That is why I thought using dup would save us here

But then you're duping the Rack::BodyProxy instance, which would still point to the same underlying wrapped object, which we don't know the type of, and may not be traversable twice.

lloeki · 2023-09-25T14:04:42Z

I'd rather be strict and explicitly list supported content types. I think those would be:

application/json (the official one registered at IANA)

text/json because someone from the team added it? I don't think I've seen it in the (Ruby) wild, ever.

lloeki · 2023-09-25T14:07:17Z

I'd rather be strict and explicitly list supported content types.

Indeed as is this would match at least text/plain and text/html but is there any point of passing those raw to libddwaf in the non-raw address that aims to carry parsed, structured data?

Is it for text/xml, which according to RFC 3023 says:

If an XML document -- that is, the unprocessed, source XML document
-- is readable by casual users, text/xml is preferable to
application/xml. MIME user agents (and web user agents) that do not
have explicit support for text/xml will treat it as text/plain, for
example, by displaying the XML MIME entity as plain text.
Application/xml is preferable when the XML MIME entity is unreadable
by casual users.

You are right, since we do not parse, there is no schema information to extract from it

I will remove the entire support for text/* content-type

lloeki · 2023-09-25T14:14:21Z

That json? tests that folks have been advertising the body as being JSON, but if content is broken JSON it'd blow up with a JSON::ParserError.

We should guard against that and return.

lloeki · 2023-09-25T14:15:57Z

Then it's not really parsed, is it? Either it's raw (server.response.body.raw) or it's parsed (server.response.body).

Since the goal is to parse it - notably to perform schema extraction - it seems to me there's not much point doing that.

GustavoCaso · 2023-09-25T15:35:00Z

I have no idea how that came to be there :weird:

using :each

github-actions Bot added appsec Application Security monitoring product integrations Involves tracing integrations labels Sep 25, 2023

GustavoCaso force-pushed the asm-response-body-waf-address branch from 22106fc to 9ed1001 Compare September 25, 2023 13:10

GustavoCaso marked this pull request as ready for review September 25, 2023 13:31

GustavoCaso requested a review from a team as a code owner September 25, 2023 13:31

GustavoCaso changed the title ~~Asm response body waf address~~ [APPSEC-10967] ASM parse response body Sep 25, 2023

lloeki approved these changes Sep 25, 2023

View reviewed changes

GustavoCaso commented Sep 25, 2023

View reviewed changes

GustavoCaso force-pushed the asm-response-body-waf-address branch 2 times, most recently from 62d7be1 to 0a2d379 Compare September 27, 2023 13:26

Gustavo Caso added 6 commits October 19, 2023 15:17

Parse response body information and propagate it to the WAF.

039f14f

Add support for Rack::BodyProxy

6ef1c90

enable a way to disable parse response body

0b74e23

only supoprt content-type */josn when parsing response body

4b3eaf1

Make sure to call :to_ary on the response body before iterating over it

b195274

using :each

Make sure to only replace the response body if is not empty

439a736

GustavoCaso force-pushed the asm-response-body-waf-address branch 2 times, most recently from bcd6e90 to 2a36a14 Compare October 23, 2023 10:31

Gustavo Caso added 3 commits October 23, 2023 13:49

make sure that parsing the response body is disable by default

0a85874

Add the ASM API security scenarios to the benchmark pipeline

7c86a43

fix typo

72ace59

GustavoCaso force-pushed the asm-response-body-waf-address branch from 2a36a14 to 72ace59 Compare October 23, 2023 11:50

GustavoCaso merged commit f725c43 into master Oct 23, 2023

GustavoCaso deleted the asm-response-body-waf-address branch October 23, 2023 12:39

GustavoCaso mentioned this pull request Oct 25, 2023

Backport ASM API security parse response body #3224

Closed

2 tasks

marcotc added this to the 1.16.0 milestone Oct 25, 2023

GustavoCaso restored the asm-response-body-waf-address branch November 10, 2023 14:22

GustavoCaso mentioned this pull request Nov 10, 2023

revert https://github.com/DataDog/dd-trace-rb/pull/3153 #3252

Merged

2 tasks

GustavoCaso pushed a commit that referenced this pull request Nov 10, 2023

revert #3153

93957a0

ekump pushed a commit that referenced this pull request Nov 10, 2023

revert #3153 (#3252)

92e379a

Uh oh!

Conversation

GustavoCaso commented Sep 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Sep 25, 2023

Codecov Report

Uh oh!

lloeki left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

GustavoCaso commented Sep 25, 2023 •

edited

Loading