fix(core): sanitize text bindings on SVG <script> elements#68669
fix(core): sanitize text bindings on SVG <script> elements#68669SkyZeroZx wants to merge 2 commits into
Conversation
Property bindings that set the source of a `<script>` element (`innerHTML`,
`outerHTML`, `text`, `textContent`, `innerText`) were treated as plain HTML
or NONE security contexts, so untrusted values could become live script.
Top-level HTML `<script>` is stripped by the template parser, but SVG
`<svg><script>` survives (`mergeNsAndName('svg','script')` produces
`:svg:script`, which `preparseElement` does not strip).
Register these properties under `SecurityContext.SCRIPT` in the DOM security
schema. The existing `ɵɵsanitizeScript` runtime guard then throws NG0905
unless the value is wrapped via `DomSanitizer.bypassSecurityTrustScript`.
Fix angular#68642
| ]); | ||
| Object.entries(schema).forEach(([key, context]) => { | ||
| if (context === SecurityContext.URL || SecurityContext.RESOURCE_URL) { | ||
| if (context === SecurityContext.URL || context === SecurityContext.RESOURCE_URL) { |
There was a problem hiding this comment.
I found this while writing the unit tests (apparently it was always evaluated as true if I'm not mistaken)
| export function ɵɵsanitizeMaybeScript(unsafeValue: any, tag: string, prop: string): any { | ||
| if (tag.toLowerCase() === 'script') { | ||
| return ɵɵsanitizeScript(unsafeValue); | ||
| } | ||
| const lowerProp = prop.toLowerCase(); | ||
| if (lowerProp === 'innerhtml' || lowerProp === 'outerhtml') { | ||
| return ɵɵsanitizeHtml(unsafeValue); | ||
| } | ||
| return unsafeValue; | ||
| } |
There was a problem hiding this comment.
Do we really need that additional instruction ? Couldn't the compiler determine the right instruction at compile time ?
There was a problem hiding this comment.
This was added for host binding cases where we only know at runtime which tag can be used, the directive/component, as exemplified in the tests.
There was a problem hiding this comment.
There's also another similar report regarding host binding, but I believe it would be outside the scope of this PR, or I'm unsure if it's supposed to work this way.
| sanitizerFn = Identifiers.sanitizeUrlOrResourceUrl; | ||
| } else if ( | ||
| Array.isArray(op.securityContext) && | ||
| op.securityContext.length > 1 && |
There was a problem hiding this comment.
I don't believe there is a reason to do this check since we are just immediately doing the includes check, and we are verifying that there is any amount of items in the array.
Since includes operators in O(n), its actually more costly to do the length check being > 0 and then the includes, we should just do the includes directly.
There was a problem hiding this comment.
and we are verifying that there is any amount of items in the array
I may be missing something, but the test was for at least two security contexts. If the only applicable context is SCRIPT, then the dynamic runtime switch wouldn't be necessary, which was achieved by the > 1 check.
| // Writes to text-like properties of a `<script>` element become live script source. SVG | ||
| // `<script>` is preserved by the template parser (`mergeNsAndName('svg', 'script')` produces | ||
| // `:svg:script`, which `preparseElement` does not strip), so this matters at runtime even | ||
| // though top-level HTML `<script>` is stripped at compile time. The binding parser performs | ||
| // schema lookups using the local tag name (`script`). |
There was a problem hiding this comment.
Have we considered revisiting this? If an element with script tag within an SVG-namespaced tree behaves like a script tag—I am actually surprised it works this way, or is Angular just not creating the element with the SVG namespace?—then I'd expect we treat it equally to a script element outside of an SVG structure (or MathML namespaced tree).
For defence in depth it may be desirable to keep the new security contexts in place, but I don't see a reason to keep the ingestion pipeline as it is today if that doesn't accurately reflect how this behaves at runtime.
There was a problem hiding this comment.
I understand this behavior for SVGScriptElement and how it works according to the HTML spec. I think the Angular compiler follows the same browser behavior, although I may still have a gap in my understanding.
Regarding MathML, when placing content inside a <script> tag (tested locally), the script is not executed, it is treated as plain text instead. Also, to achieve this, we need to disable schema validation errors, since as I understand it, putting <script> inside MathML is not allowed by default.
This change is essentially a mirror of the existing URL sink behavior. Also, if we remove this host binding / binding handling, SVG script bindings will fail.
Test failed removing this
<!-- SVG <script> via host binding !-->
'<svg><script scriptHost></script></svg>'
<!-- Bindings !-->
`<svg><script [${propName}]="code"></script></svg>At runtime, I believe there are other issues that I'm not sure are desirable to report here https://issuetracker.google.com/u/1/issues/510537066
Or maybe it always worked this way (I can't find anything in the documentation or code that indicates this is how it should be, but maybe it is correct).
There was a problem hiding this comment.
I didn't even know SVGScriptElement is a thing!
What is the current behavior?
See #68642
And previous reporter https://issuetracker.google.com/u/1/issues/509941006
What is the new behavior?
Property bindings that set the source of a
<script>element (innerHTML,outerHTML,text,textContent,innerText) were treated as plain HTML or NONE security contexts, so untrusted values could become live script. Top-level HTML<script>is stripped by the template parser, but SVG<svg><script>survives (mergeNsAndName('svg','script')produces:svg:script, whichpreparseElementdoes not strip).Register these properties under
SecurityContext.SCRIPTin the DOM security schema. The existingɵɵsanitizeScriptruntime guard then throws NG0905 unless the value is wrapped viaDomSanitizer.bypassSecurityTrustScript.Fix #68642
Other information
I believe this would be classified with the same level of importance/severity: GHSA-jrmj-c5cx-3cw6