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

Shell injection detection operator #308

Merged
merged 41 commits into from
Jul 15, 2024
Merged

Shell injection detection operator #308

merged 41 commits into from
Jul 15, 2024

Conversation

Anilm3
Copy link
Collaborator

@Anilm3 Anilm3 commented Jun 14, 2024

This PR introduces support for a new operator shi_detector which can be used for detecting Shell injections given a set of request parameters and a shell command. The heuristic primarily attempts to find injected executables and redirections, with most of the work being done by the tokenizer.

In this instance, the tokenizer does a bit more work than just finding simple tokens, as it attempts to minimise the number of needed tokens and keeps sufficient context to track nested commands and their boundaries. It currently consists in two phases, the first one generates low and high level tokens, while the second one finds executables and strips whitespaces.

And some work which will be done in a subsequent PR:

  • Supporting a shell command as an array rather than as a string
  • Expanding the pseudo-tokenizer
  • Expanding the list of forbidden injected tokens
  • Benchmark scenario

To use this new operator,a rule such as the following could be used:

  - id: rsp-930-004
    name: SHi Exploit detection
    tags:
      type: shi
      category: exploit_detection
      module: rasp
    conditions:
      - parameters:
          resource:
            - address: server.sys.shell.cmd
          params:
            - address: server.request.query
            - address: server.request.body
            - address: server.request.path_params
            - address: grpc.server.request.message
            - address: graphql.server.all_resolvers
            - address: graphql.server.resolver
        operator: shi_detector

Related Jira: APPSEC-52939

@codecov-commenter
Copy link

codecov-commenter commented Jun 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.28%. Comparing base (38a4b0e) to head (5985929).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #308      +/-   ##
==========================================
+ Coverage   83.68%   84.28%   +0.60%     
==========================================
  Files         137      141       +4     
  Lines        6086     6554     +468     
  Branches     2882     3023     +141     
==========================================
+ Hits         5093     5524     +431     
- Misses        369      374       +5     
- Partials      624      656      +32     
Flag Coverage Δ
waf_test 84.28% <ø> (+0.60%) ⬆️

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 Jun 14, 2024

Benchmarks

Benchmark execution time: 2024-07-15 12:35:05

Comparing candidate commit 5985929 in PR branch anilm3/shell-injection with baseline commit 38a4b0e in branch master.

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

scenario:ip_match_matcher.random

  • 🟩 execution_time [-80.208µs; -76.761µs] or [-4.752%; -4.548%]

scenario:is_xss_matcher.random

  • 🟩 execution_time [-3.867ms; -3.849ms] or [-4.562%; -4.541%]

scenario:phrase_match_matcher.enforce_word_boundary.random

  • 🟩 execution_time [-3.057ms; -3.052ms] or [-29.964%; -29.913%]

scenario:regex_match_matcher.case_insensitive_flag.random

  • 🟥 execution_time [+749.893µs; +754.775µs] or [+14.768%; +14.864%]

scenario:regex_match_matcher.case_insensitive_option.random

  • 🟥 execution_time [+745.048µs; +750.366µs] or [+14.655%; +14.759%]

scenario:regex_match_matcher.lowercase_transformer.random

  • 🟥 execution_time [+692.932µs; +697.938µs] or [+10.586%; +10.663%]

scenario:regex_match_matcher.random

  • 🟩 execution_time [-119.122µs; -114.641µs] or [-4.966%; -4.779%]

scenario:string_equals_matcher.random

  • 🟩 execution_time [-73.449µs; -69.163µs] or [-4.760%; -4.482%]

@Anilm3 Anilm3 marked this pull request as ready for review June 20, 2024 15:56
@Anilm3 Anilm3 requested a review from a team as a code owner June 20, 2024 15:56
src/condition/shi_detector.cpp Outdated Show resolved Hide resolved

namespace ddwaf {

template <typename T> struct base_token {
Copy link
Contributor

Choose a reason for hiding this comment

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

it would make the code more readable if you had start(), end(), size(). In particular end, because token.index + token.str.size() is a bigger expression. Or just an interval() method, as mentioned.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since this comment and the one below affect other tokenizers as well, I'll try to address them together in a separate PR.

std::size_t i = 0;
for (; i < resource_tokens.size(); ++i) {
const auto &token = resource_tokens[i];
if (end_index >= token.index && param_index < (token.index + token.str.size())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this would be easier to understand if you had an interval abstraction with contains, overlaps, ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since this comment and the one above affect other tokenizers as well, I'll try to address them together in a separate PR.

src/condition/shi_detector.cpp Show resolved Hide resolved
src/tokenizer/base.hpp Show resolved Hide resolved
src/tokenizer/shell.cpp Outdated Show resolved Hide resolved
src/tokenizer/shell.cpp Outdated Show resolved Hide resolved
src/tokenizer/shell.cpp Outdated Show resolved Hide resolved
src/tokenizer/shell.cpp Outdated Show resolved Hide resolved
src/tokenizer/shell.cpp Show resolved Hide resolved
@Anilm3 Anilm3 merged commit 09de0e0 into master Jul 15, 2024
46 checks passed
@Anilm3 Anilm3 deleted the anilm3/shell-injection branch July 15, 2024 13:13
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.

3 participants