From 040b101842b2bdb637caff91a2c547a4317e5100 Mon Sep 17 00:00:00 2001 From: Martin Probst Date: Thu, 26 May 2016 14:35:27 -0700 Subject: [PATCH] feat(security): complete DOM security schema. This addresses several oversights in assigning security contexts to DOM schema elements found by our security reviewers (thanks!). This also adds some more precise unit tests for the interaction between (Dom)ElementSchemaRegistry and the TemplateParser, and extracts the security specific parts into dom_security_schema.ts. Comparison of (potentially) dangerous property names is done case insensitive, to avoid issues like formAction vs formaction. Part of issue #8511. --- .../src/schema/dom_element_schema_registry.ts | 70 ++++++------------- .../src/schema/dom_security_schema.ts | 66 +++++++++++++++++ .../dom_element_schema_registry_spec.ts | 6 ++ .../compiler/test/template_parser_spec.ts | 43 +++++++++++- 4 files changed, 136 insertions(+), 49 deletions(-) create mode 100644 modules/@angular/compiler/src/schema/dom_security_schema.ts diff --git a/modules/@angular/compiler/src/schema/dom_element_schema_registry.ts b/modules/@angular/compiler/src/schema/dom_element_schema_registry.ts index 9dec97c367..c62ef42d4c 100644 --- a/modules/@angular/compiler/src/schema/dom_element_schema_registry.ts +++ b/modules/@angular/compiler/src/schema/dom_element_schema_registry.ts @@ -3,6 +3,7 @@ import {SecurityContext} from '../../core_private'; import {isPresent} from '../facade/lang'; import {StringMapWrapper} from '../facade/collection'; import {ElementSchemaRegistry} from './element_schema_registry'; +import {SECURITY_SCHEMA} from './dom_security_schema'; const EVENT = 'event'; const BOOLEAN = 'boolean'; @@ -51,6 +52,20 @@ const OBJECT = 'object'; * NOTE: This schema is auto extracted from `schema_extractor.ts` located in the test folder, * see dom_element_schema_registry_spec.ts */ + +// ================================================================================================= +// ================================================================================================= +// =========== S T O P - S T O P - S T O P - S T O P - S T O P - S T O P =========== +// ================================================================================================= +// ================================================================================================= +// +// DO NOT EDIT THIS DOM SCHEMA WITHOUT A SECURITY REVIEW! +// +// Newly added properties must be security reviewed and assigned an appropriate SecurityContext in +// dom_security_schema.ts. Reach out to mprobst for details. +// +// ================================================================================================= + const SCHEMA: string[] = /*@ts2dart_const*/ ([ '*|%classList,className,id,innerHTML,*beforecopy,*beforecut,*beforepaste,*copy,*cut,*paste,*search,*selectstart,*webkitfullscreenchange,*webkitfullscreenerror,*wheel,outerHTML,#scrollLeft,#scrollTop', @@ -202,59 +217,12 @@ const SCHEMA: string[] = var attrToPropMap: {[name: string]: string} = { 'class': 'className', + 'formaction': 'formAction', 'innerHtml': 'innerHTML', 'readonly': 'readOnly', 'tabindex': 'tabIndex' }; -function registerContext(map: {[k: string]: SecurityContext}, ctx: SecurityContext, specs: string[]) { - for (let spec of specs) map[spec] = ctx; -} - -/** Map from tagName|propertyName SecurityContext. Properties applying to all tags use '*'. */ -const SECURITY_SCHEMA: {[k: string]: SecurityContext} = {}; - -registerContext(SECURITY_SCHEMA, SecurityContext.HTML, [ - 'iframe|srcdoc', - '*|innerHTML', - '*|outerHTML', -]); -registerContext(SECURITY_SCHEMA, SecurityContext.STYLE, ['*|style']); -// NB: no SCRIPT contexts here, they are never allowed. -registerContext(SECURITY_SCHEMA, SecurityContext.URL, [ - 'area|href', - 'area|ping', - 'audio|src', - 'a|href', - 'a|ping', - 'blockquote|cite', - 'body|background', - 'button|formaction', - 'del|cite', - 'form|action', - 'img|src', - 'input|formaction', - 'input|src', - 'ins|cite', - 'q|cite', - 'source|src', - 'video|poster', - 'video|src', -]); -registerContext(SECURITY_SCHEMA, SecurityContext.RESOURCE_URL, [ - 'applet|code', - 'applet|codebase', - 'base|href', - 'frame|src', - 'head|profile', - 'html|manifest', - 'iframe|src', - 'object|codebase', - 'object|data', - 'script|src', - 'track|src', -]); - @Injectable() export class DomElementSchemaRegistry extends ElementSchemaRegistry { schema = <{[element: string]: {[property: string]: string}}>{}; @@ -276,6 +244,8 @@ export class DomElementSchemaRegistry extends ElementSchemaRegistry { if (property == '') { } else if (property.startsWith('*')) { // We don't yet support events. + // If ever allowing to bind to events, GO THROUGH A SECURITY REVIEW, allowing events will + // almost certainly introduce bad XSS vulnerabilities. // type[property.substring(1)] = EVENT; } else if (property.startsWith('!')) { type[property.substring(1)] = BOOLEAN; @@ -315,6 +285,10 @@ export class DomElementSchemaRegistry extends ElementSchemaRegistry { * attack vectors are assigned their appropriate context. */ securityContext(tagName: string, propName: string): SecurityContext { + // Make sure comparisons are case insensitive, so that case differences between attribute and + // property names do not have a security impact. + tagName = tagName.toLowerCase(); + propName = propName.toLowerCase(); let ctx = SECURITY_SCHEMA[tagName + '|' + propName]; if (ctx !== undefined) return ctx; ctx = SECURITY_SCHEMA['*|' + propName]; diff --git a/modules/@angular/compiler/src/schema/dom_security_schema.ts b/modules/@angular/compiler/src/schema/dom_security_schema.ts new file mode 100644 index 0000000000..69199ce8ed --- /dev/null +++ b/modules/@angular/compiler/src/schema/dom_security_schema.ts @@ -0,0 +1,66 @@ +import {SecurityContext} from '../../core_private'; + +// ================================================================================================= +// ================================================================================================= +// =========== S T O P - S T O P - S T O P - S T O P - S T O P - S T O P =========== +// ================================================================================================= +// ================================================================================================= +// +// DO NOT EDIT THIS LIST OF SECURITY SENSITIVE PROPERTIES WITHOUT A SECURITY REVIEW! +// Reach out to mprobst for details. +// +// ================================================================================================= + +/** Map from tagName|propertyName SecurityContext. Properties applying to all tags use '*'. */ +export const SECURITY_SCHEMA: {[k: string]: SecurityContext} = {}; + +function registerContext(ctx: SecurityContext, specs: string[]) { + for (let spec of specs) SECURITY_SCHEMA[spec.toLowerCase()] = ctx; +} + +// Case is insignificant below, all element and attribute names are lower-cased for lookup. + +registerContext(SecurityContext.HTML, [ + 'iframe|srcdoc', + '*|innerHTML', + '*|outerHTML', +]); +registerContext(SecurityContext.STYLE, ['*|style']); +// NB: no SCRIPT contexts here, they are never allowed due to the parser stripping them. +registerContext(SecurityContext.URL, [ + '*|formAction', + 'area|href', + 'area|ping', + 'audio|src', + 'a|href', + 'a|ping', + 'blockquote|cite', + 'body|background', + 'del|cite', + 'form|action', + 'img|src', + 'img|srcset', + 'input|src', + 'ins|cite', + 'q|cite', + 'source|src', + 'source|srcset', + 'video|poster', + 'video|src', +]); +registerContext(SecurityContext.RESOURCE_URL, [ + 'applet|code', + 'applet|codebase', + 'base|href', + 'embed|src', + 'frame|src', + 'head|profile', + 'html|manifest', + 'iframe|src', + 'link|href', + 'media|src', + 'object|codebase', + 'object|data', + 'script|src', + 'track|src', +]); diff --git a/modules/@angular/compiler/test/schema/dom_element_schema_registry_spec.ts b/modules/@angular/compiler/test/schema/dom_element_schema_registry_spec.ts index e48e4e8c4a..f8be96ca55 100644 --- a/modules/@angular/compiler/test/schema/dom_element_schema_registry_spec.ts +++ b/modules/@angular/compiler/test/schema/dom_element_schema_registry_spec.ts @@ -77,6 +77,12 @@ export function main() { expect(registry.hasProperty(nodeName, 'type')).toBeTruthy(); }); + it('should check security contexts case insensitive', () => { + expect(registry.securityContext('p', 'iNnErHtMl')).toBe(SecurityContext.HTML); + expect(registry.securityContext('p', 'formaction')).toBe(SecurityContext.URL); + expect(registry.securityContext('p', 'formAction')).toBe(SecurityContext.URL); + }); + if (browserDetection.isChromeDesktop) { it('generate a new schema', () => { // console.log(JSON.stringify(registry.properties)); diff --git a/modules/@angular/compiler/test/template_parser_spec.ts b/modules/@angular/compiler/test/template_parser_spec.ts index 8f48a1a3ed..aea243fe50 100644 --- a/modules/@angular/compiler/test/template_parser_spec.ts +++ b/modules/@angular/compiler/test/template_parser_spec.ts @@ -11,6 +11,7 @@ import { beforeEachProviders } from '@angular/core/testing/testing_internal'; import {provide} from '@angular/core'; +import {SecurityContext} from '../core_private'; import {Console} from '@angular/core/src/console'; @@ -51,6 +52,7 @@ import { import {identifierToken, Identifiers} from '../src/identifiers'; import {ElementSchemaRegistry} from '@angular/compiler/src/schema/element_schema_registry'; +import {DomElementSchemaRegistry} from '@angular/compiler/src/schema/dom_element_schema_registry'; import {MockSchemaRegistry} from '@angular/compiler/testing'; import {Unparser} from './expression_parser/unparser'; @@ -66,9 +68,13 @@ var MOCK_SCHEMA_REGISTRY = [ {useValue: new MockSchemaRegistry({'invalidProp': false}, {'mappedAttr': 'mappedProp'})}) ]; +let zeConsole = console; + export function main() { var ngIf; - var parse; + var parse: + (template: string, directives: CompileDirectiveMetadata[], pipes?: CompilePipeMetadata[]) => + TemplateAst[]; var console: ArrayConsole; function commonBeforeEach() { @@ -120,6 +126,41 @@ export function main() { }); }); + describe('TemplateParser Security', () => { + // Semi-integration test to make sure TemplateParser properly sets the security context. + // Uses the actual DomElementSchemaRegistry. + beforeEachProviders( + () => + [TEST_PROVIDERS, provide(ElementSchemaRegistry, {useClass: DomElementSchemaRegistry})]); + + commonBeforeEach(); + + describe('security context', () => { + function secContext(tpl: string): SecurityContext { + let ast = parse(tpl, []); + let propBinding = (ast[0]).inputs[0]; + return propBinding.securityContext; + } + + it('should set for properties', () => { + expect(secContext('
')).toBe(SecurityContext.NONE); + expect(secContext('
')).toBe(SecurityContext.HTML); + }); + it('should set for property value bindings', () => { + expect(secContext('