Skip to content

fix: 933160 regex#2301

Closed
theMiddleBlue wants to merge 1 commit into
coreruleset:v4.0/devfrom
theMiddleBlue:fix-933160
Closed

fix: 933160 regex#2301
theMiddleBlue wants to merge 1 commit into
coreruleset:v4.0/devfrom
theMiddleBlue:fix-933160

Conversation

@theMiddleBlue

Copy link
Copy Markdown
Contributor

removed suffix #* and //* from regex since isn't a valid PHP syntax

system#("ls");
system//("ls");

the only valid syntax should be

system/*foo*/("ls");

This was referenced Nov 10, 2021
@theMiddleBlue
theMiddleBlue marked this pull request as draft November 10, 2021 14:48
@theMiddleBlue

Copy link
Copy Markdown
Contributor Author

switched to draft because a valid syntax could be something like

$a = ["system"];
$a #foo
	[0] //bar
	('ls');

so we need a different solution :(

@theMiddleBlue
theMiddleBlue marked this pull request as ready for review November 10, 2021 15:34
Comment thread util/regexp-assemble/data/933160.data Outdated
##!+i
##!^\b
##!$(?:\s|/\*.*\*/|//.*|#.*)*\(.*\)
##!$\(.*\)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

After removing whitespace and comment markers you might miss this:

=array_map\n#dummy comment\n('gotcha')

which would be converted to:

=array_mapdummycomment('gotcha')

Removing whitespace could be ok I guess but removing comment markers will break the rule.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yep, your example payload doesn't match anymore :/ The problem we need to solve is just the //.* part of the regex. I'm switching this to draft while looking for a solution tnx

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe you can work around it by matching the end of the line, since // will comment everything up to the end:

//[^\n\r]*[\n\r]\(.*\)

@dune73

dune73 commented Dec 6, 2021

Copy link
Copy Markdown
Member

What is the status here?

@fzipi

fzipi commented Feb 3, 2022

Copy link
Copy Markdown
Member

IMHO, I don't think we can solve the aliasing problem using secrules. So we should stick with something with less coverage, but functional.

@theMiddleBlue
theMiddleBlue force-pushed the fix-933160 branch 2 times, most recently from 7a1d7a9 to 9f37d51 Compare April 18, 2023 19:34
@theMiddleBlue
theMiddleBlue changed the base branch from v3.4/dev to v4.0/dev April 18, 2023 19:36
@theMiddleBlue theMiddleBlue changed the title fix 933160 regex fix: 933160 regex Apr 18, 2023
@theMiddleBlue

Copy link
Copy Markdown
Contributor Author

rebased on v4.0/dev

@theMiddleBlue

Copy link
Copy Markdown
Contributor Author

it seems that regex assembly has done the magic, the produced regex is no more affected by ReDoS!
basically we can close this PR.

but I think the rule regression tests need to be fixed...

Can anyone explain why we're doing this?

$ grep 'Print_r' tests/regression/tests/REQUEST-933-APPLICATION-ATTACK-PHP/933160.yaml 
            data: x=Print_r%28%20%29
            data: x=Print_r%28%20%29
            data: x=Print_r%28%20%29
            data: x=Print_r%28%20%29
            data: x=Print_r%28%20%29
            data: x=Print_r%28%20%29
            data: x=Print_r%28%20%29
            data: x=Print_r%28%20%29
            data: x=Print_r%28%20%29
            data: x=Print_r%28%20%29
            data: x=Print_r%28%20%29
            data: x=Print_r%28%20%29
            data: x=Print_r%28%20%29
            data: x=Print_r%28%20%29

maybe there's a good reason, but all these tests pass just because the rule matches always the same payload.

@theMiddleBlue

Copy link
Copy Markdown
Contributor Author

better opening an issue for this.
closing 🎉

@theseion

Copy link
Copy Markdown
Contributor

Ha, nice :D

@theseion

Copy link
Copy Markdown
Contributor

I agree on the tests. They're garbage.

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.

4 participants