8000 fix($sanitize): don't allow svg animation tags by rodyhaddad · Pull Request #11290 · angular/angular.js · GitHub
[go: up one dir, main page]
Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

fix($sanitize): don't allow svg animation tags #11290

Closed
wants to merge 2 commits into from
Closed

fix($sanitize): don't allow svg animation tags #11290

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
a0957e9
chore(security): add warning banner to top of security sensitive files
rodyhaddad Mar 10, 2015
2e39153
fix($sanitize): don't allow svg animation tags
rodyhaddad Mar 10, 2015
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
Prev Previous commit
fix($sanitize): don't allow svg animation tags 
After #11124 got merged, a security vulnerability got introduced.
Animation in SVG became tolerated by the sanitizer.

Exploit Example:
```
<svg>
  <a xmlns:xlink="http://www.w3.org/1999/xlink" xlink:href="?">
    <circle r="400"></circle>
    <animate attributeName="xlink:href" begin="0" from="javascript:alert(1)" to="&" />
  </a>
</svg>
```

Here we are animating an anchor's href, starting from a value that's a javascript URI,
allowing the executing of arbitrary javascript in the process.

Preventing only the animation of links is tricky, as SVG is weird and namespaces aren't predictable.
We've decided to have the sanitizer filter out svg animation tags instead.

Considering the sanitizer is commonly used to sanitize untrusted HTML code, this shouldn't affect
many apps in the wild. Also, no release has been with #11124 in it, but not this fix.
  • Loading branch information
@rodyhaddad
rodyhaddad committed Mar 11, 2015
commit 2e391535c652931112c96cb048177902f7ec532e
38 changes: 19 additions & 19 deletions src/ngSanitize/sanitize.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,10 +206,11 @@ var inlineElements = angular.extend({}, optionalEndTagInlineElements, makeMap("a

// SVG Elements
// https://wiki.whatwg.org/wiki/Sanitization_rules#svg_Elements
var svgElements = makeMap("animate,animateColor,animateMotion,animateTransform,circle,defs," +
"desc,ellipse,font-face,font-face-name,font-face-src,g,glyph,hkern,image,linearGradient," +
"line,marker,metadata,missing-glyph,mpath,path,polygon,polyline,radialGradient,rect,set," +
"stop,svg,switch,text,title,tspan,use");
// Note: the elements animate,animateColor,animateMotion,animateTransform,set are intentionally omitted.
// They can potentially allow for arbitrary javascript to be executed. See #11290
var svgElements = makeMap("circle,defs,desc,ellipse,font-face,font-face-name,font-face-src,g,glyph," +
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a comment that the animation related elements are intentionally omitted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

"hkern,image,linearGradient,line,marker,metadata,missing-glyph,mpath,path,polygon,polyline," +
"radialGradient,rect,stop,svg,switch,text,title,tspan,use");

// Special Elements (can contain anything)
var specialElements = makeMap("script,style");
Expand All @@ -233,21 +234,20 @@ var htmlAttrs = makeMap('abbr,align,alt,axis,bgcolor,border,cellpadding,cellspac
// SVG attributes (without "id" and "name" attributes)
// https://wiki.whatwg.org/wiki/Sanitization_rules#svg_Attributes
var svgAttrs = makeMap('accent-height,accumulate,additive,alphabetic,arabic-form,ascent,' +
'attributeName,attributeType,baseProfile,bbox,begin,by,calcMode,cap-height,class,color,' +
'color-rendering,content,cx,cy,d,dx,dy,descent,display,dur,end,fill,fill-rule,font-family,' +
'font-size,font-stretch,font-style,font-variant,font-weight,from,fx,fy,g1,g2,glyph-name,' +
'gradientUnits,hanging,height,horiz-adv-x,horiz-origin-x,ideographic,k,keyPoints,' +
'keySplines,keyTimes,lang,marker-end,marker-mid,marker-start,markerHeight,markerUnits,' +
'markerWidth,mathematical,max,min,offset,opacity,orient,origin,overline-position,' +
'overline-thickness,panose-1,path,pathLength,points,preserveAspectRatio,r,refX,refY,' +
'repeatCount,repeatDur,requiredExtensions,requiredFeatures,restart,rotate,rx,ry,slope,stemh,' +
'stemv,stop-color,stop-opacity,strikethrough-position,strikethrough-thickness,stroke,' +
'stroke-dasharray,stroke-dashoffset,stroke-linecap,stroke-linejoin,stroke-miterlimit,' +
'stroke-opacity,stroke-width,systemLanguage,target,text-anchor,to,transform,type,u1,u2,' +
'underline-position,underline-thickness,unicode,unicode-range,units-per-em,values,version,' +
'viewBox,visibility,width,widths,x,x-height,x1,x2,xlink:actuate,xlink:arcrole,xlink:role,' +
'xlink:show,xlink:title,xlink:type,xml:base,xml:lang,xml:space,xmlns,xmlns:xlink,y,y1,y2,' +
'zoomAndPan', true);
'baseProfile,bbox,begin,by,calcMode,cap-height,class,color,color-rendering,content,' +
'cx,cy,d,dx,dy,descent,display,dur,end,fill,fill-rule,font-family,font-size,font-stretch,' +
'font-style,font-variant,font-weight,from,fx,fy,g1,g2,glyph-name,gradientUnits,hanging,' +
'height,horiz-adv-x,horiz-origin-x,ideographic,k,keyPoints,keySplines,keyTimes,lang,' +
'marker-end,marker-mid,marker-start,markerHeight,markerUnits,markerWidth,mathematical,' +
'max,min,offset,opacity,orient,origin,overline-position,overline-thickness,panose-1,' +
'path,pathLength,points,preserveAspectRatio,r,refX,refY,repeatCount,repeatDur,' +
'requiredExtensions,requiredFeatures,restart,rotate,rx,ry,slope,stemh,stemv,stop-color,' +
'stop-opacity,strikethrough-position,strikethrough-thickness,stroke,stroke-dasharray,' +
'stroke-dashoffset,stroke-linecap,stroke-linejoin,stroke-miterlimit,stroke-opacity,' +
'stroke-width,systemLanguage,target,text-anchor,to,transform,type,u1,u2,underline-position,' +
'underline-thickness,unicode,unicode-range,units-per-em,values,version,viewBox,visibility,' +
'width,widths,x,x-height,x1,x2,xlink:actuate,xlink:arcrole,xlink:role,xlink:show,xlink:title,' +
'xlink:type,xml:base,xml:lang,xml:space,xmlns,xmlns:xlink,y,y1,y2,zoomAndPan', true);

var validAttrs = angular.extend({},
uriAttrs,
Expand Down
9 changes: 9 additions & 0 deletions test/ngSanitize/sanitizeSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,15 @@ describe('HTML', function() {
.toEqual('<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"><a></a></svg>');
});

it('should not accept SVG animation tags', function() {
expectHTML('<svg xmlns:xlink="http://www.w3.org/1999/xlink"><a><text y="1em">Click me</text><animate attributeName="xlink:href" values="javascript:alert(1)"/></a></svg>')
.toEqual('<svg xmlns:xlink="http://www.w3.org/1999/xlink"><a><text y="1em">Click me</text></a></svg>');

expectHTML('<svg><a xmlns:xlink="http://www.w3.org/1999/xlink" xlink:href="?"><circle r="400"></circle>' +
'<animate attributeName="xlink:href" begin="0" from="javascript:alert(1)" to="&" /></a></svg>')
.toEqual('<svg><a xmlns:xlink="http://www.w3.org/1999/xlink" xlink:href="?"><circle r="400"></circle></a></svg>');
});

describe('htmlSanitizerWriter', function() {
/* global htmlSanitizeWriter: false */
if (angular.isUndefined(window.htmlSanitizeWriter)) return;
Expand Down
0