Closed Bug 1228175 Opened 4 years ago Closed 4 years ago

IsCertBuiltInRoot implementation should definitely be improved

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox45 --- affected
firefox48 --- fixed

People

(Reporter: keeler, Assigned: keeler)

Details

Attachments

(1 file)

(This is particularly relevant to key pinning (HPKP)).

IsCertBuiltInRoot is the way we determine if a certificate is a built-in root (that it, it shipped with the browser and didn't come from the user, an administrator, a PKCS11 device, etc.). It's implemented as a string comparison with the token name from each PKCS11 slot the certificate is present on. This isn't very rigorous and can go wrong if, for example, another PKCS11 slot decides to call itself the "Builtin Object Token". (See https://twitter.com/sleevi_/status/669667027640979456 ).

I imagine there's a way to see if one of the slots the certificate is present on is the same as the builtin DB we load at startup.
This is even worse than I originally thought. It turns out that when modifying the trust settings for a built-in root, this essentially moves the root to the softoken (the "NSS Certificate DB" slot). From some testing, it seems that while the call to PK11_GetAllSlotsForCert works as intended during the run of firefox when the trust changed, after closing and reopening firefox, it will return only the "NSS Certificate DB" slot, which means IsCertBuiltinRoot now considers that root not to be a builtin. Since we enforce more security properties (e.g. pinning) when roots are thought to be builtins, we really don't want false negatives like this.
Assignee: nobody → dkeeler
Summary: IsCertBuiltInRoot implementation should probably be improved → IsCertBuiltInRoot implementation should definitely be improved
When a built-in root certificate has its trust changed from the default value,
the platform has to essentially create a copy of it in the read/write
certificate database with the new trust settings. At that point, the desired
behavior is that the platform still considers that certificate a built-in root.
This would indeed happen for the duration of that run of the platform, but as
soon as it restarted, the certificate in question would only appear to be from
the read/write database, and thus was not considered a built-in root. This patch
changes the test of built-in-ness to explicitly search the built-in certificate
slot for the certificate in question. If found, it is considered a built-in
root.

Review commit: https://reviewboard.mozilla.org/r/38589/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38589/
Attachment #8727657 - Flags: review?(mgoodwin)
Attachment #8727657 - Flags: review?(cykesiopka.bmo)
Comment on attachment 8727657 [details]
MozReview Request: bug 1228175 - fix IsCertBuiltInRoot r?Cykesiopka r?mgoodwin

https://reviewboard.mozilla.org/r/38589/#review35429

Looks good to me.
Attachment #8727657 - Flags: review?(mgoodwin) → review+
Comment on attachment 8727657 [details]
MozReview Request: bug 1228175 - fix IsCertBuiltInRoot r?Cykesiopka r?mgoodwin

https://reviewboard.mozilla.org/r/38589/#review35459

Mostly looks good, but I'm going to withhold r+ pending an answer to the Service Workers question below. If it's a non-issue, then r+.

::: security/manager/ssl/nsIX509Cert.idl:44
(Diff revision 1)
> +  readonly attribute bool isBuiltInRoot;

Will this break Service Workers again? See Bug 1247580 and Bug 1248628.

I admit to not being 100% clear on how adding attributes affects the serialisation of things like nsIX509Cert.

If it does break Service Workers, maybe as a hack we can define an equivalent method in nsIX509CertDB instead.

::: security/manager/ssl/tests/compiled/TestIsCertBuiltInRoot.cpp:1
(Diff revision 1)
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

Does this file work as a GTest?

If it does, we should probably file a follow up to convert the test over since GTests are apparently preferred now.

::: security/manager/ssl/tests/compiled/TestIsCertBuiltInRoot.cpp:102
(Diff revision 1)
> +GetCertDER(void* arg, SECItem** certs, int numcerts)

Nit: |arg| is unused so maybe comment it out.

::: security/manager/ssl/tests/compiled/TestIsCertBuiltInRoot.cpp:133
(Diff revision 1)
> +  CERTCertificate* cert = CERT_NewTempCertificate(CERT_GetDefaultCertDB(),

Looks like a smart pointer can be used instead.

::: security/manager/ssl/tests/compiled/TestIsCertBuiltInRoot.cpp:227
(Diff revision 1)
> +  // (This isn't done automatically since this test doesn't have a
> +  // lot of the other boilerplate components that would otherwise
> +  // keep the certificate db alive longer than we want it to.)

To be honest I don't quite get what this sentence is trying to convey. Maybe I'm missing something.
Attachment #8727657 - Flags: review?(cykesiopka.bmo)
https://reviewboard.mozilla.org/r/38589/#review35459

Thanks for the review!

> Will this break Service Workers again? See Bug 1247580 and Bug 1248628.
> 
> I admit to not being 100% clear on how adding attributes affects the serialisation of things like nsIX509Cert.
> 
> If it does break Service Workers, maybe as a hack we can define an equivalent method in nsIX509CertDB instead.

It shouldn't - as I understand it, the issue was with changing the UUID. Since those don't change now, we don't have to worry here. (The other issue would be if two versions had incompatible serialization routines, but this isn't the case here either, since this doesn't change what data is saved.) In any case, there seems to be a test now, so it should catch if this causes a regression.

> Does this file work as a GTest?
> 
> If it does, we should probably file a follow up to convert the test over since GTests are apparently preferred now.

I don't think so - this test needs to set up an NSS cert DB in a particular way that probably wouldn't be doable if other tests that rely on NSS are also running.

> Nit: |arg| is unused so maybe comment it out.

Ok, sounds good.

> Looks like a smart pointer can be used instead.

Yep.

> To be honest I don't quite get what this sentence is trying to convey. Maybe I'm missing something.

I cargo-culted it from TestCertDB.cpp. I think the intent is that normally some other service would set up the environment appropriately, but if we just invoked that service, it would also cause NSS to be initialized and kept alive when the test wasn't expecting it to be? It might not be relevant to the new test - I'm trying it out on OS X now without it.
Attachment #8727657 - Flags: review?(cykesiopka.bmo)
Comment on attachment 8727657 [details]
MozReview Request: bug 1228175 - fix IsCertBuiltInRoot r?Cykesiopka r?mgoodwin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38589/diff/1-2/
Comment on attachment 8727657 [details]
MozReview Request: bug 1228175 - fix IsCertBuiltInRoot r?Cykesiopka r?mgoodwin

https://reviewboard.mozilla.org/r/38589/#review35659

Cool, looks good.
Attachment #8727657 - Flags: review?(cykesiopka.bmo) → review+
Well, after a day spent getting an android build working, and then another day getting debugging cppunittests on the android build working, and finally figuring out what all is going on, I've come to the conclusion that this just won't work on android for now. Basically, since IsCertBuiltInRoot now has to use a localized string to locate the builtin root module, those strings have to be available at test time. For whatever reason, the files that provide these aren't pushed to the android devices/emulators when running cppunittests (see bug 929655 for a similar situation). In lieu of the full test, I'll add an xpcshell test as a spot-check that the new IsCertBuiltInRoot implementation works at all on android.
Flags: needinfo?(dkeeler)
Comment on attachment 8727657 [details]
MozReview Request: bug 1228175 - fix IsCertBuiltInRoot r?Cykesiopka r?mgoodwin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38589/diff/2-3/
Ok - try looked good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f20855b97aa
mgoodwin, if you could have a quick look at the interdiff, that'd be great (the LazyLogModule change is because I rebased the patch, I think).
Flags: needinfo?(mgoodwin)
Still looks good to me.
Flags: needinfo?(mgoodwin)
https://hg.mozilla.org/mozilla-central/rev/86c4213bc628
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.