From 6a9d7e59690498fc2d8a6bc7816495fc30346761 Mon Sep 17 00:00:00 2001 From: Bjarki Date: Mon, 30 Nov 2020 16:49:39 +0000 Subject: [PATCH] refactor(core): express trusted constants with tagged template literals (#40082) The trustConstantHtml and trustConstantResourceUrl functions are only meant to be passed constant strings extracted from Angular application templates, as passing other strings or variables could introduce XSS vulnerabilities. To better protect these APIs, turn them into template tags. This makes it possible to assert that the associated template literals do not contain any interpolation, and thus must be constant. Also add tests for the change to prevent regression. PR Close #40082 --- .../elements/GOLDEN_PARTIAL.js | 61 +++++++++++++++++++ .../elements/TEST_CASES.json | 11 ++++ .../elements/deduplicate_attributes.ts | 2 +- .../security_sensitive_constant_attributes.js | 16 +++++ .../security_sensitive_constant_attributes.ts | 23 +++++++ .../compiler/src/render3/view/template.ts | 10 ++- .../core/src/sanitization/sanitization.ts | 38 +++++++++--- .../test/sanitization/sanitization_spec.ts | 11 +++- 8 files changed, 160 insertions(+), 12 deletions(-) create mode 100644 packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/elements/security_sensitive_constant_attributes.js create mode 100644 packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/elements/security_sensitive_constant_attributes.ts diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/elements/GOLDEN_PARTIAL.js b/packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/elements/GOLDEN_PARTIAL.js index 118af5f522b89..4b24ff82d1999 100644 --- a/packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/elements/GOLDEN_PARTIAL.js +++ b/packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/elements/GOLDEN_PARTIAL.js @@ -555,3 +555,64 @@ export declare class MyModule { static ɵinj: i0.ɵɵInjectorDef; } +/**************************************************************************************************** + * PARTIAL FILE: security_sensitive_constant_attributes.js + ****************************************************************************************************/ +import { Component, NgModule } from '@angular/core'; +import * as i0 from "@angular/core"; +export class MyComponent { +} +MyComponent.ɵfac = function MyComponent_Factory(t) { return new (t || MyComponent)(); }; +MyComponent.ɵcmp = i0.ɵɵngDeclareComponent({ version: "0.0.0-PLACEHOLDER", type: MyComponent, selector: "my-component", ngImport: i0, template: { source: ` + + + + + + + + + + + `, isInline: true } }); +(function () { (typeof ngDevMode === "undefined" || ngDevMode) && i0.ɵsetClassMetadata(MyComponent, [{ + type: Component, + args: [{ + selector: 'my-component', + template: ` + + + + + + + + + + + ` + }] + }], null, null); })(); +export class MyModule { +} +MyModule.ɵmod = i0.ɵɵdefineNgModule({ type: MyModule }); +MyModule.ɵinj = i0.ɵɵdefineInjector({ factory: function MyModule_Factory(t) { return new (t || MyModule)(); } }); +(function () { (typeof ngJitMode === "undefined" || ngJitMode) && i0.ɵɵsetNgModuleScope(MyModule, { declarations: [MyComponent] }); })(); +(function () { (typeof ngDevMode === "undefined" || ngDevMode) && i0.ɵsetClassMetadata(MyModule, [{ + type: NgModule, + args: [{ declarations: [MyComponent] }] + }], null, null); })(); + +/**************************************************************************************************** + * PARTIAL FILE: security_sensitive_constant_attributes.d.ts + ****************************************************************************************************/ +import * as i0 from "@angular/core"; +export declare class MyComponent { + static ɵfac: i0.ɵɵFactoryDef; + static ɵcmp: i0.ɵɵComponentDefWithMeta; +} +export declare class MyModule { + static ɵmod: i0.ɵɵNgModuleDefWithMeta; + static ɵinj: i0.ɵɵInjectorDef; +} + diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/elements/TEST_CASES.json b/packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/elements/TEST_CASES.json index b5932cf87dccd..e8fdcfccbdf55 100644 --- a/packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/elements/TEST_CASES.json +++ b/packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/elements/TEST_CASES.json @@ -226,6 +226,17 @@ "failureMessage": "Incorrect generated template." } ] + }, + { + "description": "should specify security-sensitive constant attributes as template literals", + "inputFiles": [ + "security_sensitive_constant_attributes.ts" + ], + "expectations": [ + { + "failureMessage": "Incorrect generated template." + } + ] } ] } diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/elements/deduplicate_attributes.ts b/packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/elements/deduplicate_attributes.ts index 66d891dd9afcc..c4df9fcd411ef 100644 --- a/packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/elements/deduplicate_attributes.ts +++ b/packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/elements/deduplicate_attributes.ts @@ -12,4 +12,4 @@ export class MyComponent { @NgModule({declarations: [MyComponent]}) export class MyModule { -} \ No newline at end of file +} diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/elements/security_sensitive_constant_attributes.js b/packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/elements/security_sensitive_constant_attributes.js new file mode 100644 index 0000000000000..eb77b0fdecef8 --- /dev/null +++ b/packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/elements/security_sensitive_constant_attributes.js @@ -0,0 +1,16 @@ +consts: [ + ["src", i0.ɵɵtrustConstantResourceUrl `https://angular.io/`], + ["srcdoc", i0.ɵɵtrustConstantHtml `

Angular

`], + ["data", i0.ɵɵtrustConstantResourceUrl `https://angular.io/`, "codebase", i0.ɵɵtrustConstantResourceUrl `/`], + ["src", "https://angular.io/"] +], +template: function MyComponent_Template(rf, ctx) { + if (rf & 1) { + $r3$.ɵɵelement(0, "embed", 0); + $r3$.ɵɵelement(1, "iframe", 1); + $r3$.ɵɵelement(2, "object", 2); + $r3$.ɵɵelement(3, "embed", 0); + $r3$.ɵɵelement(4, "img", 3); + } + … +} diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/elements/security_sensitive_constant_attributes.ts b/packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/elements/security_sensitive_constant_attributes.ts new file mode 100644 index 0000000000000..45d01e41493d2 --- /dev/null +++ b/packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/elements/security_sensitive_constant_attributes.ts @@ -0,0 +1,23 @@ +import {Component, NgModule} from '@angular/core'; + +@Component({ + selector: 'my-component', + template: ` + + + + + + + + + + + ` +}) +export class MyComponent { +} + +@NgModule({declarations: [MyComponent]}) +export class MyModule { +} diff --git a/packages/compiler/src/render3/view/template.ts b/packages/compiler/src/render3/view/template.ts index 429b1b7938b47..e5fafe611f4fd 100644 --- a/packages/compiler/src/render3/view/template.ts +++ b/packages/compiler/src/render3/view/template.ts @@ -2193,10 +2193,16 @@ function trustedConstAttribute(tagName: string, attr: t.TextAttribute): o.Expres if (isTrustedTypesSink(tagName, attr.name)) { switch (elementRegistry.securityContext(tagName, attr.name, /* isAttribute */ true)) { case core.SecurityContext.HTML: - return o.importExpr(R3.trustConstantHtml).callFn([value], attr.valueSpan); + return o.taggedTemplate( + o.importExpr(R3.trustConstantHtml), + new o.TemplateLiteral([new o.TemplateLiteralElement(attr.value)], []), undefined, + attr.valueSpan); // NB: no SecurityContext.SCRIPT here, as the corresponding tags are stripped by the compiler. case core.SecurityContext.RESOURCE_URL: - return o.importExpr(R3.trustConstantResourceUrl).callFn([value], attr.valueSpan); + return o.taggedTemplate( + o.importExpr(R3.trustConstantResourceUrl), + new o.TemplateLiteral([new o.TemplateLiteralElement(attr.value)], []), undefined, + attr.valueSpan); default: return value; } diff --git a/packages/core/src/sanitization/sanitization.ts b/packages/core/src/sanitization/sanitization.ts index f8050d39cf620..c30b079cd1d81 100644 --- a/packages/core/src/sanitization/sanitization.ts +++ b/packages/core/src/sanitization/sanitization.ts @@ -145,8 +145,10 @@ export function ɵɵsanitizeScript(unsafeScript: any): TrustedScript|string { } /** - * Promotes the given constant string to a TrustedHTML. - * @param html constant string containing trusted HTML. + * A template tag function for promoting the associated constant literal to a + * TrustedHTML. Interpolation is explicitly not allowed. + * + * @param html constant template literal containing trusted HTML. * @returns TrustedHTML wrapping `html`. * * @security This is a security-sensitive function and should only be used to @@ -155,13 +157,24 @@ export function ɵɵsanitizeScript(unsafeScript: any): TrustedScript|string { * * @codeGenApi */ -export function ɵɵtrustConstantHtml(html: string): TrustedHTML|string { - return trustedHTMLFromString(html); +export function ɵɵtrustConstantHtml(html: TemplateStringsArray): TrustedHTML|string { + // The following runtime check ensures that the function was called as a + // template tag (e.g. ɵɵtrustConstantHtml`content`), without any interpolation + // (e.g. not ɵɵtrustConstantHtml`content ${variable}`). A TemplateStringsArray + // is an array with a `raw` property that is also an array. The associated + // template literal has no interpolation if and only if the length of the + // TemplateStringsArray is 1. + if (ngDevMode && (!Array.isArray(html) || !Array.isArray(html.raw) || html.length !== 1)) { + throw new Error(`Unexpected interpolation in trusted HTML constant: ${html.join('?')}`); + } + return trustedHTMLFromString(html[0]); } /** - * Promotes the given constant string to a TrustedScriptURL. - * @param url constant string containing a trusted script URL. + * A template tag function for promoting the associated constant literal to a + * TrustedScriptURL. Interpolation is explicitly not allowed. + * + * @param url constant template literal containing a trusted script URL. * @returns TrustedScriptURL wrapping `url`. * * @security This is a security-sensitive function and should only be used to @@ -170,8 +183,17 @@ export function ɵɵtrustConstantHtml(html: string): TrustedHTML|string { * * @codeGenApi */ -export function ɵɵtrustConstantResourceUrl(url: string): TrustedScriptURL|string { - return trustedScriptURLFromString(url); +export function ɵɵtrustConstantResourceUrl(url: TemplateStringsArray): TrustedScriptURL|string { + // The following runtime check ensures that the function was called as a + // template tag (e.g. ɵɵtrustConstantResourceUrl`content`), without any + // interpolation (e.g. not ɵɵtrustConstantResourceUrl`content ${variable}`). A + // TemplateStringsArray is an array with a `raw` property that is also an + // array. The associated template literal has no interpolation if and only if + // the length of the TemplateStringsArray is 1. + if (ngDevMode && (!Array.isArray(url) || !Array.isArray(url.raw) || url.length !== 1)) { + throw new Error(`Unexpected interpolation in trusted URL constant: ${url.join('?')}`); + } + return trustedScriptURLFromString(url[0]); } /** diff --git a/packages/core/test/sanitization/sanitization_spec.ts b/packages/core/test/sanitization/sanitization_spec.ts index 20ef6c96f5e81..2ce9a6447c210 100644 --- a/packages/core/test/sanitization/sanitization_spec.ts +++ b/packages/core/test/sanitization/sanitization_spec.ts @@ -12,7 +12,7 @@ import {LView} from '@angular/core/src/render3/interfaces/view'; import {enterView, leaveView} from '@angular/core/src/render3/state'; import {bypassSanitizationTrustHtml, bypassSanitizationTrustResourceUrl, bypassSanitizationTrustScript, bypassSanitizationTrustStyle, bypassSanitizationTrustUrl} from '../../src/sanitization/bypass'; -import {getUrlSanitizer, ɵɵsanitizeHtml, ɵɵsanitizeResourceUrl, ɵɵsanitizeScript, ɵɵsanitizeStyle, ɵɵsanitizeUrl, ɵɵsanitizeUrlOrResourceUrl} from '../../src/sanitization/sanitization'; +import {getUrlSanitizer, ɵɵsanitizeHtml, ɵɵsanitizeResourceUrl, ɵɵsanitizeScript, ɵɵsanitizeStyle, ɵɵsanitizeUrl, ɵɵsanitizeUrlOrResourceUrl, ɵɵtrustConstantHtml, ɵɵtrustConstantResourceUrl} from '../../src/sanitization/sanitization'; import {SecurityContext} from '../../src/sanitization/security'; function fakeLView(): LView { @@ -134,4 +134,13 @@ describe('sanitization', () => { expect(ɵɵsanitizeUrlOrResourceUrl(bypassSanitizationTrustUrl('javascript:true'), 'a', 'href')) .toEqual('javascript:true'); }); + + it('should only trust constant strings from template literal tags without interpolation', () => { + expect(ɵɵtrustConstantHtml`

good

`.toString()).toEqual('

good

'); + expect(ɵɵtrustConstantResourceUrl`http://good.com`.toString()).toEqual('http://good.com'); + expect(() => (ɵɵtrustConstantHtml as any) `

${'evil'}

`) + .toThrowError(/Unexpected interpolation in trusted HTML constant/); + expect(() => (ɵɵtrustConstantResourceUrl as any) `http://${'evil'}.com`) + .toThrowError(/Unexpected interpolation in trusted URL constant/); + }); });