fix($sanitize): prevent clobbered elements from freezing the browser #15699

petebacondarwin · 2017-02-10T12:14:35Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

bug fix

What is the current behavior? (You can also link to an open issue here)

browser can be frozen with bad HTML

What is the new behavior (if this is a feature change)?

an exception is thrown instead

Does this PR introduce a breaking change?

No

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

Other information:

googlebot · 2017-02-10T15:37:27Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

googlebot · 2017-02-10T15:37:27Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

koto · 2017-02-14T10:04:31Z

src/ngSanitize/sanitize.js

@@ -218,6 +219,11 @@ function $SanitizeProvider() {
  htmlParser = htmlParserImpl;
  htmlSanitizeWriter = htmlSanitizeWriterImpl;

+  nodeContains = window.Node.prototype.contains || /** @this */ function(arg) {
+    // eslint-disable-next-line no-bitwise
+    return !!(this.compareDocumentPosition(arg) & 16);


compareDocumentPosition can also be clobbered. That will throw an exception - is that OK for your case i.e. is the exception caught and safely resolved?

It is OK afaict. What we are trying to avoid is having an infinite loop, while traversing the DOM tree, which will eventually crash the browser.

koto · 2017-02-14T10:10:33Z

src/ngSanitize/sanitize.js

            if (node === inertBodyElement) break;
-            nextNode = node.nextSibling;
+            nextNode = getNonDescendant('nextSibling', node);
            if (node.nodeType === 1) {


nodeType can be clobberred.

What we are trying to avoid (afaict) is having an infinite loop, while traversing the DOM tree, which will eventually crash the browser.

I couldn't find a way to achive this via nodeType or nodeName. Did I miss something?

If an infinite loop is the only concern, disregard my comments. JS-based sanitizers can usually fail by making them throw which breaks applications not catching an exception. Commonly, DOM clobbering is used to do that. If exceptions in ngSanitize do not result in apps breaking, it's fine.

If an infinite loop is the only concern, disregard my comments.

I meant that is what we are address in this PR 😃
But afaict, we are only sanitizing HTML from ngBindHtml's watch-action. Throwing inside a watch-action, will catch the error, pass it to the $exceptionHandler and move on to the next watcher.

But if someone uses it differently, it is possible that it breaks the app (as any thrown exception could). Do you think $sanitize should be handling its own errors, @koto ?

If the errors are triggered by processing untrusted HTML (which is likely the most common code), I think the safe default would be to return an empty string instead of throwing and requiring application to catch. But tat this point it's not really a security issue, but should be similar to how other core services behave (i.e. is it expected for them to throw and it should be handled by apps).

koto · 2017-02-14T10:11:07Z

src/ngSanitize/sanitize.js

@@ -381,12 +387,12 @@ function $SanitizeProvider() {
        if (node.nodeType === 1) {
          handler.end(node.nodeName.toLowerCase());


nodeName can be clobbered.

See #15699 (comment).

petebacondarwin · 2017-02-23T21:52:34Z

src/ngSanitize/sanitize.js

+  function getNonDescendant(propName, node) {
+    // An element is clobbered if its `propName` property points to one of its descendants
+    var nextNode = node[propName];
+    if (nextNode && nodeContains.call(node, nextNode)) {


are we bothered about generic descendents or only inputs that are members of a form?
if the latter then could we do:

if (nextNode && nextNode.form === node)

I don't know of another usecase that would be affected other than <form> elements. (document is affected too, but we can't access document inside the htmlParsers afaict.)
I can't be 100% sure though. DOM has many dark corners 😃

Hm...it seems that this won't work for img elements:

<form name="foo> <input name="bar" /> <img name="baz" /> </form>

document.foo.bar //--> `<input ... />` document.foo.bar.form //--> `<form ... >` document.foo.baz //--> `<img ... />` document.foo.baz.form //--> undefined

OK, let's leave it as it is then

Closes angular#15699

Closes #15699

petebacondarwin · 2017-02-24T17:43:44Z

We merged this as-is. If there turns out to be performance issues then we will revisit and consider a different algorithm.

Closes angular#15699

googlebot added cla: yes labels Feb 10, 2017

petebacondarwin added component: ngSanitize needs: review type: bug severity: security labels Feb 10, 2017

googlebot added cla: no and removed cla: yes labels Feb 10, 2017

gkalpak force-pushed the next-sibling branch from 5e3d716 to 0821355 Compare February 10, 2017 15:38

koto reviewed Feb 14, 2017

View reviewed changes

gkalpak added this to the 1.6.3 milestone Feb 15, 2017

petebacondarwin commented Feb 23, 2017

View reviewed changes

fix($sanitize): prevent clobbered elements from freezing the browser

4f69d38

Closes angular#15699

petebacondarwin force-pushed the next-sibling branch from 9c91778 to 4f69d38 Compare February 24, 2017 16:38

petebacondarwin pushed a commit that referenced this pull request Feb 24, 2017

fix($sanitize): prevent clobbered elements from freezing the browser

3bb1dd5

Closes #15699

petebacondarwin merged commit 4f69d38 into angular:master Feb 24, 2017

ellimist pushed a commit to ellimist/angular.js that referenced this pull request Mar 15, 2017

fix($sanitize): prevent clobbered elements from freezing the browser

3cb9913

Closes angular#15699

vicb mentioned this pull request Mar 15, 2017

fix(platform-browser): prevent clobbered elements from freezing the browser angular/angular#15158

Merged

cwick pushed a commit to udemy-archive/angular.js that referenced this pull request Nov 23, 2019

fix($sanitize): prevent clobbered elements from freezing the browser

f16b9e0

Closes angular#15699

This was referenced Mar 18, 2021

WS-2017-3766 (Medium) detected in angularjs-1.4.3.jar tamirverthim/fitbit-api-example-java#136

Open

WS-2017-3766 (Medium) detected in angularjs-1.4.3.jar Nehamaefi/fitbit-api-example-java#140

Open

mend-bolt-for-github bot mentioned this pull request Mar 18, 2021

WS-2017-3766 (Medium) detected in angularjs-1.4.3.jar mibo32/fitbit-api-example-java#123

Open

mend-bolt-for-github bot mentioned this pull request May 24, 2021

WS-2017-3766 (Medium) detected in angularjs-1.4.3.jar william31212/fitbit-api-example-java2#37

Open

mend-bolt-for-github bot mentioned this pull request Jun 19, 2021

WS-2017-3766 (Medium) detected in angularjs-1.4.3.jar bharathirajatut/fitbit-api-example-java2#37

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix($sanitize): prevent clobbered elements from freezing the browser #15699

fix($sanitize): prevent clobbered elements from freezing the browser #15699

petebacondarwin commented Feb 10, 2017

googlebot commented Feb 10, 2017

googlebot commented Feb 10, 2017

koto Feb 14, 2017

gkalpak Feb 14, 2017

koto Feb 14, 2017

gkalpak Feb 14, 2017

koto Feb 14, 2017

gkalpak Feb 15, 2017

koto Feb 15, 2017

koto Feb 14, 2017

gkalpak Feb 14, 2017

petebacondarwin Feb 23, 2017

gkalpak Feb 24, 2017

gkalpak Feb 24, 2017

petebacondarwin Feb 24, 2017

petebacondarwin commented Feb 24, 2017

		@@ -381,12 +387,12 @@ function $SanitizeProvider() {
		if (node.nodeType === 1) {
		handler.end(node.nodeName.toLowerCase());

fix($sanitize): prevent clobbered elements from freezing the browser #15699

fix($sanitize): prevent clobbered elements from freezing the browser #15699

Conversation

petebacondarwin commented Feb 10, 2017

googlebot commented Feb 10, 2017

googlebot commented Feb 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin commented Feb 24, 2017