Skip to content

fix: 933161 regex#2302

Merged
dune73 merged 1 commit into
coreruleset:v4.0/devfrom
theMiddleBlue:fix-933161
Aug 11, 2023
Merged

fix: 933161 regex#2302
dune73 merged 1 commit into
coreruleset:v4.0/devfrom
theMiddleBlue:fix-933161

Conversation

@theMiddleBlue

Copy link
Copy Markdown
Contributor

same fix made on #2301

@theMiddleBlue
theMiddleBlue marked this pull request as draft November 10, 2021 14:50
@theMiddleBlue

Copy link
Copy Markdown
Contributor Author

converted to draft because of #2301 (comment)

@theMiddleBlue theMiddleBlue mentioned this pull request Nov 10, 2021
@theMiddleBlue
theMiddleBlue marked this pull request as ready for review November 10, 2021 15:29
Comment thread util/regexp-assemble/data/933161.data Outdated
@dune73

dune73 commented Dec 6, 2021

Copy link
Copy Markdown
Member

Do we have a status here?

@theMiddleBlue
theMiddleBlue changed the base branch from v3.4/dev to v4.0/dev April 18, 2023 20:28
@theMiddleBlue theMiddleBlue changed the title fix 933161 regex fix: 933161 regex Apr 18, 2023
@theMiddleBlue

theMiddleBlue commented Apr 18, 2023

Copy link
Copy Markdown
Contributor Author

after rebase the regex still vulnerable to redos.
I've just inverted the position of comment sequences and crs-toolchain has done the magic!

Now seems fixed:

Welcome to Regexploit. Enter your regexes:
(?i)\b(?:a(?:bs|cosh?|r(?:ray|sort)|s(?:inh?|(?:o|se)rt)|tan[2h]?)|b(?:asename|indec)|c(?:eil|h(?:dir|eckdate|mod|o(?:p|wn)|root)|lose(?:dir|log)|o(?:(?:mpac|(?:nsta|u)n)t|py|sh?)|(?:ryp|urren)t)|d(?:ate|e(?:coct|fined?)|i(?:(?:skfreespac)?e|r(?:name)?)|(?:oubleva)?l)|e(?:a(?:ch|ster_da(?:te|ys))|cho|mpty|nd|r(?:egi?|ror_log)|x(?:(?:i|trac)t|p(?:lode)?))|f(?:close|eof|gets|ile(?:owner|pro|(?:siz|typ)e)|l(?:o(?:atval|ck|or)|ush)|(?:mo|rea)d|stat|t(?:ell|ok)|unction)|g(?:et(?:date|t(?:ext|ype))|mdate)|h(?:ash|e(?:ader(?:s_(?:lis|sen)t)?|brev)|ypot)|i(?:conv|(?:dat|mplod)e|n(?:(?:clud|vok)e|t(?:div|val))|s(?:_(?:a(?:rray)?|bool|(?:calla|dou)ble|f(?:inite|loat)|in(?:finite|t(?:eger)?)|l(?:ink|ong)|n(?:an|u(?:ll|meric))|object|re(?:al|source)|s(?:calar|tring))|set))|join|k(?:ey|sort)|l(?:(?:cfirs|sta)t|evenshtein|i(?:nk(?:info)?|st)|o(?:caltime|g(?:1[0p])?)|trim)|m(?:a(?:i[ln]|x)|b(?:ereg|split)|etaphone|hash|i(?:crotime|n)|y?sql)|n(?:atsor|ex)t|o(?:ctdec|penlog|rd)|p(?:a(?:ck|thinfo)|close|i|o[sw]|r(?:ev|intf?))|quotemeta|r(?:an(?:d|ge)|e(?:adlin[ek]|(?:cod|nam|quir)e|set|wind)|ound|sort|trim)|s(?:(?:candi|ubst)r|(?:e(?:rializ|ttyp)|huffl)e|i(?:milar_text|nh?|zeof)|leep|o(?:rt|undex)|p(?:liti?|rintf)|qrt|rand|t(?:at|r(?:coll|(?:le|sp)n))|y(?:mlink|slog))|t(?:a(?:int|nh?)|e(?:mpnam|xtdomain)|ime|ouch|rim)|u(?:cfirst|mask|n(?:iqid|link|(?:se|tain)t)|s(?:leep|ort))|virtual|wordwrap)(?:[\s\v]|/\*.*\*/|(?:#|//).*)*\(.*\)
No ReDoS found.

rolled back to t:none tfunc!
if you agree, we can merge.

@theMiddleBlue
theMiddleBlue marked this pull request as ready for review April 18, 2023 20:44
@theseion

Copy link
Copy Markdown
Contributor

I don't believe that there's a significant difference between the two versions. I rather think that regexploit doesn't know how to find a ReDOS for your new version. regex101 also reports a very low number of steps for both versions (using PHP and PCRE2), regardless of the number of # (I tried with 7000+). TBH, I'm not sure that there really is a ReDOS here (the number of steps does increase depending on the number of characters after the comment character, but that increase is close to linear).

@dune73

dune73 commented Apr 24, 2023

Copy link
Copy Markdown
Member

@airween could we have your opinion here, please?

@dune73

dune73 commented Aug 7, 2023

Copy link
Copy Markdown
Member

Anybody has a status or a plan what to do here?

@theMiddleBlue

Copy link
Copy Markdown
Contributor Author

I'm all for merging this as is, as regexploit doesn't flag any redos problems anymore.

@dune73 dune73 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good. We no longer think this is a ReDoS, so let's merge.

@dune73
dune73 merged commit 6e8231b into coreruleset:v4.0/dev Aug 11, 2023
M4tteoP added a commit to M4tteoP/coreruleset that referenced this pull request Aug 11, 2023
M4tteoP added a commit to M4tteoP/coreruleset that referenced this pull request Aug 11, 2023
M4tteoP added a commit to M4tteoP/coreruleset that referenced this pull request Sep 3, 2023
M4tteoP added a commit to M4tteoP/coreruleset that referenced this pull request Sep 4, 2023
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.

3 participants