webauthn: randomly permute the clientDataJSON.

I fear that if we don't do this, we may not be able to add things in the
future when we need to.

Bug: none
Change-Id: I94c49b8a5a336051ae7a62928fc48e4300649b19
Reviewed-on: https://chromium-review.googlesource.com/895392
Commit-Queue: Adam Langley <agl@chromium.org>
Reviewed-by: Balazs Engedy <engedy@chromium.org>
Reviewed-by: Kim Paulhamus <kpaulhamus@chromium.org>
Cr-Commit-Position: refs/heads/master@{#534700}

content/browser/webauth/authenticator_impl_unittest.cc

@@ -9,6 +9,8 @@
 #include <utility>
 #include <vector>
 
+#include "base/json/json_parser.h"
+#include "base/json/json_writer.h"
 #include "base/logging.h"
 #include "base/run_loop.h"
 #include "base/test/gtest_util.h"
@@ -373,15 +375,45 @@ TEST_F(AuthenticatorImplTest, MakeCredentialNoSupportedAlgorithm) {
             cb.GetResponseStatus());
 }
 
+// Parses its arguments as JSON and expects that all the keys in the first are
+// also in the second, and with the same value.
+void CheckJSONIsSubsetOfJSON(base::StringPiece subset_str,
+                             base::StringPiece test_str) {
+  std::unique_ptr<base::Value> subset(base::JSONReader::Read(subset_str));
+  ASSERT_TRUE(subset);
+  ASSERT_TRUE(subset->is_dict());
+  std::unique_ptr<base::Value> test(base::JSONReader::Read(test_str));
+  ASSERT_TRUE(test);
+  ASSERT_TRUE(test->is_dict());
+
+  for (const auto& item : subset->DictItems()) {
+    base::Value* test_value = test->FindKey(item.first);
+    if (test_value == nullptr) {
+      ADD_FAILURE() << item.first << " does not exist in the test dictionary";
+      continue;
+    }
+
+    if (!item.second.Equals(test_value)) {
+      std::string want, got;
+      ASSERT_TRUE(base::JSONWriter::Write(item.second, &want));
+      ASSERT_TRUE(base::JSONWriter::Write(*test_value, &got));
+      ADD_FAILURE() << "Value of " << item.first << " is unequal: want " << want
+                    << " got " << got;
+    }
+  }
+}
+
 // Test that client data serializes to JSON properly.
 TEST_F(AuthenticatorImplTest, TestSerializedRegisterClientData) {
-  EXPECT_EQ(kTestRegisterClientDataJsonString,
-            GetTestClientData(client_data::kCreateType).SerializeToJson());
+  CheckJSONIsSubsetOfJSON(
+      kTestRegisterClientDataJsonString,
+      GetTestClientData(client_data::kCreateType).SerializeToJson());
 }
 
 TEST_F(AuthenticatorImplTest, TestSerializedSignClientData) {
-  EXPECT_EQ(kTestSignClientDataJsonString,
-            GetTestClientData(client_data::kGetType).SerializeToJson());
+  CheckJSONIsSubsetOfJSON(
+      kTestSignClientDataJsonString,
+      GetTestClientData(client_data::kGetType).SerializeToJson());
 }
 
 TEST_F(AuthenticatorImplTest, TestMakeCredentialTimeout) {

content/browser/webauth/client_data_json.md

+# Advice to sites regarding `clientDataJSON`
+
+When performing a registration or authentication with webauthn, it's critical that sites confirm that the returned [`clientDataJSON`](https://w3c.github.io/webauthn/#dom-authenticatorresponse-clientdatajson) contains the [challenge](https://w3c.github.io/webauthn/#cryptographic-challenges) originally provided. Otherwise old messages can be replayed. Likewise, sites must check the `origin` member to confirm that the action is not being proxied by another site.
+
+In order to implement this, sites should parse the JSON as a [`CollectedClientData`](https://w3c.github.io/webauthn/#dictdef-collectedclientdata) structure and confirm that the `type`, `challenge`, and `origin` members (at least) are as expected.
+
+Sites should _not_ implement this by comparing the unparsed value of `clientDataJSON` against a template with the `challenge` value filled in. This would fail when new members are added to `CollectedClientData` in the future as the template would no longer be correct.
+
+In order to guide sites away from doing this, Chromium will sometimes, randomly insert an extra member into `clientDataJSON` which references this documentation.

content/browser/webauth/collected_client_data.cc

@@ -8,6 +8,7 @@
 
 #include "base/base64url.h"
 #include "base/json/json_writer.h"
+#include "base/rand_util.h"
 #include "base/strings/string_piece.h"
 #include "base/values.h"
 
@@ -67,6 +68,16 @@ std::string CollectedClientData::SerializeToJson() const {
   client_data.SetKey(kTypeKey, base::Value(type_));
   client_data.SetKey(kChallengeKey, base::Value(base64_encoded_challenge_));
 
+  if (base::RandDouble() < 0.2) {
+    // An extra key is sometimes added to ensure that RPs do not make
+    // unreasonably specific assumptions about the clientData JSON. This is
+    // done in the fashion of
+    // https://tools.ietf.org/html/draft-davidben-tls-grease-01
+    client_data.SetKey("new_keys_may_be_added_here",
+                       base::Value("do not compare clientDataJSON against a "
+                                   "template. See https://goo.gl/yabPex"));
+  }
+
   // The serialization of callerOrigin.
   client_data.SetKey(kOriginKey, base::Value(origin_));