Skip to content

Commit 78fd159

Browse files
alan-agius4crisbeto
alan-agius4
authored and
crisbeto
committed
fix(compiler): prevent XSS via SVG animation attributeName and MathML/SVG URLs
This commit implements a security fix to prevent XSS vulnerabilities where SVG animation elements (`<animate>`, `<set>`, etc.) could be used to modify the `href` or `xlink:href` attributes of other elements to `javascript:` URLs. (cherry picked from commit 1c6b070)
1 parent fec1f1a commit 78fd159

18 files changed

Lines changed: 323 additions & 150 deletions

File tree

‎goldens/public-api/core/errors.api.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -186,6 +186,8 @@ export const enum RuntimeErrorCode {
186186
// (undocumented)
187187
UNKNOWN_ELEMENT = 304,
188188
// (undocumented)
189+
UNSAFE_ATTRIBUTE_BINDING = -910,
190+
// @deprecated (undocumented)
189191
UNSAFE_IFRAME_ATTRS = -910,
190192
// (undocumented)
191193
UNSAFE_VALUE_IN_RESOURCE_URL = 904,

‎packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/elements/iframe_attrs.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,6 @@ template: function MyComponent_Template(rf, ctx) {
33
if (rf & 1) {
44
i0.ɵɵelement(0, "iframe", 0);
55
} if (rf & 2) {
6-
i0.ɵɵattribute("fetchpriority", "low", i0.ɵɵvalidateIframeAttribute)("allowfullscreen", ctx.fullscreen, i0.ɵɵvalidateIframeAttribute);
6+
i0.ɵɵattribute("fetchpriority", "low", i0.ɵɵvalidateAttribute)("allowfullscreen", ctx.fullscreen, i0.ɵɵvalidateAttribute);
77
}
88
}

‎packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_bindings/host_bindings/GOLDEN_PARTIAL.js‎

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -923,10 +923,11 @@ i0.ɵɵngDeclareClassMetadata({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDE
923923
export class HostBindingIframeDir {
924924
constructor() {
925925
this.evil = 'evil';
926+
this.nonEvil = 'nonEvil';
926927
}
927928
}
928929
HostBindingIframeDir.ɵfac = i0.ɵɵngDeclareFactory({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: HostBindingIframeDir, deps: [], target: i0.ɵɵFactoryTarget.Directive });
929-
HostBindingIframeDir.ɵdir = i0.ɵɵngDeclareDirective({ minVersion: "14.0.0", version: "0.0.0-PLACEHOLDER", type: HostBindingIframeDir, isStandalone: true, selector: "iframe[hostBindingIframeDir]", host: { properties: { "innerHtml": "evil", "attr.style": "evil", "src": "evil", "sandbox": "evil" } }, ngImport: i0 });
930+
HostBindingIframeDir.ɵdir = i0.ɵɵngDeclareDirective({ minVersion: "14.0.0", version: "0.0.0-PLACEHOLDER", type: HostBindingIframeDir, isStandalone: true, selector: "iframe[hostBindingIframeDir]", host: { properties: { "innerHtml": "evil", "attr.style": "evil", "src": "evil", "sandbox": "evil", "attr.attributeName": "nonEvil" } }, ngImport: i0 });
930931
i0.ɵɵngDeclareClassMetadata({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: HostBindingIframeDir, decorators: [{
931932
type: Directive,
932933
args: [{
@@ -936,6 +937,23 @@ i0.ɵɵngDeclareClassMetadata({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDE
936937
'[attr.style]': 'evil',
937938
'[src]': 'evil',
938939
'[sandbox]': 'evil',
940+
'[attr.attributeName]': 'nonEvil',
941+
},
942+
}]
943+
}] });
944+
export class HostBindingSvgAnimateDir {
945+
constructor() {
946+
this.evil = 'evil';
947+
}
948+
}
949+
HostBindingSvgAnimateDir.ɵfac = i0.ɵɵngDeclareFactory({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: HostBindingSvgAnimateDir, deps: [], target: i0.ɵɵFactoryTarget.Directive });
950+
HostBindingSvgAnimateDir.ɵdir = i0.ɵɵngDeclareDirective({ minVersion: "14.0.0", version: "0.0.0-PLACEHOLDER", type: HostBindingSvgAnimateDir, isStandalone: true, selector: "animateMotion[hostBindingSvgAnimateDir]", host: { properties: { "attr.attributeName": "evil" } }, ngImport: i0 });
951+
i0.ɵɵngDeclareClassMetadata({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: HostBindingSvgAnimateDir, decorators: [{
952+
type: Directive,
953+
args: [{
954+
selector: 'animateMotion[hostBindingSvgAnimateDir]',
955+
host: {
956+
'[attr.attributeName]': 'evil',
939957
},
940958
}]
941959
}] });
@@ -956,9 +974,15 @@ export declare class HostBindingImageDir {
956974
}
957975
export declare class HostBindingIframeDir {
958976
evil: string;
977+
nonEvil: string;
959978
static ɵfac: i0.ɵɵFactoryDeclaration<HostBindingIframeDir, never>;
960979
static ɵdir: i0.ɵɵDirectiveDeclaration<HostBindingIframeDir, "iframe[hostBindingIframeDir]", never, {}, {}, never, never, true, never>;
961980
}
981+
export declare class HostBindingSvgAnimateDir {
982+
evil: string;
983+
static ɵfac: i0.ɵɵFactoryDeclaration<HostBindingSvgAnimateDir, never>;
984+
static ɵdir: i0.ɵɵDirectiveDeclaration<HostBindingSvgAnimateDir, "animateMotion[hostBindingSvgAnimateDir]", never, {}, {}, never, never, true, never>;
985+
}
962986

963987
/****************************************************************************************************
964988
* PARTIAL FILE: security_sensitive_constant_attributes.js

‎packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_bindings/host_bindings/sanitization.js‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,13 @@ hostBindings: function HostBindingImageDir_HostBindings(rf, ctx) {
1414
…
1515
hostBindings: function HostBindingIframeDir_HostBindings(rf, ctx) {
1616
if (rf & 2) {
17-
$r3$.ɵɵdomProperty("innerHTML", ctx.evil, $r3$.ɵɵsanitizeHtml)("src", ctx.evil, $r3$.ɵɵsanitizeResourceUrl)("sandbox", ctx.evil, $r3$.ɵɵvalidateIframeAttribute);
18-
$r3$.ɵɵattribute("style", ctx.evil, $r3$.ɵɵsanitizeStyle);
17+
$r3$.ɵɵdomProperty("innerHTML", ctx.evil, $r3$.ɵɵsanitizeHtml)("src", ctx.evil, $r3$.ɵɵsanitizeResourceUrl)("sandbox", ctx.evil, $r3$.ɵɵvalidateAttribute);
18+
$r3$.ɵɵattribute("style", ctx.evil, $r3$.ɵɵsanitizeStyle)("attributeName", ctx.nonEvil);
1919
}
2020
}
21+
…
22+
hostBindings: function HostBindingSvgAnimateDir_HostBindings(rf, ctx) {
23+
if (rf & 2) {
24+
i0.ɵɵattribute("attributeName", ctx.evil, i0.ɵɵvalidateAttribute);
25+
}
26+
}

‎packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_bindings/host_bindings/sanitization.ts‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,8 +31,20 @@ export class HostBindingImageDir {
3131
'[attr.style]': 'evil',
3232
'[src]': 'evil',
3333
'[sandbox]': 'evil',
34+
'[attr.attributeName]': 'nonEvil',
3435
},
3536
})
3637
export class HostBindingIframeDir {
3738
evil = 'evil';
39+
nonEvil = 'nonEvil';
40+
}
41+
42+
@Directive({
43+
selector: 'animateMotion[hostBindingSvgAnimateDir]',
44+
host: {
45+
'[attr.attributeName]': 'evil',
46+
},
47+
})
48+
export class HostBindingSvgAnimateDir {
49+
evil = 'evil';
3850
}

‎packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_bindings/property_bindings/sanitization.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ template: function MyComponent_Template(rf, ctx) {
1111
$r3$.ɵɵadvance();
1212
$r3$.ɵɵdomProperty("src", ctx.evil, $r3$.ɵɵsanitizeUrl);
1313
$r3$.ɵɵadvance();
14-
$r3$.ɵɵdomProperty("sandbox", ctx.evil, $r3$.ɵɵvalidateIframeAttribute);
14+
$r3$.ɵɵdomProperty("sandbox", ctx.evil, $r3$.ɵɵvalidateAttribute);
1515
$r3$.ɵɵadvance();
1616
$r3$.ɵɵdomProperty("href", $r3$.ɵɵinterpolate2("", ctx.evil, "", ctx.evil), $r3$.ɵɵsanitizeUrl);
1717
$r3$.ɵɵadvance();

‎packages/compiler-cli/test/ngtsc/ngtsc_spec.ts‎

Lines changed: 33 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -9258,6 +9258,33 @@ runInEachFileSystem((os: string) => {
92589258
});
92599259
});
92609260

9261+
describe('SVG animation processing', () => {
9262+
it('should generate SVG animation validation instruction', () => {
9263+
env.write(
9264+
'test.ts',
9265+
`
9266+
import {Component} from '@angular/core';
9267+
9268+
@Component({
9269+
selector: 'test-cmp',
9270+
template: '<svg><animate [attr.attributeName]="attr"></animate></svg>',
9271+
standalone: false,
9272+
})
9273+
export class TestCmp {
9274+
attr = 'opacity';
9275+
}
9276+
`,
9277+
);
9278+
9279+
env.driveMain();
9280+
9281+
const jsContents = env.getContents('test.js');
9282+
expect(jsContents).toContain(
9283+
'i0.ɵɵattribute("attributeName", ctx.attr, i0.ɵɵvalidateAttribute);',
9284+
);
9285+
});
9286+
});
9287+
92619288
describe('inline resources', () => {
92629289
it('should process inline <style> tags', () => {
92639290
env.write(
@@ -9679,11 +9706,11 @@ runInEachFileSystem((os: string) => {
96799706
// Only `sandbox` has an extra validation fn (since it's security-sensitive),
96809707
// the `title` property doesn't have an extra validation fn.
96819708
expect(jsContents).toContain(
9682-
'ɵɵdomProperty("sandbox", "", i0.ɵɵvalidateIframeAttribute)("title", "Hi!")',
9709+
'ɵɵdomProperty("sandbox", "", i0.ɵɵvalidateAttribute)("title", "Hi!")',
96839710
);
96849711

96859712
// The `allow` property is also security-sensitive, thus an extra validation fn.
9686-
expect(jsContents).toContain('ɵɵattribute("allow", "", i0.ɵɵvalidateIframeAttribute)');
9713+
expect(jsContents).toContain('ɵɵattribute("allow", "", i0.ɵɵvalidateAttribute)');
96879714
});
96889715

96899716
it(
@@ -9713,7 +9740,7 @@ runInEachFileSystem((os: string) => {
97139740
// Make sure that the `sandbox` has an extra validation fn,
97149741
// and the check is case-insensitive (since the `setAttribute` DOM API
97159742
// is case-insensitive as well).
9716-
expect(jsContents).toContain('ɵɵattribute("SANDBOX", "", i0.ɵɵvalidateIframeAttribute)');
9743+
expect(jsContents).toContain('ɵɵattribute("SANDBOX", "", i0.ɵɵvalidateAttribute)');
97179744
},
97189745
);
97199746

@@ -9771,11 +9798,11 @@ runInEachFileSystem((os: string) => {
97719798
// The `sandbox` is potentially a security-sensitive attribute of an <iframe>.
97729799
// Generate an extra validation function to invoke at runtime, which would
97739800
// check if an underlying host element is an <iframe>.
9774-
expect(jsContents).toContain('ɵɵdomProperty("sandbox", "", i0.ɵɵvalidateIframeAttribute)');
9801+
expect(jsContents).toContain('ɵɵdomProperty("sandbox", "", i0.ɵɵvalidateAttribute)');
97759802

97769803
// Similar to the above, but for an attribute binding (host attributes are
97779804
// represented via `ɵɵattribute`).
9778-
expect(jsContents).toContain('ɵɵattribute("allow", "", i0.ɵɵvalidateIframeAttribute)');
9805+
expect(jsContents).toContain('ɵɵattribute("allow", "", i0.ɵɵvalidateAttribute)');
97799806
});
97809807

97819808
it(
@@ -9801,7 +9828,7 @@ runInEachFileSystem((os: string) => {
98019828

98029829
// Make sure that we generate a validation fn for the `sandbox` attribute,
98039830
// even when it was declared as `SANDBOX`.
9804-
expect(jsContents).toContain('ɵɵattribute("SANDBOX", "", i0.ɵɵvalidateIframeAttribute)');
9831+
expect(jsContents).toContain('ɵɵattribute("SANDBOX", "", i0.ɵɵvalidateAttribute)');
98059832
},
98069833
);
98079834
});

‎packages/compiler/src/core.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,7 @@ export enum SecurityContext {
8484
SCRIPT = 3,
8585
URL = 4,
8686
RESOURCE_URL = 5,
87+
ATTRIBUTE_NO_BINDING = 6,
8788
}
8889

8990
/**

‎packages/compiler/src/render3/r3_identifiers.ts‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -455,6 +455,10 @@ export class Identifiers {
455455
// sanitization-related functions
456456
static sanitizeHtml: o.ExternalReference = {name: 'ɵɵsanitizeHtml', moduleName: CORE};
457457
static sanitizeStyle: o.ExternalReference = {name: 'ɵɵsanitizeStyle', moduleName: CORE};
458+
static validateAttribute: o.ExternalReference = {
459+
name: 'ɵɵvalidateAttribute',
460+
moduleName: CORE,
461+
};
458462
static sanitizeResourceUrl: o.ExternalReference = {
459463
name: 'ɵɵsanitizeResourceUrl',
460464
moduleName: CORE,
@@ -470,10 +474,6 @@ export class Identifiers {
470474
name: 'ɵɵtrustConstantResourceUrl',
471475
moduleName: CORE,
472476
};
473-
static validateIframeAttribute: o.ExternalReference = {
474-
name: 'ɵɵvalidateIframeAttribute',
475-
moduleName: CORE,
476-
};
477477

478478
// Decorators
479479
static inputDecorator: o.ExternalReference = {name: 'Input', moduleName: CORE};

‎packages/compiler/src/schema/dom_security_schema.ts‎

Lines changed: 100 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@ import {SecurityContext} from '../core';
1515
// =================================================================================================
1616
//
1717
// DO NOT EDIT THIS LIST OF SECURITY SENSITIVE PROPERTIES WITHOUT A SECURITY REVIEW!
18-
// Reach out to mprobst for details.
1918
//
2019
// =================================================================================================
2120

@@ -36,6 +35,7 @@ export function SECURITY_SCHEMA(): {[k: string]: SecurityContext} {
3635
'area|ping',
3736
'audio|src',
3837
'a|href',
38+
'a|xlink:href',
3939
'a|ping',
4040
'blockquote|cite',
4141
'body|background',
@@ -49,7 +49,77 @@ export function SECURITY_SCHEMA(): {[k: string]: SecurityContext} {
4949
'track|src',
5050
'video|poster',
5151
'video|src',
52+
53+
// MathML namespace
54+
// https://crsrc.org/c/third_party/blink/renderer/core/sanitizer/sanitizer.cc;l=753-768;drc=b3eb16372dcd3317d65e9e0265015e322494edcd;bpv=1;bpt=1
55+
'annotation|href',
56+
'annotation|xlink:href',
57+
'annotation-xml|href',
58+
'annotation-xml|xlink:href',
59+
'maction|href',
60+
'maction|xlink:href',
61+
'malignmark|href',
62+
'malignmark|xlink:href',
63+
'math|href',
64+
'math|xlink:href',
65+
'mroot|href',
66+
'mroot|xlink:href',
67+
'msqrt|href',
68+
'msqrt|xlink:href',
69+
'merror|href',
70+
'merror|xlink:href',
71+
'mfrac|href',
72+
'mfrac|xlink:href',
73+
'mglyph|href',
74+
'mglyph|xlink:href',
75+
'msub|href',
76+
'msub|xlink:href',
77+
'msup|href',
78+
'msup|xlink:href',
79+
'msubsup|href',
80+
'msubsup|xlink:href',
81+
'mmultiscripts|href',
82+
'mmultiscripts|xlink:href',
83+
'mprescripts|href',
84+
'mprescripts|xlink:href',
85+
'mi|href',
86+
'mi|xlink:href',
87+
'mn|href',
88+
'mn|xlink:href',
89+
'mo|href',
90+
'mo|xlink:href',
91+
'mpadded|href',
92+
'mpadded|xlink:href',
93+
'mphantom|href',
94+
'mphantom|xlink:href',
95+
'mrow|href',
96+
'mrow|xlink:href',
97+
'ms|href',
98+
'ms|xlink:href',
99+
'mspace|href',
100+
'mspace|xlink:href',
101+
'mstyle|href',
102+
'mstyle|xlink:href',
103+
'mtable|href',
104+
'mtable|xlink:href',
105+
'mtd|href',
106+
'mtd|xlink:href',
107+
'mtr|href',
108+
'mtr|xlink:href',
109+
'mtext|href',
110+
'mtext|xlink:href',
111+
'mover|href',
112+
'mover|xlink:href',
113+
'munder|href',
114+
'munder|xlink:href',
115+
'munderover|href',
116+
'munderover|xlink:href',
117+
'semantics|href',
118+
'semantics|xlink:href',
119+
'none|href',
120+
'none|xlink:href',
52121
]);
122+
53123
registerContext(SecurityContext.RESOURCE_URL, [
54124
'applet|code',
55125
'applet|codebase',
@@ -65,38 +135,39 @@ export function SECURITY_SCHEMA(): {[k: string]: SecurityContext} {
65135
'object|data',
66136
'script|src',
67137
]);
138+
139+
// Keep this in sync with SECURITY_SENSITIVE_ELEMENTS in packages/core/src/sanitization/sanitization.ts
140+
// Unknown is the internal tag name for unknown elements example used for host-bindings.
141+
// These are unsafe as `attributeName` can be `href` or `xlink:href`
142+
// See: http://b/463880509#comment7
143+
144+
registerContext(SecurityContext.ATTRIBUTE_NO_BINDING, [
145+
'animate|attributeName',
146+
'set|attributeName',
147+
'animateMotion|attributeName',
148+
'animateTransform|attributeName',
149+
150+
'unknown|attributeName',
151+
152+
'iframe|sandbox',
153+
'iframe|allow',
154+
'iframe|allowFullscreen',
155+
'iframe|referrerPolicy',
156+
'iframe|csp',
157+
'iframe|fetchPriority',
158+
159+
'unknown|sandbox',
160+
'unknown|allow',
161+
'unknown|allowFullscreen',
162+
'unknown|referrerPolicy',
163+
'unknown|csp',
164+
'unknown|fetchPriority',
165+
]);
68166
}
167+
69168
return _SECURITY_SCHEMA;
70169
}
71170

72171
function registerContext(ctx: SecurityContext, specs: string[]) {
73172
for (const spec of specs) _SECURITY_SCHEMA[spec.toLowerCase()] = ctx;
74173
}
75-
76-
/**
77-
* The set of security-sensitive attributes of an `<iframe>` that *must* be
78-
* applied as a static attribute only. This ensures that all security-sensitive
79-
* attributes are taken into account while creating an instance of an `<iframe>`
80-
* at runtime.
81-
*
82-
* Note: avoid using this set directly, use the `isIframeSecuritySensitiveAttr` function
83-
* in the code instead.
84-
*/
85-
export const IFRAME_SECURITY_SENSITIVE_ATTRS = new Set([
86-
'sandbox',
87-
'allow',
88-
'allowfullscreen',
89-
'referrerpolicy',
90-
'csp',
91-
'fetchpriority',
92-
]);
93-
94-
/**
95-
* Checks whether a given attribute name might represent a security-sensitive
96-
* attribute of an <iframe>.
97-
*/
98-
export function isIframeSecuritySensitiveAttr(attrName: string): boolean {
99-
// The `setAttribute` DOM API is case-insensitive, so we lowercase the value
100-
// before checking it against a known security-sensitive attributes.
101-
return IFRAME_SECURITY_SENSITIVE_ATTRS.has(attrName.toLowerCase());
102-
}

0 commit comments

Comments
 (0)