fix(core): block creation of sensitive URI attributes from ICU messages

Translators are not allowed to write HTML which creates URI attributes. I opted to ban any values going into an attribute at all, to prevent even links to malicious content, rather than just sanitizing URIs.

I also converted this blocklist into an allowlist. Now, we only allowing setting known attributes (while sanitizing URI attributes). This significantly reduces risk of missing a vulnerable attribute and does not require an exhaustive list of all potential attributes.

BREAKING CHANGE: Angular now only applies known attributes from HTML in translated ICU content. Unknown attributes are dropped and not rendered.

(cherry picked from commit 306f367899)

packages/core/src/render3/i18n/i18n_parse.ts

@@ -808,7 +808,6 @@ function walkIcuTree(
             const attr = elAttrs.item(i)!;
             const lowerAttrName = attr.name.toLowerCase();
             const hasBinding = !!attr.value.match(BINDING_REGEXP);
-            // we assume the input string is safe, unless it's using a binding
             if (hasBinding) {
               if (VALID_ATTRS.hasOwnProperty(lowerAttrName)) {
                 if (URI_ATTRS[lowerAttrName]) {
@@ -831,8 +830,29 @@ function walkIcuTree(
                       `(see ${XSS_SECURITY_URL})`,
                   );
               }
+            } else if (VALID_ATTRS[lowerAttrName]) {
+              if (URI_ATTRS[lowerAttrName]) {
+                // Don't sanitize, because no value is acceptable in sensitive attributes.
+                // Translators are not allowed to create URIs.
+                if (typeof ngDevMode !== 'undefined' && ngDevMode) {
+                  console.warn(
+                    `WARNING: ignoring unsafe attribute ` +
+                      `${lowerAttrName} on element ${tagName} ` +
+                      `(see ${XSS_SECURITY_URL})`,
+                  );
+                }
+                addCreateAttribute(create, newIndex, attr.name, 'unsafe:blocked');
+              } else {
+                addCreateAttribute(create, newIndex, attr.name, attr.value);
+              }
             } else {
-              addCreateAttribute(create, newIndex, attr);
+              if (typeof ngDevMode !== 'undefined' && ngDevMode) {
+                console.warn(
+                  `WARNING: ignoring unknown attribute name ` +
+                    `${lowerAttrName} on element ${tagName} ` +
+                    `(see ${XSS_SECURITY_URL})`,
+                );
+              }
             }
           }
           const elementNode: I18nElementNode = {
@@ -945,10 +965,11 @@ function addCreateNodeAndAppend(
   );
 }
 
-function addCreateAttribute(create: IcuCreateOpCodes, newIndex: number, attr: Attr) {
-  create.push(
-    (newIndex << IcuCreateOpCode.SHIFT_REF) | IcuCreateOpCode.Attr,
-    attr.name,
-    attr.value,
-  );
+function addCreateAttribute(
+  create: IcuCreateOpCodes,
+  newIndex: number,
+  attrName: string,
+  attrValue: string,
+) {
+  create.push((newIndex << IcuCreateOpCode.SHIFT_REF) | IcuCreateOpCode.Attr, attrName, attrValue);
 }

packages/core/test/acceptance/i18n_spec.ts

@@ -3434,7 +3434,7 @@ describe('runtime i18n', () => {
           {parameters.length, plural,
             =1 {
               Affects parameter
-              <span class="parameter-name" attr="should_be_present">{{ parameters[0].name }}</span>
+              <span class="parameter-name" label="should_be_present">{{ parameters[0].name }}</span>
             }
             other {
               Affects {{parameters.length}} parameters, including
@@ -3453,7 +3453,7 @@ describe('runtime i18n', () => {
     const fixture = TestBed.createComponent(MyApp);
     fixture.detectChanges();
     const span = (fixture.nativeElement as HTMLElement).querySelector('span')!;
-    expect(span.getAttribute('attr')).toEqual('should_be_present');
+    expect(span.getAttribute('label')).toEqual('should_be_present');
     expect(span.getAttribute('class')).toEqual('parameter-name');
   });
 

packages/core/test/render3/i18n/i18n_parse_spec.ts

@@ -297,6 +297,50 @@ describe('i18n_parse', () => {
         );
       });
     });
+
+    it('should properly sanitize malicious URLs like `<a href="evil.test">` injected into translations', () => {
+      const tI18n = toT18n(`{
+        <EFBFBD>0<EFBFBD>, select,
+          A {<a href="javascript:console.log('hacked!');">malicious JS</a>}
+          other {<a href="https://evil.test">malicious link</a>}
+      }`);
+
+      fixture.apply(() => {
+        applyCreateOpCodes(fixture.lView, tI18n.create, fixture.host, null);
+        expect(fixture.host.innerHTML).toEqual(`<!--ICU ${HEADER_OFFSET + 0}:0-->`);
+      });
+
+      fixture.apply(() => {
+        ɵɵi18nExp('A');
+        ɵɵi18nApply(0);
+        expect(fixture.host.innerHTML).toEqual(
+          `<a href="unsafe:blocked">malicious JS</a><!--ICU ${HEADER_OFFSET + 0}:0-->`,
+        );
+      });
+
+      fixture.apply(() => {
+        ɵɵi18nExp('other');
+        ɵɵi18nApply(0);
+        expect(fixture.host.innerHTML).toEqual(
+          `<a href="unsafe:blocked">malicious link</a><!--ICU ${HEADER_OFFSET + 0}:0-->`,
+        );
+      });
+    });
+
+    it('should ignore unknown attributes', () => {
+      const tI18n = toT18n(`{<7B>0<EFBFBD>, select, A {<div unknown="unknown"></div>} }`);
+
+      fixture.apply(() => {
+        applyCreateOpCodes(fixture.lView, tI18n.create, fixture.host, null);
+        expect(fixture.host.innerHTML).toEqual(`<!--ICU ${HEADER_OFFSET + 0}:0-->`);
+      });
+
+      fixture.apply(() => {
+        ɵɵi18nExp('A');
+        ɵɵi18nApply(0);
+        expect(fixture.host.innerHTML).toEqual(`<div></div><!--ICU ${HEADER_OFFSET + 0}:0-->`);
+      });
+    });
   });
 
   function toT18n(text: string) {