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

Fix html attribute escaping #4015

Merged
merged 3 commits into from Feb 20, 2022
Merged

Conversation

mrkishi
Copy link
Member

@mrkishi mrkishi commented Feb 20, 2022

Fixes #4014.

This just adds the missing & and removes superfluous <> from the html attribute escape dictionary.

<> are treated as raw characters inside a double-quoted attribute value. This means they can contain </script> and friends inside, but if we're already in rawtext mode (meaning, inside an open <script> or other greedy html tags), then we're already inside an xss vector and we'd have to escape every single character, as kill(everyone) would be a valid value.

Try it: <tag data-value="<script>alert('pwned')</script>"> is properly handled by browsers.

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Sorry but tests timeout at some point on this weak machine.

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpx changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

@changeset-bot
Copy link

changeset-bot bot commented Feb 20, 2022

🦋 Changeset detected

Latest commit: 53982ad

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sveltejs/kit Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@benmccann
Copy link
Member

Why does & need to be encoded? OWASP suggests simply encoding " to &#x22; (should we use that rather than &quot;?)

https://cheatsheetseries.owasp.org/cheatsheets/Cross_Site_Scripting_Prevention_Cheat_Sheet.html#rule-2-attribute-encode-before-inserting-untrusted-data-into-html-common-attributes

@mrkishi
Copy link
Member Author

mrkishi commented Feb 20, 2022

Why does & need to be encoded?

It's not exactly a security issue in this case. & won't break the surrounding quotes and enable a direct xss attack, however, it will change the value we'll get back from the dom.

const parent = document.body;
const text = 'hello &quot;world&quot;';
parent.insertAdjacentHTML('afterbegin', `<div data-string="${text}"></div>`);
if (parent.firstChild.dataset.string !== text) {
	throw new Error('oops! lost information');
}

should we use that rather than &quot;?

I've no idea. I assume quot has a higher chance of being compressible, but newer compression algorithms like brotli are pre-primed with web-specific data, and both might be in there already!

Copy link
Member

@Conduitry Conduitry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense to me. Thank you!

@benmccann benmccann merged commit 827d162 into sveltejs:master Feb 20, 2022
@mrkishi mrkishi deleted the attr-escape-fix branch February 20, 2022 21:46
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.

Missing ampersand escaping in html attributes
4 participants