1349170 - [Regression] SecRandomCopyBytes() doesn't execute within an assert()

Status: VERIFIED FIXED

(Firefox for iOS :: General, defect, P1)

Description

[Catalin Suciu [:csuciu]]

Firefox v7.0b2125

1. Bookmark a few web pages.
2. Go to the Bookmarks panel.

Result: Only the first bookmarked page is displayed in the bookmarks list.

I cannot reproduce this issue on v7.0b2011 or latest master (d22e8dc9)

Comment 1

[Stephan Leroux [:sleroux]]

This seems really, really bad. I would consider this a blocker for 7.0.

Comment 2

[Catalin Suciu [:csuciu]]

Actually all home panels (except reading list) are affected.

History list is displaying only the first visited page and Top Sites does not update at all.  

Definitely a blocker.

Comment 3

[Stephan Leroux [:sleroux]]

Attachment: Github PR https://github.com/mozilla-mobile/firefox-ios/pull/2544

Comment 4

[Stephan Leroux [:sleroux]]

Filed follow up bug https://bugs.swift.org/browse/SR-4308.

Comment 5

[Catalin Suciu [:csuciu]]

I'm unable to reproduce this issue on latest beta (2150)

Comment 6

[Stephan Leroux [:sleroux]]

v7.x cb5dc5cf078066a93c6f4d02849277b9b0723a45

Needs patch to land on master as well.

Comment 7

[Richard Newman [:rnewman]]

This has broader consequences.

Providers/Profile.swift
225:        let secret = Bytes.generateRandomBytes(Length).base64EncodedString

Shared/Bytes.swift
13:    open class func generateRandomBytes(_ len: UInt) -> Data {
24:        return generateRandomBytes(9)

Sync/KeyBundle.swift
27:        // Bytes.generateRandomBytes uses SecRandomCopyBytes, which hits /dev/random, which
31:        return KeyBundle(encKey: Bytes.generateRandomBytes(32), hmacKey: Bytes.generateRandomBytes(32))
83:        let iv = iv ?? Bytes.generateRandomBytes(16)


Affected users will have predictable Sync collection keys, predictable client GUIDs that collide with other users, and a predictable local passwords DB key.

Comment 8

[Justin D'Arcangelo [:justindarc]]

Apparently, there's a response to the Swift bug:

https://bugs.swift.org/browse/SR-4308

It looks like all we need to do is remove the `assert()` around `SecRandomCopyBytes()`. We should be able to revert back and fix this proper now.

Comment 9

[Richard Newman [:rnewman]]

Suggested remediations to discuss:

1. Any user who clean-installed 7.0 beta _must_ uninstall and install a fixed version, regardless of whether they use Sync. This is worth emailing affected testers.

2. Any user who upgrade-installed from an earlier beta won't have weak keys, so long as they were not node-reassigned while syncing.

Their browsing history and any other created records will all have the GUID "AAAAAAAAAAAA", though, so really weird stuff will have occurred for them.

If they were node reassigned -- and recently we node-reassigned everyone -- and iOS got there first, they will have weak keys.

bobm should be able to find these users -- they'll have a keys record with "AAAA" as the IV -- and node-reassign them. This might need to be repeated until all affected users have upgraded.

Comment 10

[Bob Micheletto [:bobm]]

(In reply to Richard Newman [:rnewman] from comment #9)
> Suggested remediations to discuss:

> bobm should be able to find these users -- they'll have a keys record with
> "AAAA" as the IV -- and node-reassign them. This might need to be repeated
> until all affected users have upgraded.

I will start looking for these users.  I expect we will need to periodically scan for them as long as we think these clients exist in the wild.  I suppose there is some possibility that these users may re-appear with this same client after a re-assignment and re-upload the weak keys.

Comment 11

[Bob Micheletto [:bobm]]

(In reply to Richard Newman [:rnewman] from comment #9)
> Suggested remediations to discuss:
> 
> 1. Any user who clean-installed 7.0 beta _must_ uninstall and install a
> fixed version, regardless of whether they use Sync. This is worth emailing
> affected testers.
> 
> 2. Any user who upgrade-installed from an earlier beta won't have weak keys,
> so long as they were not node-reassigned while syncing.

:ulfr inviting you to the discussion.

Comment 12

[Benson Wong [:mostlygeek]]

What would the user agent for these iOS clients be? 
I'm guessing something like: "Firefox-iOS-Sync/7.*"?

Comment 13

[Bob Micheletto [:bobm]]

(In reply to Richard Newman [:rnewman] from comment #9)

> bobm should be able to find these users -- they'll have a keys record with
> "AAAA" as the IV -- and node-reassign them. This might need to be repeated
> until all affected users have upgraded.

Should I be searching collection 2 for crypto, or 5 for keys?  I don't actually see much if anything in collection 5.

Comment 14

[Bob Micheletto [:bobm]]

There are some users, on an only newly created Sync node, connecting from Firefox AndroidSync 1.45.0.1.0 (Firefox) clients with crypto IV fields that are "AAAAAAAAAAAAAAAAAAAAAA==".  Perhaps that's totally unrelated, but on the off chance it's interesting, noting that here.

Comment 15

[Benson Wong [:mostlygeek]]

:grisha we don't have to worry about comment #14 do we?

Comment 16

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

(In reply to Bob Micheletto [:bobm] from comment #14)
> There are some users, on an only newly created Sync node, connecting from
> Firefox AndroidSync 1.45.0.1.0 (Firefox) clients with crypto IV fields that
> are "AAAAAAAAAAAAAAAAAAAAAA==".  Perhaps that's totally unrelated, but on
> the off chance it's interesting, noting that here.

Bob, is it possible to confirm if we also have iOS users with a zeroed crypto field? It would be good for us to know if we need to send out a message to our iOS beta testers (we have about 150 people, plus some internal)

 S.

Comment 17

[Julien Vehent]

Moving the discussion on remediation to bug 1349919

Comment 18

[Justin D'Arcangelo [:justindarc]]

Attachment: GitHub Pull Request (v2)

This reverts the changes to generateRandomBytes() and moves the call to SecRandomCopyBytes() outside of the assert().

Comment 19

[Justin D'Arcangelo [:justindarc]]

Attachment: GitHub Pull Request (master)

Same patch for master.

Comment 20

[Bob Micheletto [:bobm]]

(In reply to Stefan Arentz [:st3fan] from comment #16)
Replied here: https://bugzilla.mozilla.org/show_bug.cgi?id=1349919#c4

Comment 21

[:Grisha Kruglov]

(In reply to Benson Wong [:mostlygeek] from comment #15)
> :grisha we don't have to worry about comment #14 do we?

Responded in https://bugzilla.mozilla.org/show_bug.cgi?id=1349919#c8

Comment 22

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

The patch that calls `fatalError()` looks good. One minor nit, can we use errSecSuccess instead of 0?

 https://developer.apple.com/reference/security/1542001-anonymous/errsecsuccess?language=objc

I know they are the same, but then it follows the documented contract.

Comment 23

[Stephan Leroux [:sleroux]]

Looks like the patch has already been reviewed so I've r+'ed it - just the nit from st3fan for completeness.

Comment 24

[Justin D'Arcangelo [:justindarc]]

Landed on v7.x:

https://github.com/mozilla-mobile/firefox-ios/commit/f65611dfa1112733ff8acd94bd9702d68ff86cbc

Landed on master:

https://github.com/mozilla-mobile/firefox-ios/commit/da3415aa29f76972733e7194e5d577f2bf11bd66

Comment 25

[Catalin Suciu [:csuciu]]

Verifying the initial issue as fixed in v.7.0b2286.