Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Regular Expression Denial of Service possible #26

Open
andyedwardsibm opened this issue Mar 8, 2021 · 7 comments · May be fixed by #27
Open

Regular Expression Denial of Service possible #26

andyedwardsibm opened this issue Mar 8, 2021 · 7 comments · May be fixed by #27

Comments

@andyedwardsibm
Copy link

https://snyk.io/vuln/SNYK-JS-HTMLPARSESTRINGIFY2-1079307 has been raised for a ReDoS vulnerability, along with CVE-2021-23346. The vulnerability is at

var tagRE = /(?:<!--[\S\s]*?-->|<(?:"[^"]*"['"]*|'[^']*'['"]*|[^'">])+>)/g;

There is a recent fix in the original repo this was forked from. Could the same fix be applied here?

@dnsmob
Copy link

dnsmob commented Mar 10, 2021

i'm sort of wondering if this repo is sorta abandoned or even if it's sort of relevant? as per the description, This could be temporary - I'll gladly drop this if activity picks back up on the original repo. while the original has got it fixed..

@andyedwardsibm
Copy link
Author

I fear you're right

The CVE is "applicable", as the following command does not return in a timely manner...

node -e "const p=require('./index.js'); p.parse(\"<\!'''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''\!\"); console.log('done')"

Replacing

var tagRE = /(?:<!--[\S\s]*?-->|<(?:"[^"]*"['"]*|'[^']*'['"]*|[^'">])+>)/g;
with /(?:<!--[\S\s]*?-->|<(?:"[^"]*"|'[^']*'|[^'">])+>)/g returns in milliseconds.

I'd offer a PR in the hope that this repo is still maintained, but I can't get the test case in HenrikJoreteg@c7274a4 to ever fail. For some reason, running under test, the RegEx fails to match much more quickly than when calling the parse code directly 😕

For now, I guess that anyone else that searches for this CVE will end up here as well

@dnsmob
Copy link

dnsmob commented Mar 10, 2021

i did the same and tests on this repo fail to match the html tag -- of course, it's not a "tag", it thinks it's valid <''>.
so this test

    var tag = "<!''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''!"
    t.deepEqual(parseTag(tag), {
        type: 'tag',
        attrs: {},
        name: '',
        voidElement: false,
        children: []
    });

expects name to be \'\' which is kinda silly and probably means that that regex doesnt really do the job.

i did a hot swap of this module with node-html-parser in react-i18next as it has a parse function too, but it needs to be compiled to work as a dependency for me. on the bright side, this node parser doesnt have vulns 😃

@andyedwardsibm
Copy link
Author

Just want to check what you mean above when you say

this node parser doesnt have vulns

If I run the following snippet, it consumes a CPU at 100% and doesn't return, therefore html-parse-stringify2 is vulnerable to the RegEx DoS attack...

require('html-parse-stringify2').parse("<!'''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''!")

@dnsmob
Copy link

dnsmob commented Mar 11, 2021

i meant node-html-parser doesnt have vulnerabilities and on a hot swap it seems to work (but tbh i havent checked the source code to see if it could be used as a replacement for html-parse-stringify2, i just did a simple test).

@andyedwardsibm
Copy link
Author

Ah gotcha - thanks 😃

@kachkaev
Copy link

node-html-parser 2.0.1 and 2.0.2 cannot be used via npm unfortunately. Reason: HenrikJoreteg#42

adrai added a commit to adrai/html-parse-stringify2 that referenced this issue Mar 21, 2021
@adrai adrai linked a pull request Mar 21, 2021 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants