Firefox Sync fails with this addon

[mhammond]

See bug https://bugzilla.mozilla.org/show_bug.cgi?id=1128402 - I maintain that Firefox code, so please feel free to reach out of you need any assistance.

[Etnier]

An everyday user here hoping a quick solution is found to this problem. Firefox is useless to me without keychain support. Thank you.

[bintoro]

I too would like Firefox Sync to function. After evaluating third-party password managers, I've concluded that FF Sync is the best way to share credentials between a Mac and an iPhone, and so I decided to investigate a bit.

Mac to server

Looking at Preferences > Security > Saved Logins, it's clear that desktop Firefox is able to retrieve a complete list of logins even with keychain support enabled. So why are they not syncing?

It looks like the extension doesn't deliver the last-modified timestamp for logins (the displayed value is always 1970-01-01). Since it's probable that the modification time is a factor when deciding what to sync, it's conceivable that this alone could explain the issue.

@jfitzell Do you happen to remember why creationDate and modDate have been commented out in KeychainItem.jsm?

Server to Mac

As I understand it, the referenced FF bug 1128402 deals with this scenario. This is what I see in the sync log:

1477411861371 Sync.Store.Passwords DEBUG Updating https://m.reddit.com
1477411861421 Sync.Store.Passwords DEBUG Modifying record 99l2-1cgwGwx
resulted in exception; not modifying: [Exception... "[JavaScript Error:
"KeychainItem.jsm - While setting keychain item password,
SecKeychainItemModifyAttributesAndData() returned -25304: The specified item
is no longer valid. It may have been deleted from the keychain." {file:
"resource://macos-keychain/KeychainItem.jsm" line: 763}]'[JavaScript Error:
"KeychainItem.jsm - While setting keychain item password,
SecKeychainItemModifyAttributesAndData() returned -25304: The specified item
is no longer valid. It may have been deleted from the keychain." {file:
"resource://macos-keychain/KeychainItem.jsm" line: 763}]' when calling
method: [nsILoginManagerStorage::modifyLogin]" nsresult: "0x80570021
(NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)" location: "JS frame ::
resource://gre/components/nsLoginManager.js :: modifyLogin :: line 323" data:
yes]
Stack trace:
modifyLogin()@resource://gre/components/nsLoginManager.js:323
< PasswordStore.prototype.update()@resource://gre/modules/services-sync/engines/passwords.js:269
< Store.prototype.applyIncoming()@resource://services-sync/engines.js:341
< Store.prototype.applyIncomingBatch()@resource://services-sync/engines.js:303
< doApplyBatch()@resource://services-sync/engines.js:1002
< doApplyBatchAndPersistFailed()@resource://services-sync/engines.js:1019
< SyncEngine.prototype._processIncoming()@resource://services-sync/engines.js:1136
< SyncEngine.prototype._sync()@resource://services-sync/engines.js:1555
< WrappedNotify()@resource://services-sync/util.js:146
< Engine.prototype.sync()@resource://services-sync/engines.js:670
< _syncEngine()@resource://services-sync/stages/enginesync.js:213
< sync()@resource://services-sync/stages/enginesync.js:163
< onNotify()@resource://gre/modules/services-sync/service.js:1262
< WrappedNotify()@resource://services-sync/util.js:146
< WrappedLock()@resource://services-sync/util.js:101
< _lockedSync()@resource://gre/modules/services-sync/service.js:1252
< sync/<()@resource://gre/modules/services-sync/service.js:1244
< WrappedCatch()@resource://services-sync/util.js:75
< sync()@resource://gre/modules/services-sync/service.js:1232
1477411861426   Sync.Engine.Passwords   INFO    Records: 1 applied, 1 successfully, 0 failed to apply, 0 newly failed to apply, 0 reconciled.

It tries to modify an existing item, even though the login "m.reddit.com" does not exist in the Mac's keychain.

After trying this out a few times, I observed that some random keychain items had had their account names changed to my Reddit username. This suggests that the combination of FF Sync and the Keychain extension can silently corrupt entries in the Keychain. The passwords hadn't been modified, however.

So, for some reason the extension is returning a random entry when queried about a nonexistent login. But why does this only happen during sync? Is the query coming from Firefox somehow different than during normal usage?

[bintoro]

Okay, I just learned about the signon.debug switch, and turning it on immediately revealed what goes wrong.

It appears each login gets a unique ID not documented in the nsILoginInfo reference. It is stored as the property guid. During sync the GUID is used instead of other criteria:

Keychain: -> searchLogins(nsIPropertyBag{guid: '99l2-1cgwGwx'})
Keychain: -> findLogins('','','')

The recently introduced searchLogins method is not aware of guid, and in the absence of any properties it would recognize, it turns the query into a findLogins query for everything. Then, the first one in the result set gets updated with the wrong info.

I suppose this means the only way to support Sync would be to store the GUID in the keychain item's comment field. GUID-based search would have to be implemented by the extension itself.

[jfitzell]

@bintoro: I have no particular memory about the date fields. It's possible I didn't see an easy mapping to Keychain fields, but most likely I was just implementing the fields that I could see being used and they weren't used by findLogins.

Your later diagnosis sounds pretty plausible... the Keychain data structure is not very flexible, so as you say you might need to store the custom properties in the comment field, which is not very nice. It sounds like you've checked and you can't search by the contents of the comment field?

Incidentally, I have some concerns about implementing Sync without some options:

1. The expected behaviour without the add-on is that only your Firefox passwords would be synced, so I would suggest the default behaviour would want to be the same (I.e. only password entries where the creator was the current app). There could be a setting to include passwords created by other browsers and maybe another to include all internet passwords.
2. Would sync try to access every password in the Keychain, thus potentially triggering a string of access requests every time if Firefox does not have permissions for all passwords? (another reason for the default to be Firefox passwords only)

Also, does your finding suggest that searchLogins should return an empty list if faced with any properties it doesn't recognize? Sounds like it would be safer...

[bintoro]

It sounds like you've checked and you can't search by the contents of the comment field?

For an exact match, you can, but not for a substring AFAICT. So, using the OS-provided search would require dedicating the entire comment field to the GUID alone, and I'm not sure how palatable that is. Personally I've never used the comment field for anything.

The expected behaviour without the add-on is that only your Firefox passwords would be synced, so I would suggest the default behaviour would want to be the same (I.e. only password entries where the creator was the current app).

Agreed.

It seems that right now the creator code ends up as "aapl". I have no idea how new apps are supposed to obtain unique creator codes, but back in the day when they were still a thing, "MOZB" was registered for Firefox. It's probably okay to just go ahead and use that.

There could be a setting to include passwords created by other browsers and maybe another to include all internet passwords.

Yeah, that would be ideal, but there are some challenges. I took a quick look at the sync code, and my initial understanding is as follows:

• On the first run, all items are synced.
• After that, whenever a login is modified, its GUID is stored in a list of changed items, and only those items get to participate in the next sync.

If this is accurate, it means there's no way for the login storage module to feed additional items into the sync process. The only way to make it sync logins that were not created or modified by Firefox itself would be to interact with the Sync API directly. 😢

Also, does your finding suggest that searchLogins should return an empty list if faced with any properties it doesn't recognize? Sounds like it would be safer...

Yes, absolutely. As it stands, unless a user has explicitly disabled password syncing, every password created or modified on a remote Firefox will mess up a semi-random password item on the Firefox that is using the Keychain. Fortunately it's only the account name that gets overwritten.

[mhammond]

Note there are 2 related issues here:

• The problem for which I opened this issue is for when sync is trying to save its own credentials (ie, keys etc) to the login manager. These credentials are not synced, but this issue will break sync even if password syncing is disabled by the user (and to be clear, by "break sync", I mean that every time the browser starts the user will be prompted to re-enter their Firefox Accounts password)
• The general issue of password syncing for users who have chosen to sync them.

If this is accurate, it means there's no way for the login storage module to feed additional items into the sync process. The only way to make it sync logins that were not created or modified by Firefox itself would be to interact with the Sync API directly.

Unless I'm missing something, I don't see that as a problem - Sync is really just about syncing passwords used by Firefox - I don't think it makes sense to try and turn it into syncing items in the keychain which are never used by Firefox.

Let me know if I can help in any way.

[bintoro]

The problem for which I opened this issue is for when sync is trying to save its own credentials (ie, keys etc) to the login manager

Right, I didn't realize this bug was about that, because it actually works for me. Perhaps the only remaining problem was the lack of searchLogins(), implemented in #84.

(The sync credentials are stored under the URI chrome://FirefoxAccounts. The extension will pass any chrome URIs through to the built-in login storage, and indeed there is a chrome://FirefoxAccounts entry in my logins.json, which is otherwise empty.)

Unless I'm missing something, I don't see that as a problem - Sync is really just about syncing passwords used by Firefox

This is about passwords used by Firefox. The use case would be including items that have been added or modified through Keychain Access or Safari, for instance.

Even though Firefox can use these passwords, it doesn't recognize them as eligible to be synced because Sync works by maintaining a list of modified GUIDs instead of filtering by modification date.

[mhammond]

Perhaps the only remaining problem was the lack of searchLogins(), implemented in #84.

Awesome, thanks - I closed that bug.

Even though Firefox can use these passwords, it doesn't recognize them as eligible to be synced because Sync works by maintaining a list of modified GUIDs instead of filtering by modification date.

That makes sense, thanks. And just to be clear, what Sync does is:

• Gets notified when an item changes rather than enumerating every item and checking it's date. I imagine this might be possible for your addon to implement with some work (eg, you could do the enumeration/last-modified check and send the same notification yourself.)
• Requires each item to have a GUID as items are indexed both locally and on the server using this ID - ie, the server specifies the ID of the entry that changed, and when a different client sees this ID as modified it knows which logininfo to update. This seems the tricky part, but the concept is used simply as identification of a specific entry to update - does the keychain have any concept of an ID for an entry?

Another thing to consider is that it seems important for these passwords entries to work correctly when one of the devices doesn't have the addon (eg, android, firefox on windows), but I'm sure we can work out how to get this going (albeit with more effort than would be ideal)

[jfitzell]

From memory, i think Keychain has a concept of a permanent reference that can be serialised. I think I'm only using temporary references in the code, though I might have ended up having to use the permanent ones for the proxy that is passed back to Firefox. A permanent one could possibly be used to maintain a separate mapping to GUIDs outside of the keychain...

Would be a chunk of work though.

[bintoro]

Another thing to consider is that it seems important for these passwords entries to work correctly when one of the devices doesn't have the addon

From Firefox's point of view, won't the externally modified passwords be just like any others, though? It should just work.

From memory, i think Keychain has a concept of a permanent reference that can be serialised.

If there is, I haven't found it.

From the source of the security tool:

// the primary key for an internet password item (i.e. the combination of
// attributes which determine whether the item is unique) consists of:
// { kSecAccountItemAttr, kSecSecurityDomainItemAttr, kSecServerItemAttr,
//   kSecProtocolItemAttr, kSecAuthenticationTypeItemAttr,
//   kSecPortItemAttr, kSecPathItemAttr }

So, if this is the case, the GUID could be generated by applying a suitable hash function to the concatenation of those attributes. It also occurred to me that there's no reason to store the GUIDs at all — simply regenerate them whenever needed. This would greatly simplify things.

[jfitzell]

Take a look at Apple's docs for SecKeychainItemCreatePersistentReference

Or KeychainItem.initWithReference in modules/KeychainItem.jsm

[mhammond]

So, if this is the case, the GUID could be generated by applying a suitable hash function to the concatenation of those attributes. It also occurred to me that there's no reason to store the GUIDs at all — simply regenerate them whenever needed. This would greatly simplify things.

The main issue I see here is how modifications would be handled. For example, if a user changes any of those attributes, it would get a new GUID and Sync would end up creating a new entry rather than modifying an existing one.

Take a look at Apple's docs for SecKeychainItemCreatePersistentReference

That looks like a better approach, although as you mention above, it probably means keeping an external mapping of these refs to actual GUIDs, which is a bunch of work.

[jfitzell]

Arguably, you could define it as an external mapping of properties-used-by-Firefox-but-not-supported-by-the-Keychain (of which GUID was just one) – it seems to me there were a few other properties I had to ignore so that would make it seem marginally more general purpose and useful. But no less work!

Hmm... I don't suppose the default password store could be that mapping... if you could store the persistent reference as a custom property then you could search by that or by the GUID. Not sure off the top of my head if that's a good fit or a bit overloaded. You'd just leave the fields that did fit in the Keychain blank in the Firefox entry... not sure how much that might break other stuff though, particularly if you removed the extension, though storing the persistent reference would mean you could theoretically migrate the data back before removing it.

Still plenty of work to check for new entries, honour preferences, move data around, etc. And probably higher risk of something blowing up. Much of that would want to be in a separate module probably to keep it together in one place.

I don't use Sync myself and I definitely don't have time to do any of this at the moment, but am willing to (try to) answer questions if anyone else wants to give it a shot.

[bintoro]

I wonder if each keychain item is permanently associated with exactly one persistent reference, and the same persistent reference is returned to any process that requests it. If this were the case, then the GUID could always be derived from the persistent reference without having to maintain a separate mapping.

[jfitzell]

It should be the same across all processes, but I'd be very surprised if it was the same on another machine you sync to. Also you have to deal with passwords that are already in Firefox and have GUIDs assigned when you first install the extension. In both cases, you can't assume you'll be able to control the GUID.

[mhammond]

To be quite honest, I don't think this addon should bother trying to support Sync in this way (ie, if it was my addon, I'd probably chose to not support it):

• If all Firefox profiles used by Sync use the keychain, it would probably make sense for the user to just use the iCloud Keychain, which seems to support iOS and Mac, and cut out the Sync middle-man.
• If some profiles don't use the addon (eg, Windows, Mac without the addon installed) then these passwords will be stored in a relatively unsecured environment on the other machines, somewhat defeating the main main benefits of using the keychain in the first place.

[bintoro]

I'd be very surprised if it was the same on another machine you sync to

Right, I totally forgot there might be multiple browsers using the extension, as my own use case is just desktop–mobile sync.

Maintaining a separate GUID database doesn't seem too complicated, though. In the event of loss of extension data, it should be feasible to recreate the mapping based on the primary keys of the keychain items. And the same logic would prevent duplicates from arising during initial setup.

it would probably make sense for the user to just use the iCloud Keychain, which seems to support iOS and Mac, and cut out the Sync middle-man

This route is blocked by access restrictions imposed by Apple. Unlike local keychains on macOS, the iCloud keychain is implemented so that only the creator of a keychain item may later access it, eliminating the need to ask for user consent.

Thus, a login saved in the iCloud keychain by this extension on macOS won't be available to any browser on iOS: Safari has no access and Firefox doesn't use the keychain.

While there is a degree of cross-application sharing of iCloud keychain items, AFAIK it is limited to a group of applications from the same vendor.

Previously, there was also the issue that only App-Store-distributed apps were allowed to access the iCloud keychain, but this restriction has recently been lifted.

[jfitzell]

@bintoro: I think you also still need to have declared an iCloud entitlement when compiling the application?

@mhammond: I tend to agree, which is why it's never been a priority for me to implement. I'm skeptical of taking passwords secured in the keychain (particularly ones that weren't created by Firefox) and copying them to another system where they'll be stored less securely. I can see the argument that some people might use the Keychain for reasons other than security, but Apple seems to be moving away from a model that supports this and I don't think 3rd party password storage is a priority for Mozilla, so it all feels increasingly fragile.

I'm not totally opposed to having some kind of support for this, but it would definitely need to be off by default and not overcomplicate the rest of the code. Doing it well wouldn't be trivial and it just isn't a priority for me.