Closed Bug 1333810 Opened 3 years ago Closed 3 years ago

Encrypt record deletes

Categories

(WebExtensions :: General, defect, P2)

defect

Tracking

(firefox53 fixed, firefox54 fixed)

RESOLVED FIXED
mozilla54
Tracking Status
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: glasserc, Assigned: glasserc)

References

Details

(Keywords: addon-compat, Whiteboard: triaged)

Attachments

(5 files)

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: nobody → eglassercamp
Blocks: 1333814
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+
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..
Status: NEW → ASSIGNED
Component: WebExtensions: Untriaged → WebExtensions: General
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 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 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 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 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 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 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 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+
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 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 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 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 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 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 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 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 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 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 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+
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.
Keywords: checkin-needed
Duplicate of this bug: 1333814
Duplicate of this bug: 1333818
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
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 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+
DXR search seems to be broken. Andy, can you get a list of add-ons using storage.sync?
Flags: needinfo?(amckay)
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
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.