Bug 1551282 and bug 1553436. Allow pages to override window.u2f but not the "sign" and "register" properties on the U2F object. r=jcj,smaug

There are two related problems this patch is trying to address.  The first, and
simpler, one is bug 1553436: there are websites that use existing variables and
functions named "u2f" and adding a non-replaceable readonly property with that
name on Window breaks them.  The fix for this is straightforward: mark the
property [Replaceable].

The second problem, covered by bug 1551282, involves sites that use the Google
U2F polyfill.  The relevant parts of that polyfill look like this:

  'use strict';
  var u2f = u2f || {};
  u2f.register = some_function_that_only_works_right_in_Chrome;
  u2f.sign = some_function_that_only_works_right_in_Chrome;

The failure mode for that code before this fix is that the assignment to "u2f"
throws because it's a readonly property and we're in strict mode, so any code
the page concatenates in the same file after the polyfill does not get run.
That's what bug 1551282 is about.  The [Replaceable] annotation fixes that
issue, because now the polyfill gets the value of window.u2f and then redefines
the property (via the [Replaceable] setter) to be a value property with that
value.  So far, so good.

But then we need to prevent the sets of u2f.register
and u2f.sign from taking effect, because if they are allowed to happen, the
actual sign/register functionality on the page will not work in Firefox.  We
can't just make the properties readonly, because then the sets will throw due
to being in strict mode, and we still have bug 1551282.  The proposed fix is to
make these accessor properties with a no-op setter, which is exactly what
[LenientSetter] gives us.

The rest of the patch is just setting up infrastructure for generating the
normal bits we would generate if "sign" and "register" were methods and using
that to create the JSFunctions at the point when the getter is called.  The
JSFunctions then get cached on the u2f instance object.

Differential Revision: https://phabricator.services.mozilla.com/D32357

--HG--
extra : moz-landing-system : lando

dom/bindings/Codegen.py

@@ -2361,7 +2361,8 @@ class MethodDefiner(PropertyDefiner):
                        m.isMethod() and m.isStatic() == static and
                        MemberIsUnforgeable(m, descriptor) == unforgeable and
                        (not crossOriginOnly or m.getExtendedAttribute("CrossOriginCallable")) and
-                       not m.isIdentifierLess()]
+                       not m.isIdentifierLess() and
+                       not m.getExtendedAttribute("Unexposed")]
         else:
             methods = []
         self.chrome = []
@@ -9643,11 +9644,19 @@ class CGMemberJITInfo(CGThing):
                 argTypes=argTypes,
                 slotAssert=slotAssert)
 
+        # Unexposed things are meant to be used from C++ directly, so we make
+        # their jitinfo non-static.  That way C++ can get at it.
+        if self.member.getExtendedAttribute("Unexposed"):
+            storageClass = "extern"
+        else:
+            storageClass = "static"
+
         return fill(
             """
-            static const JSJitInfo ${infoName} = ${jitInfo};
+            ${storageClass} const JSJitInfo ${infoName} = ${jitInfo};
             $*{slotAssert}
             """,
+            storageClass=storageClass,
             infoName=infoName,
             jitInfo=jitInfoInitializer(False),
             slotAssert=slotAssert)

dom/bindings/parser/WebIDL.py

@@ -5271,7 +5271,8 @@ class IDLMethod(IDLInterfaceMember, IDLScope):
               identifier == "NeedsSubjectPrincipal" or
               identifier == "NeedsCallerType" or
               identifier == "StaticClassOverride" or
-              identifier == "NonEnumerable"):
+              identifier == "NonEnumerable" or
+              identifier == "Unexposed"):
             # Known attributes that we don't need to do anything with here
             pass
         else:

dom/u2f/U2F.cpp

@@ -294,6 +294,39 @@ void U2F::Register(const nsAString& aAppId,
   mChild->SendRequestRegister(mTransaction.ref().mId, info);
 }
 
+using binding_detail::GenericMethod;
+using binding_detail::NormalThisPolicy;
+using binding_detail::ThrowExceptions;
+
+// register_impl_methodinfo is generated by bindings.
+namespace U2F_Binding {
+extern const JSJitInfo register_impl_methodinfo;
+}  // namespace U2F_Binding
+
+// We have 4 non-optional args.
+static const JSFunctionSpec register_spec = JS_FNSPEC(
+    "register", (GenericMethod<NormalThisPolicy, ThrowExceptions>),
+    &U2F_Binding::register_impl_methodinfo, 4, JSPROP_ENUMERATE, nullptr);
+
+void U2F::GetRegister(JSContext* aCx,
+                      JS::MutableHandle<JSObject*> aRegisterFunc,
+                      ErrorResult& aRv) {
+  JS::Rooted<JSString*> str(aCx, JS_AtomizeAndPinString(aCx, "register"));
+  if (!str) {
+    aRv.NoteJSContextException(aCx);
+    return;
+  }
+
+  JS::Rooted<jsid> id(aCx, INTERNED_STRING_TO_JSID(aCx, str));
+  JSFunction* fun = JS::NewFunctionFromSpec(aCx, &register_spec, id);
+  if (!fun) {
+    aRv.NoteJSContextException(aCx);
+    return;
+  }
+
+  aRegisterFunc.set(JS_GetFunctionObject(fun));
+}
+
 void U2F::FinishMakeCredential(const uint64_t& aTransactionId,
                                const WebAuthnMakeCredentialResult& aResult) {
   MOZ_ASSERT(NS_IsMainThread());
@@ -454,6 +487,34 @@ void U2F::Sign(const nsAString& aAppId, const nsAString& aChallenge,
   mChild->SendRequestSign(mTransaction.ref().mId, info);
 }
 
+// sign_impl_methodinfo is generated by bindings.
+namespace U2F_Binding {
+extern const JSJitInfo sign_impl_methodinfo;
+}  // namespace U2F_Binding
+
+// We have 4 non-optional args.
+static const JSFunctionSpec sign_spec =
+    JS_FNSPEC("sign", (GenericMethod<NormalThisPolicy, ThrowExceptions>),
+              &U2F_Binding::sign_impl_methodinfo, 4, JSPROP_ENUMERATE, nullptr);
+
+void U2F::GetSign(JSContext* aCx, JS::MutableHandle<JSObject*> aSignFunc,
+                  ErrorResult& aRv) {
+  JS::Rooted<JSString*> str(aCx, JS_AtomizeAndPinString(aCx, "sign"));
+  if (!str) {
+    aRv.NoteJSContextException(aCx);
+    return;
+  }
+
+  JS::Rooted<jsid> id(aCx, INTERNED_STRING_TO_JSID(aCx, str));
+  JSFunction* fun = JS::NewFunctionFromSpec(aCx, &sign_spec, id);
+  if (!fun) {
+    aRv.NoteJSContextException(aCx);
+    return;
+  }
+
+  aSignFunc.set(JS_GetFunctionObject(fun));
+}
+
 void U2F::FinishGetAssertion(const uint64_t& aTransactionId,
                              const WebAuthnGetAssertionResult& aResult) {
   MOZ_ASSERT(NS_IsMainThread());

dom/u2f/U2F.h

@@ -105,6 +105,9 @@ class U2F final : public WebAuthnManagerBase, public nsWrapperCache {
                 const Optional<Nullable<int32_t>>& opt_aTimeoutSeconds,
                 ErrorResult& aRv);
 
+  void GetRegister(JSContext* aCx, JS::MutableHandle<JSObject*> aRegisterFunc,
+                   ErrorResult& aRv);
+
   MOZ_CAN_RUN_SCRIPT
   void Sign(const nsAString& aAppId, const nsAString& aChallenge,
             const Sequence<RegisteredKey>& aRegisteredKeys,
@@ -112,6 +115,9 @@ class U2F final : public WebAuthnManagerBase, public nsWrapperCache {
             const Optional<Nullable<int32_t>>& opt_aTimeoutSeconds,
             ErrorResult& aRv);
 
+  void GetSign(JSContext* aCx, JS::MutableHandle<JSObject*> aSignFunc,
+               ErrorResult& aRv);
+
   // WebAuthnManagerBase
 
   MOZ_CAN_RUN_SCRIPT

dom/u2f/tests/mochitest.ini

@@ -20,6 +20,9 @@ scheme = https
 # Feature does not function without e10s (Disabled in Bug 1297552)
 skip-if = !e10s
 
+[test_bind.html]
+[test_polyfill_interaction.html]
+[test_u2f_replaceable.html]
 [test_util_methods.html]
 [test_no_token.html]
 [test_register.html]

dom/u2f/tests/test_bind.html

@@ -0,0 +1,31 @@
+<!DOCTYPE HTML>
+<html>
+<head>
+  <meta charset="utf-8">
+  <title>Test that bind() can be called on u2f.register/sign</title>
+  <script src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" href="/tests/SimpleTest/test.css"/>
+  <script>
+    SimpleTest.waitForExplicitFinish();
+    SpecialPowers.pushPrefEnv({ "set": [["security.webauth.u2f", true]] },
+                              doTest);
+    function doTest() {
+        // For some reason, exceptions from inside here do not cause a test
+        // failure!  Run the actual code we want off something that will report
+        // errors properly.
+        SimpleTest.executeSoon(function() {
+            is(typeof(u2f.register.bind(u2f)), "function",
+               "Should be able to bind u2f.register");
+            is(typeof(u2f.sign.bind(u2f)), "function",
+               "Should be able to bind u2f.sign");
+            SimpleTest.finish();
+        });
+    }
+  </script>
+</head>
+<body>
+<p id="display"></p>
+<div id="content" style="display: none"></div>
+<pre id="test"></pre>
+</body>
+</html>

dom/u2f/tests/test_polyfill_interaction.html

@@ -0,0 +1,40 @@
+<!DOCTYPE HTML>
+<html>
+<head>
+  <meta charset="utf-8">
+  <title>Test that our U2F implementation interacts OK with the Google polyfill</title>
+  <script src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" href="/tests/SimpleTest/test.css"/>
+  <script>
+    // We want to run our test script at toplevel, because that's where the
+    // polyfill runs.  But we also need to wait until the pref is set, so go
+    // ahead and use a dynamically created script element.
+    SimpleTest.waitForExplicitFinish();
+    SpecialPowers.pushPrefEnv({ "set": [["security.webauth.u2f", true]] },
+                              doTest);
+
+    function doTest() {
+        var s = document.createElement("script");
+        s.textContent = `
+          'use strict';
+          var savedU2f = u2f;
+          var savedRegister = savedU2f.register;
+          var savedSign = savedU2f.sign;
+          var u2f = u2f || {};
+          u2f.register = function() {};
+          u2f.sign = function() {};
+          is(u2f, savedU2f, "Should still have the right object");
+          is(u2f.register, savedRegister, "Should not allow overriding 'sign'");
+          is(u2f.sign, savedSign, "Should not allow overriding 'sign'");
+          SimpleTest.finish();
+        `;
+        document.documentElement.appendChild(s);
+    }
+  </script>
+</head>
+<body>
+<p id="display"></p>
+<div id="content" style="display: none"></div>
+<pre id="test"></pre>
+</body>
+</html>

dom/u2f/tests/test_u2f_replaceable.html

@@ -0,0 +1,20 @@
+<!DOCTYPE HTML>
+<html>
+<head>
+  <meta charset="utf-8">
+  <title>Make sure that pages can define a variable named "u2f" and have it work</title>
+  <script src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" href="/tests/SimpleTest/test.css"/>
+  <script>
+    var u2f = 5;
+    is(u2f, 5, "Should have allowed reassignment");
+    is(Object.getOwnPropertyDescriptor(window, "u2f").value, 5,
+       "Should be a value property");
+  </script>
+</head>
+<body>
+<p id="display"></p>
+<div id="content" style="display: none"></div>
+<pre id="test"></pre>
+</body>
+</html>

dom/webidl/U2F.webidl

@@ -11,7 +11,7 @@
 
 [NoInterfaceObject]
 interface GlobalU2F {
-  [SecureContext, Throws, Pref="security.webauth.u2f"]
+  [SecureContext, Throws, Pref="security.webauth.u2f", Replaceable]
   readonly attribute U2F u2f;
 };
 
@@ -79,17 +79,29 @@ interface U2F {
   const unsigned short DEVICE_INELIGIBLE = 4;
   const unsigned short TIMEOUT = 5;
 
-  [Throws]
-  void register (DOMString appId,
-                 sequence<RegisterRequest> registerRequests,
-                 sequence<RegisteredKey> registeredKeys,
-                 U2FRegisterCallback callback,
-                 optional long? opt_timeoutSeconds);
+  // Returns a Function.  It's readonly + [LenientSetter] to keep the Google
+  // U2F polyfill from stomping on the value.
+  [LenientSetter, Pure, Cached, Throws]
+  readonly attribute object register;
 
-  [Throws]
-  void sign (DOMString appId,
-             DOMString challenge,
-             sequence<RegisteredKey> registeredKeys,
-             U2FSignCallback callback,
-             optional long? opt_timeoutSeconds);
+  // A way to generate the actual implementation of register()
+  [Unexposed, Throws, BinaryName="Register"]
+  void register_impl(DOMString appId,
+                     sequence<RegisterRequest> registerRequests,
+                     sequence<RegisteredKey> registeredKeys,
+                     U2FRegisterCallback callback,
+                     optional long? opt_timeoutSeconds);
+
+  // Returns a Function.  It's readonly + [LenientSetter] to keep the Google
+  // U2F polyfill from stomping on the value.
+  [LenientSetter, Pure, Cached, Throws]
+  readonly attribute object sign;
+
+  // A way to generate the actual implementation of sign()
+  [Unexposed, Throws, BinaryName="Sign"]
+  void sign_impl (DOMString appId,
+                  DOMString challenge,
+                  sequence<RegisteredKey> registeredKeys,
+                  U2FSignCallback callback,
+                  optional long? opt_timeoutSeconds);
 };