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

Commit

Permalink
fix($sanitize): sanitize xml:base attributes
Browse files Browse the repository at this point in the history
On Firefox there is a XSS vulnerability if a malicious attacker
can write into the `xml:base` attribute on an SVG anchor.

Thanks to Masato Kinugawa at Cure23
  • Loading branch information
petebacondarwin authored and Narretz committed Jan 17, 2018
1 parent e06b4fb commit b9ef658
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 1 deletion.
2 changes: 1 addition & 1 deletion src/ngSanitize/sanitize.js
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,7 @@ function $SanitizeProvider() {
optionalEndTagElements);

//Attributes that have href and hence need to be sanitized
var uriAttrs = toMap('background,cite,href,longdesc,src,xlink:href');
var uriAttrs = toMap('background,cite,href,longdesc,src,xlink:href,xml:base');

var htmlAttrs = toMap('abbr,align,alt,axis,bgcolor,border,cellpadding,cellspacing,class,clear,' +
'color,cols,colspan,compact,coords,dir,face,headers,height,hreflang,hspace,' +
Expand Down
9 changes: 9 additions & 0 deletions test/ngSanitize/sanitizeSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,15 @@ describe('HTML', function() {
'<svg xmlns="http://www.w3.org/2000/svg"><a xmlns:xlink="http://www.w3.org/1999/xlink" xlink:href="https://example.com"></a></svg>');
});

it('should sanitize SVG xml:base attribute values', function() {
expectHTML('<svg xmlns="http://www.w3.org/2000/svg"><a xml:base="javascript:alert(1)//" href="#"></a></svg>')
.toEqual('<svg xmlns="http://www.w3.org/2000/svg"><a href="#"></a></svg>');

expectHTML('<svg xmlns="http://www.w3.org/2000/svg"><a xml:base="https://example.com" href="#"></a></svg>')
.toEqual('<svg xmlns="http://www.w3.org/2000/svg"><a xml:base="https://example.com" href="#"></a></svg>');

});

it('should sanitize unknown namespaced SVG attributes', function() {
expectHTML('<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"><a xlink:foo="javascript:alert()"></a></svg>')
.toBeOneOf('<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"><a></a></svg>',
Expand Down

13 comments on commit b9ef658

@cure53
Copy link

@cure53 cure53 commented on b9ef658 Sep 21, 2018

Choose a reason for hiding this comment

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

Cure23 again... they are the worst, right?

@petebacondarwin
Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry!

@JLLeitschuh
Copy link

Choose a reason for hiding this comment

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

This is reported as a security vulnerability by Snyk, shouldn't it have a CVE assigned to it?

https://snyk.io/vuln/npm:angular:20180202

@petebacondarwin
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure what you mean @JLLeitschuh. The Snyk refers to https://cwe.mitre.org/data/definitions/79.html

@JLLeitschuh
Copy link

Choose a reason for hiding this comment

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

@petebacondarwin It refers to a CWE which != CVE. CVE numbers are assignments for publicly disclosed vulnerabilities.

@JLLeitschuh
Copy link

Choose a reason for hiding this comment

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

@petebacondarwin
Copy link
Member Author

Choose a reason for hiding this comment

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

So I think the answer to your original question is yes? Are there other CVEs assigned for AngularJS vulnerabilities?

@JLLeitschuh
Copy link

Choose a reason for hiding this comment

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

There are a few, less than Snyk reports though. That doesn't mean there shouldn't be more though. Does the angular team collect vulnerability reports in a common location or are they scattered across all the release notes?

I'm just a vulnerability researcher poking at an angular based app and trying to find potential vulnerabilities in a target, but as a core contributor, you should probably be ensuring that things like this are properly reported/disclosed to the public via the CVE system.

@petebacondarwin
Copy link
Member Author

Choose a reason for hiding this comment

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

I'll find out what the policy is. Note that this project "AngularJS" is in LTS mode so is not actively worked on right now. The "Angular" project is the one being actively maintained. See https://github.com/angular/angular

@JLLeitschuh
Copy link

Choose a reason for hiding this comment

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

The "Angular" project is the one being actively maintained.

Got it. Still, this is only a year and a half old and there are still lots of companies running old versions of Angular.

@mprobst
Copy link
Contributor

Choose a reason for hiding this comment

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

/CC @rjamet might know, or might know someone who does :-)

@rjamet
Copy link
Contributor

@rjamet rjamet commented on b9ef658 Sep 11, 2019

Choose a reason for hiding this comment

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

My team generally don't file for CVEs, but we're not against them being filed either, they're a really useful tool. I'm not familiar with the process to do so, but at a glance, it's easy enough to do: https://cveform.mitre.org/

@JLLeitschuh
Copy link

Choose a reason for hiding this comment

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

@rjamet Please consider making it an official part of your disclosure process.

You can also use Snyk as a CNA. They may dig into the open source ecosystem a bit more to provide more detailed remediation information.

https://snyk.io/vulnerability-disclosure/

Please sign in to comment.