8000 feat: added rule to detect Bash Brace Expansion by Xhoenix · Pull Request #3780 · coreruleset/coreruleset · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat: added rule to detect Bash Brace Expansion #3780

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

Open
wants to merge 29 commits into
base: main
Choose a base branch
from

Conversation

Xhoenix
Copy link
Member
@Xhoenix Xhoenix commented Jul 25, 2024

Bash Brace Expansion: {,ip,a} {l,-lh}s {,ifconfig} etc.

I think there is still room for improvement on this rule as there are other things to cover as per Bash/Zsh manual. Any ideas are welcome.

@Xhoenix Xhoenix requested a review from a team July 25, 2024 14:41
@Xhoenix Xhoenix requested a review from azurit July 31, 2024 08:20
@azurit
Copy link
Member
azurit commented Aug 1, 2024

Do we need a completely new rule for this?

@Xhoenix
Copy link
Member Author
Xhoenix commented Aug 1, 2024

Any suggestions on a rule where this might fit into are welcome.

@theseion
Copy link
Contributor
theseion commented Aug 2, 2024

Might be something to discuss in the next meeting. I'll add an item to the list.

@Xhoenix
Copy link
Member Author
Xhoenix commented Aug 2, 2024

Any suggestions on a rule where this might fit into are welcome. I created a new rule to keep things simple, but willing to adapt if decided to.

@theseion
Copy link
Contributor
theseion commented Aug 2, 2024

I just realised that we already try to detect brace expansion in combination with shell RCE:

# - ,: brace expansion, e.g., `""{nc,-p,777}`

@Xhoenix
Copy link
Member Author
Xhoenix commented Aug 3, 2024

But the payloads I mentioned are bypassing CRS, maybe because there isn't a rule for it?

@theseion
Copy link
Contributor
theseion commented Aug 5, 2024

Well, the pattern is only detected when the ~ modifier is used for the cmdline processor. I've added , to the anti-evation.unix pattern experimentally: [\x5c'\"\[),]*(?:(?:(?:\|\||&&)\s*)?\$[a-z0-9_@?!#{(*-]*)?\x5c?. With this, I can detect brace expansion of known commands: https://regex101.com/r/8mBQBh/1.

@theseion
Copy link
Contributor

The patterns for the command li 8000 ne processor are defined here: https://github.com/coreruleset/coreruleset/blob/main/regex-assembly/toolchain.yaml.

@theseion
Copy link
Contributor

Only commands that are part of the commands list and are not filtered due to FPs for a given rule will be detected.

7zx is one of the commands from the list and ;{7,z,x} was not detected before the change. To detect ;{,... (leading comma), we need to update the prefix expressions, e.g., in unix-shell-evasion-prefix.ra:

##! ${ifconfig}
\${
##! ${,ifconfig} - brace expansion with leading comma
\${,

We probably also need to extend 932240 to detect brace expansion as part of the generic detection.

Well, we would need to make a couple of changes in any case, at different locations. While it would be consistent, I'm leaning towards a single rule (maybe two at different PLs). The complexity will not do us any good.

@Xhoenix
Copy link
Member Author
Xhoenix commented Aug 23, 2024

Should I add the + character to PL1 or PL2? It's used while specifying options for some commands.

@fzipi
Copy link
Member
fzipi commented Sep 3, 2024

Can you add an example on how to use the +? Then we can decide if it fits PL1 or PL2.

@Xhoenix
Copy link
Member Author
Xhoenix commented Sep 3, 2024

There are only a few commands that use it, e.g. alias(which we are already detecting) and a few other bash/zsh builtins. I think it'll be better suited at PL2.

@fzipi
Copy link
8000 Member
fzipi commented Sep 4, 2024

@Xhoenix Can you add an example on how to use the +?

@Xhoenix
Copy link
Member Author
Xhoenix commented Sep 4, 2024

From bash manpage:

set +abefh...
set +o option-name
pushd +n
popd +n
dirs +n
compopt +o option

Alias in zsh also uses the +.

@fzipi
Copy link
Member
fzipi commented Sep 7, 2024

Thanks for the explanation. I don't see the + usage as critical even if you execute set + I don't see it as something that might end up in an exploitation? But if you can find a good use case, please document it so we add the +.

Copy link
Member
@fzipi fzipi left a comment

Choose a reason for hiding this comment

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

What I see here is that both @azurit and @theseion commented about their concern on adding a new rule. I share that same concern.

If we should capture this in rule 932240 I think it is best to focus on that case.

Can you follow on Max's findings here #3780 (comment) and here #3780 (comment), and update and test the relevant files?

@theseion
Copy link
Contributor
theseion commented Sep 9, 2024

@fzipi As I wrote in #3780 (comment), I'm leaning more towards a new rule now, because we'd need to make changes in multiple places to get this working with the current RCE rules. Also, the rules are already too complex and adding another detection will only make it harder to maintain this particular detection.

Copy link
Contributor
github-actions bot commented Dec 20, 2024

📊 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 14, 2025 06:55
@Xhoenix
Copy link
Member Author
Xhoenix commented Jan 14, 2025

LGTM!

@Xhoenix
Copy link
Member Author
Xhoenix commented Mar 6, 2025

Ping!

@Xhoenix Xhoenix added the release:new-detection In this PR we introduce a new detection label Apr 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:new-detection In this PR we introduce a new detection
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants
0