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

fix XSS vulnerability in ng-srcset #8975

Closed
wants to merge 9 commits into from
Closed

fix XSS vulnerability in ng-srcset #8975

wants to merge 9 commits into from

Conversation

ltrillaud
Copy link
Contributor

There is a security issue with ng-srcset.

URIs aren't sanitized.

The behiavor is correct for ng-src but not for ng-srcset.

The probleme is with src/ng/compile.js that ignore srcset.

Here is the fix.

Laurent Trillaud

@ltrillaud
Copy link
Contributor Author

I'm afraid that this PR in not enough. srcset can be a set of URIs. Therefore Sanitization should loop over each URIs.
I gonna prepare another commit.

@ltrillaud
Copy link
Contributor Author

It's better now. For example, if you have a srcset with
http://example.com/image1.png x1, javascript:doEvilStuff() 2x
it will be sanitized to
http://example.com/image1.png x1, unsafe:javascript:doEvilStuff() 2x

@ltrillaud
Copy link
Contributor Author

The travis job fall in timeout.
How can I restart it ?
I haven't a refresh button on the travis job page https://travis-ci.org/angular/angular.js/builds/34794143

@ltrillaud ltrillaud changed the title fix(compile): use sanitizeUri also for srcset image attribut fix XSS vulnerability in ng-srcset Sep 10, 2014
jeffbcross
jeffbcross reviewed Sep 18, 2014
View reviewed changes
src/ng/compile.js
if ((nodeName === 'a' && key === 'href') ||
(nodeName === 'img' && key === 'src')) {
this[key] = value = $$sanitizeUri(value, key === 'src');
if ((nodeName === 'a' && key === 'href') ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use 2 spaces instead of tabs

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 that. It's my first pull request!

@jeffbcross jeffbcross added this to the 1.3.0-rc.3 milestone Sep 18, 2014
@jeffbcross jeffbcross self-assigned this Sep 18, 2014
@ltrillaud
Copy link
Contributor Author

Who can restart Travis job ?

@jeffbcross jeffbcross modified the milestones: 1.3.0-rc.3, 1.3.0-rc.4 Sep 22, 2014
@jeffbcross
Copy link
Contributor

Made a couple of changes to the test and pattern to make sure urls that end with values that look like width/pixel descriptors are not accidentally treated as such.
#9354

@jeffbcross
Copy link
Contributor

Squashed, rebased, and landed at ab80cd9

Thanks for submitting!

@jeffbcross jeffbcross closed this Sep 30, 2014
@ltrillaud
Copy link
Contributor Author

The pleasure was for me.
I am very proud to be become a contributor of this awesome AngularJS.

@sitthykun
Copy link

Thanks for fixing

maistro-nathaniel-wooding added a commit to maistro-plc/angular.js that referenced this pull request Jun 7, 2019
@maistro-nathaniel-wooding
apply fix from angular#8975
60bc490
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@jeffbcross jeffbcross

Labels
cla: yes severity: security
Projects
None yet
Milestone
1.3.0-rc.4
Development

Successfully merging this pull request may close these issues.
4 participants
@ltrillaud @jeffbcross @sitthykun @mary-poppins