[DeviceSync v2] Base64-encode metadata/group-public-key strings in prefs

Debug builds are seeing crashes after successful SyncMetadata responses,
when serialized protos and public keys are being stored in prefs. These
strings will likely contain non-UTF-8 characters, which are disallowed
in pref values. In this CL, we base64-encode these values before
persisting to prefs.

We manually reproduced the crash without this patch and verified that
this patch fixes the crash. We also updated unit test strings to
include non-UTF-8 characters.

Fixed: 1150031
Change-Id: I81d6f937110c8b010ce3bed3cad7c38c0099a025
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2545765
Auto-Submit: Josh Nohle <nohle@chromium.org>
Commit-Queue: James Vecore <vecore@google.com>
Reviewed-by: James Vecore <vecore@google.com>
Cr-Commit-Position: refs/heads/master@{#828514}

chromeos/services/device_sync/cryptauth_metadata_syncer_impl.cc

@@ -377,18 +377,27 @@ void CryptAuthMetadataSyncerImpl::AttemptNextStep() {
 
 bool CryptAuthMetadataSyncerImpl::
     ShouldUseCachedEncryptedLocalDeviceMetadata() {
-  std::string last_synced_unencrypted_metadata = pref_service_->GetString(
-      prefs::kCryptAuthLastSyncedUnencryptedLocalDeviceMetadata);
-  std::string last_synced_group_public_key =
-      pref_service_->GetString(prefs::kCryptAuthLastSyncedGroupPublicKey);
+  base::Optional<std::string> last_synced_unencrypted_metadata =
+      util::DecodeFromString(pref_service_->GetString(
+          prefs::kCryptAuthLastSyncedUnencryptedLocalDeviceMetadata));
+  base::Optional<std::string> last_synced_group_public_key =
+      util::DecodeFromString(
+          pref_service_->GetString(prefs::kCryptAuthLastSyncedGroupPublicKey));
+  base::Optional<std::string> last_synced_encrypted_metadata =
+      util::DecodeFromString(pref_service_->GetString(
+          prefs::kCryptAuthLastSyncedEncryptedLocalDeviceMetadata));
+
+  // Persisted values are not encoded properly.
+  if (!last_synced_unencrypted_metadata || !last_synced_group_public_key ||
+      !last_synced_encrypted_metadata) {
+    return false;
+  }
 
   // Prefs should be all set or all unset.
   DCHECK_EQ(last_synced_unencrypted_metadata == kUnsetPrefValue,
             last_synced_group_public_key == kUnsetPrefValue);
   DCHECK_EQ(last_synced_unencrypted_metadata == kUnsetPrefValue,
-            pref_service_->GetString(
-                prefs::kCryptAuthLastSyncedEncryptedLocalDeviceMetadata) ==
-                kUnsetPrefValue);
+            last_synced_encrypted_metadata == kUnsetPrefValue);
 
   if (last_synced_unencrypted_metadata == kUnsetPrefValue)
     return false;
@@ -402,8 +411,9 @@ void CryptAuthMetadataSyncerImpl::EncryptLocalDeviceMetadata() {
   SetState(State::kWaitingForLocalDeviceMetadataEncryption);
 
   if (ShouldUseCachedEncryptedLocalDeviceMetadata()) {
-    OnLocalDeviceMetadataEncrypted(pref_service_->GetString(
-        prefs::kCryptAuthLastSyncedEncryptedLocalDeviceMetadata));
+    OnLocalDeviceMetadataEncrypted(
+        *util::DecodeFromString(pref_service_->GetString(
+            prefs::kCryptAuthLastSyncedEncryptedLocalDeviceMetadata)));
     return;
   }
 
@@ -532,12 +542,12 @@ void CryptAuthMetadataSyncerImpl::OnSyncMetadataSuccess(
   // the key returned in the respone.
   pref_service_->SetString(
       prefs::kCryptAuthLastSyncedUnencryptedLocalDeviceMetadata,
-      local_device_metadata_.SerializeAsString());
+      util::EncodeAsString(local_device_metadata_.SerializeAsString()));
   pref_service_->SetString(prefs::kCryptAuthLastSyncedGroupPublicKey,
-                           GetGroupKey()->public_key());
+                           util::EncodeAsString(GetGroupKey()->public_key()));
   pref_service_->SetString(
       prefs::kCryptAuthLastSyncedEncryptedLocalDeviceMetadata,
-      *encrypted_local_device_metadata_);
+      util::EncodeAsString(*encrypted_local_device_metadata_));
 
   sync_metadata_response_ = response;
 

chromeos/services/device_sync/cryptauth_metadata_syncer_impl_unittest.cc

@@ -33,6 +33,7 @@
 #include "chromeos/services/device_sync/proto/cryptauth_devicesync.pb.h"
 #include "chromeos/services/device_sync/proto/cryptauth_directive.pb.h"
 #include "chromeos/services/device_sync/proto/cryptauth_v2_test_util.h"
+#include "chromeos/services/device_sync/value_string_encoding.h"
 #include "components/prefs/testing_pref_service.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -42,7 +43,9 @@ namespace device_sync {
 
 namespace {
 
-const char kStaleGroupPublicKey[] = "stale_group_public_key";
+// Note: Include a non-UTF-8 character to ensure that we handle them correctly
+// in prefs.
+const char kStaleGroupPublicKey[] = "stale_group_public_key\xff";
 
 const char kAccessTokenUsed[] = "access token used by CryptAuthClient";
 
@@ -118,13 +121,13 @@ class DeviceSyncCryptAuthMetadataSyncerImplTest
       const std::string& group_public_key) {
     pref_service_.SetString(
         prefs::kCryptAuthLastSyncedUnencryptedLocalDeviceMetadata,
-        unencrypted_local_device_metadata);
+        util::EncodeAsString(unencrypted_local_device_metadata));
     pref_service_.SetString(prefs::kCryptAuthLastSyncedGroupPublicKey,
-                            group_public_key);
+                            util::EncodeAsString(group_public_key));
     pref_service_.SetString(
         prefs::kCryptAuthLastSyncedEncryptedLocalDeviceMetadata,
-        MakeFakeEncryptedString(unencrypted_local_device_metadata,
-                                group_public_key));
+        util::EncodeAsString(MakeFakeEncryptedString(
+            unencrypted_local_device_metadata, group_public_key)));
   }
 
   void SyncMetadata(
@@ -402,14 +405,14 @@ class DeviceSyncCryptAuthMetadataSyncerImplTest
       const std::string& expected_unencrypted_metadata,
       const std::string& expected_group_public_key) {
     EXPECT_EQ(expected_encrypted_metadata,
-              pref_service_.GetString(
-                  prefs::kCryptAuthLastSyncedEncryptedLocalDeviceMetadata));
+              util::DecodeFromString(pref_service_.GetString(
+                  prefs::kCryptAuthLastSyncedEncryptedLocalDeviceMetadata)));
     EXPECT_EQ(expected_unencrypted_metadata,
-              pref_service_.GetString(
-                  prefs::kCryptAuthLastSyncedUnencryptedLocalDeviceMetadata));
-    EXPECT_EQ(
-        expected_group_public_key,
-        pref_service_.GetString(prefs::kCryptAuthLastSyncedGroupPublicKey));
+              util::DecodeFromString(pref_service_.GetString(
+                  prefs::kCryptAuthLastSyncedUnencryptedLocalDeviceMetadata)));
+    EXPECT_EQ(expected_group_public_key,
+              util::DecodeFromString(pref_service_.GetString(
+                  prefs::kCryptAuthLastSyncedGroupPublicKey)));
   }
 
   void OnMetadataSyncComplete(
@@ -599,8 +602,9 @@ TEST_F(
       GetPrivateKeyFromPublicKeyForTest(group_public_key);
 
   // Say the local device metadata was already encrypted with the current group
-  // key but the device metadata is stale.
-  SetLocalDeviceMetadataPrefs("old_metadata", group_public_key);
+  // key but the device metadata is stale. Note: Include a non-UTF-8 character
+  // to ensure that we handle them correctly in prefs.
+  SetLocalDeviceMetadataPrefs("old_metadata\xC0", group_public_key);
 
   SyncMetadata(group_public_key, group_private_key);
 
@@ -632,11 +636,12 @@ TEST_F(
       GetPrivateKeyFromPublicKeyForTest(group_public_key);
 
   // Say the local device metadata was already encrypted with the current device
-  // metadata but a stale group public key.
+  // metadata but a stale group public key. Note: Include a non-UTF-8 character
+  // to ensure that we handle them correctly in prefs.
   SetLocalDeviceMetadataPrefs(
       GetLocalDeviceForTest()
           .better_together_device_metadata->SerializeAsString(),
-      "old_group_public_key");
+      "old_group_public_key\xC1");
 
   SyncMetadata(group_public_key, group_private_key);