Fix ReDoS when parsing colors #3382
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
piaskowyk
approved these changes
Jul 25, 2022
piaskowyk
pushed a commit
that referenced
this pull request
Jul 25, 2022
## Description The regular expression used to parse numbers could be abused to cause a denial of service if the color to be parsed is controlled by an attacker. For information about this vulnerability, see https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS. ## Changes Changed the code so the new regex isn't vulnerable, since there's always only one possible path to take given any input. Here is a proof-of-concept of how I made the existing regular expression cause a 100% CPU usage for several seconds, after being interrupted: ``` Welcome to Node.js v14.19.3. Type ".help" for more information. > (new RegExp('^[-+]?\\d*\\.?\\d+$')).test(`${'1'.repeat(1e6)}z`) ^CUncaught Error: Script execution was interrupted by `SIGINT` at Script.runInThisContext (vm.js:134:12) at REPLServer.defaultEval (repl.js:566:29) at bound (domain.js:421:15) at REPLServer.runBound [as eval] (domain.js:432:12) at REPLServer.onLine (repl.js:909:10) at REPLServer.emit (events.js:412:35) at REPLServer.emit (domain.js:475:12) at REPLServer.Interface._onLine (readline.js:434:10) at REPLServer.Interface._line (readline.js:791:8) at REPLServer.Interface._ttyWrite (readline.js:1136:14) { code: 'ERR_SCRIPT_EXECUTION_INTERRUPTED' } > (new RegExp('^[-+]?\\d*(?:\\.\\d*)?$')).test(`${'1'.repeat(1e6)}z`) // the new regex works fine false > ``` ## Checklist - [X] Included code example that can be used to test this change - [ ] Updated TS types - [ ] Added TS types tests - [ ] Added unit / integration tests - [ ] Updated documentation - [ ] Ensured that CI passes
|
@matias-la @piaskowyk ... just to be sure, is it OK that the new regex accepts empty string and returns true? OLD > (new RegExp('^[-+]?\\d*\\.?\\d+$')).test('')
falseNEW > (new RegExp('^[-+]?\\d*(?:\\.\\d*)?$')).test(``)
true |
|
Oops, nice catch! I hadn't notice that. The change in #3419 look good, as it prevents empty strings from matching while also not being vulnerable to ReDoS. |
piaskowyk
added a commit
that referenced
this pull request
Aug 2, 2022
## Description PR based on issue requested in comment: #3382 (comment) I updated the regex to prevent empty string matches. The new regex `[-+]?(?:\d+(?:\.\d*)?|\.\d+)` - https://regex101.com/r/aHhlXG/1 is equivalent with the old version `[-+]?\d*\.?\d+` - https://regex101.com/r/Z0xMqR/1 I also tested ReDoS, and the new regex is safe. ``` (new RegExp('^[-+]?(?:\d+(?:\.\d*)?|\.\d+)$')).test(`${'1'.repeat(1e6)}z`) ```
piaskowyk
added a commit
that referenced
this pull request
Aug 2, 2022
## Description PR based on issue requested in comment: #3382 (comment) I updated the regex to prevent empty string matches. The new regex `[-+]?(?:\d+(?:\.\d*)?|\.\d+)` - https://regex101.com/r/aHhlXG/1 is equivalent with the old version `[-+]?\d*\.?\d+` - https://regex101.com/r/Z0xMqR/1 I also tested ReDoS, and the new regex is safe. ``` (new RegExp('^[-+]?(?:\d+(?:\.\d*)?|\.\d+)$')).test(`${'1'.repeat(1e6)}z`) ```
EvertEt
added a commit
to EvertEt/normalize-css-color
that referenced
this pull request
Oct 27, 2022
5 tasks
fluiddot
pushed a commit
to wordpress-mobile/react-native-reanimated
that referenced
this pull request
Jun 5, 2023
## Description The regular expression used to parse numbers could be abused to cause a denial of service if the color to be parsed is controlled by an attacker. For information about this vulnerability, see https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS. ## Changes Changed the code so the new regex isn't vulnerable, since there's always only one possible path to take given any input. Here is a proof-of-concept of how I made the existing regular expression cause a 100% CPU usage for several seconds, after being interrupted: ``` Welcome to Node.js v14.19.3. Type ".help" for more information. > (new RegExp('^[-+]?\\d*\\.?\\d+$')).test(`${'1'.repeat(1e6)}z`) ^CUncaught Error: Script execution was interrupted by `SIGINT` at Script.runInThisContext (vm.js:134:12) at REPLServer.defaultEval (repl.js:566:29) at bound (domain.js:421:15) at REPLServer.runBound [as eval] (domain.js:432:12) at REPLServer.onLine (repl.js:909:10) at REPLServer.emit (events.js:412:35) at REPLServer.emit (domain.js:475:12) at REPLServer.Interface._onLine (readline.js:434:10) at REPLServer.Interface._line (readline.js:791:8) at REPLServer.Interface._ttyWrite (readline.js:1136:14) { code: 'ERR_SCRIPT_EXECUTION_INTERRUPTED' } > (new RegExp('^[-+]?\\d*(?:\\.\\d*)?$')).test(`${'1'.repeat(1e6)}z`) // the new regex works fine false > ``` ## Checklist - [X] Included code example that can be used to test this change - [ ] Updated TS types - [ ] Added TS types tests - [ ] Added unit / integration tests - [ ] Updated documentation - [ ] Ensured that CI passes
fluiddot
pushed a commit
to wordpress-mobile/react-native-reanimated
that referenced
this pull request
Jun 5, 2023
## Description PR based on issue requested in comment: software-mansion#3382 (comment) I updated the regex to prevent empty string matches. The new regex `[-+]?(?:\d+(?:\.\d*)?|\.\d+)` - https://regex101.com/r/aHhlXG/1 is equivalent with the old version `[-+]?\d*\.?\d+` - https://regex101.com/r/Z0xMqR/1 I also tested ReDoS, and the new regex is safe. ``` (new RegExp('^[-+]?(?:\d+(?:\.\d*)?|\.\d+)$')).test(`${'1'.repeat(1e6)}z`) ```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
The regular expression used to parse numbers could be abused to cause a denial of service if the color to be parsed is controlled by an attacker.
For information about this vulnerability, see https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS.
Changes
Changed the code so the new regex isn't vulnerable, since there's always only one possible path to take given any input.
Here is a proof-of-concept of how I made the existing regular expression cause a 100% CPU usage for several seconds, after being interrupted:
Checklist