Skip to content

Commit 94beafe

Browse files
alan-agius4alan-agius4
committed
fix(core): prevent binding unsafe attributes on SVG animation elements
SVG animation elements (`animate` and `set`) can be used to animate sensitive attributes like `href` or `xlink:href`. Binding to these animation attributes (like `to`, `from`, or `values`) with a sensitive target creates an XSS vector. This change mitigates this risk by: 1. Classifying `to`, `from`, and `values` on `<animate>` and `<set>` elements as `ATTRIBUTE_NO_BINDING` in the DOM security schema to prevent standard dynamic bindings. 2. Adding runtime validations in `ɵɵvalidateAttribute` to verify that `attributeName` is not a sensitive attribute (such as `href` or `xlink:href`) when processed by a set of `SECURITY_SENSITIVE_ATTRIBUTE_NAMES`. If it is, a runtime error `UNSAFE_ATTRIBUTE_BINDING` is thrown. 3. Adding regression tests in `integration_spec.ts` to ensure unsafe bindings throw an error while safe ones pass correctly.
1 parent 3784b57 commit 94beafe

5 files changed

Lines changed: 101 additions & 42 deletions

File tree

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

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -105,13 +105,6 @@ export function SECURITY_SCHEMA(): {[k: string]: SecurityContext} {
105105
'none|href',
106106
'none|xlink:href',
107107

108-
// SVG animation value attributes — may animate URL-bearing attrs (e.g. attributeName="href")
109-
// https://www.w3.org/TR/SVG11/animate.html#ToAttribute
110-
'animate|to',
111-
'animate|from',
112-
'animate|values',
113-
'set|to',
114-
115108
// The below two items are safe and should be removed but they require a G3 clean-up as a small number of tests fail.
116109
'img|src',
117110
'video|src',
@@ -139,11 +132,18 @@ export function SECURITY_SCHEMA(): {[k: string]: SecurityContext} {
139132

140133
registerContext(SecurityContext.ATTRIBUTE_NO_BINDING, [
141134
'animate|attributeName',
135+
'animate|values',
136+
'animate|to',
137+
'animate|from',
138+
'set|to',
142139
'set|attributeName',
143140
'animateMotion|attributeName',
144141
'animateTransform|attributeName',
145142

146143
'unknown|attributeName',
144+
'unknown|values',
145+
'unknown|to',
146+
'unknown|from',
147147

148148
'iframe|sandbox',
149149
'iframe|allow',

‎packages/compiler/test/schema/dom_element_schema_registry_spec.ts‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -156,10 +156,16 @@ If 'onAnything' is a directive input, make sure the directive is imported by the
156156
expect(registry.securityContext('base', 'href', false)).toBe(SecurityContext.RESOURCE_URL);
157157

158158
// SVG animate and set attributes
159-
expect(registry.securityContext('animate', 'to', false)).toBe(SecurityContext.URL);
160-
expect(registry.securityContext('animate', 'from', false)).toBe(SecurityContext.URL);
161-
expect(registry.securityContext('animate', 'values', false)).toBe(SecurityContext.URL);
162-
expect(registry.securityContext('set', 'to', false)).toBe(SecurityContext.URL);
159+
expect(registry.securityContext('animate', 'to', false)).toBe(
160+
SecurityContext.ATTRIBUTE_NO_BINDING,
161+
);
162+
expect(registry.securityContext('animate', 'from', false)).toBe(
163+
SecurityContext.ATTRIBUTE_NO_BINDING,
164+
);
165+
expect(registry.securityContext('animate', 'values', false)).toBe(
166+
SecurityContext.ATTRIBUTE_NO_BINDING,
167+
);
168+
expect(registry.securityContext('set', 'to', false)).toBe(SecurityContext.ATTRIBUTE_NO_BINDING);
163169
});
164170

165171
it('should detect properties on namespaced elements', () => {

‎packages/core/src/sanitization/sanitization.ts‎

Lines changed: 38 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -282,11 +282,19 @@ function getSanitizer(): Sanitizer | null {
282282
return lView && lView[ENVIRONMENT].sanitizer;
283283
}
284284

285+
/**
286+
* Set of attributes that are sensitive and should be sanitized.
287+
*/
288+
const SECURITY_SENSITIVE_ATTRIBUTE_NAMES: ReadonlySet<string> = new Set(['href', 'xlink:href']);
289+
285290
/**
286291
* @remarks Keep this in sync with DOM Security Schema.
287292
* @see [SECURITY_SCHEMA](../../../compiler/src/schema/dom_security_schema.ts)
288293
*/
289-
const SECURITY_SENSITIVE_ELEMENTS: Record<string, Record<string, true | undefined> | undefined> = {
294+
const SECURITY_SENSITIVE_ELEMENTS: Record<
295+
string,
296+
Record<string, true | undefined | ReadonlySet<string>> | undefined
297+
> = {
290298
iframe: {
291299
sandbox: true,
292300
allow: true,
@@ -295,8 +303,13 @@ const SECURITY_SENSITIVE_ELEMENTS: Record<string, Record<string, true | undefine
295303
csp: true,
296304
fetchpriority: true,
297305
},
298-
animate: {attributename: true},
299-
set: {attributename: true},
306+
animate: {
307+
attributename: true,
308+
to: SECURITY_SENSITIVE_ATTRIBUTE_NAMES,
309+
values: SECURITY_SENSITIVE_ATTRIBUTE_NAMES,
310+
from: SECURITY_SENSITIVE_ATTRIBUTE_NAMES,
311+
},
312+
set: {attributename: true, to: SECURITY_SENSITIVE_ATTRIBUTE_NAMES},
300313
animatemotion: {attributename: true},
301314
animatetransform: {attributename: true},
302315
};
@@ -311,7 +324,8 @@ const SECURITY_SENSITIVE_ELEMENTS: Record<string, Record<string, true | undefine
311324
export function ɵɵvalidateAttribute<T = any>(value: T, tagName: string, attributeName: string): T {
312325
const lowerCaseTagName = tagName.toLowerCase();
313326
const lowerCaseAttrName = attributeName.toLowerCase();
314-
if (!SECURITY_SENSITIVE_ELEMENTS[lowerCaseTagName]?.[lowerCaseAttrName]) {
327+
const validationConfig = SECURITY_SENSITIVE_ELEMENTS[lowerCaseTagName]?.[lowerCaseAttrName];
328+
if (!validationConfig) {
315329
return value;
316330
}
317331

@@ -326,6 +340,26 @@ export function ɵɵvalidateAttribute<T = any>(value: T, tagName: string, attrib
326340
enforceIframeSecurity(element as HTMLIFrameElement);
327341
}
328342

343+
if (typeof validationConfig !== 'boolean') {
344+
const element = getNativeByTNode(tNode, lView) as SVGAnimateElement;
345+
const attributeNameValue = element.getAttribute('attributeName');
346+
347+
if (attributeNameValue && validationConfig.has(attributeNameValue.toLowerCase())) {
348+
const errorMessage =
349+
ngDevMode &&
350+
`Angular has detected that the \`${attributeName}\` was applied ` +
351+
`as a binding to the <${tagName}> element${getTemplateLocationDetails(lView)}. ` +
352+
`For security reasons, the \`${attributeName}\` can be set on the <${tagName}> element ` +
353+
`as a static attribute only when the "attributeName" is set to \'${attributeNameValue}\'. \n` +
354+
`To fix this, switch the \`${attributeNameValue}\` binding to a static attribute ` +
355+
`in a template or in host bindings section.`;
356+
357+
throw new RuntimeError(RuntimeErrorCode.UNSAFE_ATTRIBUTE_BINDING, errorMessage);
358+
}
359+
360+
return value;
361+
}
362+
329363
const errorMessage =
330364
ngDevMode &&
331365
`Angular has detected that the \`${attributeName}\` was applied ` +

‎packages/core/test/render3/integration_spec.ts‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -645,6 +645,52 @@ describe('sanitization', () => {
645645

646646
expect(anchor.getAttribute('href')).toEqual('http://foo');
647647
});
648+
649+
it('should throw when binding to animate element with attributeName="href"', () => {
650+
@Component({
651+
selector: 'test-comp',
652+
template: `<svg><animate attributeName="href" [to]="'foo'"></animate></svg>`,
653+
})
654+
class TestComp {}
655+
656+
TestBed.configureTestingModule({
657+
providers: [provideZoneChangeDetection()],
658+
});
659+
const fixture = TestBed.createComponent(TestComp);
660+
expect(() => fixture.detectChanges()).toThrowError(
661+
/Angular has detected that the `to` was applied/,
662+
);
663+
});
664+
665+
it('should throw when binding to set element with attributeName="href"', () => {
666+
@Component({
667+
selector: 'test-comp',
668+
template: `<svg><set attributeName="href" [to]="'foo'"></set></svg>`,
669+
})
670+
class TestComp {}
671+
672+
TestBed.configureTestingModule({
673+
providers: [provideZoneChangeDetection()],
674+
});
675+
const fixture = TestBed.createComponent(TestComp);
676+
expect(() => fixture.detectChanges()).toThrowError(
677+
/Angular has detected that the `to` was applied/,
678+
);
679+
});
680+
681+
it('should not throw when binding to animate element when attributeName is not href', () => {
682+
@Component({
683+
selector: 'test-comp',
684+
template: `<svg><animate attributeName="display" [to]="'foo'"></animate></svg>`,
685+
})
686+
class TestComp {}
687+
688+
TestBed.configureTestingModule({
689+
providers: [provideZoneChangeDetection()],
690+
});
691+
const fixture = TestBed.createComponent(TestComp);
692+
expect(() => fixture.detectChanges()).not.toThrow();
693+
});
648694
});
649695

650696
class LocalSanitizedValue {

‎packages/core/test/sanitization/sanitization_spec.ts‎

Lines changed: 0 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -137,10 +137,6 @@ describe('sanitization', () => {
137137
expect(() => ɵɵsanitizeUrlOrResourceUrl('javascript:true', 'iframe', 'src')).toThrowError(
138138
ERROR,
139139
);
140-
expect(() => ɵɵsanitizeUrlOrResourceUrl('http://server', 'object', 'data')).toThrowError(ERROR);
141-
expect(() => ɵɵsanitizeUrlOrResourceUrl('javascript:true', 'object', 'data')).toThrowError(
142-
ERROR,
143-
);
144140
expect(() =>
145141
ɵɵsanitizeUrlOrResourceUrl(bypassSanitizationTrustHtml('javascript:true'), 'iframe', 'src'),
146142
).toThrowError(/Required a safe ResourceURL, got a HTML/);
@@ -151,13 +147,6 @@ describe('sanitization', () => {
151147
'src',
152148
).toString(),
153149
).toEqual('javascript:true');
154-
expect(
155-
ɵɵsanitizeUrlOrResourceUrl(
156-
bypassSanitizationTrustResourceUrl('javascript:true'),
157-
'object',
158-
'data',
159-
).toString(),
160-
).toEqual('javascript:true');
161150
});
162151

163152
it('should sanitize urls via sanitizeUrlOrResourceUrl', () => {
@@ -177,22 +166,6 @@ describe('sanitization', () => {
177166
expect(
178167
ɵɵsanitizeUrlOrResourceUrl(bypassSanitizationTrustUrl('javascript:true'), 'a', 'href'),
179168
).toEqual('javascript:true');
180-
181-
// SVG animate and set attributes
182-
expect(ɵɵsanitizeUrlOrResourceUrl('javascript:alert(1)', 'animate', 'to')).toEqual(
183-
'unsafe:javascript:alert(1)',
184-
);
185-
expect(ɵɵsanitizeUrlOrResourceUrl('0.2', 'animate', 'to')).toEqual('0.2');
186-
expect(ɵɵsanitizeUrlOrResourceUrl('javascript:alert(1)', 'animate', 'from')).toEqual(
187-
'unsafe:javascript:alert(1)',
188-
);
189-
expect(ɵɵsanitizeUrlOrResourceUrl('javascript:alert(1)', 'animate', 'values')).toEqual(
190-
'unsafe:javascript:alert(1)',
191-
);
192-
expect(ɵɵsanitizeUrlOrResourceUrl('javascript:alert(1)', 'set', 'to')).toEqual(
193-
'unsafe:javascript:alert(1)',
194-
);
195-
expect(ɵɵsanitizeUrlOrResourceUrl('0.2', 'set', 'to')).toEqual('0.2');
196169
});
197170

198171
it('should only trust constant strings from template literal tags without interpolation', () => {

0 commit comments

Comments
 (0)