Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Next Next commit
feat($compile): Lower the security context of SVG's a and image xlink…
…: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.
  • Loading branch information
rjamet committed Feb 24, 2017
commit 0b439090464507e0ea168075e4a5b196b8332a95
9 changes: 6 additions & 3 deletions src/ng/compile.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Copy link
Member

@gkalpak gkalpak Feb 28, 2017

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

Ping @rjamet

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

} else if (nodeName === 'img' && key === 'srcset' && isDefined(value)) {
Expand Down Expand Up @@ -3254,8 +3255,10 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
if (['img', 'video', 'audio', 'source', 'track'].indexOf(tag) === -1) {
return $sce.RESOURCE_URL;
}
// maction[xlink:href] can source SVG. It's not limited to <maction>.
} else if (attrNormalizedName === 'xlinkHref' ||
} else if (
// Some xlink:href are okay, most aren't
(attrNormalizedName === 'xlinkHref' && (tag !== 'image' && tag !== 'a')) ||
// Formaction
(tag === 'form' && attrNormalizedName === 'action') ||
// If relative URLs can go where they are not expected to, then
// all sorts of trust issues can arise.
Expand Down
39 changes: 35 additions & 4 deletions test/ng/compileSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -11122,23 +11122,42 @@ describe('$compile', function() {
});

it('should use $$sanitizeUri when working with svg and xlink:href', function() {
var $$sanitizeUri = jasmine.createSpy('$$sanitizeUri');
module(function($provide) {
$provide.value('$$sanitizeUri', $$sanitizeUri);
});
inject(function($compile, $rootScope) {
element = $compile('<svg><a xlink:href="{{ testUrl }}"></a></svg>')($rootScope);

//both of these fail the RESOURCE_URL test, that shouldn't be run
$rootScope.testUrl = 'https://bad.example.org';
$$sanitizeUri.and.returnValue('https://clean.example.org');

$rootScope.$apply();
expect(element.find('a').attr('xlink:href')).toBe('https://clean.example.org');
expect($$sanitizeUri).toHaveBeenCalledWith($rootScope.testUrl, false);
});
});

it('should use $$sanitizeUri when working with svg and xlink:href through ng-href', function() {
var $$sanitizeUri = jasmine.createSpy('$$sanitizeUri');
module(function($provide) {
$provide.value('$$sanitizeUri', $$sanitizeUri);
});
inject(function($compile, $rootScope) {
element = $compile('<svg><a xlink:href="" ng-href="{{ testUrl }}"></a></svg>')($rootScope);
$rootScope.testUrl = 'evilUrl';
//both of these fail the RESOURCE_URL test, that shouldn't be run
$rootScope.testUrl = 'https://bad.example.org';
$$sanitizeUri.and.returnValue('https://clean.example.org');

$$sanitizeUri.and.returnValue('someSanitizedUrl');
$rootScope.$apply();
expect(element.find('a').prop('href').baseVal).toBe('someSanitizedUrl');
expect(element.find('a').prop('href').baseVal).toBe('https://clean.example.org');
expect($$sanitizeUri).toHaveBeenCalledWith($rootScope.testUrl, false);
});
});


it('should use $$sanitizeUri when working with svg and xlink:href', function() {
it('should use $$sanitizeUri when working with svg and xlink:href through ng-href', function() {
var $$sanitizeUri = jasmine.createSpy('$$sanitizeUri');
module(function($provide) {
$provide.value('$$sanitizeUri', $$sanitizeUri);
Expand All @@ -11153,6 +11172,18 @@ describe('$compile', function() {
expect($$sanitizeUri).toHaveBeenCalledWith($rootScope.testUrl, false);
});
});


it('should have a RESOURCE_URL context for xlink:href by default', function() {
inject(function($compile, $rootScope) {
element = $compile('<svg><whatever xlink:href="{{ testUrl }}"></whatever></svg>')($rootScope);
$rootScope.testUrl = 'https://bad.example.org';

expect(function() {
$rootScope.$apply();
}).toThrowError(/\$sce:insecurl/);
});
});
});

describe('interpolation on HTML DOM event handler attributes onclick, onXYZ, formaction', function() {
Expand Down