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

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.
  • Loading branch information
AndrewKushnir committed Nov 11, 2022
1 parent 3b91101 commit 61575df
Show file tree
Hide file tree
Showing 14 changed files with 965 additions and 6 deletions.
2 changes: 2 additions & 0 deletions goldens/public-api/core/errors.md
Expand Up @@ -67,6 +67,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
152 changes: 152 additions & 0 deletions packages/compiler-cli/test/ngtsc/ngtsc_spec.ts
Expand Up @@ -6990,6 +6990,158 @@ 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, Directive, NgModule} from '@angular/core';
@Directive({
selector: 'iframe',
inputs: ['sandbox']
})
class SomeDirective {}
@Component({
template: \`
<iframe src="http://angular.io"
[sandbox]="''" [attr.allow]="''"
[title]="'Hi!'"
></iframe>
\`
})
class SomeComponent {}
@NgModule({
declarations: [SomeDirective, SomeComponent]
})
class SomeNgModule {}
`);

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, NgModule} from '@angular/core';
@Directive({
selector: '[sandbox]',
inputs: ['sandbox']
})
class Dir {}
@Component({
imports: [Dir],
template: \`
<div [sandbox]="''" [title]="'Hi!'"></div>
\`
})
export class SomeComponent {}
@NgModule({
declarations: [Dir, SomeComponent]
})
class SomeNgModule {}
`);

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 @@ -336,4 +336,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 @@ -551,6 +552,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 @@ -765,8 +766,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 @@ -2195,6 +2207,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 @@ -62,3 +62,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 @@ -269,6 +269,9 @@ export {
ɵɵtrustConstantHtml,
ɵɵtrustConstantResourceUrl,
} from './sanitization/sanitization';
export {
ɵɵvalidateIframeAttribute,
} from './sanitization/iframe_attrs_validation';
export {
noSideEffects as ɵnoSideEffects,
} from './util/closure';
Expand Down
3 changes: 2 additions & 1 deletion packages/core/src/errors.ts
Expand Up @@ -62,7 +62,8 @@ export const enum RuntimeErrorCode {
VIEW_ALREADY_ATTACHED = 902,
INVALID_INHERITANCE = 903,
UNSAFE_VALUE_IN_RESOURCE_URL = 904,
UNSAFE_VALUE_IN_SCRIPT = 905
UNSAFE_VALUE_IN_SCRIPT = 905,
UNSAFE_IFRAME_ATTRS = 910,
}

/**
Expand Down
51 changes: 51 additions & 0 deletions packages/core/src/render3/instructions/element_validation.ts
@@ -0,0 +1,51 @@
/**
* @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 {throwError} from '../../util/assert';
import {getComponentDef} from '../definition';
import {ComponentDef} from '../interfaces/definition';
import {CONTEXT, DECLARATION_COMPONENT_VIEW, LView} from '../interfaces/view';

/**
* WARNING: this is a **dev-mode only** function (thus should always be guarded by the `ngDevMode`)
* and must **not** be used in production bundles. The function makes megamorphic reads, which might
* be too slow for production mode and also it relies on the constructor function being available.
*
* Gets a reference to the host component def (where a current component is declared).
*
* @param lView An `LView` that represents a current component that is being rendered.
*/
function getDeclarationComponentDef(lView: LView): ComponentDef<unknown>|null {
!ngDevMode && throwError('Must never be called in production mode');

const declarationLView = lView[DECLARATION_COMPONENT_VIEW] as LView;
const context = declarationLView[CONTEXT];

// Unable to obtain a context.
if (!context) return null;

return context.constructor ? getComponentDef(context.constructor) : null;
}

/**
* WARNING: this is a **dev-mode only** function (thus should always be guarded by the `ngDevMode`)
* and must **not** be used in production bundles. The function makes megamorphic reads, which might
* be too slow for production mode.
*
* Constructs a string describing the location of the host component template. The function is used
* in dev mode to produce error messages.
*
* @param lView An `LView` that represents a current component that is being rendered.
*/
export function getTemplateLocationDetails(lView: LView): string {
!ngDevMode && throwError('Must never be called in production mode');

const hostComponentDef = getDeclarationComponentDef(lView);
const componentClassName = hostComponentDef?.type?.name;
return componentClassName ? ` (used in the '${componentClassName}' component template)` : '';
}
2 changes: 2 additions & 0 deletions packages/core/src/render3/jit/environment.ts
Expand Up @@ -9,6 +9,7 @@
import {forwardRef, resolveForwardRef} from '../../di/forward_ref';
import {ɵɵinject, ɵɵinvalidFactoryDep} from '../../di/injector_compatibility';
import {ɵɵdefineInjectable, ɵɵdefineInjector} from '../../di/interface/defs';
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 @@ -164,6 +165,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

0 comments on commit 61575df

Please sign in to comment.