1333810 - Encrypt record deletes

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P2)

Description

[Ethan Glasser-Camp (:glasserc)]

Right now, when we delete a record in chrome.storage.sync, this eventually turns into a Kinto DELETE call. Anyone with the proper FxA credentials can see that this record has been deleted, even if they don't have the kB they would need to decrypt the record had it not been deleted. Instead, we should send to Kinto a record that looks like any other record, but when decrypted, is a tombstone (telling the client that it needs to be deleted).

Comment 1

[Ethan Glasser-Camp (:glasserc)]

Attachment: Bug 1333810: Update kinto.js version to 8.0.0,

This allows us to transform deletes. See
https://github.com/Kinto/kinto.js/pull/640/.

Review commit: https://reviewboard.mozilla.org/r/107916/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/107916/

Comment 2

[Ethan Glasser-Camp (:glasserc)]

Attachment: Bug 1333810: Encrypt record deletes,

Camouflage deletes as updates, according to the rules given by the new
RemoteTransformer contract.

Review commit: https://reviewboard.mozilla.org/r/107918/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/107918/

Comment 3

[Matthew N. [:MattN]]

Comment on attachment 8831364 [details]
Bug 1333810: Update kinto.js version to 8.0.0,

https://reviewboard.mozilla.org/r/107916/#review109470

Comment 4

[Ethan Glasser-Camp (:glasserc)]

Attachment: Bug 1333810: Add salts to the keys record,

Because these need to be encrypted with kB like the keyring does, and
wiped at the same time as the keyring does, just smush them into the
keyring. Rename `ensureKeysFor` to `ensureCanSync`, but don't update
it to return the salts -- for now, we don't need that.

Review commit: https://reviewboard.mozilla.org/r/108382/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/108382/

Comment 5

[Ethan Glasser-Camp (:glasserc)]

Attachment: Bug 1333810: Redo hashing of collection ID,

This ties the collection ID used for a given extension to the "salts"
record. This blocks an attack where, given the user's ID, an attacker
could just hash all the extension IDs on addons.mozilla.org to
generate possible hashed collection IDs.

Review commit: https://reviewboard.mozilla.org/r/108384/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/108384/

Comment 6

[Ethan Glasser-Camp (:glasserc)]

I wasn't sure exactly what to do to update some of the tests in that last commit, and some of the changes I settled on are kind of ugly. I'd be open to alternatives..

Comment 7

[Ethan Glasser-Camp (:glasserc)]

Attachment: Bug 1333810: Hash record IDs during encryption,

This does a sha256 of record IDs, the same way we do for collection
IDs, during encryption.

The way we compute the new ID (using an overridden method) is a little
bit of a hack, but we use the new ID as part of the HMAC. This also
invalidates a previous assumption, which is that we kept record IDs
the same during decryption.

Review commit: https://reviewboard.mozilla.org/r/108482/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/108482/

Comment 8

[Ethan Glasser-Camp (:glasserc)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c5c115baff4a

Comment 9

[Richard Newman [:rnewman]]

Comment on attachment 8831365 [details]
Bug 1333810: Encrypt record deletes,

https://reviewboard.mozilla.org/r/107918/#review109734

::: services/sync/modules/engines/extension-storage.js:138
(Diff revision 1)
>        if (!record.id) {
>          throw new Error("Record ID is missing or invalid");
>        }
>  
>        let IV = Svc.Crypto.generateRandomIV();
>        let ciphertext = Svc.Crypto.encrypt(JSON.stringify(record),

So to make sure I'm understanding correctly, `record` here has `_status: "deleted"`, and so that ends up in the ciphertext. Kinto will now simply encode the deleted record as if it were any other kind, running through this code path.

Comment 10

[Kris Maglione [:kmag]]

Comment on attachment 8831365 [details]
Bug 1333810: Encrypt record deletes,

https://reviewboard.mozilla.org/r/107918/#review109742

r=me for the test changes. I'll defer to rnewman on the rest.

Comment 11

[Kris Maglione [:kmag]]

Comment on attachment 8831887 [details]
Bug 1333810: Add salts to the keys record,

https://reviewboard.mozilla.org/r/108382/#review109744

::: toolkit/components/extensions/ExtensionStorageSync.jsm:545
(Diff revision 1)
>      });
>    }),
>  
> +  ensureSaltsFor: Task.async(function* (keysRecord, extIds) {
> +    const newSalts = Object.assign({}, keysRecord.salts);
> +    for (let c of extIds) {

Nit: Please don't use a 1-letter variable name for this.

::: toolkit/components/extensions/test/xpcshell/test_ext_storage_sync.js:582
(Diff revision 1)
>  
>        // Another client generates a key for extensionId2
>        const newKey = new BulkKeyBundle(extensionId2);
>        newKey.generateRandom();
>        keysData.collections[extensionId2] = newKey.keyPairB64;
> +      saltData[extensionId2] = getRandomSalt();

Should we test with only the key, or only the salt, changing? I know that shouldn't happen, but should we make sure we handle it if it does?

Comment 12

[Kris Maglione [:kmag]]

Comment on attachment 8831888 [details]
Bug 1333810: Redo hashing of collection ID,

https://reviewboard.mozilla.org/r/108384/#review109746

::: toolkit/components/extensions/ExtensionStorageSync.jsm:30
(Diff revision 1)
>  const STORAGE_SYNC_SERVER_URL_PREF = "webextensions.storage.sync.serverURL";
>  const STORAGE_SYNC_SCOPE = "sync:addon_storage";
>  const STORAGE_SYNC_CRYPTO_COLLECTION_NAME = "storage-sync-crypto";
>  const STORAGE_SYNC_CRYPTO_KEYRING_RECORD_ID = "keys";
>  const STORAGE_SYNC_CRYPTO_SALT_LENGTH = 128;
> +this.STORAGE_SYNC_CRYPTO_SALT_LENGTH = STORAGE_SYNC_CRYPTO_SALT_LENGTH;

Please don't do this. If this needs to be exported, please add it to an object we're already exporting.

::: toolkit/components/extensions/ExtensionStorageSync.jsm:262
(Diff revision 1)
> +      const salts = yield this.getSalts();
> +      const salt = salts && salts[extensionId];
> +      if (!salt) {
> +        // This should never happen; salts should be populated before
> +        // we need them by ensureCanSync.
> +        throw new ExtensionUtils.ExtensionError(`no salt available for ${extensionId}; how did this happen?`);

This is really an internal error. We should throw a normal error here, and let the unexpected internal error exception be reported to the exception.

::: toolkit/components/extensions/ExtensionStorageSync.jsm:264
(Diff revision 1)
> +      if (!salt) {
> +        // This should never happen; salts should be populated before
> +        // we need them by ensureCanSync.
> +        throw new ExtensionUtils.ExtensionError(`no salt available for ${extensionId}; how did this happen?`);
> +      }
> +      return CryptoUtils.sha256(salt + ":" + extensionId);

Nit: Please use template strings.

And, it probably doesn't matter, but can we use something like `\x00` or `\n` rather than `:` for the separator?

::: toolkit/components/extensions/test/xpcshell/test_ext_storage_sync.js:486
(Diff revision 1)
> +  const salt = "IÈ1ñ\x12|cJñ0aW\x02µ\")æ°\x95o´Ì\x9C\x85\x9AÔþº" +
> +        "=¨úYÚøÏb1\x9C{³êÌØ\x064pË|\x12Ë\x1Fò\x94·\rË4$@ï\\ª\x9A" +
> +        "\x90\x198'P±\n\x87K¦-¿x\x12Ló.̲\x08\x7FP\x8Ay\x18\x96°" +
> +        "ú\x13Úå\x93\x9DþÃÁÏÎ0x\xAD\xAD¿\x90\xA0\x9D\x94þët$¬©È:ç\x94v\x04¨ÅàNû";

Nit: Can you please base64 this and use `atob(...)` to decode?

Comment 13

[Kris Maglione [:kmag]]

Comment on attachment 8832125 [details]
Bug 1333810: Hash record IDs during encryption,

https://reviewboard.mozilla.org/r/108482/#review109748

::: toolkit/components/extensions/test/xpcshell/test_ext_storage_sync.js:970
(Diff revision 1)
>        // FIXME: Keys were pushed in a previous test
>        equal(posts.length, 1,
>              "pushing a value should cause a post to the server");
>        const post = posts[0];
>        assertPostedNewRecord(post);
> -      equal(post.path, collectionRecordsPath(collectionId) + "/key-my_2D_key",
> +      equal(post.path, collectionRecordsPath(collectionId) + "/" + hashedId,

Nit: Template string please.

::: toolkit/components/extensions/test/xpcshell/test_ext_storage_sync.js:1001
(Diff revision 1)
>        posts = server.getPosts();
>        equal(posts.length, 2,
>              "updating a value should trigger another push");
>        const updatePost = posts[1];
>        assertPostedUpdatedRecord(updatePost, 1000);
> -      equal(updatePost.path, collectionRecordsPath(collectionId) + "/key-my_2D_key",
> +      equal(updatePost.path, collectionRecordsPath(collectionId) + "/" + hashedId,

And here.

::: toolkit/components/extensions/test/xpcshell/test_ext_storage_sync.js:1099
(Diff revision 1)
>        posts = server.getPosts();
>        equal(posts.length, 3,
>              "deleting a value should trigger another push");
>        const post = posts[2];
>        assertPostedUpdatedRecord(post, 1000);
> -      equal(post.path, collectionRecordsPath(collectionId) + "/key-my_2D_key",
> +      equal(post.path, collectionRecordsPath(collectionId) + "/" + hashedId,

And here.

Comment 14

[Richard Newman [:rnewman]]

Comment on attachment 8831887 [details]
Bug 1333810: Add salts to the keys record,

https://reviewboard.mozilla.org/r/108382/#review109750

::: toolkit/components/extensions/ExtensionStorageSync.jsm:29
(Diff revision 1)
>  const STORAGE_SYNC_ENABLED_PREF = "webextensions.storage.sync.enabled";
>  const STORAGE_SYNC_SERVER_URL_PREF = "webextensions.storage.sync.serverURL";
>  const STORAGE_SYNC_SCOPE = "sync:addon_storage";
>  const STORAGE_SYNC_CRYPTO_COLLECTION_NAME = "storage-sync-crypto";
>  const STORAGE_SYNC_CRYPTO_KEYRING_RECORD_ID = "keys";
> +const STORAGE_SYNC_CRYPTO_SALT_LENGTH = 128;

Bits, bytes, or characters?

N.B., the rule of thumb for salts is to make them as long as your digest. So if you're using SHA-256, you might as well use a 256-bit (32-octet) salt. Anything longer is wasted.

::: toolkit/components/extensions/ExtensionStorageSync.jsm:212
(Diff revision 1)
> +     *   try to sync the collection.
> +     * - uuid: a record identifier. This will only change when we wipe
> +     *   the collection (due to kB getting reset).
> +     * - keys: a "WBO" form of a CollectionKeyManager.
> +     * - salts: a normal JS Object with keys being collection IDs and
> +     *   values being string versions of salts to use when hashing IDs

This isn't clear.

Is it:

- The octet sequence of the salt interpreted as a JS UCS-2 string?
- The octet sequence of the salt, encoded in base64-urlsafe, as a JS string?
- Something else?

Further down this file I see you call `getRandomBytes`, which calls `CommonUtils.byteArrayToString`, which returns each byte as a JS character using `String.fromCharCode`. You should think about whether this is what you actually want to do.

::: toolkit/components/extensions/test/xpcshell/test_ext_storage_sync.js:458
(Diff revision 1)
>    const uuidgen = Cc["@mozilla.org/uuid-generator;1"].getService(Ci.nsIUUIDGenerator);
>    return uuidgen.generateUUID().toString();
>  }
>  
> +function getRandomSalt() {
> +  return CryptoUtils.generateRandomBytes(STORAGE_SYNC_CRYPTO_SALT_LENGTH);

This is a bit risky. Find a way to make this use the implementation that the real code uses, such that it changes when the real code changes -- otherwise your tests are not testing what we think they are.

Comment 15

[Richard Newman [:rnewman]]

Comment on attachment 8831888 [details]
Bug 1333810: Redo hashing of collection ID,

https://reviewboard.mozilla.org/r/108384/#review109752

::: toolkit/components/extensions/ExtensionStorageSync.jsm:264
(Diff revision 1)
> +      if (!salt) {
> +        // This should never happen; salts should be populated before
> +        // we need them by ensureCanSync.
> +        throw new ExtensionUtils.ExtensionError(`no salt available for ${extensionId}; how did this happen?`);
> +      }
> +      return CryptoUtils.sha256(salt + ":" + extensionId);

`sha256` passes off to `digestUTF8`, which UTF-8-encodes the input (which is expected to be a UCS-2 string) into a byte array and hashes it. Think about what role your salt (and its format/encoding) is playing here. If you want to add 32 bytes to the front of that byte array, how many 16-bit UCS-2 characters should be in the salt?

`sha256` then returns a hex-encoded string representation of the hashed value. Is hex what you want to use as an ID? It's pretty verbose…

I'd be interested to see discussion of these in the docstring for this method. The docstring should be pretty gigantic.

::: toolkit/components/extensions/test/xpcshell/test_ext_storage_sync.js:489
(Diff revision 1)
>    const extensionId = "{9419cce6-5435-11e6-84bf-54ee758d6342}";
> -  const extensionId2 = "{9419cce6-5435-11e6-84bf-54ee758d6343}";
> +  // Fake a static keyring since the server doesn't exist.
> +  const salt = "IÈ1ñ\x12|cJñ0aW\x02µ\")æ°\x95o´Ì\x9C\x85\x9AÔþº" +
> +        "=¨úYÚøÏb1\x9C{³êÌØ\x064pË|\x12Ë\x1Fò\x94·\rË4$@ï\\ª\x9A" +
> +        "\x90\x198'P±\n\x87K¦-¿x\x12Ló.̲\x08\x7FP\x8Ay\x18\x96°" +
> +        "ú\x13Úå\x93\x9DþÃÁÏÎ0x\xAD\xAD¿\x90\xA0\x9D\x94þët$¬©È:ç\x94v\x04¨ÅàNû";

Or, indeed, pick a different representation for the salt… after all, this ends up in JSON, which is exactly this verbose for this kind of stuff!

Comment 16

[Richard Newman [:rnewman]]

Comment on attachment 8832125 [details]
Bug 1333810: Hash record IDs during encryption,

https://reviewboard.mozilla.org/r/108482/#review109754

This is fine, modulo kmag's comments and earlier patch notes about salts.

Comment 17

[Ethan Glasser-Camp (:glasserc)]

Comment on attachment 8831365 [details]
Bug 1333810: Encrypt record deletes,

https://reviewboard.mozilla.org/r/107918/#review109734

> So to make sure I'm understanding correctly, `record` here has `_status: "deleted"`, and so that ends up in the ciphertext. Kinto will now simply encode the deleted record as if it were any other kind, running through this code path.

That's correct. Previously, `kinto.js` would encode deletes, but delete them regardless of what the output was. See https://github.com/Kinto/kinto.js/pull/640 for more details.

Comment 18

[Ethan Glasser-Camp (:glasserc)]

Comment on attachment 8831887 [details]
Bug 1333810: Add salts to the keys record,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/108382/diff/1-2/

Comment 19

[Ethan Glasser-Camp (:glasserc)]

Comment on attachment 8831888 [details]
Bug 1333810: Redo hashing of collection ID,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/108384/diff/1-2/

Comment 20

[Ethan Glasser-Camp (:glasserc)]

Comment on attachment 8832125 [details]
Bug 1333810: Hash record IDs during encryption,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/108482/diff/1-2/

Comment 21

[Ethan Glasser-Camp (:glasserc)]

Comment on attachment 8831887 [details]
Bug 1333810: Add salts to the keys record,

https://reviewboard.mozilla.org/r/108382/#review109750

> Bits, bytes, or characters?
> 
> N.B., the rule of thumb for salts is to make them as long as your digest. So if you're using SHA-256, you might as well use a 256-bit (32-octet) salt. Anything longer is wasted.

Thanks, I was pretty sure what I was doing was wrong, but I couldn't figure out who to ask, so I just slapped something in there and figured it would get caught during review.

> This isn't clear.
> 
> Is it:
> 
> - The octet sequence of the salt interpreted as a JS UCS-2 string?
> - The octet sequence of the salt, encoded in base64-urlsafe, as a JS string?
> - Something else?
> 
> Further down this file I see you call `getRandomBytes`, which calls `CommonUtils.byteArrayToString`, which returns each byte as a JS character using `String.fromCharCode`. You should think about whether this is what you actually want to do.

I've updated the docstring. Yes, it seems baffling but I guess JS doesn't have a bytestring type, you just use strings that have code points between 0 and 255..

Comment 22

[Ethan Glasser-Camp (:glasserc)]

Comment on attachment 8831887 [details]
Bug 1333810: Add salts to the keys record,

https://reviewboard.mozilla.org/r/108382/#review109744

> Nit: Please don't use a 1-letter variable name for this.

Oops, sorry. This was just based on code I copied from services-sync/record.js.

> Should we test with only the key, or only the salt, changing? I know that shouldn't happen, but should we make sure we handle it if it does?

This was a great idea, thanks!

Comment 23

[Kris Maglione [:kmag]]

Comment on attachment 8831887 [details]
Bug 1333810: Add salts to the keys record,

https://reviewboard.mozilla.org/r/108382/#review109750

> I've updated the docstring. Yes, it seems baffling but I guess JS doesn't have a bytestring type, you just use strings that have code points between 0 and 255..

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Uint8Array

Comment 24

[Ethan Glasser-Camp (:glasserc)]

Comment on attachment 8831888 [details]
Bug 1333810: Redo hashing of collection ID,

https://reviewboard.mozilla.org/r/108384/#review109746

> Please don't do this. If this needs to be exported, please add it to an object we're already exporting.

I've removed this, but exported a method `cryptoCollection.getNewSalt` in the previous commit.

> This is really an internal error. We should throw a normal error here, and let the unexpected internal error exception be reported to the exception.

OK, thanks. I didn't really understand the distinction between `Error` and `ExtensionError` except that `Error` replaces my error message with something meaningless.

> Nit: Can you please base64 this and use `atob(...)` to decode?

Per :rnewman's suggestions, I now store them as base64-encoded bytestrings, so this is now just a base64 string.

Comment 25

[Kris Maglione [:kmag]]

Comment on attachment 8831887 [details]
Bug 1333810: Add salts to the keys record,

https://reviewboard.mozilla.org/r/108382/#review109750

> I've updated the docstring. Yes, it seems baffling but I guess JS doesn't have a bytestring type, you just use strings that have code points between 0 and 255..

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Uint8Array

Comment 26

[Ethan Glasser-Camp (:glasserc)]

Comment on attachment 8831888 [details]
Bug 1333810: Redo hashing of collection ID,

https://reviewboard.mozilla.org/r/108384/#review109752

> `sha256` passes off to `digestUTF8`, which UTF-8-encodes the input (which is expected to be a UCS-2 string) into a byte array and hashes it. Think about what role your salt (and its format/encoding) is playing here. If you want to add 32 bytes to the front of that byte array, how many 16-bit UCS-2 characters should be in the salt?
> 
> `sha256` then returns a hex-encoded string representation of the hashed value. Is hex what you want to use as an ID? It's pretty verbose…
> 
> I'd be interested to see discussion of these in the docstring for this method. The docstring should be pretty gigantic.

I've updated the method, handling the hasher a little more explicitly and taking care to not mix strings and bytestrings. I've also updated the docstring, but I didn't get to "pretty gigantic".. there isn't really that much to say on the subject.

Comment 27

[Kris Maglione [:kmag]]

Comment on attachment 8831888 [details]
Bug 1333810: Redo hashing of collection ID,

https://reviewboard.mozilla.org/r/108384/#review109746

> OK, thanks. I didn't really understand the distinction between `Error` and `ExtensionError` except that `Error` replaces my error message with something meaningless.

When an API method throws an error that isn't an ExtensionError instance, and doesn't belong to the extension scope, it's reported to the error console, and the extension receives a sanitized error in its place. Generally, unexpected internal errors aren't meant to be reported to untrusted code.

Comment 28

[Ethan Glasser-Camp (:glasserc)]

Comment on attachment 8831887 [details]
Bug 1333810: Add salts to the keys record,

https://reviewboard.mozilla.org/r/108382/#review109750

> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Uint8Array

I found this documentation, but I couldn't figure out how to convert to/from base64 in Gecko without going through a "bytestring". I found https://developer.mozilla.org/en-US/docs/Web/API/WindowBase64/Base64_encoding_and_decoding#Solution_2_%E2%80%93_rewrite_the_DOMs_atob()_and_btoa()_using_JavaScript%27s_TypedArrays_and_UTF-8 , but it didn't seem very helpful..

Comment 29

[Ethan Glasser-Camp (:glasserc)]

Comment on attachment 8831888 [details]
Bug 1333810: Redo hashing of collection ID,

https://reviewboard.mozilla.org/r/108384/#review109746

> When an API method throws an error that isn't an ExtensionError instance, and doesn't belong to the extension scope, it's reported to the error console, and the extension receives a sanitized error in its place. Generally, unexpected internal errors aren't meant to be reported to untrusted code.

OK, great, thanks. I kept running into issues with getting sanitized errors in my unit tests, and not seeing the real error anywhere.

Comment 30

[Richard Newman [:rnewman]]

Comment on attachment 8831888 [details]
Bug 1333810: Redo hashing of collection ID,

https://reviewboard.mozilla.org/r/108384/#review112640

::: toolkit/components/extensions/ExtensionStorageSync.jsm:261
(Diff revision 2)
> +
> +    /**
> +     * Hash an extension ID for a given user so that an attacker can't
> +     * identify the extensions a user has installed.
> +     *
> +     * The extension ID are assumed to be a string (i.e. series of

s/are/is

::: toolkit/components/extensions/ExtensionStorageSync.jsm:289
(Diff revision 2)
> +      const hasher = Cc["@mozilla.org/security/hash;1"]
> +          .createInstance(Ci.nsICryptoHash);
> +      hasher.init(hasher.SHA256);
> +
> +      const salt = atob(saltBase64);
> +      const message = `${salt}\x00${CommonUtils.encodeUTF8(extensionId)}`;

So the `extensionId` is a string: a sequence of codepoints.

encodeUTF8 turns that into UTF-8 bytes.

We need to ensure that a given extension yields the exact same byte sequence on all platforms.

We also need to ensure that a given extension always gets the same salt.

This conversion process should be deterministic, because a given sequence of codepoints has only one UTF-8 encoding. However, I'm slightly concerned about the possibility of an extension ID varying its codepoints over time, or otherwise running into Unicode normalization issues[1].

If one piece of software (e.g., AMO or some other part of Firefox) performs Unicode normalization but this code does not, it would be possible for an extension's ID to change while still being considered equivalent.

I suggest you use `String.prototype.normalize` on the `extensionId` before hashing it and before looking it up in the salts collection. Alternatively, ensure that it's normalized before reaching this code.

Feel free to file this as a follow-up, because I expect it's already an issue in the wider codebase.

[1] <https://en.wikipedia.org/wiki/Unicode_equivalence#Normalization>

Comment 31

[Richard Newman [:rnewman]]

Comment on attachment 8831887 [details]
Bug 1333810: Add salts to the keys record,

https://reviewboard.mozilla.org/r/108382/#review112646

Comment 32

[Ethan Glasser-Camp (:glasserc)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dca590769e1e

Comment 33

[Ethan Glasser-Camp (:glasserc)]

Comment on attachment 8831888 [details]
Bug 1333810: Redo hashing of collection ID,

https://reviewboard.mozilla.org/r/108384/#review112640

> So the `extensionId` is a string: a sequence of codepoints.
> 
> encodeUTF8 turns that into UTF-8 bytes.
> 
> We need to ensure that a given extension yields the exact same byte sequence on all platforms.
> 
> We also need to ensure that a given extension always gets the same salt.
> 
> This conversion process should be deterministic, because a given sequence of codepoints has only one UTF-8 encoding. However, I'm slightly concerned about the possibility of an extension ID varying its codepoints over time, or otherwise running into Unicode normalization issues[1].
> 
> If one piece of software (e.g., AMO or some other part of Firefox) performs Unicode normalization but this code does not, it would be possible for an extension's ID to change while still being considered equivalent.
> 
> I suggest you use `String.prototype.normalize` on the `extensionId` before hashing it and before looking it up in the salts collection. Alternatively, ensure that it's normalized before reaching this code.
> 
> Feel free to file this as a follow-up, because I expect it's already an issue in the wider codebase.
> 
> [1] <https://en.wikipedia.org/wiki/Unicode_equivalence#Normalization>

I have filed https://bugzilla.mozilla.org/show_bug.cgi?id=1338376.

Comment 34

[Ethan Glasser-Camp (:glasserc)]

Comment on attachment 8831888 [details]
Bug 1333810: Redo hashing of collection ID,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/108384/diff/2-3/

Comment 35

[Ethan Glasser-Camp (:glasserc)]

Comment on attachment 8832125 [details]
Bug 1333810: Hash record IDs during encryption,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/108482/diff/2-3/

Comment 36

[Ethan Glasser-Camp (:glasserc)]

Comment on attachment 8832125 [details]
Bug 1333810: Hash record IDs during encryption,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/108482/diff/3-4/

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0331a6dbc24d
Update kinto.js version to 8.0.0, r=MattN
https://hg.mozilla.org/integration/autoland/rev/df5261b5476b
Encrypt record deletes, r=kmag,rnewman
https://hg.mozilla.org/integration/autoland/rev/b9711bce61d0
Add salts to the keys record, r=kmag,rnewman
https://hg.mozilla.org/integration/autoland/rev/721e2e29c1c7
Redo hashing of collection ID, r=kmag,rnewman
https://hg.mozilla.org/integration/autoland/rev/8043e0ac7edf
Hash record IDs during encryption, r=kmag,rnewman

Comment 40

[Ethan Glasser-Camp (:glasserc)]

Comment on attachment 8831364 [details]
Bug 1333810: Update kinto.js version to 8.0.0,

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1253740 introduced an insufficiently-secure mechanism for encrypting data; this bug is about redoing the encryption
[User impact if declined]: We'll have to do a migration/delete of user data for all users using this feature
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: All the other patches on this bug: https://bugzilla.mozilla.org/attachment.cgi?id=8831365&action=edit, https://bugzilla.mozilla.org/attachment.cgi?id=8831887&action=edit, https://bugzilla.mozilla.org/attachment.cgi?id=8831888&action=edit, https://bugzilla.mozilla.org/attachment.cgi?id=8832125&action=edit
[Is the change risky?]: No
[Why is the change risky/not risky?]: This API is still brand-new (only switched on by default in Aurora) and not used by many extensions
[String changes made/needed]: None

Comment 41

[Kris Maglione [:kmag]]

This is going to affect extensions which have already started using storage.sync. We should probably consider sending those developers a warning.

Comment 42

[Jorge Villalobos [:jorgev] (he/him)]

What's the impact and what do developers need to do in order to avoid negative effects?

Comment 43

[Kris Maglione [:kmag]]

I'm not sure there's anything they can do, but any of their (nightly or Aurora) users who have data stored in storage.sync are going to lose that data, so they should at least be prepared to deal with it.

Comment 44

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0331a6dbc24d
https://hg.mozilla.org/mozilla-central/rev/df5261b5476b
https://hg.mozilla.org/mozilla-central/rev/b9711bce61d0
https://hg.mozilla.org/mozilla-central/rev/721e2e29c1c7
https://hg.mozilla.org/mozilla-central/rev/8043e0ac7edf

Comment 45

[Gerry Chang [:gchang]]

Comment on attachment 8831364 [details]
Bug 1333810: Update kinto.js version to 8.0.0,

Update kinto.js and fix encryption issue. Aurora53+.

Comment 46

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9f50d03c4b01
https://hg.mozilla.org/releases/mozilla-aurora/rev/7c112d81c854
https://hg.mozilla.org/releases/mozilla-aurora/rev/3c6f5168bada
https://hg.mozilla.org/releases/mozilla-aurora/rev/d344c6088604
https://hg.mozilla.org/releases/mozilla-aurora/rev/d0ba912fe2c8

Comment 47

[Jorge Villalobos [:jorgev] (he/him)]

DXR search seems to be broken. Andy, can you get a list of add-ons using storage.sync?

Comment 48

[Andy McKay]

I currently can't get unlisted ones. What's the ETA on the dxr fix?

Comment 49

[Jorge Villalobos [:jorgev] (he/him)]

Looks like it's working now. Thanks, anyway.

However, there are thousands of results, so I'm not sure what to do in this case. A blog post probably wouldn't reach the right audience.

https://dxr.mozilla.org/addons/search?q=chrome.storage.sync&redirect=true

Comment 50

[Kris Maglione [:kmag]]

There are a bunch that use browser.storage.sync rather than chrome.storage.sync:

https://dxr.mozilla.org/addons/search?q=browser.storage.sync