Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

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

Merged
merged 1 commit into from
Feb 24, 2017
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions docs/content/error/$sanitize/elclob.ngdoc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
@ngdoc error
@name $sanitize:elclob
@fullName Failed to sanitize html because the element is clobbered
@description

This error occurs when `$sanitize` sanitizer is unable to traverse the HTML because one or more of the elements in the
HTML have been "clobbered". This could be a sign that the payload contains code attempting to cause a DoS attack on the
browser.

Typically clobbering breaks the `nextSibling` property on an element so that it points to one of its child nodes. This
makes it impossible to walk the HTML tree without getting stuck in an infinite loop, which causes the browser to freeze.
23 changes: 19 additions & 4 deletions src/ngSanitize/sanitize.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ var forEach;
var isDefined;
var lowercase;
var noop;
var nodeContains;
var htmlParser;
var htmlSanitizeWriter;

Expand Down Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

};

// Regular Expressions for parsing tags and attributes
var SURROGATE_PAIR_REGEXP = /[\uD800-\uDBFF][\uDC00-\uDFFF]/g,
// Match everything outside of normal chars and " (quote character)
Expand Down Expand Up @@ -381,12 +387,12 @@ function $SanitizeProvider() {
if (node.nodeType === 1) {
handler.end(node.nodeName.toLowerCase());
Copy link
Contributor

Choose a reason for hiding this comment

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

nodeName can be clobbered.

Copy link
Member

Choose a reason for hiding this comment

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

See #15699 (comment).

}
nextNode = node.nextSibling;
nextNode = getNonDescendant('nextSibling', node);
if (!nextNode) {
while (nextNode == null) {
node = node.parentNode;
node = getNonDescendant('parentNode', node);
if (node === inertBodyElement) break;
nextNode = node.nextSibling;
nextNode = getNonDescendant('nextSibling', node);
if (node.nodeType === 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nodeType can be clobberred.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

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).

handler.end(node.nodeName.toLowerCase());
}
Expand Down Expand Up @@ -518,8 +524,17 @@ function $SanitizeProvider() {
stripCustomNsAttrs(nextNode);
}

node = node.nextSibling;
node = getNonDescendant('nextSibling', node);
}
}

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)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

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 😃

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, let's leave it as it is then

throw $sanitizeMinErr('elclob', 'Failed to sanitize html because the element is clobbered: {0}', node.outerHTML || node.outerText);
}
return nextNode;
}
}

Expand Down
20 changes: 20 additions & 0 deletions test/ngSanitize/sanitizeSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,26 @@ describe('HTML', function() {
.toEqual('<p>text1text2</p>');
});

it('should remove clobbered elements', function() {
inject(function($sanitize) {
expect(function() {
$sanitize('<form><input name="parentNode" /></form>');
}).toThrowMinErr('$sanitize', 'elclob');

expect(function() {
$sanitize('<form><div><div><input name="parentNode" /></div></div></form>');
}).toThrowMinErr('$sanitize', 'elclob');

expect(function() {
$sanitize('<form><input name="nextSibling" /></form>');
}).toThrowMinErr('$sanitize', 'elclob');

expect(function() {
$sanitize('<form><div><div><input name="nextSibling" /></div></div></form>');
}).toThrowMinErr('$sanitize', 'elclob');
});
});


describe('SVG support', function() {

Expand Down