Skip to content
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

Forwardport "Update to libddwaf 1.5.1" #2320

Merged
merged 31 commits into from
Oct 19, 2022
Merged

Conversation

lloeki
Copy link
Contributor

@lloeki lloeki commented Oct 18, 2022

What does this PR do?

Forward port #2306

Motivation

Apply changes done to 1.5-stable related to libddwaf update.

Also update vendored rulesets to 1.4.1

Notable libddwaf API changes include:
- ability to set sideband rule data
- ability to toggle rules
- proper separation of return code vs decided action
- ability to hint at multiple actions
- non-reliance on garbage collector (finalization must be explicit)
While the presence of a ruby-platform gem for these should be picked up
by bundler, sometimes it is not. Also, this makes it a bit more
future-proof, so that bundler doesn't attempt to pick a version that has
no ruby platform gem, and then proceed to fail.
While the presence of a ruby-platform gem for these should be picked up
by bundler, sometimes it is not. Also, this makes it a bit more
future-proof, so that bundler doesn't attempt to pick a version that has
no ruby platform gem, and then proceed to fail.
Some AppSec spec examples have to be tested against rack-contrib
Previoulsy setting the tag would work but the change would be silently
dropped, resulting in the change being absent from the final trace.
Prevents accumulation of instrumentation middlewares if multiple
configure blocks are being called, like over app hot-reloading (e.g
Rails development mode, which rereads initializers) or across a sequence
of examples within a spec suite.

Since watchers are essentially static, they need only to be set up once
per process, ever.
Rack and Rails are lazily populating upon access. Depending on the
access pattern this may mean that body data would not be available to
AppSec for analysis. This is worked around by a call to the
side-effectful accessors.
Consequently, context will not be set in Rack env, trickling down to
either disablement or enablement of other instrumented calls,
consistently for the whole request.
This would apply to handle as well, yet currently handle is a value that
exists only once per application, so is never to be freed since there is
no place for its finalize to be called.
This covers the following AppSec integrations:
- Rack
- Rails
- Sinatra
JSONBodyParser replaces PostBodyContentTypeParser
With POST requests, a CSRF token is theoretically needed, but we have
none. Skip the filter, using the appropriate method depending on Rails
versions. Also the mock app may not have the filter defined.
Prior to 0.7 an argument is lacking to generate multipart requests
without uploading a file.
@lloeki lloeki requested a review from a team October 18, 2022 13:47
@github-actions github-actions bot added the appsec Application Security monitoring product label Oct 18, 2022
@github-actions github-actions bot added the integrations Involves tracing integrations label Oct 18, 2022
# skip CSRF token check for non-GET requests
begin
if respond_to?(:skip_before_action)
skip_before_action :verify_authenticity_token

Check failure

Code scanning / CodeQL

CSRF protection weakened or disabled

Potential CSRF vulnerability due to forgery protection being disabled or weakened.
@lloeki lloeki force-pushed the forwardport/update-libddwaf branch from 78df080 to 759526d Compare October 18, 2022 14:26
@lloeki
Copy link
Contributor Author

lloeki commented Oct 18, 2022

Silly me forgot about JRuby locks

Copy link
Member

@ivoanjo ivoanjo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since I previously reviewed and approved #2306 I mostly went through this PR to doublecheck that the diff still looked good, and it did. LGTM 👍

Appraisals Outdated Show resolved Hide resolved
integration/apps/rack/Dockerfile-ci Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

Codecov Report

Merging #2320 (4f19ab1) into master (a06ca0e) will increase coverage by 0.73%.
The diff coverage is 97.46%.

@@            Coverage Diff             @@
##           master    #2320      +/-   ##
==========================================
+ Coverage   97.56%   98.30%   +0.73%     
==========================================
  Files        1076     1079       +3     
  Lines       56800    57451     +651     
==========================================
+ Hits        55416    56475    +1059     
+ Misses       1384      976     -408     
Impacted Files Coverage Δ
lib/datadog/appsec/event.rb 95.23% <ø> (+4.32%) ⬆️
...datadog/tracing/contrib/utils/quantization/http.rb 84.93% <50.00%> (-2.74%) ⬇️
...ec/datadog/tracing/contrib/rails/support/rails3.rb 92.50% <66.66%> (-1.09%) ⬇️
...ib/datadog/appsec/contrib/rack/reactive/request.rb 92.10% <83.33%> (+66.39%) ⬆️
...tadog/appsec/contrib/rack/reactive/request_body.rb 90.32% <83.33%> (+58.17%) ⬆️
...b/datadog/appsec/contrib/rack/reactive/response.rb 90.32% <83.33%> (+58.17%) ⬆️
...ib/datadog/appsec/contrib/rails/reactive/action.rb 90.90% <83.33%> (+60.90%) ⬆️
.../datadog/appsec/contrib/sinatra/reactive/routed.rb 90.32% <83.33%> (+61.75%) ⬆️
...tadog/appsec/contrib/rack/integration_test_spec.rb 95.50% <95.50%> (ø)
...adog/appsec/contrib/rails/integration_test_spec.rb 99.40% <99.40%> (ø)
... and 48 more

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

@lloeki lloeki merged commit 60f71fc into master Oct 19, 2022
@lloeki lloeki deleted the forwardport/update-libddwaf branch October 19, 2022 12:17
@github-actions github-actions bot added this to the 1.6.0 milestone Oct 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
appsec Application Security monitoring product integrations Involves tracing integrations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants