Skip to content

Commit

Permalink
fix(core): hardening attribute and property binding rules for <iframe…
Browse files Browse the repository at this point in the history
…> elements (#47964)

This commit updates the logic related to the attribute and property binding rules for <iframe> elements. There is a set of <iframe> attributes that may affect the behavior of an iframe and this change enforces that these attributes are only applied as static attributes, making sure that they are taken into account while creating an <iframe>.

If Angular detects that some of the security-sensitive attributes are applied as an attribute or property binding, it throws an error message, which contains the name of an attribute that is causing the problem and the name of a Component where an iframe is located.

BREAKING CHANGE:

Existing iframe usages may have security-sensitive attributes applied as an attribute or property binding in a template or via host bindings in a directive. Such usages would require an update to ensure compliance with the new stricter rules around iframe bindings.

PR Close #47964
  • Loading branch information
AndrewKushnir authored and alxhub committed Nov 9, 2022
1 parent 54d0981 commit 28f289b
Show file tree
Hide file tree
Showing 14 changed files with 999 additions and 6 deletions.
2 changes: 2 additions & 0 deletions goldens/public-api/core/errors.md
Expand Up @@ -101,6 +101,8 @@ export const enum RuntimeErrorCode {
// (undocumented)
UNKNOWN_ELEMENT = 304,
// (undocumented)
UNSAFE_IFRAME_ATTRS = 910,
// (undocumented)
UNSAFE_VALUE_IN_RESOURCE_URL = 904,
// (undocumented)
UNSAFE_VALUE_IN_SCRIPT = 905,
Expand Down
138 changes: 138 additions & 0 deletions packages/compiler-cli/test/ngtsc/ngtsc_spec.ts
Expand Up @@ -7506,6 +7506,144 @@ function allTests(os: string) {
});
});

describe('iframe processing', () => {
it('should generate attribute and property bindings with a validator fn when on <iframe>',
() => {
env.write('test.ts', `
import {Component} from '@angular/core';
@Component({
template: \`
<iframe src="http://angular.io"
[sandbox]="''" [attr.allow]="''"
[title]="'Hi!'"
></iframe>
\`
})
export class SomeComponent {}
`);

env.driveMain();
const jsContents = env.getContents('test.js');

// Only `sandbox` has an extra validation fn (since it's security-sensitive),
// the `title` property doesn't have an extra validation fn.
expect(jsContents)
.toContain(
'ɵɵproperty("sandbox", "", i0.ɵɵvalidateIframeAttribute)("title", "Hi!")');

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

it('should generate an attribute binding instruction with a validator function ' +
'(making sure it\'s case-insensitive, since this is allowed in Angular templates)',
() => {
env.write('test.ts', `
import {Component} from '@angular/core';
@Component({
template: \`
<IFRAME
src="http://angular.io"
[attr.SANDBOX]="''"
></IFRAME>
\`
})
export class SomeComponent {}
`);

env.driveMain();
const jsContents = env.getContents('test.js');

// Make sure that the `sandbox` has an extra validation fn,
// and the check is case-insensitive (since the `setAttribute` DOM API
// is case-insensitive as well).
expect(jsContents).toContain('ɵɵattribute("SANDBOX", "", i0.ɵɵvalidateIframeAttribute)');
});

it('should *not* generate a validator fn for attribute and property bindings when *not* on <iframe>',
() => {
env.write('test.ts', `
import {Component, Directive} from '@angular/core';
@Directive({
standalone: true,
selector: '[sandbox]',
inputs: ['sandbox']
})
class Dir {}
@Component({
standalone: true,
imports: [Dir],
template: \`
<div [sandbox]="''" [title]="'Hi!'"></div>
\`
})
export class SomeComponent {}
`);

env.driveMain();
const jsContents = env.getContents('test.js');

// Note: no extra validation fn, since a security-sensitive attribute is *not* on an
// <iframe>.
expect(jsContents).toContain('ɵɵproperty("sandbox", "")("title", "Hi!")');
});

it('should generate a validator fn for attribute and property host bindings on a directive',
() => {
env.write('test.ts', `
import {Directive} from '@angular/core';
@Directive({
host: {
'[sandbox]': "''",
'[attr.allow]': "''",
'src': 'http://angular.io'
}
})
export class SomeDir {}
`);

env.driveMain();
const jsContents = env.getContents('test.js');

// The `sandbox` is potentially a security-sensitive attribute of an <iframe>.
// Generate an extra validation function to invoke at runtime, which would
// check if an underlying host element is an <iframe>.
expect(jsContents)
.toContain('ɵɵhostProperty("sandbox", "", i0.ɵɵvalidateIframeAttribute)');

// Similar to the above, but for an attribute binding (host attributes are
// represented via `ɵɵattribute`).
expect(jsContents).toContain('ɵɵattribute("allow", "", i0.ɵɵvalidateIframeAttribute)');
});

it('should generate a validator fn for attribute host bindings on a directive ' +
'(making sure the check is case-insensitive)',
() => {
env.write('test.ts', `
import {Directive} from '@angular/core';
@Directive({
host: {
'[attr.SANDBOX]': "''"
}
})
export class SomeDir {}
`);

env.driveMain();
const jsContents = env.getContents('test.js');

// Make sure that we generate a validation fn for the `sandbox` attribute,
// even when it was declared as `SANDBOX`.
expect(jsContents).toContain('ɵɵattribute("SANDBOX", "", i0.ɵɵvalidateIframeAttribute)');
});
});

describe('undecorated providers', () => {
it('should error when an undecorated class, with a non-trivial constructor, is provided directly in a module',
() => {
Expand Down
2 changes: 2 additions & 0 deletions packages/compiler/src/render3/r3_identifiers.ts
Expand Up @@ -346,4 +346,6 @@ export class Identifiers {
static trustConstantHtml: o.ExternalReference = {name: 'ɵɵtrustConstantHtml', moduleName: CORE};
static trustConstantResourceUrl:
o.ExternalReference = {name: 'ɵɵtrustConstantResourceUrl', moduleName: CORE};
static validateIframeAttribute:
o.ExternalReference = {name: 'ɵɵvalidateIframeAttribute', moduleName: CORE};
}
14 changes: 14 additions & 0 deletions packages/compiler/src/render3/view/compiler.ts
Expand Up @@ -12,6 +12,7 @@ import * as core from '../../core';
import {AST, ParsedEvent, ParsedEventType, ParsedProperty} from '../../expression_parser/ast';
import * as o from '../../output/output_ast';
import {ParseError, ParseSourceSpan, sanitizeIdentifier} from '../../parse_util';
import {isIframeSecuritySensitiveAttr} from '../../schema/dom_security_schema';
import {CssSelector} from '../../selector';
import {ShadowCss} from '../../shadow_css';
import {BindingParser} from '../../template_parser/binding_parser';
Expand Down Expand Up @@ -574,6 +575,19 @@ function createHostBindingsFunction(
const instructionParams = [o.literal(bindingName), bindingExpr.currValExpr];
if (sanitizerFn) {
instructionParams.push(sanitizerFn);
} else {
// If there was no sanitization function found based on the security context
// of an attribute/property binding - check whether this attribute/property is
// one of the security-sensitive <iframe> attributes.
// Note: for host bindings defined on a directive, we do not try to find all
// possible places where it can be matched, so we can not determine whether
// the host element is an <iframe>. In this case, if an attribute/binding
// name is in the `IFRAME_SECURITY_SENSITIVE_ATTRS` set - append a validation
// function, which would be invoked at runtime and would have access to the
// underlying DOM element, check if it's an <iframe> and if so - runs extra checks.
if (isIframeSecuritySensitiveAttr(bindingName)) {
instructionParams.push(o.importExpr(R3.validateIframeAttribute));
}
}

updateVariables.push(...bindingExpr.stmts);
Expand Down
20 changes: 18 additions & 2 deletions packages/compiler/src/render3/view/template.ts
Expand Up @@ -23,6 +23,7 @@ import {mapLiteral} from '../../output/map_util';
import * as o from '../../output/output_ast';
import {ParseError, ParseSourceSpan, sanitizeIdentifier} from '../../parse_util';
import {DomElementSchemaRegistry} from '../../schema/dom_element_schema_registry';
import {isIframeSecuritySensitiveAttr} from '../../schema/dom_security_schema';
import {isTrustedTypesSink} from '../../schema/trusted_types_sinks';
import {CssSelector} from '../../selector';
import {BindingParser} from '../../template_parser/binding_parser';
Expand Down Expand Up @@ -783,8 +784,19 @@ export class TemplateDefinitionBuilder implements t.Visitor<void>, LocalResolver
const params: any[] = [];
const [attrNamespace, attrName] = splitNsName(input.name);
const isAttributeBinding = inputType === BindingType.Attribute;
const sanitizationRef = resolveSanitizationFn(input.securityContext, isAttributeBinding);
if (sanitizationRef) params.push(sanitizationRef);
let sanitizationRef = resolveSanitizationFn(input.securityContext, isAttributeBinding);
if (!sanitizationRef) {
// If there was no sanitization function found based on the security context
// of an attribute/property - check whether this attribute/property is
// one of the security-sensitive <iframe> attributes (and that the current
// element is actually an <iframe>).
if (isIframeElement(element.name) && isIframeSecuritySensitiveAttr(input.name)) {
sanitizationRef = o.importExpr(R3.validateIframeAttribute);
}
}
if (sanitizationRef) {
params.push(sanitizationRef);
}
if (attrNamespace) {
const namespaceLiteral = o.literal(attrNamespace);

Expand Down Expand Up @@ -2211,6 +2223,10 @@ function isTextNode(node: t.Node): boolean {
return node instanceof t.Text || node instanceof t.BoundText || node instanceof t.Icu;
}

function isIframeElement(tagName: string): boolean {
return tagName.toLowerCase() === 'iframe';
}

function hasTextChildrenOnly(children: t.Node[]): boolean {
return children.every(isTextNode);
}
Expand Down
22 changes: 22 additions & 0 deletions packages/compiler/src/schema/dom_security_schema.ts
Expand Up @@ -76,3 +76,25 @@ export function SECURITY_SCHEMA(): {[k: string]: SecurityContext} {
function registerContext(ctx: SecurityContext, specs: string[]) {
for (const spec of specs) _SECURITY_SCHEMA[spec.toLowerCase()] = ctx;
}

/**
* The set of security-sensitive attributes of an `<iframe>` that *must* be
* applied as a static attribute only. This ensures that all security-sensitive
* attributes are taken into account while creating an instance of an `<iframe>`
* at runtime.
*
* Note: avoid using this set directly, use the `isIframeSecuritySensitiveAttr` function
* in the code instead.
*/
export const IFRAME_SECURITY_SENSITIVE_ATTRS =
new Set(['sandbox', 'allow', 'allowfullscreen', 'referrerpolicy', 'csp', 'fetchpriority']);

/**
* Checks whether a given attribute name might represent a security-sensitive
* attribute of an <iframe>.
*/
export function isIframeSecuritySensitiveAttr(attrName: string): boolean {
// The `setAttribute` DOM API is case-insensitive, so we lowercase the value
// before checking it against a known security-sensitive attributes.
return IFRAME_SECURITY_SENSITIVE_ATTRS.has(attrName.toLowerCase());
}
30 changes: 30 additions & 0 deletions packages/compiler/test/security_spec.ts
@@ -0,0 +1,30 @@
/**
* @license
* Copyright Google LLC All Rights Reserved.
*
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/

import {IFRAME_SECURITY_SENSITIVE_ATTRS, SECURITY_SCHEMA} from '../src/schema/dom_security_schema';


describe('security-related tests', () => {
it('should have no overlap between `IFRAME_SECURITY_SENSITIVE_ATTRS` and `SECURITY_SCHEMA`',
() => {
// The `IFRAME_SECURITY_SENSITIVE_ATTRS` and `SECURITY_SCHEMA` tokens configure sanitization
// and validation rules and used to pick the right sanitizer function.
// This test verifies that there is no overlap between two sets of rules to flag
// a situation when 2 sanitizer functions may be needed at the same time (in which
// case, compiler logic should be extended to support that).
const schema = new Set();
Object.keys(SECURITY_SCHEMA()).forEach((key: string) => schema.add(key.toLowerCase()));
let hasOverlap = false;
IFRAME_SECURITY_SENSITIVE_ATTRS.forEach(attr => {
if (schema.has('*|' + attr) || schema.has('iframe|' + attr)) {
hasOverlap = true;
}
});
expect(hasOverlap).toBeFalse();
});
});
3 changes: 3 additions & 0 deletions packages/core/src/core_render3_private_export.ts
Expand Up @@ -272,6 +272,9 @@ export {
ɵɵtrustConstantHtml,
ɵɵtrustConstantResourceUrl,
} from './sanitization/sanitization';
export {
ɵɵvalidateIframeAttribute,
} from './sanitization/iframe_attrs_validation';
export {
noSideEffects as ɵnoSideEffects,
} from './util/closure';
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/errors.ts
Expand Up @@ -82,6 +82,7 @@ export const enum RuntimeErrorCode {
TYPE_IS_NOT_STANDALONE = 907,
MISSING_ZONEJS = 908,
UNEXPECTED_ZONE_STATE = 909,
UNSAFE_IFRAME_ATTRS = 910,
}

/**
Expand Down
Expand Up @@ -268,7 +268,7 @@ export function isHostComponentStandalone(lView: LView): boolean {
*
* @param lView An `LView` that represents a current component that is being rendered.
*/
function getTemplateLocationDetails(lView: LView): string {
export function getTemplateLocationDetails(lView: LView): string {
!ngDevMode && throwError('Must never be called in production mode');

const hostComponentDef = getDeclarationComponentDef(lView);
Expand Down
2 changes: 2 additions & 0 deletions packages/core/src/render3/jit/environment.ts
Expand Up @@ -10,6 +10,7 @@ import {forwardRef, resolveForwardRef} from '../../di/forward_ref';
import {ɵɵinject, ɵɵinvalidFactoryDep} from '../../di/injector_compatibility';
import {ɵɵdefineInjectable, ɵɵdefineInjector} from '../../di/interface/defs';
import {registerNgModuleType} from '../../linker/ng_module_registration';
import * as iframe_attrs_validation from '../../sanitization/iframe_attrs_validation';
import * as sanitization from '../../sanitization/sanitization';
import * as r3 from '../index';

Expand Down Expand Up @@ -169,6 +170,7 @@ export const angularCoreEnv: {[name: string]: Function} =
'ɵɵsanitizeUrlOrResourceUrl': sanitization.ɵɵsanitizeUrlOrResourceUrl,
'ɵɵtrustConstantHtml': sanitization.ɵɵtrustConstantHtml,
'ɵɵtrustConstantResourceUrl': sanitization.ɵɵtrustConstantResourceUrl,
'ɵɵvalidateIframeAttribute': iframe_attrs_validation.ɵɵvalidateIframeAttribute,

'forwardRef': forwardRef,
'resolveForwardRef': resolveForwardRef,
Expand Down
57 changes: 57 additions & 0 deletions packages/core/src/sanitization/iframe_attrs_validation.ts
@@ -0,0 +1,57 @@
/**
* @license
* Copyright Google LLC All Rights Reserved.
*
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/

import {RuntimeError, RuntimeErrorCode} from '../errors';
import {getTemplateLocationDetails} from '../render3/instructions/element_validation';
import {TNodeType} from '../render3/interfaces/node';
import {RComment, RElement} from '../render3/interfaces/renderer_dom';
import {RENDERER} from '../render3/interfaces/view';
import {nativeRemoveNode} from '../render3/node_manipulation';
import {getLView, getSelectedTNode} from '../render3/state';
import {getNativeByTNode} from '../render3/util/view_utils';
import {trustedHTMLFromString} from '../util/security/trusted_types';


/**
* Validation function invoked at runtime for each binding that might potentially
* represent a security-sensitive attribute of an <iframe>.
* See `IFRAME_SECURITY_SENSITIVE_ATTRS` in the
* `packages/compiler/src/schema/dom_security_schema.ts` script for the full list
* of such attributes.
*
* @codeGenApi
*/
export function ɵɵvalidateIframeAttribute(attrValue: any, tagName: string, attrName: string) {
const lView = getLView();
const tNode = getSelectedTNode()!;
const element = getNativeByTNode(tNode, lView) as RElement | RComment;

// Restrict any dynamic bindings of security-sensitive attributes/properties
// on an <iframe> for security reasons.
if (tNode.type === TNodeType.Element && tagName.toLowerCase() === 'iframe') {
const iframe = element as HTMLIFrameElement;

// Unset previously applied `src` and `srcdoc` if we come across a situation when
// a security-sensitive attribute is set later via an attribute/property binding.
iframe.src = '';
iframe.srcdoc = trustedHTMLFromString('') as unknown as string;

// Also remove the <iframe> from the document.
nativeRemoveNode(lView[RENDERER], iframe);

const errorMessage = ngDevMode &&
`Angular has detected that the \`${attrName}\` was applied ` +
`as a binding to an <iframe>${getTemplateLocationDetails(lView)}. ` +
`For security reasons, the \`${attrName}\` can be set on an <iframe> ` +
`as a static attribute only. \n` +
`To fix this, switch the \`${attrName}\` binding to a static attribute ` +
`in a template or in host bindings section.`;
throw new RuntimeError(RuntimeErrorCode.UNSAFE_IFRAME_ATTRS, errorMessage);
}
return attrValue;
}

0 comments on commit 28f289b

Please sign in to comment.