8000 feat: update `unix-shell-evasion-prefix` by Xhoenix · Pull Request #3544 · coreruleset/coreruleset · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat: update unix-shell-evasion-prefix #3544

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 0 commits into from
Closed

feat: update unix-shell-evasion-prefix #3544

wants to merge 0 commits into from

Conversation

Xhoenix
Copy link
Member
@Xhoenix Xhoenix commented Feb 10, 2024

Update unix-shell-evasion-prefix.ra

@Xhoenix Xhoenix requested review from dune73 and fzipi February 10, 2024 15:54
@theseion
Copy link
Contributor

I'm afraid the list of commands in the prefix will become endless. In theory, we could just expect any of the unix commands in the prefix and generate the expression with that, but that won't work due to size (maximum size of regular expression) and performance (processing the expression will become prohibitively expensive). @Xhoenix, if you think this will be the last addition, I'm ok with it, otherwise we need to think about a different approach (and postpone that until after v4 is out).

@dune73
Copy link
Member
dune73 commented Feb 11, 2024

I lost track of how to different includes for unix shell stuff are really assembled. We might need a graphical representation going forward. So I can not really comment in any qualified way.

@theseion
Copy link
Contributor

For me there are two main issues here:

  1. we're again compiling a list of commands that isn't automated
  2. this list of commands will be assembled with cmdline unix, so for each character an additional suffix of [\x5c'\"\[)]*(?:(?:(?:\|\||&&)\s*)?\$[a-z0-9_@?!#{(*-]*)?\x5c? is added to the expression; the length of the entire expression thus becomes polynomial and the same thing probably goes for the performance impact.

@Xhoenix
Copy link
Member Author
Xhoenix commented Feb 11, 2024 •
edited
Loading

@theseion I don't think there will be a lot more addition, atleast not from me.

@dune73 This is actually your and fzipi's idea, I just added them to my PR. :-)

theseion
theseion previously approved these changes Feb 11, 2024
View reviewed changes
@theseion
Copy link
Contributor

OK, fine with me. You'll need to remove the superfluous empty line before I can merge.

@theseion
Copy link
Contributor

@Xhoenix Please rebase and resolve conflicts.

@Xhoenix Xhoenix closed this Feb 13, 2024
@Xhoenix
Copy link
Member Author
Xhoenix commented Feb 13, 2024

Accidently closed this PR. New one is #3557

@fzipi
Copy link
Member
fzipi commented Feb 13, 2024

PRs can be reopen 🤷

@fzipi
Copy link
Member
fzipi commented Feb 13, 2024

Can you please copy comments from here to the new one?

@Xhoenix
Copy link
Member Author
Xhoenix commented Feb 13, 2024

Actually, I accidently removed the commits for this PR, using the option "Discard commits" in GH web. Due to merge conflict, I found it easier to open a new PR.

@Xhoenix Xhoenix mentioned this pull request Feb 13, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@theseion theseion theseion left review comments

@dune73 dune73 Awaiting requested review from dune73

@fzipi fzipi Awaiting requested review from fzipi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Xhoenix @theseion @dune73 @fzipi
0