fix(compiler): disallow i18n of security-sensitive attributes #39554

bjarkler · 2020-11-03T17:05:29Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe: Security improvements.

What is the current behavior?

It is currently possible to mark security-sensitive attributes for i18n translation, e.g.:

<embed src="https://google.com" i18n-src />

This is a feature that should have very little use, but at the same time introduces complexity and security risk in the i18n pipeline.

What is the new behavior?

The above template now generates a parsing error:

Error: src/app/app.component.html:69:35 - error NG5002: Translating attribute 'src' is disallowed for security reasons.

69   <embed src="https://google.com" i18n-src />
                                     ~~~~~~~~

(The same error occurs when using ng extract-i18n.)

Note that this only applies to attributes/properties that are relevant to Trusted Types, which are now enumerated in trusted_types_schema.ts:

frame.srcdoc
*.innerHTML
*.outerHTML
embed.src
object.codebase
object.data

These can be considered the most risky attributes/properties in terms of XSS. (Note that the script tag, inline event handlers, and other attributes/properties of a script context are not relevant here as they are already forbidden or stripped out by the compiler.)

Piggyback:

Remove trustConstScript, as it was never needed.
Use the new trusted_types_schema to reduce the number of constants that are converted to Trusted Types, and thus improve tree-shakability.

Does this PR introduce a breaking change?

Yes
No

This seems to be used extremely rarely. Sifting through GitHub search results for i18n-src only reveals a single result that might be affected by this change. (Note that e.g. <img i18n-src is not affected, since it is not a security-sensitive attribute.)

For applications that need to migrate away from this pattern, translation can be done in the corresponding TS file and regular interpolation used in the Angular template instead.

Other information

AndrewKushnir

LGTM, just added a couple comments 👍

packages/compiler/src/render3/view/i18n/meta.ts

packages/compiler/src/schema/trusted_types_sinks.ts

IgorMinar

just a few nits, but otherwise this looks good to me.

Reviewed-for: global-approvers, fw-security

packages/compiler/src/schema/trusted_types_sinks.ts

mhevery

With cleanup

packages/compiler/src/render3/view/i18n/meta.ts

Create a schema with an associated function to classify Trusted Types sinks. Piggyback a typo fix.

Make it possible to report errors from the I18nMetaVisitor parser.

To minimize security risk (XSS in particular) in the i18n pipeline, disallow i18n translation of attributes that are Trusted Types sinks. Add integration tests to ensure that such sinks cannot be translated.

Previously all constant values of security-sensitive attributes and properties were promoted to Trusted Types. While this is not inherently bad, it is also not optimal. Use the newly added Trusted Types schema to restrict promotion to constants that are in a Trusted Types-relevant context.

Script tags, inline event handlers and other script contexts are forbidden or stripped from Angular templates by the compiler. In the context of Trusted Types, this leaves no sinks that require use of a TrustedScript. This means that trustConstantScript is never used, and can be removed.

bjarkler · 2020-11-19T21:39:06Z

Thanks for the comments everyone. I think all of them have been addressed now. Just started a global presubmit; will update the PR tomorrow with the results.

mhevery · 2020-11-19T23:08:25Z

presubmit

IgorMinar

Awesome, thanks for the updates @bjarkler!

Reviewed-for: global-approvers, fw-security

IgorMinar · 2020-11-19T23:35:01Z

I'm unflagging this as a breaking change since we have no evidence that there is any valid use of this feature today.

AndrewKushnir · 2020-11-20T16:37:35Z

FYI, adding the "presubmit" label since we are waiting for TAP run results from @bjarkler.

bjarkler · 2020-11-20T16:43:23Z

global presubmit

AndrewKushnir · 2020-11-20T22:31:00Z

@bjarkler, please let us know once you get presubmit results so we can proceed with merging this PR. Thank you.

AndrewKushnir · 2020-11-21T00:54:29Z

Just a quick note: we've discussed this offline with @bjarkler and this PR is ready to go. Thank you.

Make it possible to report errors from the I18nMetaVisitor parser. PR Close #39554

To minimize security risk (XSS in particular) in the i18n pipeline, disallow i18n translation of attributes that are Trusted Types sinks. Add integration tests to ensure that such sinks cannot be translated. PR Close #39554

…#39554) Previously all constant values of security-sensitive attributes and properties were promoted to Trusted Types. While this is not inherently bad, it is also not optimal. Use the newly added Trusted Types schema to restrict promotion to constants that are in a Trusted Types-relevant context. PR Close #39554

…39554) Script tags, inline event handlers and other script contexts are forbidden or stripped from Angular templates by the compiler. In the context of Trusted Types, this leaves no sinks that require use of a TrustedScript. This means that trustConstantScript is never used, and can be removed. PR Close #39554

angular-automatic-lock-bot · 2020-12-24T16:46:28Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

google-cla bot added the cla: yes label Nov 3, 2020

josephperrott added the area: compiler Issues related to `ngc`, Angular's template compiler label Nov 3, 2020

ngbot bot added this to the needsTriage milestone Nov 3, 2020

bjarkler force-pushed the i18n-attribute-security branch from ee12e9f to 7d0c6b1 Compare November 3, 2020 17:24

AndrewKushnir added the breaking changes label Nov 3, 2020

petebacondarwin added the target: major This PR is targeted for the next major release label Nov 4, 2020

IgorMinar self-requested a review November 4, 2020 17:58

bjarkler mentioned this pull request Nov 17, 2020

fix(core): promote i18n constants to Trusted Types #39221

Closed

14 tasks

bjarkler force-pushed the i18n-attribute-security branch from 7d0c6b1 to 2898f35 Compare November 18, 2020 17:10

bjarkler changed the title ~~WIP(compiler): disallow i18n of security-sensitive attributes~~ fix(compiler): disallow i18n of security-sensitive attributes Nov 18, 2020

pullapprove bot requested review from AndrewKushnir, jelbourn and mhevery November 18, 2020 17:18

AndrewKushnir approved these changes Nov 19, 2020

View reviewed changes

packages/compiler/src/render3/view/i18n/meta.ts Outdated Show resolved Hide resolved

packages/compiler/src/schema/trusted_types_sinks.ts Outdated Show resolved Hide resolved

packages/compiler/src/schema/trusted_types_sinks.ts Outdated Show resolved Hide resolved

AndrewKushnir added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Nov 19, 2020

bjarkler force-pushed the i18n-attribute-security branch from 2898f35 to d391b8d Compare November 19, 2020 10:53

IgorMinar approved these changes Nov 19, 2020

View reviewed changes

packages/compiler/src/schema/trusted_types_sinks.ts Outdated Show resolved Hide resolved

IgorMinar self-requested a review November 19, 2020 18:25

mhevery approved these changes Nov 19, 2020

View reviewed changes

packages/compiler/src/render3/view/i18n/meta.ts Outdated Show resolved Hide resolved

mhevery self-assigned this Nov 19, 2020

bjarkler added 5 commits November 19, 2020 20:27

feat(compiler): add schema for Trusted Types sinks

77ed31c

Create a schema with an associated function to classify Trusted Types sinks. Piggyback a typo fix.

feat(compiler): support error reporting in I18nMetaVisitor

f5d74ea

Make it possible to report errors from the I18nMetaVisitor parser.

fix(compiler): disallow i18n of security-sensitive attributes

77e2d9a

To minimize security risk (XSS in particular) in the i18n pipeline, disallow i18n translation of attributes that are Trusted Types sinks. Add integration tests to ensure that such sinks cannot be translated.

bjarkler force-pushed the i18n-attribute-security branch from d391b8d to e676df2 Compare November 19, 2020 20:49

IgorMinar approved these changes Nov 19, 2020

View reviewed changes

IgorMinar removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Nov 19, 2020

IgorMinar added action: merge The PR is ready for merge by the caretaker target: minor This PR is targeted for the next minor release and removed breaking changes labels Nov 19, 2020

IgorMinar removed the target: major This PR is targeted for the next major release label Nov 19, 2020

IgorMinar removed the request for review from jelbourn November 19, 2020 23:45

AndrewKushnir added the action: presubmit The PR is in need of a google3 presubmit label Nov 20, 2020

AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label Nov 20, 2020

AndrewKushnir closed this in 358c50e Nov 23, 2020

AndrewKushnir pushed a commit that referenced this pull request Nov 23, 2020

feat(compiler): support error reporting in I18nMetaVisitor (#39554)

bb70a9b

Make it possible to report errors from the I18nMetaVisitor parser. PR Close #39554

angular-automatic-lock-bot bot locked and limited conversation to collaborators Dec 24, 2020

pullapprove bot removed the area: compiler Issues related to `ngc`, Angular's template compiler label Dec 24, 2020

ngbot bot removed this from the needsTriage milestone Dec 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(compiler): disallow i18n of security-sensitive attributes #39554

fix(compiler): disallow i18n of security-sensitive attributes #39554

bjarkler commented Nov 3, 2020 •

edited

AndrewKushnir left a comment

IgorMinar left a comment

mhevery left a comment

bjarkler commented Nov 19, 2020

mhevery commented Nov 19, 2020

IgorMinar left a comment

IgorMinar commented Nov 19, 2020

AndrewKushnir commented Nov 20, 2020

bjarkler commented Nov 20, 2020 •

edited

AndrewKushnir commented Nov 20, 2020

AndrewKushnir commented Nov 21, 2020

angular-automatic-lock-bot bot commented Dec 24, 2020

fix(compiler): disallow i18n of security-sensitive attributes #39554

fix(compiler): disallow i18n of security-sensitive attributes #39554

Conversation

bjarkler commented Nov 3, 2020 • edited

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

AndrewKushnir left a comment

Choose a reason for hiding this comment

IgorMinar left a comment

Choose a reason for hiding this comment

mhevery left a comment

Choose a reason for hiding this comment

bjarkler commented Nov 19, 2020

mhevery commented Nov 19, 2020

IgorMinar left a comment

Choose a reason for hiding this comment

IgorMinar commented Nov 19, 2020

AndrewKushnir commented Nov 20, 2020

bjarkler commented Nov 20, 2020 • edited

AndrewKushnir commented Nov 20, 2020

AndrewKushnir commented Nov 21, 2020

angular-automatic-lock-bot bot commented Dec 24, 2020

bjarkler commented Nov 3, 2020 •

edited

bjarkler commented Nov 20, 2020 •

edited