Skip to content

fix: 932270 FP #3917

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

Merged
merged 30 commits into from
Apr 17, 2025
Merged

fix: 932270 FP #3917

merged 30 commits into from
Apr 17, 2025

Conversation

Xhoenix
Copy link
Member

@Xhoenix Xhoenix commented Nov 1, 2024

Fixes #3844

@Xhoenix Xhoenix requested a review from theseion November 1, 2024 04:05
@Xhoenix
Copy link
Member Author

Xhoenix commented Nov 1, 2024

@theseion using the cmdline processor for word boundary makes crs-toolchain throw error. Can you look into this.

@theseion
Copy link
Contributor

theseion commented Nov 2, 2024

Can you give me an example that I can run?

@Xhoenix
Copy link
Member Author

Xhoenix commented Nov 2, 2024

I updated the 932270.ra file with the following content, and crs-toolchain regex update throws error:

##! Please refer to the documentation at                                                                
##! https://coreruleset.org/docs/development/regex_assembly/.

##! Bash - Tilde Expansion
##! Detects the following patterns which are common in Unix shell scripts and one-liners

##!> cmdline unix
  ~\+$
  ~\+[\d\s]+
  ~\-$
  ~\-[\d\s]+
  ~\d+~
##!<

Is this syntax even right, this is what mentioned in our docs. Something about this doesn't seem right to me. How do we put a word boundary on the number.

@theseion
Copy link
Contributor

theseion commented Nov 2, 2024

Works for me with version 2.3.0:

crs-toolchain regex generate - <<EOF
##!> cmdline unix
heredoc>   ~\+$
heredoc>   ~\+[\d\s]+
heredoc>   ~\-$
heredoc>   ~\-[\d\s]+
heredoc>   ~\d+~
heredoc> ##!<
heredoc> EOF
(?:\+|\x5c-)(?:$|[\s\x0b0-9]+)|[0-9]+

@theseion
Copy link
Contributor

theseion commented Nov 2, 2024

Just so we're talking about the same thing, $ is not a word boundary. It is the end of line anchor.

@Xhoenix
Copy link
Member Author

Xhoenix commented Nov 2, 2024

The issue is with ~\d+~ the ~ is a boundary. This is the line causing FPs.

@theseion
Copy link
Contributor

theseion commented Nov 7, 2024

What do you mean by "~ is a boundary"? In regular expressions, the token \b is called "word boundary". ~ has no special meaning. As I showed above, the input is valid. Can you post a reproducible case that fails?

@Xhoenix
Copy link
Member Author

Xhoenix commented Nov 9, 2024

Don't know, but maybe you forgot this: https://coreruleset.org/docs/development/regex_assembly/index.html#description

The ~ is a special token, to prevent issues like this. If this won't work here, can you read the issue and suggest a solution?

@theseion
Copy link
Contributor

theseion commented Nov 9, 2024

Don't know, but maybe you forgot this: https://coreruleset.org/docs/development/regex_assembly/index.html#description

The ~ is a special token, to prevent issues like this. If this won't work here, can you read the issue and suggest a solution?

Right. Now I know what you mean.

I still don't see the problem. On my machine, that snippet works. It also works in the CI pipeline, otherwise you'd see an error in the linting step.
You wrote that you see an error. What does the error say exactly? Which version are you running (crs-toolchain --version)?

@Xhoenix
Copy link
Member Author

Xhoenix commented Nov 11, 2024

Why are you "Quote Replying" my message, it makes it hard to read your reply. Don't worry, I'm not going to delete my message.

Check the latest commit, crs-toolchain regex update produces an error.

@Xhoenix
Copy link
Member Author

Xhoenix commented Nov 11, 2024

@theseion
Copy link
Contributor

Why are you "Quote Replying" my message, it makes it hard to read your reply. Don't worry, I'm not going to delete my message.

That was a mistake.

You're right, it doesn't work. I didn't see it when I tested it because I ran it in a terminal where the toolchain couldn't find the configuration file, so it didn't load any of the evasion replacement strings.

The reason it doesn't work is that ~ and @ are supposed to be used only as suffixes. Unfortunately, I didn't enforce that behavior, so the toolchain replaces ~, just as you intended, but the regular expression wasn't designed to be placed anywhere else than at the end.

You'll have to write your own regular expression for what you intended to achieve with ~. I'll open an issue with crs-toolchain to enforce the positional use of ~ and @.

@Xhoenix
Copy link
Member Author

Xhoenix commented Nov 13, 2024

Cool. I'll be watching the progress of the issue.

@theseion
Copy link
Contributor

It won't change anything. When I implement that change to the toolchain, the ~ will be interpreted literally, which I don't think is what you want to achieve.

@cello86
Copy link

cello86 commented Dec 13, 2024

I replicated the problemi with this curl command:

curl -i https://test.local/test.html?test_modsec=bsv03A~574EB1E308246F263B337837F249CCFE
HTTP/1.1 403 Forbidden
Date: Fri, 13 Dec 2024 14:12:32 GMT
Content-Type: text/html
Content-Length: 159
Connection: keep-alive
X-Powered-By: ASP.NET
Server: Microsoft-IIS/10.0
X-Xss-Protection: 1; mode=block
X-Content-Type-Options: nosniff
X-UA-Compatible: IE=edge,chrome=1
Strict-Transport-Security: max-age=16070400; includeSubDomains; preload

I tested the configuration reported into the PR:

SecRule REQUEST_COOKIES|!REQUEST_COOKIES:/__utm/|REQUEST_COOKIES_NAMES|ARGS_NAMES|ARGS|XML:/* "@rx ~(?:[\+\-](?:$|[\s\x0b0-9]+)|[0-9]+~)" \
    "id:932270,\
    phase:2,\
    block,\
    capture,\
    t:none,t:cmdLine,\
    msg:'Remote Command Execution: Unix Shell Expression Found',\
    logdata:'Matched Data: %{TX.0} found within %{MATCHED_VAR_NAME}: %{MATCHED_VAR}',\
    tag:'application-multi',\
    tag:'language-shell',\
    tag:'platform-unix',\
    tag:'attack-rce',\
    tag:'paranoia-level/1',\
    tag:'OWASP_CRS',\
    tag:'capec/1000/152/248/88',\
    ver:'OWASP_CRS/4.8.0',\
    severity:'CRITICAL',\
    setvar:'tx.rce_score=+%{tx.critical_anomaly_score}',\
    setvar:'tx.inbound_anomaly_score_pl1=+%{tx.critical_anomaly_score}'"

The curl command result was OK (404 was correct because the page didn't exist):

curl -i  https://test.local/test.html?test_modsec=bsv03A~574EB1E308246F263B337837F249CCFE
HTTP/1.1 404 Not Found
Date: Fri, 13 Dec 2024 14:22:48 GMT
Content-Type: text/html
Content-Length: 34378
Connection: keep-alive
X-Powered-By: ASP.NET
Server: Microsoft-IIS/10.0
X-Xss-Protection: 1; mode=block
X-Content-Type-Options: nosniff
X-UA-Compatible: IE=edge,chrome=1
Strict-Transport-Security: max-age=16070400; includeSubDomains; preload

Copy link
Contributor

github-actions bot commented Jan 3, 2025

📊 Quantitative test results for language: eng, year: 2023, size: 10K, paranoia level: 1:
🚀 Quantitative testing did not detect new false positives

@Xhoenix Xhoenix requested a review from a team January 3, 2025 14:49
@Xhoenix
Copy link
Member Author

Xhoenix commented Mar 30, 2025

Ping

Copy link
Contributor

@theseion theseion left a comment

Choose a reason for hiding this comment

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

Almost there. Just a couple of tests to add.

Copy link
Contributor

@theseion theseion left a comment

Choose a reason for hiding this comment

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

Thanks @Xhoenix, I know this took a long time.

@theseion theseion added this pull request to the merge queue Apr 17, 2025
Merged via the queue into coreruleset:main with commit 80fad65 Apr 17, 2025
6 checks passed
@Xhoenix Xhoenix deleted the fix-fp-932270 branch April 17, 2025 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rule 932270 False Positives for Tilde + Number
3 participants