1603221 - certificates enterprise policy should use nsIX509CertDB.isCertTrusted instead of nsIX509CertDB.asyncVerifyCertAtTime

Status: RESOLVED FIXED

(Firefox :: Enterprise Policies, defect, P1)

Description

[Victor Hoang]

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Firefox/68.0

Steps to reproduce:

setup: Clean windows device without firefox installed, and have 14 certificates ready to be installed. Also have a policies.json file ready for insertion.

1. Start off with a Windows device and install firefox 68.2 or higher (32 bit en-US). do not open firefox until policies.json file is placed in the distribution folder, and certs are placed in the certs directory I created.

2. go into C:\Program Files (x86)\Mozilla Firefox\distribution and place the policies.json in there to consume certificates. Do not open firefox.

(note: my policies.json file consumes certificates from a directory called: "C:\Program Files (x86)\Mozilla Firefox\certs" because the default path firefox uses to consume certificates requires a specific user name. I prefer to create the certs in the C drive where the path is not relative when creating in the "%APPDATA%\mozilla\firefox\certificates" directory.)

3. place the 14 certificates into "C:\Program Files (x86)\Mozilla Firefox\certs", the policies.json file will point here and pull the 14 certificates.

4. Open firefox and check the about:preferences and go to Privacy & Security to check the certificates.

5. check about:policies to see if my certificates are enabled and spelled correctly

Actual results:

I observe that only 5 of my 14 certificates are installed. I've even tried mitigating this by using the default directory for consuming certificates afterwards with the same results for versions 68.2 ESR and 68.3 ESR.

I tested the normal versions (68, 70, and 71) and they all imported the 14 certificates, regardless of the directory (such as the firefox default certificates folder, and the one I created called "certs")

Expected results:

All 14 certificates should have imported correctly. The last working version that imported correctly on a clean install was 68.1 ESR.

Comment 1

[Mike Kaply [:mkaply]]

I've gone through the 68.1->68.2 checkins and can't find anything that stands out (or would be ESR specific)

https://hg.mozilla.org/releases/mozilla-esr68/pushloghtml?fromchange=FIREFOX_68_1_0esr_RELEASE&tochange=FIREFOX_68_2_0esr_RELEASE

Dana, do you have any idea on this one? I know we had one other person report this.

Comment 2

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

Not off the top of my head.

Can you attach the 14 certificates to this bug?

Comment 3

[Victor Hoang]

Unfortunately I don't think I can, but I will be happy to figure out if i'm allowed to do so from my company.

If not, I would be happy to do a share screen session and work with and meet with whomever would be willing to help. (I operate on Pacific time but can meet at times that work best for both parties).

Can we pursue both routes? I will ask if I'm allowed to share the certs, and someone try to meet with me? my email is [e-mail] and I check pretty regularly.

Comment 4

[Victor Hoang]

I was just wondering if there's any word on this.

Just wondering If I include the certs, is there a way to keep it private from the general public? I would like to keep them confidential as possible.

Thanks,
Victor

Comment 5

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

Can you email them to me? Another option would be to upload them as an attachment to this bug but mark it private (checkbox near the bottom).

Comment 6

[Victor Hoang]

Hey Dana,

I couldn't find the mark private button for attachment on the page, so I emailed them to you a few minutes ago.

Let me know if that is sufficient. Thanks!
Victor

Comment 7

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

Thanks! Can you include the policies.json file you're using as well?

Comment 8

[Victor Hoang]

Done (also by email).

Comment 9

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

This is probably due to bug 1541927, where we added code to first see if a given certificate was already a valid CA before importing it and modifying its trust, because doing that can prompt the user for their profile password. Depending on the order in which a set of certificates are imported, if some of those certificates are actually issuers of other certificates in the same set, importing the issuers first can cause the later certificates to not be imported, because they will already chain up to a valid issuer. Now, this may not actually be a problem, depending on how the TLS servers the user needs to connect to are configured. If they include the intermediate CAs in the TLS handshake, then it won't matter that Firefox didn't already have them as trust anchors, because Firefox can build a trusted path through the intermediates in the handshake to a previously-imported root. Unfortunately we can't rely on this - TLS servers often don't include any intermediates. Note also that importing intermediate certificates as trust anchors is a bit odd. In theory doing this can cause problems with e.g. key pinning, but in practice Firefox doesn't even enforce pins for connections where the root has been imported by default, so this probably doesn't matter.

That all said, I think we can do what we want by using nsIX509CertDB.isCertTrusted instead of nsIX509CertDB.asyncVerifyCertAtTime.

Comment 10

[Mike Kaply [:mkaply]]

This is probably due to bug 1541927, where we added code to first see if a given certificate was already a valid CA before importing it and modifying its trust, because doing that can prompt the user for their profile password.

I was wondering that as well, but it's odd that this problem doesn't occur on release at all and seems to have broken between 68.1 and 68.2 ESR (the fix went in 68)

Comment 11

[Victor Hoang]

morning all,

read through Dana's comments and just chiming in in case there is anything i can do/test from my end.

I'm assuming i'll need to validate/test for mac users as well?

Comment 12

[Victor Hoang]

hello,

Just wondering if we've confirmed that this is a bug that will be addressed? If so just wondering if anyone is working on it. For now my organization will probably have to remain on 68.1 unless something can be done to address this.

Again, willing to help send/test wherever possible.

Comment 13

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

Yes, this is a bug that will be addressed. Thank you for the information you've provided.

Comment 14

[Victor Hoang]

Hello all,

Just thought I'd chime in again and verified a couple things we might want to keep in mind:

• i tried on macOS 68.3 and it has the same problem (probably already knew that)
• when a user clicks refresh firefox from about:support, it goes into the "fresh install" state where it doesnt inherit any of the old certificates pre68.2 or 68.3. In otherwords, if a user was using a version 68.1 or older, upgraded, and decided to run a "refresh firefox", they would be refreshed to the broken state in which 68.2 or 68.3 was initially installed, so the certs will disappear as if they did a fresh install of 68.2 or 68.3

That's all, thanks! (also checked on macOS for 71 and it worked fine)

Comment 15

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

The priority flag is not set for this bug.
:mkaply, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 16

[Victor Hoang]

Sorry to sound like a broken record...just wondering on which security patch this fix might be rolled out with? (just for tentative scheduling purposes)

Also, happy new year!

Comment 17

[Mike Kaply [:mkaply]]

I'm working on this next week. I'm hoping to have it in for the Feb 11 Firefox release (ESR and regular)

Comment 18

[Mike Kaply [:mkaply]]

I have the beginning of a patch, but I'm running into one problem.

If the cert doesn't exist, this code:

https://searchfox.org/mozilla-central/source/security/manager/ssl/nsNSSCertificateDB.cpp#697

returns an NS_ERROR_FAILURE which shows up in the console even if I catch it.

It seems like if this function is supposed to return true/false, it shouldn't be doing NS_ERROR_FAILURE in some of these cases.

[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIX509CertDB.isCertTrusted]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: resource:///modules/policies/Policies.jsm :: onBeforeAddons/reader.onloadend :: line 272" data: no]

Comment 19

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

Yeah, that should return NS_OK there. Feel free to fix that (and make that not a one-line-if) (maybe add a comment along the lines of "CERT_GetCertTrust returns SECFailure if given a temporary cert that doesn't have any trust information yet. This isn't an error.)"

Comment 20

[Mike Kaply [:mkaply]]

Attachment: Bug 1603221 - Use isCertTrusted instead of asyncVerify to check for policy installed certs. r?keeler

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/831b95c06ae1
Use isCertTrusted instead of asyncVerify to check for policy installed certs. r=keeler

Comment 22

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/831b95c06ae1

Comment 23

[Mike Kaply [:mkaply]]

Sorry, I don't know how I accidentally marked this fixed.

Comment 24

[Mike Kaply [:mkaply]]

Comment on attachment 9119522 [details]
Bug 1603221 - Use isCertTrusted instead of asyncVerify to check for policy installed certs. r?keeler

Beta/Release Uplift Approval Request

• User impact if declined: Policy can't install more than a few certificates
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce: Has been verified by client
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Policy only
• String changes made/needed:

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Policy only
• User impact if declined: Policy can't install more than a few certificates
• Fix Landed on Version: 75
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Policy only.
• String or UUID changes made by this patch:

Comment 25

[Mike Kaply [:mkaply]]

Sorry for the delay on this, was waiting for customer to verify.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9119522 [details]
Bug 1603221 - Use isCertTrusted instead of asyncVerify to check for policy installed certs. r?keeler

Fixes a bug limiting the ability to install certificates via policy. Approved for 73.0b8. Will revisit the ESR request after this has baked a bit.

Comment 27

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/releases/mozilla-beta/rev/75a81bd77eae

Comment 28

[Victor Hoang]

Following up Mike's comment,

I've validated that it worked on the nightly build and I will be ready/available to test when the change makes it to an ESR variant/track.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9119522 [details]
Bug 1603221 - Use isCertTrusted instead of asyncVerify to check for policy installed certs. r?keeler

Approved for 68.5esr.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/430298b13c46