1755786 - OSKeyStore.sys.mjs needs to use unique key labels per profile

Status: NEW

(Toolkit :: Form Autofill, defect, P2)

Description

[Eric Rescorla (:ekr)]

URL: https://www.eventbrite.com/e/fame-camp-tickets-225972358407

STR:

1. Order tickets, select credit card payment, etc.
2. Click on CC box
3. Choose your CC

Expected results:
CC is filled in

Actual results:
Field stays empty.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Toolkit::Form Autofill' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.

Comment 2

[Tim Giles [:tgiles]]

Thanks for filing this bug. I'm not able to reproduce this on my Windows box or my Macbook, filling the credit card details works as expected on eventbrite.

:ekr, can you check and see if your autofill works on our test site? https://mozilla.github.io/form-fill-examples/basic_cc.html

If it's not working on our test site, you may need to remove and re-add the credit card to your profile. For example, if you go to about:preferences and search for "saved credit cards" you should encounter a button named "Saved Credit Cards...". First edit this card and see if it displays a credit card number. If it doesn't show a cc number, then you need to re-add your card to your profile. If you happen to have multiple saved cards, I would check to make sure the cc number shows as expected when you are in the edit/management dialog. Otherwise, you'll most likely encounter this issue again when using one of the other saved cards.

Comment 3

[Eric Rescorla (:ekr)]

OK, so:

1. Yes, it doesn't work.
2. Yes, it doesn't show in edit.

However, as it knows the last four digits, this seems like it's likely some kind of Fx bug. Do we know how it happens?

Comment 4

[Eric Rescorla (:ekr)]

And just to follow up: if it doesn't have a value here, then it shouldn't offer me the card, no?

Comment 5

[Tim Giles [:tgiles]]

We're not entirely sure how this issue appears. For example, I have a profile that seems to produce this same issue but it's hard to tell because there's a NS_ERROR_FAILURE being thrown without any line numbers associated with it (in my case). When debugging the stack, we've found (again for my particular profile) that eventually some bad data appears and causes the decryption of the credit card to fail. The DecryptUpdate function in intel-gcm-wrap ends up setting a SEC_ERROR_BAD_DATA and returns SECFailure at least in my case.

We've seen similar symptoms in Bug 1754101 and Bug 1753358, it seems like long lived profiles eventually cause this issue to appear...but I'm not sure of a solid root cause or anything since we would need to debug these particular profiles and ensure they're hitting the "bad data" path that I'm also running into.

However, as it knows the last four digits,

So the AutoCompleteController is able to grab these results which will contain the encrypted credit card number and the masked credit card number. The masked credit card number appears to be created and stored when the credit card itself is first saved so that would explain why we're still able to show the last four digits in these cases, even though the filling of this credit card data will fail.

As for your follow up: form autofill will lazily decrypt the cc number so the autocomplete dropdown doesn't know it shouldn't show a particular entry because the decryption doesn't happen until the fill occurs. The same kind of flow happens in the management/edit dialog, we try to decrypt the credit card number and we get the NS_ERROR_FAILURE since we're still using OSKeyStore.decrypt in this case.

Comment 6

[Dimi Lee [:dimi]]

I also have a local build (15. Feb build) that can reproduce this issue. The root cause is the same as what :tgiles mentioned in Comment 5, we fail to decrypt the credit card number. In my local build, I have 3 credit cards saved, 2 of them can’t be decrypted, 1 of them can be decrypted. The two credit cards that can't be decrypted were added a few days ago before I tested this issue. The credit card that can be decrypted is a newly created one. But I don’t actually remember what I did in between.
I also traced the stack a bit, the error returned by C_Decrypt is CKR_ENCRYPTED_DATA_INVALID

Hi dana, Do you have any idea what might cause this problem? Thanks!

Comment 7

[Dana Keeler (she/her) (use needinfo) [:keeler]]

At a quick glance, I can't find anything related that changed in that timeframe. Maybe this is related to bug 1729930 or bug 1729550?
John - any ideas?

Comment 8

[John Schanck [:jschanck]]

I can reproduce this in a fresh profile as follows:

1. Save credit card "A".
2. Copy the value of the "Firefox Encrypted Storage" key in the operating system keyring.
3. Delete the "Firefox Encrypted Storage" key.
4. Save a new credit card "B". This will create a new "Firefox Encrypted Storage" key with a new value.

At this point I can autofill "B", but attempting to autofill "A" fails with SEC_ERROR_BAD_DATA in the same location tgiles mentioned in Comment 5. Unsurprisingly, if I restore the original value of the "Firefox Encrypted Storage" key then I can decrypt "A" but not "B".

I wonder if this has something to do with the fact that the string "Firefox Encrypted Storage" is generated from "{AppConstants.MOZ_APP_BASENAME} Encrypted Storage" (https://searchfox.org/mozilla-central/source/toolkit/modules/OSKeyStore.jsm#54). Maybe someone experiencing this problem can check their operating system keyring for other "Encrypted Storage" keys.

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:tgiles, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 10

[Tim Giles [:tgiles]]

John, I checked my credential manager and I only saw once instance of "Encrypted Storage" and it was the "Firefox Encrypted Storage". I'm not sure if the offending credential is under a different name since my broken profile existed only on my local build, but just wanted to confirm there are no additional "Encrypted Storage" instances in my particular case.

Setting S3 so bot won't bother me.

Comment 11

[Dimi Lee [:dimi]]

Hi John,
In my case, there is also only 1 "Firefox Encrypted Storage". The "Date Modified" of the key is "25. Feb 2022".
As I mentioned in Comment 6, I have two card that have this problem, which are created at 22. Feb.
Does that mean the old key that is used to encrypt those credit cards is overwritten by a new key for some reason?

Comment 12

[Tim Giles [:tgiles]]

Moving this over to NSS since the issue appears to do with the Firefox Encrypted Storage key changing and causing decryption errors. Feel free to kick back if this is not the case.

Comment 13

[Dana Keeler (she/her) (use needinfo) [:keeler]]

(In reply to Dimi Lee [:dimi][:dlee] from comment #11)

Hi John,
In my case, there is also only 1 "Firefox Encrypted Storage". The "Date Modified" of the key is "25. Feb 2022".
As I mentioned in Comment 6, I have two card that have this problem, which are created at 22. Feb.
Does that mean the old key that is used to encrypt those credit cards is overwritten by a new key for some reason?

It looks that way. Part of the issue is that way OSKeyStore.jsm uses nsIOSKeyStore - all profiles in all copies of Firefox that have the same application identifier will end up with the same key. If one of them sets a new key, all profiles will be affected, which is probably not what we want. Another issue is that there's no way for the backend to tell OSKeyStore.jsm that the key it's looking for isn't there - it just silently fails.

Our approach to addressing this will be to change the API to include a way for OSKeyStore.jsm to tell the backend which key it's expecting. Additionally, OSKeyStore.jsm should be changed to use a different key for each profile.

Comment 14

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Actually, I think a good first step would be to use unique key labels per profile.

Comment 15

[Dimi Lee [:dimi]]

(In reply to Dana Keeler (she/her) (use needinfo) (:keeler for reviews) from comment #13)

all profiles in all copies of Firefox that have the same application identifier will end up with the same key. If one of them sets a new key, all profiles will be affected, which is probably not what we want.

Hi Dana, do we know when a Firefox build will set a new key even we already have a key and the other builds are using it? And when that happens, is it a bug or is it an expected behavior? I ask this question because that can help us determine the priority of this issue.

Comment 16

[Dana Keeler (she/her) (use needinfo) [:keeler]]

The backend will set a new key (overwriting an existing one) if nsIOSKeyStore.asyncGenerateSecret or nsIOSKeyStore.asyncRecoverSecret is called in any instance of Firefox (which is why using distinct labels is important). Looking at the frontend, it only calls asyncGenerateSecret if the backend says there is no key already.

I have a guess for how this situation happens. On macOS, the keychain entry that the backend creates is only available to the Firefox application bundle that has the same application-identifier as the instance of Firefox that created the entry. When a different Firefox application bundle tries to access that entry, a UI prompt comes up that the user can use to authorize the other bundle of Firefox. If the user cancels that prompt because of course they're going to cancel this random prompt that they weren't expecting and don't know what it's asking to do, the backend says that the secret is unavailable. The frontend then decides to set a new key, thus rendering useless everything encrypted with the preexisting key.

I would say this is a bug in that the frontend is using the backend in a way that unintentionally deletes saved data.

Comment 17

[Sergey Galich [:serg]]

(In reply to Dana Keeler (she/her) (use needinfo) (:keeler for reviews) from comment #16)

I have a guess for how this situation happens. On macOS, the keychain entry that the backend creates is only available to the Firefox application bundle that has the same application-identifier as the instance of Firefox that created the entry. When a different Firefox application bundle tries to access that entry, a UI prompt comes up that the user can use to authorize the other bundle of Firefox. If the user cancels that prompt because of course they're going to cancel this random prompt that they weren't expecting and don't know what it's asking to do, the backend says that the secret is unavailable. The frontend then decides to set a new key, thus rendering useless everything encrypted with the preexisting key.

Thank you Dana, this should be possible to reproduce.

Comment 18

[Tim Giles [:tgiles]]

I've been trying to reproduce this issue based on Dana's comment in Comment #16 and haven't been able to reproduce. I've deleted the "Firefox Encrypted Storage" key and then created a new one by saving a credit card on Release and then tried to save a credit card on my local build but then cancel the auth prompts that appear...but the key remains. I've confirmed that these steps call ensuredLoggedIn with the generateKeyIfNotAvailable parameter receiving its default value of true. I've also stepped through and can see the call we make to the native OS keystore if these authentication prompts are cancelled. But for some reason, I can't get MacOS to actually delete the "Firefox Encrypted Storage" key during the asyncGenerateSecret call. I consistently get the following debug logs:

1. D/keychainsecret SecItemCopyMatching failed: -128
2. D/keychainsecret SecItemDelete failed: -25244
3. D/keychainsecret DeleteSecret before StoreSecret failed

I guess the previous logs are expected because "the keychain does not overwrite secrets by default (unlike other backends like libsecret and credential manager). To be consistent, we first delete any previously-stored secrets that use the given label." but it seems like in my case, we aren't actually able to delete the secrets. I have no idea why.

I've tried deleting the "Firefox Encrypted Storage" key and then regenerating it by saving a credit card on Nightly and then trying to hit the "async secret not available for 'Firefox Encrypted Storage'" path by ignoring the auth prompts on my local build, but to no avail. I get the same NS_ERROR_FAILURE that comes from trying to delete the secrets of 'Firefox Encrypted Storage' before deleting the label.

I'm still trying to understand how we accidentally delete the secrets in the first place but it seems like sometimes KeychainSecret.cpp prevents the (unintended) deletion from happening in the first place.

Comment 19

[Tim Giles [:tgiles]]

I've tried to reproduce and figure out what's going on with the original report and I'm unable to reproduce the failure. While changing the key store label is simple enough, the migration will also be difficult IMO...but since we can't reproduce the original condition, it will be difficult to verify that changing the labels to be unique will prevent this issue in the future.

Comment 21

[leftblank15]

I deleted my auto-fill credit-card, and added-it-back, and checked the box, for Mac authorization (which WASN'T checked, before), and now it autofills, correctly in v107.0.1 .

Comment 24

[anna]

I'm actively having issues with my credit card numbers being automatically cleared somehow. I thought it was happening when I restarted the browser and felt like I could actively reproduce that... I submitted a bug report and was asked if I had additional profiles set up in Firefox. When I checked, there was a second one. I deleted it, added the credit card numbers back into my primary profile, restarted the browser several times and the numbers stayed present after the restarts. Unfortunately, today I went to use the credit card autofill and the numbers were missing again. I'm happy to troubleshoot or provide additional information if it helps get this working.

Comment 28

[Gian-Carlo Pascutto [:gcp]]

Daniel,

given that there's no triage owner (or anything similar) for this component, but it seems to fall broadly under Credential Management, can you triage this?

Comment 29

[Johannes]

I suspect that the problem is more that different Firefox distributions (Nightly, Beta, Stable) are accessing the same profile. However, since the OS keystore only allows access to the application that created the key, not every Firefox can receive the key – and therefore generates a new one, which means that previously stored credit card numbers can no longer be decrypted.

There may be subtle differences in the way the OS keystore verifies the application's identity. I was unable to reproduce this behavior on Linux; both Firefox Stable and Nighly can apparently use the same key.

Could this be the cause on Mac OS?

Comment 30

[Simon Friedberger (:simonf)]

I can reproduce this somewhat by removing the access permission for Firefox from the corresponding keychain item. If Firefox cannot access the encryption key item, it deletes it and replaces it with a new one. This is probably due to overwriting being the default behavior on Windows and Linux.

There will be other reasons why we fail to access the keychain item which lead to the same issue. The Apple docs are unfortunately not very clear about which of the possible return values can actually happen on a call to SecItemCopyMatching and when: https://developer.apple.com/documentation/security/security-framework-result-codes?language=objc#Keychain-Result-Codes

You can look at the Keychain Items being overwritten by running open /System/Library/CoreServices/Applications/Keychain\ Access.app and selecting "Open Keychain Access" instead of the suggested newer "Passwords" App. The item is called "Firefox Encrypted Storage".

It looks like rejecting access also prevents the deletion with errSecInvalidOwnerEdit unless no other application has access to the item.