Encrypt record deletes

RESOLVED FIXED in Firefox 53

Status

()

Toolkit
WebExtensions: General
P2
normal
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: glasserc, Assigned: glasserc)

Tracking

({addon-compat})

unspecified
mozilla54
addon-compat
Points:
---

Firefox Tracking Flags

(firefox53 fixed, firefox54 fixed)

Details

(Whiteboard: triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments)

(Assignee)

Description

5 months ago
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).
(Assignee)

Updated

5 months ago
Assignee: nobody → eglassercamp
(Assignee)

Updated

5 months ago
(Assignee)

Updated

5 months ago
Blocks: 1333814
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Updated

5 months ago
Priority: -- → P2
Whiteboard: triaged
Comment on attachment 8831364 [details]
Bug 1333810: Update kinto.js version to 8.0.0,

https://reviewboard.mozilla.org/r/107916/#review109470
Attachment #8831364 - Flags: review?(MattN+bmo) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 6

5 months ago
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 hidden (mozreview-request)
(Assignee)

Comment 8

5 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c5c115baff4a
Status: NEW → ASSIGNED
Component: WebExtensions: Untriaged → WebExtensions: General

Comment 9

5 months ago
mozreview-review
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.
Attachment #8831365 - Flags: review?(rnewman) → review+

Comment 10

5 months ago
mozreview-review
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.
Attachment #8831365 - Flags: review?(kmaglione+bmo) → review+

Comment 11

5 months ago
mozreview-review
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?
Attachment #8831887 - Flags: review?(kmaglione+bmo) → review+

Comment 12

5 months ago
mozreview-review
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?
Attachment #8831888 - Flags: review?(kmaglione+bmo) → review+

Comment 13

5 months ago
mozreview-review
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.
Attachment #8832125 - Flags: review?(kmaglione+bmo) → review+

Comment 14

5 months ago
mozreview-review
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

5 months ago
mozreview-review
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

5 months ago
mozreview-review
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.
Attachment #8832125 - Flags: review?(rnewman) → review+
(Assignee)

Comment 17

5 months ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 21

5 months ago
mozreview-review-reply
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..
(Assignee)

Comment 22

5 months ago
mozreview-review-reply
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

5 months ago
mozreview-review-reply
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
(Assignee)

Comment 24

5 months ago
mozreview-review-reply
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

5 months ago
mozreview-review-reply
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
(Assignee)

Comment 26

5 months ago
mozreview-review-reply
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

5 months ago
mozreview-review-reply
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.
(Assignee)

Comment 28

5 months ago
mozreview-review-reply
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..
(Assignee)

Comment 29

5 months ago
mozreview-review-reply
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

5 months ago
mozreview-review
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>
Attachment #8831888 - Flags: review?(rnewman) → review+

Comment 31

5 months ago
mozreview-review
Comment on attachment 8831887 [details]
Bug 1333810: Add salts to the keys record,

https://reviewboard.mozilla.org/r/108382/#review112646
Attachment #8831887 - Flags: review?(rnewman) → review+
(Assignee)

Comment 32

5 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dca590769e1e
(Assignee)

Comment 33

5 months ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

5 months ago
Keywords: checkin-needed
(Assignee)

Updated

5 months ago
Duplicate of this bug: 1333814
(Assignee)

Updated

5 months ago
Duplicate of this bug: 1333818

Comment 39

5 months ago
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
(Assignee)

Comment 40

5 months ago
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
Attachment #8831364 - Flags: approval-mozilla-aurora?
This is going to affect extensions which have already started using storage.sync. We should probably consider sending those developers a warning.
Flags: needinfo?(jorge)
Keywords: addon-compat
What's the impact and what do developers need to do in order to avoid negative effects?
Flags: needinfo?(jorge)
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

5 months ago
bugherder
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
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54

Updated

5 months ago
status-firefox53: --- → affected
Comment on attachment 8831364 [details]
Bug 1333810: Update kinto.js version to 8.0.0,

Update kinto.js and fix encryption issue. Aurora53+.
Attachment #8831364 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 46

4 months ago
bugherderuplift
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
status-firefox53: affected → fixed
DXR search seems to be broken. Andy, can you get a list of add-ons using storage.sync?
Flags: needinfo?(amckay)

Comment 48

4 months ago
I currently can't get unlisted ones. What's the ETA on the dxr fix?
Flags: needinfo?(amckay)
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
There are a bunch that use browser.storage.sync rather than chrome.storage.sync:

https://dxr.mozilla.org/addons/search?q=browser.storage.sync
You need to log in before you can comment on or make changes to this bug.