Skip to content

Conversation

smowton
Copy link
Contributor

@smowton smowton commented Sep 9, 2021

Requires semmle-code PR to pass

@smowton smowton requested a review from a team as a code owner September 9, 2021 15:40
@github-actions github-actions bot added the JS label Sep 9, 2021
erik-krogh
erik-krogh previously approved these changes Sep 9, 2021
@smowton
Copy link
Contributor Author

smowton commented Sep 10, 2021

@github/codeql-javascript this change (extracting invalid surrogates as U+FFFD not ?) turns out to introduce new FPs: ReDoS now thinks that [\uDC00\uDC01]|[\uDC02\uDC03] is vulnerable to exponential backtracking because all four "chars" are extracted as U+FFFD, and similarly the duplicate-character-in-class alert will misfire. Previously the extracted string was also problematic, using ? repeatedly, but both queries stayed quiet, perhaps mistaking the ? for a metacharacter?

Do you want to accept the new FPs or find a way to fix them?

Python's version of ReDoS had the same tests but they didn't work; it does produce these FPs once the test is fixed: #6665

@smowton smowton force-pushed the smowton/admin/javascript-unpaired-surrogate-test branch from c846398 to 963f684 Compare September 10, 2021 16:04
@erik-krogh
Copy link
Contributor

erik-krogh commented Sep 10, 2021

Do you want to accept the new FPs or find a way to fix them?

I want to fix them.
I've pushed a fix straight to this PR.

For now it handles both how surrogates look with the old/new databases.

@smowton
Copy link
Contributor Author

smowton commented Sep 10, 2021

@erik-krogh accidental inclusion of RegExpUnicodeEscape myThing(string v, int i) { ?

@erik-krogh erik-krogh force-pushed the smowton/admin/javascript-unpaired-surrogate-test branch from 4b615e9 to 76de6d1 Compare September 10, 2021 16:12
@smowton
Copy link
Contributor Author

smowton commented Sep 10, 2021

LGTM, will push the semmle-code pointer bump to this branch tip

@smowton smowton changed the title Add test for Javascript literal with an unpaired surrogate character Java and JS: Add/adapt tests for literals with an unpaired surrogate character Sep 10, 2021
@smowton smowton requested a review from a team as a code owner September 10, 2021 17:41
@github-actions github-actions bot added the Java label Sep 10, 2021
@smowton smowton force-pushed the smowton/admin/javascript-unpaired-surrogate-test branch from 6008edd to 3e3c97b Compare September 13, 2021 11:04
@smowton smowton force-pushed the smowton/admin/javascript-unpaired-surrogate-test branch from 3e3c97b to abdd3a5 Compare September 13, 2021 13:12
@smowton smowton merged commit 47b5165 into github:main Sep 13, 2021
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