950268 - "delete or distrust" on a root CA cert doesn't persist across restarts

Status: VERIFIED FIXED

(Core :: Security: PSM, defect)

Description

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

STR:
1. Visit google.com, determine what root certificate signs for it (e.g. "GlobalTrust Global CA")
2. Find that certificate in the certificate manager, click "delete or distrust" (the certificate will disappear from the manager until it is reopened (which is also a bug, but less serious) - at that point its trust flags should all be cleared (see "edit trust"))
3. Shift-reload to clear the cache on google.com - this should result in an untrusted certificate warning
4. Restart the browser, visit google.com again - it should show the untrusted certificate warning again. Instead, the load succeeds. Back in the certificate manager, the root certificate's trust flags are all back.

Comment 1

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

This was discovered via the dev.security.policy mailing list: https://groups.google.com/forum/#!topic/mozilla.dev.security.policy/3lcacXCHeIA

Comment 2

[David E. Ross]

This is a major discrepancy with implications for secure browsing.  Users will not know that root certificates marked as untrusted (Certificate Manager's Delete or Distrust button) become trusted again when the browser is terminated and then relaunched.  

A user might check the trust settings via Certificate Manager's Edit Trust button upon marking the certificate as untrusted, but how many user would do that later after terminating and relaunching the browser?  

I immediately marked PM/SGDN's root IGC/A as untrusted when the news broke about the improper use of one of its intermediate certificates and after noting its further delay of 1.5 to 2 years in implementing Mozilla policies and CA/Browser Forum's Baseline Requirements.  Having just updated from SeaMonkey 2.22.1 to 2.23, I thought I would check some of the other seven root certificates that I previously marked as untrusted.  I found that all of them -- including IGC/A -- were again trusted.  This is scary.

Comment 3

[Daniel Veditz [:dveditz]]

This sounds suspiciously like Bug 917380 which we (thought) fixed in Firefox 26. But worse -- that bug was only a problem for "EV" certs.

Comment 4

[Daniel Veditz [:dveditz]]

I was unable to block https://www.turktrust.com.tr at all, even after removing all the turktrust roots not just the one shown in the cert details dialog. Is that one cross-signed, and our dialog doesn't show the same chain as the one we validated?

I was able to follow the steps and prevent https://www.google.com from loading by removing trust from the GeoTrust root. When I restarted Firefox the site was still blocked and the trust bits for GeoTrust were still removed. When I re-added them the site started working again.

I'm using a Nightly (Firefox 29).

Has anyone confirmed the problem with the version the user was using, and if so which versions? It's possible that the user had a corrupted key/cert database and the changes to the trust bits were only kept in memory and could not be flushed to disk. That would be a problem, of course, but not nearly as bad a problem as having broken trust bits for everyone. It would also need to be fixed by each affected user, probably by deleting those files from the profile. WARNING: you may have personal certs in there that you need to back up before taking such a step.

Comment 5

[Daniel Veditz [:dveditz]]

David: did you reproduce the problem (if so, which version(s)?) or are you just reporting on the dev-security thread?

Comment 6

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

(In reply to Daniel Veditz [:dveditz] from comment #4)
> I was unable to block https://www.turktrust.com.tr at all, even after
> removing all the turktrust roots not just the one shown in the cert details
> dialog. Is that one cross-signed, and our dialog doesn't show the same chain
> as the one we validated?

I believe there is an issue with the dialog not necessarily showing the same chain as was used to validate the certificate at handshake time.

(In reply to Daniel Veditz [:dveditz] from comment #5)
> David: did you reproduce the problem (if so, which version(s)?) or are you
> just reporting on the dev-security thread?

Yes - I can reproduce the problem. I'm working on a regression range now. Have you tried with a fresh profile?

Comment 7

[Matt Wobensmith [:mwobensmith][:matt:]]

On FF26, clean profile, I can repro the problem in comment 0, except that the page always loads, even after the explicit distrust. The SSL lock icon for the page remains intact and I see no SSL errors.

However, after the distrust step, looking at the cert UI, it indicates that the cert is untrusted.

Comment 8

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

From what I can tell, this is the first bad changeset:
https://hg.mozilla.org/mozilla-central/rev/a8ff177abfbe
(which is bug 802378)

Comment 9

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Image pointing out the THREE TurkTrust roots we currently have

I noticed that if I use "edit trust" to remove the "This certificate can identifiy websites" bit from all three TurkTrust roots, and then I clear my cache and restart Firefox, then I get the untrusted certificate error on https://www.turktrust.com.tr. However, if I use "delete or distrust" then the website starts working again.

Comment 10

[Camilo Viecco (:cviecco)]

The issue seems to be inside nsCertTree::DeleteEntryObject. 
Two things here:
1. We remove the cert from the ui even when we think we cannot delete the cert from the db
2. The branch that removes trust from undeletable certs is never reached.

Comment 11

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

Attachment: patch

Camilo and I discussed this a bit on irc:

13:28 cviecco | I have an idea, but will change the behavior slightly.
13:29 cviecco | the issue is here: https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsCertTree.cpp#807
13:30 cviecco | This branch is never true. So we never untrust the cert to be deleted.
13:30  keeler | interesting
13:30  keeler | I did notice one other thing that I think is an issue, but I'm not sure it's the only one
13:30 cviecco | the other issue is line 835, where the cert is deleted from the treeview even tough the cert is not actually deleted.
13:30  keeler | yeah, I would file another bug on that
13:31  keeler | anyway, in GetDispInfoAtIndex, when returning the pointer, it should just be 'return raw.forget();', not all the other junk with NS_IF_ADDREF, etc.
13:31  keeler | (line 317
13:31  keeler | )
13:32 cviecco | sure, but that (while important) Is mostly cosmetic. Here is my proposal: for line 807:
13:33 cviecco | instead for chekcing for this conditional, always remove the trusts for CA certs.
13:33  keeler | actually, that fixes the bug for me (I'm not entirely sure why)
13:33 cviecco | (check if the cert is a CA_
13:34 cviecco | my giess is the NS_IFADDREF changes the value of the usage? (and how did you got to line 317?_
13:36  keeler | I basically took out changes from the bad changeset until it didn't fail - the failing change seemed to be related to switching to TemporaryRef
13:36  keeler | for that function
13:39 cviecco | We can take your chage or mine. what do you think?
13:40 cviecco | (yours might be cleaner) but mine easier to read.
13:42  keeler | I'm not sure. I think both issues might be part of the bug and/or other bugs.
13:42  keeler | I would like to know what the refcounting issue is such that using .forget() works while the other way doesn't...
13:42  keeler | I think it may be something to do with this:
13:43  keeler | https://mxr.mozilla.org/mozilla-central/source/mfbt/RefPtr.h#177
13:47 cviecco | my guess was that we are using a constructor instead of an assigment to a pointer. an thus the addref affects a different pointer?
13:47 cviecco | but this is only a guess.
13:52  keeler | hmmm - I think it was using the constructor at https://mxr.mozilla.org/mozilla-central/source/mfbt/RefPtr.h#222 which does an addref (I think), but using forget() uses the private constructor on line 237, which doesn't
13:59 cviecco | Lets use your change as it affects less code.
14:00 cviecco | do you want to take over the bug? (I can review).
14:00  keeler | ok - sounds good

Comment 12

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Please re-review the changeset that was checked in for bug 802378 (not the patch assigned to the bug, but the actual checkin) to see if there are more instances of this type of problem of using the assignment operator when we should be using a constructor.

Comment 13

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

The issue wasn't assignment operator vs. constructor, the issue was with the conversion from   already_AddRefed<nsCertTreeDispInfo> to TemporaryRef<nsCertTreeDispInfo> (which only happens in that one place, so the rest of the changeset is fine in that regard). AIUI, already_AddRefed<T> will not increment the refcount of T (because it is expected the programmer has already called NS_ADDREF/NS_IF_ADDREF or otherwise ensured that the refcount won't be decreased upon scope exit). TemporaryRef<T>, when constructed with a bare pointer, will essentially wrap a RefPtr<T>, which increases the refcount. The refcount will be decremented when the TemporaryRef<T> goes out of scope. So, in this case, this leaves an extra NS_IF_ADDREF which leaks the nsCertTreeDispInfo (and consequently the CERTCertificate we wanted to distrust).
On shutdown, we call UnloadLoadableRoots(), which apparently has the effect of replacing the CERTCertificate's read-only slot with a non-read-only one. After UnloadLoadableRoots, we call mShutdownObjectList->evaporateAllNSSResources() which eventually destroys the nsNSSCertificate wrapping the CERTCertificate in question. At that point (in nsNSSCertificate::destructorSafeDestroyNSSReference()), it sees that the certificate is marked for permanent deletion and that its slot is not read-only, so it deletes it. This deletes the entry with the trust bits turned off that overrides the built-in entry.
Fixing this leak means the certificate is destructed when it has the correct slot information (meaning it doesn't delete the override we just added). However, I am concerned about the order in which we shutdown NSS - shouldn't we call mShutdownObjectList->evaporateAllNSSResources() before UnloadLoadableRoots()?

Comment 14

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

Attachment: test

This adds a test that opens and closes the certificate manager. The test harness observes the leaked memory without the patch and observes no leaked memory with it. (See also bug 839193 for a more complicated version of the same kind of test.)
I don't know if you're working today, but even if you are, don't feel like you have to get to this today - we can finish this up after the break. (I think bugzilla whines are turned off over the break, so this hopefully won't spam you repeatedly...)

Comment 15

[Camilo Viecco (:cviecco)]

Comment on attachment 8350798 [details] [diff] [review]
test

Review of attachment 8350798 [details] [diff] [review]:
-----------------------------------------------------------------

David, does this test actually test the leak? the only thing I see is opening and closing the cert-manager. IE Does this test fails wihtout the fix? (otherwise it is an r+, just wanted an aswer to this before I r+)

Comment 16

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

(In reply to Camilo Viecco (:cviecco) from comment #15)
> Comment on attachment 8350798 [details] [diff] [review]
> test
> 
> Review of attachment 8350798 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> David, does this test actually test the leak? the only thing I see is
> opening and closing the cert-manager. IE Does this test fails wihtout the
> fix? (otherwise it is an r+, just wanted an aswer to this before I r+)

Right - it's a bit strange. The test harness checks for leaks itself, so there's no need to check for them in the test (if that's even possible). When I wrote this, I tested it without the fix, and the harness reported leaks and called it a failure.

Comment 17

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

Let me know if the above is sufficient explanation or if you want me to add more comments in the test (or if those comments aren't clear).

Comment 18

[Camilo Viecco (:cviecco)]

Comment on attachment 8350798 [details] [diff] [review]
test

Review of attachment 8350798 [details] [diff] [review]:
-----------------------------------------------------------------

OK. Please addd a comment. (I whished that it acually deleted a cert, but if this catches the previous issue then is OK).

Comment 19

[Camilo Viecco (:cviecco)]

r+ with the issue solved.

Comment 20

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

Attachment: patch + test

Thanks, Camilo. I put in a more descriptive comment. Carrying over r+.
https://hg.mozilla.org/integration/mozilla-inbound/rev/47193746e11c

Comment 21

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

Oh, I also meant to say yes, I agree it would be great if we had tests that made sure the certificate manager worked as expected, but since I think we should re-do the entire thing anyway, I don't think it's worthwhile to write tests right now when we're just going to have to re-write them in the near future.

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/47193746e11c

Comment 23

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

Comment on attachment 8357862 [details] [diff] [review]
patch + test

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: memory leak. Also, users who have distrusted root CAs will find them unexpectedly trusted upon restart.
User impact if declined: ^
Fix Landed on Version: 29
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 802378
User impact if declined: (see above)
Testing completed (on m-c, etc.): has been on m-c for a while
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 24

[Matt Wobensmith [:mwobensmith][:matt:]]

This bug is currently being verified by Mozilla contributor Nagasahas. So, if anyone else was thinking about doing so, best to wait until they've finished their testing. Thank you.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/67e3616f0b17
https://hg.mozilla.org/releases/mozilla-beta/rev/92017dd2c9f1

Please nominate this for b2g26 uplift as well.

Comment 26

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

Comment on attachment 8357862 [details] [diff] [review]
patch + test

NOTE: This flag is now for security issues only. Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 802378
User impact if declined: memory leak. Also, users who have distrusted root CAs will find them unexpectedly trusted upon restart. However, since the certificate manager UI isn't exposed on B2G, I don't think this bug will ever be encountered.
Testing completed: on m-c
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8357862 [details] [diff] [review]
patch + test

(In reply to David Keeler (:keeler) from comment #26)
> User impact if declined: memory leak. Also, users who have distrusted root
> CAs will find them unexpectedly trusted upon restart. However, since the
> certificate manager UI isn't exposed on B2G, I don't think this bug will
> ever be encountered.

Nevermind then, thanks :)

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr24/rev/2ab54230159b

Comment 29

[Paul Silaghi, QA [:pauly]]

per comment 24

Comment 30

[Matt Wobensmith [:mwobensmith][:matt:]]

I'm working with Nagasahas, and we'll update this bug shortly with test results.

Comment 31

[Nagasahas DS]

Confirmed issue in Firefox 26.
Verified fixed in Firefox 27, 2014-01-21
Verified fixed in Firefox 28, 2014-01-21
Verified fixed in Firefox 29, 2014-01-22

Comment 32

[Matt Wobensmith [:mwobensmith][:matt:]]

Verified fixed on 24esr by Nagasahas as well.

Comment 33

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Clearing NEEDINFO for me. Dan, if your issue wasn't addressed by others, please let me know what I can help with.

Comment 34

[Jack]

I'd like to reopen this issue, as I can still reproduce this bug in Firefox ESR 31.2.0 (which is what the latest Tor Browser is based on). As Tor engineer confirmed, he was also able to reproduce such issue.

Config: Debian Wheezy 7.7 32bit, Firefox ESR 31.2.0 (Tor Browser 4.0.1 Linux 32bit version)

Below approaches to distrust/delete CNNIC root and China Internet Network Information Center EV certificate root had been tried. But neither of they worked.

1. "Delete or distrust" two CA root in Preference/Advanced/Certificates. But after restarting browser, these CA roots were still there. And websites signed by CNNIC SSL (for example https://www.cnnic.cn) can still be opened without any warning.

2. In Preference/Advanced/Certificates, clicked "Edit Trust" and unchecked all the boxes. However clicking "OK" cannot be close it, while only "Cancel" worked. Then restarted the
browser and those unchecked boxes were checked again, while websites signed by CNNIC SSL can still be  opened without any warning.

Comment 35

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

Jack, it would be best if you opened a new bug for this issue. Thanks.

Comment 36

[Jack]

OK, new bug 1100734 was just created for it.