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

feat($compile): Lower the security context of SVG's a and image xlink:href #15736

Closed
wants to merge 2 commits into from
Closed

feat($compile): Lower the security context of SVG's a and image xlink:href #15736

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0b43909
feat($compile): Lower the security context of SVG's a and image xlink…
rjamet Feb 21, 2017
0a22243
fix($compile): image xlink:href should be sanitized as images, not as…
rjamet Mar 3, 2017
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
9 changes: 6 additions & 3 deletions src/ng/compile.js
View file
Open in desktop
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 •
edited
Loading

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
View file
Open in desktop
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