feat($compile): Lower the security context of SVG's a and image xlink:href #15736
Conversation
src/ng/compile.js
Outdated
| } else if (attrNormalizedName === 'xlinkHref' || | ||
| } else if ( | ||
| // Some xlink:href are okay, most aren't | ||
| (attrNormalizedName === 'xlinkHref' && (tag !== 'image' && tag !== 'a')) || |
There was a problem hiding this comment.
Awww, I'm bad at counting spaces :/ Fixed !
|
@rjamet: We discussed this and it does indeed qualify as a breaking change (even if the broken cases are dubious/uncommon). Can you update the commit message with a BC notice (and potentially change the scope from |
rjamet
changed the title
|
Done. I don't feel strongly about anything here: this change is to be more permissive, but I can wait for the next breaking change window without worries. |
|
👍 |
|
How can I do so? Rewriting history locally then force-pushing to my branch? |
Yup. |
…:href attributes. They should go through regular sanitization, RESOURCE_URL is overkill there. This does not change the context for the rest of xlink:href attributes. This is a breaking change in very unlikely cases. If someone whitelisted RESOURCE_URL for the purpose of binding into xlink:href, and that these can't pass the regular URL sanitization, they'll get broken: the fix is to whitelist in the $compileProvider's URL sanitization.
|
Really done so this time, then. Thanks for the help :) |
src/ng/compile.js
Outdated
| @@ -1671,7 +1671,8 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { | |||
| nodeName = nodeName_(this.$$element); | |||
|
|
|||
| if ((nodeName === 'a' && (key === 'href' || key === 'xlinkHref')) || | |||
| (nodeName === 'img' && key === 'src')) { | |||
| (nodeName === 'img' && key === 'src') || | |||
| (nodeName === 'image' && key === 'xlinkHref')) { | |||
| // sanitize a[href] and img[src] values | |||
| this[key] = value = $$sanitizeUri(value, key === 'src'); | |||
There was a problem hiding this comment.
Shouldn't key === src be updated to also account for <image> elements. Shouldn't image[xlink:href] be sanitized based on imgSrcSanitizationWhitelist rather than aHrefSanitizationWhitelist?
There was a problem hiding this comment.
Sorry for the delay :/
For the two whitelists: yes, good catch, fixed! I swapped testing for 'src' with testing for the right tagnames, and I added a small test for that.
A few remarks:
- The whitelists purpose aren't super clear: I interpret them as navigational / media URLs, but maybe someone has a different opinion?
- Come to think of it, we could also add video/audio/picture src to media sanitization? That would require adding more data: types to the default media whitelist and break a few users, but it seems like an oversight on my part. I don't believe that they might cause XSS in the current state, but it's not like
- I'm advocating for a single whitelist in feat($sce): handle URLs through the $sce service, plus allow concatenation in that $sce contexts. #14250 anyways :)
src/ng/compile.js
Outdated
| @@ -1674,7 +1674,8 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { | |||
| (nodeName === 'img' && key === 'src') || | |||
| (nodeName === 'image' && key === 'xlinkHref')) { | |||
| // sanitize a[href] and img[src] values | |||
| this[key] = value = $$sanitizeUri(value, key === 'src'); | |||
| this[key] = value = | |||
| $$sanitizeUri(value, nodeName === 'img' || nodeName === 'image'); | |||
There was a problem hiding this comment.
Nit: There is no need to wrap this line (as it does not exceed 100 chars).
… and `image` elements Previously, `xlink:href` on SVG's `<a>` and `<image>` elements, was `$sce.RESOURCE_URL`. While this makes sense for other `xlink:href` usecases, it was an overkill for these elements. This commit lowers the `xlink:href` security context for these specific elements, treating it in the same way as `a[href]` or `img[src]` respectively. The `xlink:href` security context for other elements is not affected. BREAKING CHANGE: In the unlikely case that an app relied on RESOURCE_URL whitelisting for the purpose of binding to the `xlink:href` property of SVG's `<a>` or `<image>` elements and if the values do not pass the regular URL sanitization, they will break. To fix this you need to ensure that the values used for binding to the affected `xlink:href` contexts are considered safe URLs, e.g. by whitelisting them in `$compileProvider`'s `aHrefSanitizationWhitelist` (for `<a>` elements) or `imgSrcSanitizationWhitelist` (for `<image>` elements). Closes angular#15736
They should go through regular sanitization, RESOURCE_URL is overkill there.
This does not change the context for the rest of xlink:href attributes.
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Minor breaking change.
What is the current behavior? (You can also link to an open issue here)
xlink:href are always RESOURCE_URL context, so they go through the $sce whitelist, but aren't sanitized. That context is intended for links to things that are potentially executed as scripts in your origin, which isn't the case here.
What is the new behavior (if this is a feature change)?
image and a xlink:href are now sanitized as link URLs (like a href), and won't complain on different-origin links anymore.
Does this PR introduce a breaking change?
This is breaking, but only in marginal (and probably unsafe) cases. If you whitelisted something for a xlink:href binding on one of those two, you'll need to change the aHrefSanitizationWhitelist instead. In the overwhelming majority of cases, everything that works with the default settings will keep working, and those defaults aren't usually modified.
Other information:
I'm almost sure there are issues around ng-href in svg lying around, and probably general svg bindings security issues too :/ ng-href security context currently doesn't match the xlink:href ones. It can't be fixed without breaking changes though, so that's for another PR. Some simple tests: https://plnkr.co/edit/PbR1FH2v4luScSYdU16t?p=preview