Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(core): hardening rules related to the attribute order on <iframe> elements #47935

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
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
2 changes: 1 addition & 1 deletion goldens/size-tracking/aio-payloads.json
Expand Up @@ -12,7 +12,7 @@
"aio-local": {
"uncompressed": {
"runtime": 4325,
"main": 455228,
"main": 456041,
"polyfills": 33952,
"styles": 73964,
"light-theme": 78157,
Expand Down
2 changes: 1 addition & 1 deletion goldens/size-tracking/integration-payloads.json
Expand Up @@ -48,7 +48,7 @@
"animations": {
"uncompressed": {
"runtime": 1070,
"main": 155920,
"main": 156446,
"polyfills": 33814
}
},
Expand Down
Expand Up @@ -6,7 +6,7 @@ consts: [
],
template: function MyComponent_Template(rf, ctx) {
if (rf & 1) {
$r3$.ɵɵelement(0, "embed", 0)(1, "iframe", 1)(2, "object", 2)(3, "embed", 0)(4, "img", 3);
$r3$.ɵɵelement(0, "embed", 0)(1, "iframe", 1, null, i0.ɵɵvalidateIframeStaticAttributes)(2, "object", 2)(3, "embed", 0)(4, "img", 3);
}
}
212 changes: 212 additions & 0 deletions packages/compiler-cli/test/ngtsc/ngtsc_spec.ts
Expand Up @@ -7506,6 +7506,218 @@ 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)');

// Expect an extra validation function on the `element` instruction for an <iframe>
// to validate static attributes.
expect(jsContents)
.toContain('ɵɵelement(0, "iframe", 0, null, i0.ɵɵvalidateIframeStaticAttributes)');
});

it('should generate attribute and property bindings with a validator fn when on <iframe> ' +
'with an *ngIf on it as well',
() => {
env.write('test.ts', `
import {Component} from '@angular/core';

@Component({
template: \`
<iframe
*ngIf="visible"
src="http://angular.io"
[sandbox]="''"
></iframe>
\`
})
export class SomeComponent {
visible = true;
}
`);

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

// Expect an extra validation function on the `element` instruction for an <iframe>
// to validate static attributes.
expect(jsContents)
.toContain('ɵɵelement(0, "iframe", 1, null, i0.ɵɵvalidateIframeStaticAttributes)');

// The `template` instruction that represents an <iframe> remains as is
// (without additional validation fns).
expect(jsContents)
.toContain('ɵɵtemplate(0, SomeComponent_iframe_0_Template, 1, 1, "iframe", 0)');
});

it('should generate an element instruction with a validator function for <iframe>s ' +
'(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)');

// Expect an extra validation function on the `element` instruction for an <iframe>
// to validate static attributes.
expect(jsContents)
.toContain('ɵɵelement(0, "IFRAME", 0, null, i0.ɵɵvalidateIframeStaticAttributes)');
});

it('should include static attribute validation fn when on <iframe> ' +
'(even if there are no static attributes)',
() => {
env.write('test.ts', `
import {Component} from '@angular/core';

@Component({
template: \`
<iframe [src]="'http://angular.io'"></iframe>
\`
})
export class SomeComponent {}
`);

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

// Expect an extra validation function on the `element` instruction for an <iframe>
// to validate static attributes even if there are no static attributes present
// on an <iframe>. There might be directives with static host attributes mathcing
// this <iframe>, the validation function will be used to check the final set of
// static attributes (from all directives and an element itself).
expect(jsContents)
.toContain('ɵɵelement(0, "iframe", 0, null, i0.ɵɵvalidateIframeStaticAttributes)');
});

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!")');

// No extra validation fn on an `element` instruction too, since it's
// not an <iframe>.
expect(jsContents).toContain('ɵɵelement(0, "div", 0)');
});

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
4 changes: 4 additions & 0 deletions packages/compiler/src/render3/r3_identifiers.ts
Expand Up @@ -346,4 +346,8 @@ 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};
static validateIframeStaticAttributes:
o.ExternalReference = {name: 'ɵɵvalidateIframeStaticAttributes', 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
30 changes: 28 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 @@ -675,6 +676,16 @@ export class TemplateDefinitionBuilder implements t.Visitor<void>, LocalResolver
const refs = this.prepareRefsArray(element.references);
parameters.push(this.addToConsts(refs));

// If this element is an <iframe>, append an extra validation
// function, which would be invoked at runtime to make sure that
// all security-sensitive attributes defined statically (both on
// the element itself as well as on all matched directives) are
// set on the underlying <iframe> *before* setting its `src` or
// `srcdoc` (otherwise they'd not be taken into account).
if (isIframeElement(element.name)) {
parameters.push(o.importExpr(R3.validateIframeStaticAttributes));
}

const wasInNamespace = this._namespace;
const currentNamespace = this.getNamespaceInstruction(namespaceKey);

Expand Down Expand Up @@ -783,8 +794,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 +2233,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
25 changes: 25 additions & 0 deletions packages/compiler/src/schema/dom_security_schema.ts
Expand Up @@ -76,3 +76,28 @@ 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 before setting the `src` or `srcdoc` attribute value.
* This ensures that all security-sensitive attributes are taken into account
* while creating an instance of an `<iframe>` at runtime.
*
* Keep this list in sync with the `IFRAME_SECURITY_SENSITIVE_ATTRS` token
* from the `packages/core/src/sanitization/iframe_attrs_validation.ts` script.
*
* 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', 'loading', '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());
}