Fix ReDoS vulnerability#309232
Conversation
In commit 87e50c6, they changed [^\[]+ → [^\[\s][^\[]*\s*. The PR description says [^\[]+ was overlapping with \s+. So they made the first char of the label exclude whitespace with [^\[\s]. But then [^\[]* (the rest of the label) still matches spaces, and \s* immediately follows it, same class of overlap.
There was a problem hiding this comment.
Pull request overview
This PR updates a regex used by the terminal chat agent’s output monitoring logic (detectsInputRequiredPattern) to address a potential ReDoS/backtracking issue when detecting PowerShell-style multi-option prompts.
Changes:
- Tightens the “PowerShell-style multi-option line” detection regex by excluding whitespace from the option label match.
| // PowerShell-style multi-option line (supports [?] Help and optional default suffix) ending | ||
| // in whitespace | ||
| /\s*(?:\[[^\]]\]\s+[^\[\s][^\[]*\s*)+(?:\(default is\s+"[^"]+"\):)?\s+$/, | ||
| /\s*(?:\[[^\]]\]\s+[^\[\s][^\[\s]*\s*)+(?:\(default is\s+"[^"]+"\):)?\s+$/, |
There was a problem hiding this comment.
The PR description notes that the change was only to exclude whitespace for the first label character while allowing spaces later ([^\[\s][^\[]*\s*). The current regex also excludes whitespace for the rest of the label ([^\[\s]*), which changes matching semantics for multi-word labels. Consider updating the PR description to reflect the actual change (or adjusting the regex if multi-word labels must remain supported).
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Thanks for looking into this @tolotrasam! The ReDoS diagnosis is spot on. I pushed a commit to your branch that adjusts the fix slightly — the |
|
@microsoft-github-policy-service agree |
|
Hi @meganrogge — looks like the regex fix from this PR was included in commit b97c65a (PR #309611) that was merged to main. Could a Co-authored-by: tolotrasam <tolotrasam@gmail.com@users.noreply.github.com> trailer be added to the squash commit? I identified the remaining ReDoS overlap and submitted the initial fix here. Happy to close this PR since the fix is already on main. Thanks! |
|
Yes! I will add that to #310145, which is another regex fix 😅 |
In commit 87e50c6, they changed [^\[]+ → [^\[\s][^\[]\s. The PR description says [^\[]+ was overlapping with \s+. So they made the first char of the label exclude whitespace with [^\[\s]. But then [^\[]* (the rest of the label) still matches spaces, and \s* immediately follows it, same class of overlap.
This causes catastrophic backtracking on large output and leads to 100% CPU usage causing UI freezes
#307347
#306293
#305077
#307723
Fixes #307723