From 908a102a874ed38db7026a4fe69575bf14fbd845 Mon Sep 17 00:00:00 2001 From: Martin Probst Date: Fri, 29 Apr 2016 16:04:08 -0700 Subject: [PATCH] feat: security implementation in Angular 2. Summary: This adds basic security hooks to Angular 2. * `SecurityContext` is a private API between core, compiler, and platform-browser. `SecurityContext` communicates what context a value is used in across template parser, compiler, and sanitization at runtime. * `SanitizationService` is the bare bones interface to sanitize values for a particular context. * `SchemaElementRegistry.securityContext(tagName, attributeOrPropertyName)` determines the security context for an attribute or property (it turns out attributes and properties match for the purposes of sanitization). Based on these hooks: * `DomSchemaElementRegistry` decides what sanitization applies in a particular context. * `DomSanitizationService` implements `SanitizationService` and adds *Safe Value*s, i.e. the ability to mark a value as safe and not requiring further sanitization. * `url_sanitizer` and `style_sanitizer` sanitize URLs and Styles, respectively (surprise!). `DomSanitizationService` is the default implementation bound for browser applications, in the three contexts (browser rendering, web worker rendering, server side rendering). BREAKING CHANGES: *** SECURITY WARNING *** Angular 2 Release Candidates do not implement proper contextual escaping yet. Make sure to correctly escape all values that go into the DOM. *** SECURITY WARNING *** Reviewers: IgorMinar Differential Revision: https://reviews.angular.io/D103 --- modules/@angular/compiler/core_private.ts | 4 + modules/@angular/compiler/src/identifiers.ts | 6 + .../src/schema/dom_element_schema_registry.ts | 22 ++- .../src/schema/element_schema_registry.ts | 7 +- modules/@angular/compiler/src/template_ast.ts | 7 +- .../@angular/compiler/src/template_parser.ts | 24 ++- .../src/view_compiler/property_binder.ts | 35 +++- .../src/view_compiler/view_builder.ts | 8 +- .../dom_element_schema_registry_spec.ts | 34 ++-- .../compiler/testing/schema_registry_mock.ts | 5 + modules/@angular/core/private_export.ts | 7 + .../@angular/core/src/linker/view_utils.ts | 8 +- modules/@angular/core/src/security.ts | 23 +++ .../test/linker/security_integration_spec.ts | 157 ++++++++++++++++ .../integration_test/public_api_spec.ts | 3 + .../@angular/platform-browser/core_private.ts | 4 + .../platform-browser/src/browser_common.ts | 12 +- .../platform-browser/src/platform_browser.ts | 2 + .../src/security/dom_sanitization_service.ts | 171 ++++++++++++++++++ .../src/security/style_sanitizer.ts | 43 +++++ .../src/security/url_sanitizer.ts | 32 ++++ .../src/webworker/worker_render_common.ts | 4 + .../platform-server/testing/server.ts | 4 +- tools/public_api_guard/public_api_spec.ts | 2 + 24 files changed, 590 insertions(+), 34 deletions(-) create mode 100644 modules/@angular/core/src/security.ts create mode 100644 modules/@angular/core/test/linker/security_integration_spec.ts create mode 100644 modules/@angular/platform-browser/src/security/dom_sanitization_service.ts create mode 100644 modules/@angular/platform-browser/src/security/style_sanitizer.ts create mode 100644 modules/@angular/platform-browser/src/security/url_sanitizer.ts diff --git a/modules/@angular/compiler/core_private.ts b/modules/@angular/compiler/core_private.ts index cdb8a1e889..7d1a09834a 100644 --- a/modules/@angular/compiler/core_private.ts +++ b/modules/@angular/compiler/core_private.ts @@ -37,6 +37,10 @@ export var ValueUnwrapper: typeof t.ValueUnwrapper = r.ValueUnwrapper; export var TemplateRef_: typeof t.TemplateRef_ = r.TemplateRef_; export type RenderDebugInfo = t.RenderDebugInfo; export var RenderDebugInfo: typeof t.RenderDebugInfo = r.RenderDebugInfo; +export var SecurityContext: typeof t.SecurityContext = r.SecurityContext; +export type SecurityContext = t.SecurityContext; +export var SanitizationService: typeof t.SanitizationService = r.SanitizationService; +export type SanitizationService = t.SanitizationService; export var createProvider: typeof t.createProvider = r.createProvider; export var isProviderLiteral: typeof t.isProviderLiteral = r.isProviderLiteral; export var EMPTY_ARRAY: typeof t.EMPTY_ARRAY = r.EMPTY_ARRAY; diff --git a/modules/@angular/compiler/src/identifiers.ts b/modules/@angular/compiler/src/identifiers.ts index 9254c0dd29..fd9c71e8eb 100644 --- a/modules/@angular/compiler/src/identifiers.ts +++ b/modules/@angular/compiler/src/identifiers.ts @@ -11,6 +11,7 @@ import { ViewEncapsulation, TemplateRef } from '@angular/core'; +import {SecurityContext} from '../core_private'; import { AppElement, AppView, @@ -199,6 +200,11 @@ export class Identifiers { new CompileIdentifierMetadata( {name: 'pureProxy10', moduleUrl: VIEW_UTILS_MODULE_URL, runtime: pureProxy10}), ]; + static SecurityContext = new CompileIdentifierMetadata({ + name: 'SecurityContext', + moduleUrl: assetUrl('core', 'security'), + runtime: SecurityContext, + }); } export function identifierToken(identifier: CompileIdentifierMetadata): CompileTokenMetadata { 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 dcf9d0ff8a..395c4f7350 100644 --- a/modules/@angular/compiler/src/schema/dom_element_schema_registry.ts +++ b/modules/@angular/compiler/src/schema/dom_element_schema_registry.ts @@ -1,4 +1,5 @@ import {Injectable} from '@angular/core'; +import {SecurityContext} from '../../core_private'; import {isPresent} from '../facade/lang'; import {StringMapWrapper} from '../facade/collection'; import {ElementSchemaRegistry} from './element_schema_registry'; @@ -207,10 +208,11 @@ var attrToPropMap: {[name: string]: string} = { @Injectable() -export class DomElementSchemaRegistry implements ElementSchemaRegistry { +export class DomElementSchemaRegistry extends ElementSchemaRegistry { schema = <{[element: string]: {[property: string]: string}}>{}; constructor() { + super(); SCHEMA.forEach(encodedType => { var parts = encodedType.split('|'); var properties = parts[1].split(','); @@ -254,6 +256,24 @@ export class DomElementSchemaRegistry implements ElementSchemaRegistry { } } + /** + * securityContext returns the security context for the given property on the given DOM tag. + * + * Tag and property name are statically known and cannot change at runtime, i.e. it is not + * possible to bind a value into a changing attribute or tag name. + * + * The filtering is white list based. All attributes in the schema above are assumed to have the + * 'NONE' security context, i.e. that they are safe inert string values. Only specific well known + * attack vectors are assigned their appropriate context. + */ + securityContext(tagName: string, propName: string): SecurityContext { + // TODO(martinprobst): Fill in missing properties. + if (propName === 'style') return SecurityContext.STYLE; + if (tagName === 'a' && propName === 'href') return SecurityContext.URL; + if (propName === 'innerHTML') return SecurityContext.HTML; + return SecurityContext.NONE; + } + getMappedPropName(propName: string): string { var mappedPropName = StringMapWrapper.get(attrToPropMap, propName); return isPresent(mappedPropName) ? mappedPropName : propName; diff --git a/modules/@angular/compiler/src/schema/element_schema_registry.ts b/modules/@angular/compiler/src/schema/element_schema_registry.ts index bfbd5db161..8b3e0c70b1 100644 --- a/modules/@angular/compiler/src/schema/element_schema_registry.ts +++ b/modules/@angular/compiler/src/schema/element_schema_registry.ts @@ -1,4 +1,5 @@ -export class ElementSchemaRegistry { - hasProperty(tagName: string, propName: string): boolean { return true; } - getMappedPropName(propName: string): string { return propName; } +export abstract class ElementSchemaRegistry { + abstract hasProperty(tagName: string, propName: string): boolean; + abstract securityContext(tagName: string, propName: string): any; + abstract getMappedPropName(propName: string): string; } diff --git a/modules/@angular/compiler/src/template_ast.ts b/modules/@angular/compiler/src/template_ast.ts index 303391b6ee..0ea10bd321 100644 --- a/modules/@angular/compiler/src/template_ast.ts +++ b/modules/@angular/compiler/src/template_ast.ts @@ -6,6 +6,7 @@ import { CompileProviderMetadata, } from './compile_metadata'; import {ParseSourceSpan} from './parse_util'; +import {SecurityContext} from '../core_private'; /** * An Abstract Syntax Tree node representing part of a parsed Angular template. @@ -54,8 +55,10 @@ export class AttrAst implements TemplateAst { * A binding for an element property (e.g. `[property]="expression"`). */ export class BoundElementPropertyAst implements TemplateAst { - constructor(public name: string, public type: PropertyBindingType, public value: AST, - public unit: string, public sourceSpan: ParseSourceSpan) {} + constructor( + public name: string, public type: PropertyBindingType, + public securityContext: SecurityContext, public value: AST, public unit: string, + public sourceSpan: ParseSourceSpan) {} visit(visitor: TemplateAstVisitor, context: any): any { return visitor.visitElementProperty(this, context); } diff --git a/modules/@angular/compiler/src/template_parser.ts b/modules/@angular/compiler/src/template_parser.ts index 71ba50a9a7..ca2817e8d5 100644 --- a/modules/@angular/compiler/src/template_parser.ts +++ b/modules/@angular/compiler/src/template_parser.ts @@ -1,5 +1,5 @@ import {Injectable, Inject, OpaqueToken, Optional} from '@angular/core'; -import {MAX_INTERPOLATION_VALUES, Console} from '../core_private'; +import {MAX_INTERPOLATION_VALUES, Console, SecurityContext} from '../core_private'; import { ListWrapper, @@ -632,7 +632,7 @@ class TemplateParseVisitor implements HtmlAstVisitor { } targetReferences.push(new ReferenceAst(elOrDirRef.name, refToken, elOrDirRef.sourceSpan)); } - }); + }); // fix syntax highlighting issue: ` return directiveAsts; } @@ -705,10 +705,12 @@ class TemplateParseVisitor implements HtmlAstVisitor { sourceSpan: ParseSourceSpan): BoundElementPropertyAst { var unit = null; var bindingType; - var boundPropertyName; + var boundPropertyName: string; var parts = name.split(PROPERTY_PARTS_SEPARATOR); + let securityContext: SecurityContext; if (parts.length === 1) { boundPropertyName = this._schemaRegistry.getMappedPropName(parts[0]); + securityContext = this._schemaRegistry.securityContext(elementName, boundPropertyName); bindingType = PropertyBindingType.Property; if (!this._schemaRegistry.hasProperty(elementName, boundPropertyName)) { this._reportError( @@ -718,27 +720,41 @@ class TemplateParseVisitor implements HtmlAstVisitor { } else { if (parts[0] == ATTRIBUTE_PREFIX) { boundPropertyName = parts[1]; + if (boundPropertyName.toLowerCase().startsWith('on')) { + this._reportError( + `Binding to event attribute '${boundPropertyName}' is disallowed ` + + `for security reasons, please use (${boundPropertyName.slice(2)})=...`, + sourceSpan); + } + // NB: For security purposes, use the mapped property name, not the attribute name. + securityContext = this._schemaRegistry.securityContext( + elementName, this._schemaRegistry.getMappedPropName(boundPropertyName)); let nsSeparatorIdx = boundPropertyName.indexOf(':'); if (nsSeparatorIdx > -1) { let ns = boundPropertyName.substring(0, nsSeparatorIdx); let name = boundPropertyName.substring(nsSeparatorIdx + 1); boundPropertyName = mergeNsAndName(ns, name); } + bindingType = PropertyBindingType.Attribute; } else if (parts[0] == CLASS_PREFIX) { boundPropertyName = parts[1]; bindingType = PropertyBindingType.Class; + securityContext = SecurityContext.NONE; } else if (parts[0] == STYLE_PREFIX) { unit = parts.length > 2 ? parts[2] : null; boundPropertyName = parts[1]; bindingType = PropertyBindingType.Style; + securityContext = SecurityContext.STYLE; } else { this._reportError(`Invalid property name '${name}'`, sourceSpan); bindingType = null; + securityContext = null; } } - return new BoundElementPropertyAst(boundPropertyName, bindingType, ast, unit, sourceSpan); + return new BoundElementPropertyAst(boundPropertyName, bindingType, securityContext, ast, unit, + sourceSpan); } diff --git a/modules/@angular/compiler/src/view_compiler/property_binder.ts b/modules/@angular/compiler/src/view_compiler/property_binder.ts index 7d1d05ce77..9d08460f16 100644 --- a/modules/@angular/compiler/src/view_compiler/property_binder.ts +++ b/modules/@angular/compiler/src/view_compiler/property_binder.ts @@ -1,3 +1,4 @@ +import {SecurityContext} from '../../core_private'; import {LifecycleHooks, isDefaultChangeDetectionStrategy} from '../../core_private'; import {isBlank, isPresent} from '../../src/facade/lang'; @@ -5,7 +6,7 @@ import {isBlank, isPresent} from '../../src/facade/lang'; import * as cdAst from '../expression_parser/ast'; import * as o from '../output/output_ast'; import {Identifiers} from '../identifiers'; -import {DetectChangesVars} from './constants'; +import {DetectChangesVars, ViewProperties} from './constants'; import { BoundTextAst, @@ -30,7 +31,7 @@ function createBindFieldExpr(exprIndex: number): o.ReadPropExpr { } function createCurrValueExpr(exprIndex: number): o.ReadVarExpr { - return o.variable(`currVal_${exprIndex}`); + return o.variable(`currVal_${exprIndex}`); // fix syntax highlighting: ` } function bind(view: CompileView, currValExpr: o.ReadVarExpr, fieldExpr: o.ReadPropExpr, @@ -94,7 +95,7 @@ function bindAndWriteToRenderer(boundProps: BoundElementPropertyAst[], context: var fieldExpr = createBindFieldExpr(bindingIndex); var currValExpr = createCurrValueExpr(bindingIndex); var renderMethod: string; - var renderValue: o.Expression = currValExpr; + var renderValue: o.Expression = sanitizedValue(boundProp, currValExpr); var updateStmts = []; switch (boundProp.type) { case PropertyBindingType.Property: @@ -130,6 +131,34 @@ function bindAndWriteToRenderer(boundProps: BoundElementPropertyAst[], context: }); } +function sanitizedValue(boundProp: BoundElementPropertyAst, renderValue: o.Expression): o.Expression { + let enumValue: string; + switch (boundProp.securityContext) { + case SecurityContext.NONE: + return renderValue; // No sanitization needed. + case SecurityContext.HTML: + enumValue = 'HTML'; + break; + case SecurityContext.STYLE: + enumValue = 'STYLE'; + break; + case SecurityContext.SCRIPT: + enumValue = 'SCRIPT'; + break; + case SecurityContext.URL: + enumValue = 'URL'; + break; + case SecurityContext.RESOURCE_URL: + enumValue = 'RESOURCE_URL'; + break; + default: + throw new Error(`internal error, unexpected SecurityContext ${boundProp.securityContext}.`); + } + let ctx = ViewProperties.viewUtils.prop('sanitizer'); + let args = [o.importExpr(Identifiers.SecurityContext).prop(enumValue), renderValue]; + return ctx.callMethod('sanitize', args); +} + export function bindRenderInputs(boundProps: BoundElementPropertyAst[], compileElement: CompileElement): void { bindAndWriteToRenderer(boundProps, compileElement.view.componentContext, compileElement); diff --git a/modules/@angular/compiler/src/view_compiler/view_builder.ts b/modules/@angular/compiler/src/view_compiler/view_builder.ts index 1513aa175c..2eaea85daf 100644 --- a/modules/@angular/compiler/src/view_compiler/view_builder.ts +++ b/modules/@angular/compiler/src/view_compiler/view_builder.ts @@ -214,7 +214,7 @@ class ViewBuilderVisitor implements TemplateAstVisitor { var nestedComponentIdentifier = new CompileIdentifierMetadata({name: getViewFactoryName(component, 0)}); this.targetDependencies.push(new ViewCompileDependency(component, nestedComponentIdentifier)); - compViewExpr = o.variable(`compView_${nodeIndex}`); + compViewExpr = o.variable(`compView_${nodeIndex}`); // fix highlighting: ` compileElement.setComponentView(compViewExpr); this.view.createMethod.addStmt(compViewExpr.set(o.importExpr(nestedComponentIdentifier) .callFn([ @@ -336,7 +336,8 @@ function mapToKeyValueArray(data: {[key: string]: string}): string[][] { function createViewTopLevelStmts(view: CompileView, targetStatements: o.Statement[]) { var nodeDebugInfosVar: o.Expression = o.NULL_EXPR; if (view.genConfig.genDebugInfo) { - nodeDebugInfosVar = o.variable(`nodeDebugInfos_${view.component.type.name}${view.viewIndex}`); + nodeDebugInfosVar = o.variable( + `nodeDebugInfos_${view.component.type.name}${view.viewIndex}`); // fix highlighting: ` targetStatements.push( (nodeDebugInfosVar) .set(o.literalArr(view.nodes.map(createStaticNodeDebugInfo), @@ -346,7 +347,8 @@ function createViewTopLevelStmts(view: CompileView, targetStatements: o.Statemen } - var renderCompTypeVar: o.ReadVarExpr = o.variable(`renderType_${view.component.type.name}`); + var renderCompTypeVar: o.ReadVarExpr = + o.variable(`renderType_${view.component.type.name}`); // fix highlighting: ` if (view.viewIndex === 0) { targetStatements.push(renderCompTypeVar.set(o.NULL_EXPR) .toDeclStmt(o.importType(Identifiers.RenderComponentType))); 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 95412032b0..9acbdb94b8 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 @@ -11,11 +11,12 @@ import { } from '@angular/core/testing/testing_internal'; import {DomElementSchemaRegistry} from '@angular/compiler/src/schema/dom_element_schema_registry'; +import {SecurityContext} from '../../core_private'; import {extractSchema} from './schema_extractor'; export function main() { describe('DOMElementSchema', () => { - var registry: DomElementSchemaRegistry; + let registry: DomElementSchemaRegistry; beforeEach(() => { registry = new DomElementSchemaRegistry(); }); it('should detect properties on regular elements', () => { @@ -33,21 +34,20 @@ export function main() { expect(registry.hasProperty('div', 'unknown')).toBeFalsy(); }); - it('should detect different kinds of types', - () => { - // inheritance: video => media => * - expect(registry.hasProperty('video', 'className')).toBeTruthy(); // from * - expect(registry.hasProperty('video', 'id')).toBeTruthy(); // string - expect(registry.hasProperty('video', 'scrollLeft')).toBeTruthy(); // number - expect(registry.hasProperty('video', 'height')).toBeTruthy(); // number - expect(registry.hasProperty('video', 'autoplay')).toBeTruthy(); // boolean - expect(registry.hasProperty('video', 'classList')).toBeTruthy(); // object - // from *; but events are not properties - expect(registry.hasProperty('video', 'click')).toBeFalsy(); - }) + it('should detect different kinds of types', () => { + // inheritance: video => media => * + expect(registry.hasProperty('video', 'className')).toBeTruthy(); // from * + expect(registry.hasProperty('video', 'id')).toBeTruthy(); // string + expect(registry.hasProperty('video', 'scrollLeft')).toBeTruthy(); // number + expect(registry.hasProperty('video', 'height')).toBeTruthy(); // number + expect(registry.hasProperty('video', 'autoplay')).toBeTruthy(); // boolean + expect(registry.hasProperty('video', 'classList')).toBeTruthy(); // object + // from *; but events are not properties + expect(registry.hasProperty('video', 'click')).toBeFalsy(); + }); - it('should return true for custom-like elements', - () => { expect(registry.hasProperty('custom-like', 'unknown')).toBeTruthy(); }); + it('should return true for custom-like elements', + () => { expect(registry.hasProperty('custom-like', 'unknown')).toBeTruthy(); }); it('should re-map property names that are specified in DOM facade', () => { expect(registry.getMappedPropName('readonly')).toEqual('readOnly'); }); @@ -57,6 +57,10 @@ export function main() { expect(registry.getMappedPropName('exotic-unknown')).toEqual('exotic-unknown'); }); + it('should return security contexts for elements', () => { + expect(registry.securityContext('a', 'href')).toBe(SecurityContext.URL); + }); + it('should detect properties on namespaced elements', () => { expect(registry.hasProperty('@svg:g', 'id')).toBeTruthy(); }); diff --git a/modules/@angular/compiler/testing/schema_registry_mock.ts b/modules/@angular/compiler/testing/schema_registry_mock.ts index 7ec461c812..38aa003a34 100644 --- a/modules/@angular/compiler/testing/schema_registry_mock.ts +++ b/modules/@angular/compiler/testing/schema_registry_mock.ts @@ -1,4 +1,5 @@ import {isPresent} from '../src/facade/lang'; +import {SecurityContext} from '../core_private'; import {ElementSchemaRegistry} from '../index'; export class MockSchemaRegistry implements ElementSchemaRegistry { @@ -10,6 +11,10 @@ export class MockSchemaRegistry implements ElementSchemaRegistry { return isPresent(result) ? result : true; } + securityContext(tagName: string, property: string): SecurityContext { + return SecurityContext.NONE; + } + getMappedPropName(attrName: string): string { var result = this.attrPropMapping[attrName]; return isPresent(result) ? result : attrName; diff --git a/modules/@angular/core/private_export.ts b/modules/@angular/core/private_export.ts index 9f3a608136..29131241b6 100644 --- a/modules/@angular/core/private_export.ts +++ b/modules/@angular/core/private_export.ts @@ -1,4 +1,5 @@ import * as constants from './src/change_detection/constants'; +import * as security from './src/security'; import * as reflective_provider from './src/di/reflective_provider'; import * as lifecycle_hooks from './src/metadata/lifecycle_hooks'; import * as reflector_reader from './src/reflection/reflector_reader'; @@ -52,6 +53,10 @@ export declare namespace __core_private_types__ { export var ValueUnwrapper: typeof change_detection_util.ValueUnwrapper; export type RenderDebugInfo = api.RenderDebugInfo; export var RenderDebugInfo: typeof api.RenderDebugInfo; + export var SecurityContext: typeof security.SecurityContext; + export type SecurityContext = security.SecurityContext; + export var SanitizationService: typeof security.SanitizationService; + export type SanitizationService = security.SanitizationService; export type TemplateRef_ = template_ref.TemplateRef_; export var TemplateRef_: typeof template_ref.TemplateRef_; export var wtfInit: typeof wtf_init.wtfInit; @@ -104,6 +109,8 @@ export var __core_private__ = { uninitialized: change_detection_util.uninitialized, ValueUnwrapper: change_detection_util.ValueUnwrapper, RenderDebugInfo: api.RenderDebugInfo, + SecurityContext: security.SecurityContext, + SanitizationService: security.SanitizationService, TemplateRef_: template_ref.TemplateRef_, wtfInit: wtf_init.wtfInit, ReflectionCapabilities: reflection_capabilities.ReflectionCapabilities, diff --git a/modules/@angular/core/src/linker/view_utils.ts b/modules/@angular/core/src/linker/view_utils.ts index b11ce3eb97..0b6cb1c53f 100644 --- a/modules/@angular/core/src/linker/view_utils.ts +++ b/modules/@angular/core/src/linker/view_utils.ts @@ -1,3 +1,4 @@ +import {SanitizationService} from '../security'; import {isBlank, isPresent, looseIdentical} from '../../src/facade/lang'; import {ListWrapper, StringMapWrapper} from '../../src/facade/collection'; import {BaseException} from '../../src/facade/exceptions'; @@ -12,9 +13,14 @@ import {uninitialized} from "../change_detection/change_detection_util"; @Injectable() export class ViewUtils { + sanitizer: SanitizationService; private _nextCompTypeId: number = 0; - constructor(private _renderer: RootRenderer, @Inject(APP_ID) private _appId: string) {} + constructor( + private _renderer: RootRenderer, @Inject(APP_ID) private _appId: string, + sanitizer: SanitizationService) { + this.sanitizer = sanitizer; + } /** * Used by the generated code diff --git a/modules/@angular/core/src/security.ts b/modules/@angular/core/src/security.ts new file mode 100644 index 0000000000..72d2b67ef6 --- /dev/null +++ b/modules/@angular/core/src/security.ts @@ -0,0 +1,23 @@ +/** + * A SecurityContext marks a location that has dangerous security implications, e.g. a DOM property + * like `innerHTML` that could cause Cross Site Scripting (XSS) security bugs when improperly + * handled. + * + * See DomSanitizationService for more details on security in Angular applications. + */ +export enum SecurityContext { + NONE, + HTML, + STYLE, + SCRIPT, + URL, + RESOURCE_URL, +} + +/** + * SanitizationService is used by the views to sanitize potentially dangerous values. This is a + * private API, use code should only refer to DomSanitizationService. + */ +export abstract class SanitizationService { + abstract sanitize(context: SecurityContext, value: string): string; +} diff --git a/modules/@angular/core/test/linker/security_integration_spec.ts b/modules/@angular/core/test/linker/security_integration_spec.ts new file mode 100644 index 0000000000..6b25762816 --- /dev/null +++ b/modules/@angular/core/test/linker/security_integration_spec.ts @@ -0,0 +1,157 @@ +import { + ddescribe, + describe, + expect, + inject, + beforeEachProviders, + it, +} from '@angular/core/testing/testing_internal'; +import {TestComponentBuilder} from '@angular/compiler/testing'; +import {AsyncTestCompleter} from '@angular/core/testing/testing_internal'; +import {getDOM} from '@angular/platform-browser/src/dom/dom_adapter'; +import {PromiseWrapper} from '../../src/facade/async'; +import {provide, Injectable, OpaqueToken} from '@angular/core'; +import {CompilerConfig} from '@angular/compiler'; +import {Component, ViewMetadata} from '@angular/core/src/metadata'; +import {IS_DART} from '../../src/facade/lang'; +import {el} from '@angular/platform-browser/testing'; + +import {DomSanitizationService} from '@angular/platform-browser'; + +const ANCHOR_ELEMENT = /*@ts2dart_const*/ new OpaqueToken('AnchorElement'); + +export function main() { + if (IS_DART) { + declareTests(false); + } else { + describe('jit', () => { + beforeEachProviders( + () => [provide(CompilerConfig, {useValue: new CompilerConfig(true, false, true)})]); + declareTests(true); + }); + + describe('no jit', () => { + beforeEachProviders( + () => [provide(CompilerConfig, {useValue: new CompilerConfig(true, false, false)})]); + declareTests(false); + }); + } +} + +@Component({selector: 'my-comp', directives: []}) +@Injectable() +class SecuredComponent { + ctxProp: string; + constructor() { this.ctxProp = 'some value'; } +} + +function itAsync(msg: string, injections: Function[], f: Function); +function itAsync(msg: string, f: (tcb: TestComponentBuilder, atc: AsyncTestCompleter) => void); +function itAsync(msg: string, + f: Function[] | ((tcb: TestComponentBuilder, atc: AsyncTestCompleter) => void), + fn?: Function) { + if (f instanceof Function) { + it(msg, inject([TestComponentBuilder, AsyncTestCompleter], f)); + } else { + let injections = f; + it(msg, inject(injections, fn)); + } +} + +function declareTests(isJit: boolean) { + describe('security integration tests', function() { + + beforeEachProviders(() => [provide(ANCHOR_ELEMENT, {useValue: el('
')})]); + + describe('safe HTML values', function() { + itAsync('should disallow binding on*', (tcb: TestComponentBuilder, async) => { + let tpl = `
`; + tcb = tcb.overrideView(SecuredComponent, new ViewMetadata({template: tpl})); + PromiseWrapper.catchError(tcb.createAsync(SecuredComponent), (e) => { + expect(e.message).toContain( + `Template parse errors:\n` + `Binding to event attribute 'onclick' is disallowed ` + + `for security reasons, please use (click)=... `); + async.done(); + return null; + }); + }); + + itAsync('should escape unsafe attributes', (tcb: TestComponentBuilder, async) => { + let tpl = `Link Title`; + tcb.overrideView(SecuredComponent, new ViewMetadata({template: tpl, directives: []})) + .createAsync(SecuredComponent) + .then((fixture) => { + let e = fixture.debugElement.children[0].nativeElement; + fixture.debugElement.componentInstance.ctxProp = 'hello'; + fixture.detectChanges(); + // In the browser, reading href returns an absolute URL. On the server side, + // it just echoes back the property. + expect(getDOM().getProperty(e, 'href')).toMatch(/.*\/?hello$/); + + fixture.debugElement.componentInstance.ctxProp = 'javascript:alert(1)'; + fixture.detectChanges(); + expect(getDOM().getProperty(e, 'href')).toEqual('unsafe:javascript:alert(1)'); + + async.done(); + }); + }); + + itAsync('should not escape values marked as trusted', + [TestComponentBuilder, AsyncTestCompleter, DomSanitizationService], + (tcb: TestComponentBuilder, async, sanitizer: DomSanitizationService) => { + let tpl = `Link Title`; + tcb.overrideView(SecuredComponent, + new ViewMetadata({template: tpl, directives: []})) + .createAsync(SecuredComponent) + .then((fixture) => { + let e = fixture.debugElement.children[0].nativeElement; + let trusted = sanitizer.bypassSecurityTrustUrl('javascript:alert(1)'); + fixture.debugElement.componentInstance.ctxProp = trusted; + fixture.detectChanges(); + expect(getDOM().getProperty(e, 'href')).toEqual('javascript:alert(1)'); + + async.done(); + }); + }); + + itAsync('should error when using the wrong trusted value', + [TestComponentBuilder, AsyncTestCompleter, DomSanitizationService], + (tcb: TestComponentBuilder, async, sanitizer: DomSanitizationService) => { + let tpl = `Link Title`; + tcb.overrideView(SecuredComponent, + new ViewMetadata({template: tpl, directives: []})) + .createAsync(SecuredComponent) + .then((fixture) => { + let trusted = sanitizer.bypassSecurityTrustScript('javascript:alert(1)'); + fixture.debugElement.componentInstance.ctxProp = trusted; + expect(() => fixture.detectChanges()) + .toThrowErrorWith('Required a safe URL, got a Script'); + + async.done(); + }); + }); + + itAsync('should escape unsafe style values', (tcb: TestComponentBuilder, async) => { + let tpl = `
Text
`; + tcb.overrideView(SecuredComponent, new ViewMetadata({template: tpl, directives: []})) + .createAsync(SecuredComponent) + .then((fixture) => { + let e = fixture.debugElement.children[0].nativeElement; + // Make sure binding harmless values works. + fixture.debugElement.componentInstance.ctxProp = 'red'; + fixture.detectChanges(); + // In some browsers, this will contain the full background specification, not just + // the color. + expect(getDOM().getStyle(e, 'background')).toMatch(/red.*/); + + fixture.debugElement.componentInstance.ctxProp = 'url(javascript:evil())'; + fixture.detectChanges(); + // Updated value gets rejected, no value change. + expect(getDOM().getStyle(e, 'background')).not.toContain('javascript'); + + async.done(); + }); + }); + }); + }); +} \ No newline at end of file diff --git a/modules/@angular/integration_test/public_api_spec.ts b/modules/@angular/integration_test/public_api_spec.ts index 783e8a2f4b..5a7093cd70 100644 --- a/modules/@angular/integration_test/public_api_spec.ts +++ b/modules/@angular/integration_test/public_api_spec.ts @@ -362,6 +362,7 @@ var PLATFORM_BROWSER: string[] = [ 'BROWSER_APP_STATIC_PROVIDERS', 'BROWSER_PROVIDERS', 'BROWSER_APP_COMMON_PROVIDERS', + 'BROWSER_SANITIZATION_PROVIDERS', 'DOCUMENT', 'ELEMENT_PROBE_PROVIDERS', 'DomEventsPlugin', @@ -375,6 +376,8 @@ var PLATFORM_BROWSER: string[] = [ 'BrowserPlatformLocation', 'AngularEntrypoint:dart', 'By', + 'DomSanitizationService', + 'SecurityContext', 'Title', 'disableDebugTools', 'enableDebugTools' diff --git a/modules/@angular/platform-browser/core_private.ts b/modules/@angular/platform-browser/core_private.ts index fed2266f7c..1756ffe2b6 100644 --- a/modules/@angular/platform-browser/core_private.ts +++ b/modules/@angular/platform-browser/core_private.ts @@ -8,3 +8,7 @@ export var VIEW_ENCAPSULATION_VALUES: typeof t.VIEW_ENCAPSULATION_VALUES = r.VIEW_ENCAPSULATION_VALUES; export type DebugDomRootRenderer = t.DebugDomRootRenderer; export var DebugDomRootRenderer: typeof t.DebugDomRootRenderer = r.DebugDomRootRenderer; +export var SecurityContext: typeof t.SecurityContext = r.SecurityContext; +export type SecurityContext = t.SecurityContext; +export var SanitizationService: typeof t.SanitizationService = r.SanitizationService; +export type SanitizationService = t.SanitizationService; diff --git a/modules/@angular/platform-browser/src/browser_common.ts b/modules/@angular/platform-browser/src/browser_common.ts index 43fe217939..1ec7a8cbed 100644 --- a/modules/@angular/platform-browser/src/browser_common.ts +++ b/modules/@angular/platform-browser/src/browser_common.ts @@ -10,8 +10,12 @@ import { OpaqueToken, Testability } from '@angular/core'; -import {wtfInit} from '../core_private'; +import {wtfInit, SanitizationService} from '../core_private'; import {COMMON_DIRECTIVES, COMMON_PIPES, FORM_PROVIDERS} from '@angular/common'; +import { + DomSanitizationService, + DomSanitizationServiceImpl +} from './security/dom_sanitization_service'; import {IS_DART} from './facade/lang'; import {BrowserDomAdapter} from './browser/browser_adapter'; @@ -62,6 +66,11 @@ function _document(): any { return getDOM().defaultDoc(); } +export const BROWSER_SANITIZATION_PROVIDERS: Array = /*@ts2dart_const*/[ + /* @ts2dart_Provider */ {provide: SanitizationService, useExisting: DomSanitizationService}, + /* @ts2dart_Provider */ {provide: DomSanitizationService, useClass: DomSanitizationServiceImpl}, +]; + /** * A set of providers to initialize an Angular application in a web browser. * @@ -71,6 +80,7 @@ export const BROWSER_APP_COMMON_PROVIDERS: Array` hyperlink, `someValue` will be + * sanitized so that an attacker cannot inject e.g. a `javascript:` URL that would execute code on + * the website. + * + * In specific situations, it might be necessary to disable sanitization, for example if the + * application genuinely needs to produce a `javascript:` style link with a dynamic value in it. + * Users can bypass security by constructing a value with one of the `bypassSecurityTrust...` + * methods, and then binding to that value from the template. + * + * These situations should be very rare, and extraordinary care must be taken to avoid creating a + * Cross Site Scripting (XSS) security bug! + * + * When using `bypassSecurityTrust...`, make sure to call the method as early as possible and as + * close as possible to the source of the value, to make it easy to verify no security bug is + * created by its use. + * + * It is not required (and not recommended) to bypass security if the value is safe, e.g. a URL that + * does not start with a suspicious protocol, or an HTML snippet that does not contain dangerous + * code. The sanitizer leaves safe values intact. + */ +export abstract class DomSanitizationService implements SanitizationService { + /** + * Sanitizes a value for use in the given SecurityContext. + * + * If value is trusted for the context, this method will unwrap the contained safe value and use + * it directly. Otherwise, value will be sanitized to be safe in the given context, for example + * by replacing URLs that have an unsafe protocol part (such as `javascript:`). The implementation + * is responsible to make sure that the value can definitely be safely used in the given context. + */ + abstract sanitize(context: SecurityContext, value: any): string; + + /** + * Bypass security and trust the given value to be safe HTML. Only use this when the bound HTML + * is unsafe (e.g. contains `