From a0957e9ba560303f632c93527f7e2984ab5da2bd Mon Sep 17 00:00:00 2001 From: rodyhaddad Date: Tue, 10 Mar 2015 13:26:27 -0700 Subject: [PATCH 1/2] chore(security): add warning banner to top of security sensitive files --- src/jqLite.js | 11 +++++++++++ src/ng/compile.js | 11 +++++++++++ src/ng/parse.js | 11 +++++++++++ src/ng/sce.js | 11 +++++++++++ src/ngSanitize/sanitize.js | 11 +++++++++++ 5 files changed, 55 insertions(+) diff --git a/src/jqLite.js b/src/jqLite.js index df1ab7312ea4..879af165e0a5 100644 --- a/src/jqLite.js +++ b/src/jqLite.js @@ -1,5 +1,16 @@ 'use strict'; +/* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * + * Any commits to this file should be reviewed with security in mind. * + * Changes to this file can potentially create security vulnerabilities. * + * An approval from 2 Core members with history of modifying * + * this file is required. * + * * + * Does the change somehow allow for arbitrary javascript to be executed? * + * Or allows for someone to change the prototype of built-in objects? * + * Or gives undesired access to variables likes document or window? * + * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * */ + /* global JQLitePrototype: true, addEventListenerFn: true, removeEventListenerFn: true, diff --git a/src/ng/compile.js b/src/ng/compile.js index 3de0d53f0388..1afd08606e8b 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -1,5 +1,16 @@ 'use strict'; +/* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * + * Any commits to this file should be reviewed with security in mind. * + * Changes to this file can potentially create security vulnerabilities. * + * An approval from 2 Core members with history of modifying * + * this file is required. * + * * + * Does the change somehow allow for arbitrary javascript to be executed? * + * Or allows for someone to change the prototype of built-in objects? * + * Or gives undesired access to variables likes document or window? * + * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * */ + /* ! VARIABLE/FUNCTION NAMING CONVENTIONS THAT APPLY TO THIS FILE! * * DOM-related variables: diff --git a/src/ng/parse.js b/src/ng/parse.js index 54cad090a5b8..f3386c74e80b 100644 --- a/src/ng/parse.js +++ b/src/ng/parse.js @@ -1,5 +1,16 @@ 'use strict'; +/* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * + * Any commits to this file should be reviewed with security in mind. * + * Changes to this file can potentially create security vulnerabilities. * + * An approval from 2 Core members with history of modifying * + * this file is required. * + * * + * Does the change somehow allow for arbitrary javascript to be executed? * + * Or allows for someone to change the prototype of built-in objects? * + * Or gives undesired access to variables likes document or window? * + * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * */ + var $parseMinErr = minErr('$parse'); // Sandboxing Angular Expressions diff --git a/src/ng/sce.js b/src/ng/sce.js index bedb12f7101d..65cdcb4c5138 100644 --- a/src/ng/sce.js +++ b/src/ng/sce.js @@ -1,5 +1,16 @@ 'use strict'; +/* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * + * Any commits to this file should be reviewed with security in mind. * + * Changes to this file can potentially create security vulnerabilities. * + * An approval from 2 Core members with history of modifying * + * this file is required. * + * * + * Does the change somehow allow for arbitrary javascript to be executed? * + * Or allows for someone to change the prototype of built-in objects? * + * Or gives undesired access to variables likes document or window? * + * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * */ + var $sceMinErr = minErr('$sce'); var SCE_CONTEXTS = { diff --git a/src/ngSanitize/sanitize.js b/src/ngSanitize/sanitize.js index 81302e3ffca4..91d3b2f0ecfe 100644 --- a/src/ngSanitize/sanitize.js +++ b/src/ngSanitize/sanitize.js @@ -1,5 +1,16 @@ 'use strict'; +/* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * + * Any commits to this file should be reviewed with security in mind. * + * Changes to this file can potentially create security vulnerabilities. * + * An approval from 2 Core members with history of modifying * + * this file is required. * + * * + * Does the change somehow allow for arbitrary javascript to be executed? * + * Or allows for someone to change the prototype of built-in objects? * + * Or gives undesired access to variables likes document or window? * + * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * */ + var $sanitizeMinErr = angular.$$minErr('$sanitize'); /** From 2e391535c652931112c96cb048177902f7ec532e Mon Sep 17 00:00:00 2001 From: rodyhaddad Date: Tue, 10 Mar 2015 14:27:10 -0700 Subject: [PATCH 2/2] 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: ``` ``` 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. --- src/ngSanitize/sanitize.js | 38 ++++++++++++++++----------------- test/ngSanitize/sanitizeSpec.js | 9 ++++++++ 2 files changed, 28 insertions(+), 19 deletions(-) diff --git a/src/ngSanitize/sanitize.js b/src/ngSanitize/sanitize.js index 91d3b2f0ecfe..9240309c5b48 100644 --- a/src/ngSanitize/sanitize.js +++ b/src/ngSanitize/sanitize.js @@ -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," + + "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"); @@ -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, diff --git a/test/ngSanitize/sanitizeSpec.js b/test/ngSanitize/sanitizeSpec.js index 121d3284736f..33d036c97efc 100644 --- a/test/ngSanitize/sanitizeSpec.js +++ b/test/ngSanitize/sanitizeSpec.js @@ -273,6 +273,15 @@ describe('HTML', function() { .toEqual(''); }); + it('should not accept SVG animation tags', function() { + expectHTML('Click me') + .toEqual('Click me'); + + expectHTML('' + + '') + .toEqual(''); + }); + describe('htmlSanitizerWriter', function() { /* global htmlSanitizeWriter: false */ if (angular.isUndefined(window.htmlSanitizeWriter)) return;