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

Sanitize innert document #12524

Closed
wants to merge 10 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@IgorMinar
Member

IgorMinar commented Aug 7, 2015

No description provided.

@googlebot

This comment has been minimized.

Show comment
Hide comment
@googlebot

googlebot Aug 7, 2015

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.

googlebot commented Aug 7, 2015

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.

@IgorMinar

This comment has been minimized.

Show comment
Hide comment
@IgorMinar

IgorMinar Aug 7, 2015

Member

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.

Member

IgorMinar commented Aug 7, 2015

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.

@lgalfaso

This comment has been minimized.

Show comment
Hide comment
@lgalfaso

lgalfaso Aug 8, 2015

Member

@IgorMinar even after sorting the attributes, there are two test failures with IE9

IE 9.0.0 (Windows 7) HTML htmlParser should throw badparse if text content contains "<" followed by an ASCII letter without matching ">" FAILED
    Expected { tag : 'a', attrs : { body : '', bar< : '' } } to equal undefined.
IE 9.0.0 (Windows 7) HTML htmlParser should parse empty value attribute of node FAILED
    Expected null to equal { tag : 'option', attrs : { selected : '', value : '' } }.

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
The first issue, I am not sure on how to work around it

Member

lgalfaso commented Aug 8, 2015

@IgorMinar even after sorting the attributes, there are two test failures with IE9

IE 9.0.0 (Windows 7) HTML htmlParser should throw badparse if text content contains "<" followed by an ASCII letter without matching ">" FAILED
    Expected { tag : 'a', attrs : { body : '', bar< : '' } } to equal undefined.
IE 9.0.0 (Windows 7) HTML htmlParser should parse empty value attribute of node FAILED
    Expected null to equal { tag : 'option', attrs : { selected : '', value : '' } }.

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
The first issue, I am not sure on how to work around it

Show outdated Hide outdated src/ngSanitize/sanitize.js
* 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

This comment has been minimized.

@gkalpak

gkalpak Aug 17, 2015

Member

sanitize svg ??

@gkalpak

gkalpak Aug 17, 2015

Member

sanitize svg ??

This comment has been minimized.

@IgorMinar

IgorMinar Sep 8, 2015

Member

fixed.

@IgorMinar

IgorMinar Sep 8, 2015

Member

fixed.

Show outdated Hide outdated src/ngSanitize/sanitize.js
*
* @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.

This comment has been minimized.

@gkalpak

gkalpak Aug 17, 2015

Member

a --> an

@gkalpak

gkalpak Aug 17, 2015

Member

a --> an

This comment has been minimized.

@IgorMinar

IgorMinar Sep 8, 2015

Member

fixed. thanks.

@IgorMinar

IgorMinar Sep 8, 2015

Member

fixed. thanks.

# - 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

This comment has been minimized.

@petebacondarwin

petebacondarwin Sep 11, 2015

Member

Is this disabling just temporary or should we give up with browserstack altogether?

@petebacondarwin

petebacondarwin Sep 11, 2015

Member

Is this disabling just temporary or should we give up with browserstack altogether?

This comment has been minimized.

@IgorMinar

IgorMinar Sep 11, 2015

Member

I think it's just temporary

@IgorMinar

IgorMinar Sep 11, 2015

Member

I think it's just temporary

Show outdated Hide outdated src/ngSanitize/sanitize.js
* @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

This comment has been minimized.

@petebacondarwin

petebacondarwin Sep 11, 2015

Member

I would put this warning inside a <div class="alert alert-warning"> block to make it stand out more.

@petebacondarwin

petebacondarwin Sep 11, 2015

Member

I would put this warning inside a <div class="alert alert-warning"> block to make it stand out more.

Show outdated Hide outdated src/ngSanitize/sanitize.js
}
var docElement = doc.documentElement || doc.getDocumentElement();
var bodyElements = docElement.getElementsByTagName('body');
if (bodyElements.length === 1) {

This comment has been minimized.

@petebacondarwin

petebacondarwin Sep 11, 2015

Member

Is it possible that bodyElements can have length > 1? If so, can we not just take the first one?

@petebacondarwin

petebacondarwin Sep 11, 2015

Member

Is it possible that bodyElements can have length > 1? If so, can we not just take the first one?

This comment has been minimized.

@IgorMinar

IgorMinar Sep 11, 2015

Member

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

@IgorMinar

IgorMinar Sep 11, 2015

Member

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

Show outdated Hide outdated src/ngSanitize/sanitize.js
break;
}
var nextNode;
if (!(nextNode = node.firstChild)) {

This comment has been minimized.

@petebacondarwin

petebacondarwin Sep 11, 2015

Member

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;
@petebacondarwin

petebacondarwin Sep 11, 2015

Member

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;

This comment has been minimized.

@IgorMinar

IgorMinar Sep 11, 2015

Member

nice catch

@IgorMinar

IgorMinar Sep 11, 2015

Member

nice catch

@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Sep 11, 2015

Member

A few small comments but otherwise LGTM

Member

petebacondarwin commented Sep 11, 2015

A few small comments but otherwise LGTM

@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Sep 11, 2015

Member

How far back can this be backported?

Member

petebacondarwin commented Sep 11, 2015

How far back can this be backported?

@IgorMinar

This comment has been minimized.

Show comment
Hide comment
@IgorMinar

IgorMinar Sep 11, 2015

Member

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

Member

IgorMinar commented Sep 11, 2015

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

mhevery and others added some commits May 1, 2015

refactor($sanitize): new implementation of the html sanitized parser
This implementation is based on using inert document parsed by the browser

Closes #11442
Closes #11443
feat($sanitize): make svg support an opt-in
BREAKING CHANGE: The svg support in  is now an opt-in option

Applications that depend on this option can use  to turn the option back on,
but while doing so, please read the warning provided in the  documentation for
information on preventing click-hijacking attacks when this option is turned on.
chore(travis): disable browserstack builds for now
we don't really pay attention to them anyway.
refactor($sanitize): remove <script> from valid block elements
the script and style tag are explicitly blacklisted, so this doesn't change any functionality.
the change is done to improve code clarity.

IgorMinar added a commit that referenced this pull request Sep 18, 2015

feat($sanitize): make svg support an opt-in
Closes #12524

BREAKING CHANGE: The svg support in  is now an opt-in option

Applications that depend on this option can use  to turn the option back on,
but while doing so, please read the warning provided in the  documentation for
information on preventing click-hijacking attacks when this option is turned on.

IgorMinar added a commit that referenced this pull request Sep 18, 2015

chore(travis): disable browserstack builds for now
we don't really pay attention to them anyway.

Closes #12524

IgorMinar added a commit that referenced this pull request Sep 18, 2015

IgorMinar added a commit that referenced this pull request Sep 18, 2015

refactor($sanitize): remove <script> from valid block elements
the script and style tag are explicitly blacklisted, so this doesn't change any functionality.
the change is done to improve code clarity.

Closes #12524
@h3rald

This comment has been minimized.

Show comment
Hide comment
@h3rald

h3rald Sep 29, 2015

@IgorMinar, @petebacondarwin

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?

h3rald commented Sep 29, 2015

@IgorMinar, @petebacondarwin

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?

@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Sep 29, 2015

Member

safer to keep it in master only

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.

Member

petebacondarwin commented Sep 29, 2015

safer to keep it in master only

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.

@h3rald

This comment has been minimized.

Show comment
Hide comment
@h3rald

h3rald Sep 29, 2015

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.
If the fix is not applied, apparently it may be possible to perform a XSS attack in certain conditions -- is it right?

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

h3rald commented Sep 29, 2015

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.
If the fix is not applied, apparently it may be possible to perform a XSS attack in certain conditions -- is it right?

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

@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Sep 30, 2015

Member

I'll have a further chat with @IgorMinar about this.

Member

petebacondarwin commented Sep 30, 2015

I'll have a further chat with @IgorMinar about this.

@IgorMinar

This comment has been minimized.

Show comment
Hide comment
@IgorMinar

IgorMinar Sep 30, 2015

Member

aside from the option to disable svg support (which is not a security issue if you set overflow: hidden on the element containing the sanitized output), I don't believe that there is a reason for considering the previous implementation insecure. The new implementation is simpler and easier to maintain going forward.

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

Member

IgorMinar commented Sep 30, 2015

aside from the option to disable svg support (which is not a security issue if you set overflow: hidden on the element containing the sanitized output), I don't believe that there is a reason for considering the previous implementation insecure. The new implementation is simpler and easier to maintain going forward.

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

@h3rald

This comment has been minimized.

Show comment
Hide comment
@h3rald

h3rald Sep 30, 2015

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!

h3rald commented Sep 30, 2015

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment