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

Sanitize spread attributes in SSR #1623

Merged
merged 3 commits into from
Aug 1, 2018
Merged

Sanitize spread attributes in SSR #1623

merged 3 commits into from
Aug 1, 2018

Conversation

Rich-Harris
Copy link
Member

@Conduitry
Copy link
Member

I'm not sure I see where the regex you're testing for comes from. Your test looks less complicated than the one that React is using, but more complicated than the ones that Preact and Vue are using (which themselves look different from each other). The rest of the changes look fine, but I guess I'm confused as to why the three (and, with this one, four) different libraries all use different tests.

@Rich-Harris
Copy link
Member Author

Yeah, it's a combination of this...

Attribute names must consist of one or more characters other than controls, U+0020 SPACE, U+0022 ("), U+0027 ('), U+003E (>), U+002F (/), U+003D (=), and noncharacters

...and this (defines 'noncharacter'):

A noncharacter is a code point that is in the range U+FDD0 to U+FDEF, inclusive, or U+FFFE, U+FFFF, U+1FFFE, U+1FFFF, U+2FFFE, U+2FFFF, U+3FFFE, U+3FFFF, U+4FFFE, U+4FFFF, U+5FFFE, U+5FFFF, U+6FFFE, U+6FFFF, U+7FFFE, U+7FFFF, U+8FFFE, U+8FFFF, U+9FFFE, U+9FFFF, U+AFFFE, U+AFFFF, U+BFFFE, U+BFFFF, U+CFFFE, U+CFFFF, U+DFFFE, U+DFFFF, U+EFFFE, U+EFFFF, U+FFFFE, U+FFFFF, U+10FFFE, or U+10FFFF

The Preact test is definitely a lot simpler — I'm just not sure if \s and \0 cover the non-characters listed above, but I chose to err on the side of completeness. The same goes for the Vue test. I don't think it's material though — the vulnerability comes from rendering " unescaped, since that's how an attacker is able to close the attribute in order to close the element in order to insert malicious markup.

The React test is using a whitelist approach instead of a blacklist, which I guess is theoretically safer? I just figured I'd adhere as closely as possible to the spec.

Agree that it's weird that there isn't a shared library for this, but hey ho.

@Rich-Harris Rich-Harris merged commit 065d0f7 into master Aug 1, 2018
@Rich-Harris Rich-Harris deleted the CVE-2018-6341 branch August 1, 2018 21:38
@mrkishi
Copy link
Member

mrkishi commented Aug 1, 2018

Attribute names must consist of one or more characters other than controls, U+0020 SPACE, U+0022 ("), U+0027 ('), U+003E (>), U+002F (/), U+003D (=), and noncharacters

Ooops. There's a typo in the code then:

export const invalidAttributeNameCharacter = /[\s'"<\/=\u{FD...]/u;

// should be a greater than

export const invalidAttributeNameCharacter = /[\s'">\/=\u{FD...]/u;

@Rich-Harris
Copy link
Member Author

oops! good catch @mrkishi — have released a fix

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 this pull request may close these issues.

None yet

3 participants