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

Server-side request forgery (SSRF) detection operator #268

Merged
merged 34 commits into from
Mar 8, 2024
Merged

Conversation

Anilm3
Copy link
Collaborator

@Anilm3 Anilm3 commented Feb 26, 2024

  • SSRF detection operator
  • URL parsing helpers
  • Fuzzer tests for both URL parser and SSRF detector
  • Unit tests

Related Jira: APPSEC-51265

@codecov-commenter
Copy link

codecov-commenter commented Feb 26, 2024

Codecov Report

Attention: Patch coverage is 87.60331% with 45 lines in your changes are missing coverage. Please review.

Project coverage is 83.08%. Comparing base (c51ae81) to head (a05f2ac).

Files Patch % Lines
src/uri_utils.cpp 88.42% 7 Missing and 18 partials ⚠️
src/condition/ssrf_detector.cpp 83.51% 1 Missing and 14 partials ⚠️
src/ip_utils.cpp 92.59% 1 Missing and 1 partial ⚠️
src/matcher/ip_match.hpp 81.81% 1 Missing and 1 partial ⚠️
src/matcher/ip_match.cpp 93.33% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #268      +/-   ##
==========================================
+ Coverage   82.86%   83.08%   +0.22%     
==========================================
  Files         113      115       +2     
  Lines        4225     4564     +339     
  Branches     1992     2138     +146     
==========================================
+ Hits         3501     3792     +291     
- Misses        292      301       +9     
- Partials      432      471      +39     
Flag Coverage Δ
waf_test 83.08% <87.60%> (+0.22%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pr-commenter
Copy link

pr-commenter bot commented Feb 26, 2024

Benchmarks

Benchmark execution time: 2024-03-08 11:07:53

Comparing candidate commit 8362937 in PR branch anilm3/ssrf with baseline commit c51ae81 in branch master.

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

scenario:bool_equals_matcher.random

  • 🟩 execution_time [-73.955µs; -73.000µs] or [-6.260%; -6.179%]

scenario:exact_match_matcher.random

  • 🟩 execution_time [-91.189µs; -90.251µs] or [-5.938%; -5.877%]

scenario:float_equals_matcher.random

  • 🟩 execution_time [-39.620µs; -35.899µs] or [-3.352%; -3.037%]

scenario:lowercase_transformer.random

  • 🟩 execution_time [-90.507µs; -89.450µs] or [-5.306%; -5.244%]

scenario:regex_match_matcher.case_insensitive_flag.random

  • 🟥 execution_time [+503.956µs; +505.429µs] or [+11.841%; +11.875%]

scenario:regex_match_matcher.case_insensitive_option.random

  • 🟥 execution_time [+507.538µs; +508.981µs] or [+11.938%; +11.972%]

scenario:regex_match_matcher.lowercase_transformer.random

  • 🟥 execution_time [+534.016µs; +535.766µs] or [+9.380%; +9.411%]

scenario:regex_match_matcher.random

  • 🟩 execution_time [-151.071µs; -149.932µs] or [-7.667%; -7.610%]

scenario:signed_equals_matcher.random

  • 🟩 execution_time [-72.200µs; -71.120µs] or [-6.115%; -6.023%]

scenario:string_equals_matcher.random

  • 🟩 execution_time [-74.838µs; -73.514µs] or [-5.160%; -5.069%]

scenario:unsigned_equals_matcher.random

  • 🟩 execution_time [-77.997µs; -76.709µs] or [-6.590%; -6.481%]

@Anilm3 Anilm3 marked this pull request as ready for review February 29, 2024 17:25
@Anilm3 Anilm3 requested a review from a team as a code owner February 29, 2024 17:25
src/condition/ssrf_detector.cpp Show resolved Hide resolved
src/uri_utils.cpp Outdated Show resolved Hide resolved
src/uri_utils.cpp Outdated Show resolved Hide resolved
src/uri_utils.cpp Outdated Show resolved Hide resolved
src/uri_utils.cpp Outdated Show resolved Hide resolved
src/condition/ssrf_detector.cpp Outdated Show resolved Hide resolved
src/condition/ssrf_detector.cpp Outdated Show resolved Hide resolved
src/condition/ssrf_detector.cpp Outdated Show resolved Hide resolved
src/condition/ssrf_detector.cpp Outdated Show resolved Hide resolved
src/condition/ssrf_detector.cpp Outdated Show resolved Hide resolved
@@ -1,7 +1,7 @@
---
# readability-function-cognitive-complexity temporarily disabled until clang-tidy is fixed
# right now emalloc causes it to misbehave
Checks: '*,misc-const-correctness,-bugprone-reserved-identifier,-hicpp-signed-bitwise,-llvmlibc-restrict-system-libc-headers,-altera-unroll-loops,-hicpp-named-parameter,-cert-dcl37-c,-cert-dcl51-cpp,-read,-cppcoreguidelines-init-variables,-cppcoreguidelines-avoid-non-const-global-variables,-altera-id-dependent-backward-branch,-performance-no-int-to-ptr,-altera-struct-pack-align,-google-readability-casting,-modernize-use-trailing-return-type,-llvmlibc-implementation-in-namespace,-llvmlibc-callee-namespace,-cppcoreguidelines-pro-bounds-pointer-arithmetic,-fuchsia-default-arguments-declarations,-fuchsia-overloaded-operator,-cppcoreguidelines-pro-type-union-access,-fuchsia-default-arguments-calls,-cppcoreguidelines-non-private-member-variables-in-classes,-misc-non-private-member-variables-in-classes,-google-readability-todo,-llvm-header-guard,-readability-function-cognitive-complexity,-readability-identifier-length,-cppcoreguidelines-owning-memory,-cert-err58-cpp,-fuchsia-statically-constructed-objects,-google-build-using-namespace,-hicpp-avoid-goto,-cppcoreguidelines-avoid-goto,-hicpp-no-array-decay,-cppcoreguidelines-pro-bounds-array-to-pointer-decay,-cppcoreguidelines-pro-bounds-constant-array-index,-cppcoreguidelines-avoid-magic-numbers,-readability-magic-numbers,-abseil-string-find-str-contains'
Checks: '*,misc-const-correctness,-bugprone-reserved-identifier,-hicpp-signed-bitwise,-llvmlibc-restrict-system-libc-headers,-altera-unroll-loops,-hicpp-named-parameter,-cert-dcl37-c,-cert-dcl51-cpp,-read,-cppcoreguidelines-init-variables,-cppcoreguidelines-avoid-non-const-global-variables,-altera-id-dependent-backward-branch,-performance-no-int-to-ptr,-altera-struct-pack-align,-google-readability-casting,-modernize-use-trailing-return-type,-llvmlibc-implementation-in-namespace,-llvmlibc-callee-namespace,-cppcoreguidelines-pro-bounds-pointer-arithmetic,-fuchsia-default-arguments-declarations,-fuchsia-overloaded-operator,-cppcoreguidelines-pro-type-union-access,-fuchsia-default-arguments-calls,-cppcoreguidelines-non-private-member-variables-in-classes,-misc-non-private-member-variables-in-classes,-google-readability-todo,-llvm-header-guard,-readability-function-cognitive-complexity,-readability-identifier-length,-cppcoreguidelines-owning-memory,-cert-err58-cpp,-fuchsia-statically-constructed-objects,-google-build-using-namespace,-hicpp-avoid-goto,-cppcoreguidelines-avoid-goto,-hicpp-no-array-decay,-cppcoreguidelines-pro-bounds-array-to-pointer-decay,-cppcoreguidelines-pro-bounds-constant-array-index,-cppcoreguidelines-avoid-magic-numbers,-readability-magic-numbers,-abseil-string-find-str-contains,-bugprone-unchecked-optional-access'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

-bugprone-unchecked-optional-access seems to have an issue which is causing it to get stuck in an infinite loop...

llvm/llvm-project#59492

estringana
estringana previously approved these changes Mar 5, 2024
Copy link
Collaborator

@estringana estringana left a comment

Choose a reason for hiding this comment

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

Left some comments but nothing big

cataphract
cataphract previously approved these changes Mar 5, 2024
src/condition/ssrf_detector.cpp Outdated Show resolved Hide resolved
src/condition/ssrf_detector.cpp Outdated Show resolved Hide resolved
src/condition/ssrf_detector.cpp Outdated Show resolved Hide resolved
src/condition/ssrf_detector.cpp Outdated Show resolved Hide resolved
src/condition/ssrf_detector.cpp Show resolved Hide resolved
* Remove questionable false positive check
* Improve code to reduce unnecessary checks
@Anilm3 Anilm3 requested a review from cataphract March 6, 2024 13:32
Taiki-San
Taiki-San previously approved these changes Mar 7, 2024
Copy link
Collaborator

@Taiki-San Taiki-San left a comment

Choose a reason for hiding this comment

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

Partial review. The core heuristic needs to be tuned before prod release

src/condition/ssrf_detector.cpp Outdated Show resolved Hide resolved
Co-authored-by: Taiki <emilehugo.spir@datadoghq.com>
src/matcher/ip_match.cpp Show resolved Hide resolved
src/uri_utils.cpp Outdated Show resolved Hide resolved
src/uri_utils.cpp Outdated Show resolved Hide resolved
src/uri_utils.cpp Outdated Show resolved Hide resolved
src/uri_utils.cpp Outdated Show resolved Hide resolved
src/uri_utils.cpp Outdated Show resolved Hide resolved
src/uri_utils.cpp Outdated Show resolved Hide resolved
src/uri_utils.cpp Show resolved Hide resolved
src/uri_utils.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@estringana estringana left a comment

Choose a reason for hiding this comment

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

lgtm

@Anilm3 Anilm3 merged commit 88df1d4 into master Mar 8, 2024
34 checks passed
@Anilm3 Anilm3 deleted the anilm3/ssrf branch March 8, 2024 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants