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

Xss fix #40136

Closed
wants to merge 1 commit into from
Closed

Xss fix #40136

wants to merge 1 commit into from

Conversation

mhevery
Copy link
Contributor

@mhevery mhevery commented Dec 15, 2020

Escape the content of the strings so that it can be safely inserted into a comment node.

The issue is that HTML does not specify any way to escape comment end text inside the comment. <!-- The way you close a comment is with "-->". -->. Above the "-->" is meant to be text not an end to the comment. This can be created programmatically through DOM APIs.

div.innerHTML = div.innerHTML

One would expect that the above code would be safe to do, but it turns out that because comment text is not escaped, the comment may contain text which will prematurely close the comment opening up the application for XSS attack. (In SSR we programmatically create comment nodes which may contain such text and expect them to be safe.)

This function escapes the comment text by looking for the closing char sequence --> and replace it with -_-_> where the _ is a zero width space \u200B. The result is that if a comment contains --> text it will render normally but it will not cause the HTML parser to close the comment.

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@google-cla google-cla bot added the cla: yes label Dec 15, 2020
@mhevery mhevery requested review from IgorMinar and AndrewKushnir and removed request for IgorMinar Dec 15, 2020
@pullapprove pullapprove bot requested review from atscott and jelbourn Dec 15, 2020
@pullapprove pullapprove bot added comp: core Runtime issues comp: docs/api Used in combination with comp: docs, for issues specific to API reference docs and removed cla: yes labels Dec 15, 2020
@google-cla google-cla bot added the cla: yes label Dec 15, 2020
@google-cla
Copy link

@google-cla google-cla bot commented Dec 15, 2020

☹️ Sorry, but only Googlers may change the label cla: yes.

@ngbot ngbot bot modified the milestone: Backlog Dec 15, 2020
@pullapprove pullapprove bot requested a review from alxhub Dec 15, 2020
@mhevery mhevery added target: lts This PR is targeting a version currently in long-term support target: patch This PR is targeted for the next patch release and removed comp: core Runtime issues comp: docs/api Used in combination with comp: docs, for issues specific to API reference docs labels Dec 15, 2020
@ngbot ngbot bot modified the milestone: Backlog Dec 15, 2020
@mhevery mhevery added the action: merge PR author is ready for this to merge label Dec 15, 2020
@pullapprove pullapprove bot added comp: core Runtime issues comp: docs/api Used in combination with comp: docs, for issues specific to API reference docs labels Dec 15, 2020
@ngbot ngbot bot modified the milestone: Backlog Dec 15, 2020
@pullapprove pullapprove bot removed the comp: docs/api Used in combination with comp: docs, for issues specific to API reference docs label Dec 15, 2020
Copy link
Member

@jelbourn jelbourn left a comment

LGTM

Reviewed-for: fw-security

packages/core/test/acceptance/security_spec.ts Outdated Show resolved Hide resolved
packages/core/test/acceptance/security_spec.ts Outdated Show resolved Hide resolved
packages/core/test/util/dom_spec.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

LGTM, thanks for the fix @mhevery 👍
(just left a couple minor comments related to comments, nothing critical)

packages/core/test/acceptance/security_spec.ts Outdated Show resolved Hide resolved
packages/core/src/util/dom.ts Show resolved Hide resolved
@mhevery
Copy link
Contributor Author

@mhevery mhevery commented Dec 15, 2020

presubmit

Escape the content of the strings so that it can be safely inserted into a comment node.
The issue is that HTML does not specify any way to escape comment end text inside the comment.
`<!-- The way you close a comment is with "-->". -->`. Above the `"-->"` is meant to be text
not an end to the comment. This can be created programmatically through DOM APIs.

```
div.innerHTML = div.innerHTML
```
One would expect that the above code would be safe to do, but it turns out that because comment
text is not escaped, the comment may contain text which will prematurely close the comment
opening up the application for XSS attack. (In SSR we programmatically create comment nodes which
may contain such text and expect them to be safe.)
This function escapes the comment text by looking for the closing char sequence `-->` and replace
it with `-_-_>` where the `_` is a zero width space `\u200B`. The result is that if a comment
contains `-->` text it will render normally but it will not cause the HTML parser to close the
comment.
*/

const END_COMMENT = /-->/g;
const END_COMMENT_ESCAPED = '-\u200B-\u200B>';
Copy link
Member

@gkalpak gkalpak Dec 16, 2020

Choose a reason for hiding this comment

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

OOC, why use two \u200Bs? Wouldn't --\u200B> also work?

@alxhub alxhub removed the target: lts This PR is targeting a version currently in long-term support label Dec 16, 2020
@alxhub alxhub closed this in 47d9b6d Dec 16, 2020
alxhub pushed a commit that referenced this issue Dec 16, 2020
Escape the content of the strings so that it can be safely inserted into a comment node.
The issue is that HTML does not specify any way to escape comment end text inside the comment.
`<!-- The way you close a comment is with "-->". -->`. Above the `"-->"` is meant to be text
not an end to the comment. This can be created programmatically through DOM APIs.

```
div.innerHTML = div.innerHTML
```
One would expect that the above code would be safe to do, but it turns out that because comment
text is not escaped, the comment may contain text which will prematurely close the comment
opening up the application for XSS attack. (In SSR we programmatically create comment nodes which
may contain such text and expect them to be safe.)
This function escapes the comment text by looking for the closing char sequence `-->` and replace
it with `-_-_>` where the `_` is a zero width space `\u200B`. The result is that if a comment
contains `-->` text it will render normally but it will not cause the HTML parser to close the
comment.

PR Close #40136
twerske pushed a commit to twerske/angular that referenced this issue Jan 5, 2021
…ar#40136)

Escape the content of the strings so that it can be safely inserted into a comment node.
The issue is that HTML does not specify any way to escape comment end text inside the comment.
`<!-- The way you close a comment is with "-->". -->`. Above the `"-->"` is meant to be text
not an end to the comment. This can be created programmatically through DOM APIs.

```
div.innerHTML = div.innerHTML
```
One would expect that the above code would be safe to do, but it turns out that because comment
text is not escaped, the comment may contain text which will prematurely close the comment
opening up the application for XSS attack. (In SSR we programmatically create comment nodes which
may contain such text and expect them to be safe.)
This function escapes the comment text by looking for the closing char sequence `-->` and replace
it with `-_-_>` where the `_` is a zero width space `\u200B`. The result is that if a comment
contains `-->` text it will render normally but it will not cause the HTML parser to close the
comment.

PR Close angular#40136
@angular-automatic-lock-bot
Copy link

@angular-automatic-lock-bot angular-automatic-lock-bot bot commented Jan 16, 2021

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jan 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge PR author is ready for this to merge cla: yes comp: core Runtime issues target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants