8000 fix: 932236 932237 932239 FP with word settings by EsadCetiner · Pull Request #3394 · coreruleset/coreruleset · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix: 932236 932237 932239 FP with word settings #3394

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
Merged
merged 25 commits into from
Jan 10, 2024
Merged

fix: 932236 932237 932239 FP with word settings #3394

merged 25 commits into from
Jan 10, 2024

Conversation

EsadCetiner
Copy link
Member
@EsadCetiner EsadCetiner commented Nov 29, 2023 •
edited by theseion
Loading

closes #3393

depends on #3401

@EsadCetiner EsadCetiner mentioned this pull request Nov 29, 2023
Closed
@dune73 dune73 mentioned this pull request Nov 29, 2023
Closed
5 tasks
@EsadCetiner
Copy link
Member Author

I know set help isn't a real command but for whatever reason go-ftw doesn't like go --help, so I've set it as that for 932237 regression test.

@dune73
Copy link
Member
dune73 commented Dec 1, 2023

Ping @theseion: Something that ought to be fixed, or do we live with it?

@theseion
Copy link
Contributor
theseion commented Dec 2, 2023

@EsadCetiner I don't understand the issue with set help. Did you want use set --help originally? Where does go --help come into this? If I knew what you wanted to test I could take a look.

@EsadCetiner
Copy link
Member Author

@theseion sorry, I meant set --help and not go --help. I wanted to originally use set --help but the go-ftw tests kept on failing see: https://github.com/coreruleset/coreruleset/actions/runs/7036802306/job/19150223518 and afc9507

@theseion theseion mentioned this pull request Dec 3, 2023
Closed
@theseion
Copy link
Contributor
theseion commented Dec 3, 2023

Thanks. So the issue is the final word boundary match (\b) in the regular expression. At a word boundary, two adjacent tokens must not both match \w and \W, i.e., one must be a word token and the other must not. The space character is a non-word token and the dash character is a non-word token as well, which means the \b will not match. I believe that is a bug and should be fixed, possibly by changing \b to \S.

Issue: #3401

Out of curiosity: what is set --help? My bash man page doesn't list any long options for set...

@EsadCetiner
Copy link
Member Author

That explains why the test failed then, but how come https://example.com/?test=set --help is blocked but not when it's in the referrer or user agent?

set --help just shows the help page, for me (Ubuntu 22.04) there's no man page for the set command.
sample output of set --help:

8000
set: set [-abefhkmnptuvxBCHP] [-o option-name] [--] [arg ...]
    Set or unset values of shell options and positional parameters.

    Change the value of shell attributes and positional parameters, or
    display the names and values of shell variables.

    Options:
      -a  Mark variables which are modified or created for export.
      -b  Notify of job termination immediately.
      -e  Exit immediately if a command exits with a non-zero status.
      -f  Disable file name generation (globbing).
      -h  Remember the location of commands as they are looked up.
      -k  All assignment arguments are placed in the environment for a
          command, not just those that precede the command name.
      -m  Job control is enabled.
      -n  Read commands but do not execute them.
      -o option-name

@fzipi fzipi mentioned this pull request Dec 4, 2023
Closed
@theseion
Copy link
Contributor

What kind of shell are you using? --help isn't valid in neither bash (3.2) nor zsh.

I also any rule that would block https://example.com/?test=set --help (I've checked out your PR branch to test).

@EsadCetiner
Copy link
Member Author
EsadCetiner commented Dec 11, 2023 •
edited
Loading

I'm just using whatever comes with Ubuntu, bash 5.1.16(1)-release
and what do you mean by "I also any rule that would block", I'm not sure on what your trying to say?

@theseion
Copy link
Contributor

and what do you mean by "I also any rule that would block", I'm not sure on what your trying to say?

😅 Sorry about that. I meant to write: I also can't find any rule that would block.

The Ubuntu release will have 3.3.5 or something like that. Rewrote the *nix detection rules heavily, so it doesn't surprise me that there's a difference between that version and 4.0/dev. Detection in Referer and User-Agent headers is something we introduced in v4, which would explain what you see if you're using 3.3.5.

@dune73 dune73 added the v4 Should go into release v4 label Dec 12, 2023
@dune73
Copy link
Member
dune73 commented Dec 13, 2023

What's the status here? Just a final approval and we're good to merge?

theseion
theseion requested changes Dec 13, 2023
View reviewed changes
@theseion
Copy link
Contributor

This PR depends on the fix discussed in #3401. I've added a dependency check.

theseion
theseion previously approved these changes Dec 14, 2023
View reviewed changes
@theseion
Copy link
Contributor

#3425

@EsadCetiner EsadCetiner requested a review from theseion January 5, 2024 12:03
@EsadCetiner
Copy link
Member Author

I just noticed that 932237 is running at phase 2, shouldn't it be running at phase 1 for better performance?

@M4tteoP
Copy link
Member
M4tteoP commented Jan 5, 2024

I think it has been added to phase 2 because it is a stricter sibling of several rules (932230, 932235, 932250, 932260) that are all running at phase 2 because they match ARGS. But I agree that dealing only with headers, it should formally belong to phase 1, just like 932239 (phase:1), sibling rule of 932236 (phase:2)

theseion
theseion requested changes Jan 5, 2024
View reviewed changes
@theseion
Copy link
Contributor
theseion commented Jan 5, 2024

I just noticed that 932237 is running at phase 2, shouldn't it be running at phase 1 for better performance?

You're probably right. It's not an urgent issue IMO, but please open an issue.

@EsadCetiner EsadCetiner mentioned this pull request Jan 5, 2024
Closed
@dune73 dune73 mentioned this pull request Jan 8, 2024
Closed
@theseion theseion merged commit cced396 into coreruleset:v4.0/dev Jan 10, 2024
@github-actions github-actions bot mentioned this pull request Jan 11, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@theseion theseion theseion approved these changes

Assignees
No one assigned
Labels
v4 Should go into release v4
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

932236 932237 932239 settings false positive
4 participants
@EsadCetiner @dune73 @theseion @M4tteoP
0