Sanitize innert document #12524
7 participants
|
We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm. |
|
hmm.. tests are failing because different browsers enumerate over the list of tags in a different order which results in non-deterministic. I'll either have to sort the attribute list or change the matcher to account for this. |
|
@IgorMinar even after sorting the attributes, there are two test failures with IE9 I think it should be possible to fix the second issue with something like https://github.com/angular/angular.js/blob/master/src/jqLite.js#L159 |
gkalpak
and 1 other
commented on an outdated diff
| @@ -153,6 +154,42 @@ function $SanitizeProvider() { | ||
| return buf.join(''); | ||
| }; | ||
| }]; | ||
| + | ||
| + | ||
| + /** | ||
| + * @ngdoc method | ||
| + * @name $sanitizeProvider#enableSvg | ||
| + * @kind function | ||
| + * | ||
| + * @description | ||
| + * Enables a subset of svg to be supported by the sanitizer. | ||
| + * | ||
| + * **Warning**: By enabling this setting without taking other precautions, you might expose your | ||
| + * application to click-hijacking attacks. In these attacks, a sanitize svg could be positioned |
|
|
gkalpak
and 1 other
commented on an outdated diff
| + * application to click-hijacking attacks. In these attacks, a sanitize svg could be positioned | ||
| + * outside of the containing element and be rendered over other elements on the page (e.g. a login | ||
| + * link). Such behavior can then result in phishing incidents. | ||
| + * | ||
| + * To protect against these, explicitly setup `overflow: hidden` css rule for all potential svg | ||
| + * tags within the sanitized content: | ||
| + * | ||
| + * ``` | ||
| + * .rootOfTheIncludedContent svg { | ||
| + * overflow: hidden !important; | ||
| + * } | ||
| + * ``` | ||
| + * | ||
| + * @param {boolean=} regexp New regexp to whitelist urls with. | ||
| + * @returns {boolean|ng.$sanitizeProvider} Returns the currently configured value if called | ||
| + * without a argument or self for chaining otherwise. |
|
|
| @@ -19,10 +19,10 @@ env: | ||
| - JOB=docs-e2e BROWSER_PROVIDER=saucelabs | ||
| - JOB=e2e TEST_TARGET=jqlite BROWSER_PROVIDER=saucelabs | ||
| - JOB=e2e TEST_TARGET=jquery BROWSER_PROVIDER=saucelabs | ||
| - - JOB=unit BROWSER_PROVIDER=browserstack | ||
| - - JOB=docs-e2e BROWSER_PROVIDER=browserstack | ||
| - - JOB=e2e TEST_TARGET=jqlite BROWSER_PROVIDER=browserstack | ||
| - - JOB=e2e TEST_TARGET=jquery BROWSER_PROVIDER=browserstack | ||
| +# - JOB=unit BROWSER_PROVIDER=browserstack | ||
| +# - JOB=docs-e2e BROWSER_PROVIDER=browserstack | ||
| +# - JOB=e2e TEST_TARGET=jqlite BROWSER_PROVIDER=browserstack | ||
| +# - JOB=e2e TEST_TARGET=jquery BROWSER_PROVIDER=browserstack |
petebacondarwin
Is this disabling just temporary or should we give up with browserstack altogether? |
petebacondarwin
commented on an outdated diff
| @@ -153,6 +154,42 @@ function $SanitizeProvider() { | ||
| return buf.join(''); | ||
| }; | ||
| }]; | ||
| + | ||
| + | ||
| + /** | ||
| + * @ngdoc method | ||
| + * @name $sanitizeProvider#enableSvg | ||
| + * @kind function | ||
| + * | ||
| + * @description | ||
| + * Enables a subset of svg to be supported by the sanitizer. | ||
| + * | ||
| + * **Warning**: By enabling this setting without taking other precautions, you might expose your |
|
petebacondarwin
I would put this warning inside a |
petebacondarwin
and 1 other
commented on an outdated diff
| @@ -262,11 +289,30 @@ function makeMap(str, lowercaseKeys) { | ||
| return obj; | ||
| } | ||
| +var inertBodyElement; | ||
| +(function(window) { | ||
| + var doc; | ||
| + if (window.document && window.document.implementation) { | ||
| + doc = window.document.implementation.createHTMLDocument("inert"); | ||
| + } else { | ||
| + throw $sanitizeMinErr('noinert', "Can't create an inert html document"); | ||
| + } | ||
| + var docElement = doc.documentElement || doc.getDocumentElement(); | ||
| + var bodyElements = docElement.getElementsByTagName('body'); | ||
| + if (bodyElements.length === 1) { |
|
petebacondarwin
Is it possible that 
IgorMinar
usually there should be only one body, but in IE there is none, so we need to create it. I'll add a comment to that code |
petebacondarwin
and 1 other
commented on an outdated diff
| + html = inertBodyElement.innerHTML; //trigger mXSS | ||
| + inertBodyElement.innerHTML = html; | ||
| + } while (html !== inertBodyElement.innerHTML); | ||
| + | ||
| + var node = inertBodyElement.firstChild; | ||
| + while (node) { | ||
| + switch (node.nodeType) { | ||
| + case 1: // ELEMENT_NODE | ||
| + handler.start(node.nodeName.toLowerCase(), attrToMap(node.attributes)); | ||
| + break; | ||
| + case 3: // TEXT NODE | ||
| + handler.chars(node.textContent); | ||
| + break; | ||
| + } | ||
| + var nextNode; | ||
| + if (!(nextNode = node.firstChild)) { |
|
petebacondarwin
I think this block can be simplified: var nextNode;
if (!(nextNode = node.firstChild)) {
if (node.nodeType == 1) {
handler.end(node.nodeName.toLowerCase());
}
nextNode = node.nextSibling;
if (!nextNode) {
while (nextNode == null) {
node = node.parentNode;
if (node === inertBodyElement) break;
nextNode = node.nextSibling;
if (node.nodeType == 1) {
handler.end(node.nodeName.toLowerCase());
}
}
}
}
node = nextNode; |
|
A few small comments but otherwise LGTM |
|
How far back can this be backported? |
|
@petebacondarwin since there are breaking changes included in here, I don't think that we should backport this to older versions. It's safer to keep it in master only. |
|
Hi, what do you mean with "it's safer to keep it in master only"? Is there any plan to include this security fix in a 1.4.x release? |
This means that since there are breaking changes, trying to backport it to older versions could cause havoc with live applications. The message is that if you are concerned about these security features then you should upgrade to 1.5 as soon as it comes out. |
|
Hi Pete, thanks a lot for the clarification. I read through the commits and the only breaking change seems to be the fact that SVG sanitization is not enabled by default (and if enabled you recommend extra care), right? The fact that you are now relying on the browser itself to parse HTML code by creating an inert HTML document should not be too much of a problem because AFAIK this functionality is available in all major browsers (including IE9+... I was notified of this fix by a security notification issued throughout my company which recommended that we apply the patch manually as a temporary workaround until the fix is officially distributed. Thanks again for your time! P.S.: I actually tried to apply the fixes to v1.4.3 and it doesn't seem to break much (I still have to test it with directives using SVG though). |
|
I'll have a further chat with @IgorMinar about this. |
|
aside from the option to disable svg support (which is not a security issue if you set The reason why we didn't backport this to 1.4 was that the new implementation could parse html slightly differently which could result in hard-to-debug issues in the rendered output (but not security vulnerabilities). |
|
Hi Igor, Thanks a lot for your comment: it really clarifies things a lot! My original concern about backporting was due to the fact that this particular pull request was notified to me as a "fix" for possible vulnerabilities affecting Angular sanitization. Based on your comments (and also after doing a bit of testing with both implementations at this point) I think that perhaps the security notification I received was perhaps a bit misleading or simply tagged incorrectly. I will investigate further within my company on why this happened. Thanks a lot for your support! |
No description provided.