987816 - verifying with certificateUsageVerifyCA always return failure

Status: VERIFIED FIXED

(Core :: Security: PSM, defect)

Description

[sciaticnerd]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux i686; rv:28.0) Gecko/20100101 Firefox/28.0 (Beta/Release)
Build ID: 20140317233339

Steps to reproduce:

To add the root chains needed in Firefox 28, a .p7b file containing everything, root and intermediates, is downloaded.  Then open the Certificates browser, go to the authorities tab, and click Import. Place a check in each of the three boxes and click OK.


Actual results:

I get an error that says, "This certificate can't be verified and will not be imported. The certificate issuer might be unknown or untrusted, the certificate might have expired or been revoked, or the certificate might not have been approved." 

Alternatively, I tried importing the root first, to establish the trust, and then import the rest, but it didn't work.  Only the first root chain in the .p7b's list imported.  Any ideas what changed or if the method to import a bunch of roots at once has changed?


Expected results:

The root chain and all the intermediates should have been imported, creating a new subgroup within the Authorities tab of the Certificates browser.

Comment 1

[Daniel Veditz [:dveditz]]

There are lots of reasons this could be failing, from bugs in the code to tougher cert standards (do any use MD5 as the signature hash?). Can't really say for sure without trying it... is the .p7b in question available? If it's on the public web you could just add a link to the bug, otherwise an attachment would be great

Comment 2

[sciaticnerd]

Attachment: An example of a .p7b file containing multiple root chain certificates that doesn't import correctly anymore.

An example of a group of trust chain certificates that no longer imports correctly has been attached per request.

Comment 3

[Camilo Viecco (:cviecco)]

I tried importing this on esr 24 at failed there too. Do you know the last version of firefox where importing succeded?

Comment 4

[Camilo Viecco (:cviecco)]

Actually 24-esr  imports non-expired cerificates in the pkcs7 structure correctly. nightly only imports the first certificate in the bundle. Looking at the code, 28 (current relase) has the same issue as nightly.

Comment 5

[sciaticnerd]

v24 may have been the last time this worked (importing all non-expired from a PKCS7 formatted file).

Comment 6

[sciaticnerd]

Just confirmed this morning that firefox v27.0.1 does allow the import of the non-expired roots from the .p7b file. Hope that's helpful.

Comment 7

[Camilo Viecco (:cviecco)]

Attachment: fix-dod-import

Comment 8

[Camilo Viecco (:cviecco)]

Attachment: test-anycacanreturnvalid

Comment 9

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

Comment on attachment 8398610 [details] [diff] [review]
fix-dod-import

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

I'm fine with this to fix the immediate regression, but I have two concerns:
1. Do you know why we removed handling this usage in the first place?
2. Why are we even trying to verify CA certificates when importing them? If they're self-signed, they won't verify (and indeed the user probably wants to explicitly trust them), and if they're not self-signed, then either they chain to a trusted root or don't, but it only matters when trying to verify an end-entity certificate later on.

::: security/certverifier/CertVerifier.cpp
@@ +144,5 @@
>          break;
>        case certificateUsageObjectSigner:
>          enumUsage = certUsageObjectSigner;
>          break;
> +      case  certificateUsageVerifyCA:

nit: two spaces after 'case'

@@ +145,5 @@
>        case certificateUsageObjectSigner:
>          enumUsage = certUsageObjectSigner;
>          break;
> +      case  certificateUsageVerifyCA:
> +        enumUsage =  certUsageVerifyCA;

nit: two spaces after '='

@@ +477,5 @@
>      case certificateUsageSSLCA:
>      case certificateUsageEmailSigner:
>      case certificateUsageEmailRecipient:
>      case certificateUsageObjectSigner:
> +    case certificateUsageVerifyCA:

Looks like we removed this (and other usages) in bug 878932. Do you know why?

Comment 10

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

Comment on attachment 8398772 [details] [diff] [review]
test-anycacanreturnvalid

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

r=me with comments addressed

::: security/manager/ssl/tests/unit/head_psm.js
@@ +102,5 @@
>  
> +function checkCertErrorGeneric(cert, expectedError, usage) {
> +  let hasEVPolicy = {};
> +  let verifiedChain = {};
> +  let error = certdb.verifyCertNow(cert, usage,

what certdb does this reference?

@@ +103,5 @@
> +function checkCertErrorGeneric(cert, expectedError, usage) {
> +  let hasEVPolicy = {};
> +  let verifiedChain = {};
> +  let error = certdb.verifyCertNow(cert, usage,
> +                                   NO_FLAGS, verifiedChain, hasEVPolicy);

nit: re-organize these two lines so the first one has as much on it as possible before it reaches 80 characters

@@ +104,5 @@
> +  let hasEVPolicy = {};
> +  let verifiedChain = {};
> +  let error = certdb.verifyCertNow(cert, usage,
> +                                   NO_FLAGS, verifiedChain, hasEVPolicy);
> +  // expected error == 1 is a special marker for any error is OK

"expectedError == -1"

::: security/manager/ssl/tests/unit/test_certificate_usages.js
@@ +123,5 @@
>      var cert = certdb.findCertByNickname(null, ca_name);
>      cert.getUsagesString(true, verified, usages);
>      do_print("usages.value=" + usages.value);
>      do_check_eq(ca_usages[i], usages.value);
> +    if (-1 != ca_usages[i].indexOf('SSL CA')) {

nit: ca_usages[i].indexOf('SSL CA') != -1

Comment 11

[Camilo Viecco (:cviecco)]

(In reply to David Keeler (:keeler) from comment #9)
> Comment on attachment 8398610 [details] [diff] [review]
> fix-dod-import
> 
> Review of attachment 8398610 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm fine with this to fix the immediate regression, but I have two concerns:
> 1. Do you know why we removed handling this usage in the first place?
I was an error that was introduced when we tried to optimize verifications as
in certain conditions anyCA did not do anything.

> 2. Why are we even trying to verify CA certificates when importing them? If
> they're self-signed, they won't verify (and indeed the user probably wants
> to explicitly trust them), and if they're not self-signed, then either they
> chain to a trusted root or don't, but it only matters when trying to verify
> an end-entity certificate later on.
The code creates a temporary cert and makes it trusted (according to the UI 
settings), then it makes the temporary cert permanent if it can validate (
has appropiate extensions/values)).


> 
> ::: security/certverifier/CertVerifier.cpp
> @@ +144,5 @@
> >          break;
> >        case certificateUsageObjectSigner:
> >          enumUsage = certUsageObjectSigner;
> >          break;
> > +      case  certificateUsageVerifyCA:
> 
> nit: two spaces after 'case'
> 
> @@ +145,5 @@
> >        case certificateUsageObjectSigner:
> >          enumUsage = certUsageObjectSigner;
> >          break;
> > +      case  certificateUsageVerifyCA:
> > +        enumUsage =  certUsageVerifyCA;
> 
> nit: two spaces after '='
> 
> @@ +477,5 @@
> >      case certificateUsageSSLCA:
> >      case certificateUsageEmailSigner:
> >      case certificateUsageEmailRecipient:
> >      case certificateUsageObjectSigner:
> > +    case certificateUsageVerifyCA:
> 
> Looks like we removed this (and other usages) in bug 878932. Do you know why?
Premature optimization and insufficient testing :o

Comment 12

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

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/dd433d12561b for XPCshell orange: https://tbpl.mozilla.org/php/getParsedLog.php?id=36908138&tree=Mozilla-Inbound

Comment 13

[Camilo Viecco (:cviecco)]

(In reply to Wes Kocher (:KWierso) from comment #12)
> Backed out in
> http://hg.mozilla.org/integration/mozilla-inbound/rev/dd433d12561b for
> XPCshell orange:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=36908138&tree=Mozilla-
> Inbound

Thank you.

Comment 14

[sciaticnerd]

It's really encouraging to see this issue getting worked on.  Just a quick question though.  Are any resources available, like a roadmap, that could be kept up with that might highlight PSM and certificate related information?  I did some digging, but the existing documentation seems to run out sometime last year.  What am I missing?  Thank you!

Comment 15

[Camilo Viecco (:cviecco)]

(In reply to sciaticnerd from comment #14)
> It's really encouraging to see this issue getting worked on.  Just a quick
> question though.  Are any resources available, like a roadmap, that could be
> kept up with that might highlight PSM and certificate related information? 
> I did some digging, but the existing documentation seems to run out sometime
> last year.  What am I missing?  Thank you!

Offtopic: you can take a look at our current 2014 q1 goals (defining q2 goals
is our task for this week).https://wiki.mozilla.org/SecurityEngineering/2014/Q1Goals

Comment 16

[Camilo Viecco (:cviecco)]

Attachment: update-tests-por-passing

Comment 17

[Camilo Viecco (:cviecco)]

Comment on attachment 8399610 [details] [diff] [review]
update-tests-por-passing

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

We need to update test_cert_trust as this test actually tried to use verifyCA to match the new behavior.

Comment 18

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

Comment on attachment 8399610 [details] [diff] [review]
update-tests-por-passing

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

::: security/manager/ssl/tests/unit/test_cert_trust.js
@@ +51,5 @@
>    check_cert_err_generic(ee_cert, useMozillaPKIX ? 0
>                                                   : SEC_ERROR_INADEQUATE_CERT_TYPE,
>                           certificateUsageObjectSigner); // expected
>    check_cert_err_generic(ee_cert, useMozillaPKIX ? SEC_ERROR_CA_CERT_INVALID
> +                                                 : 0,

Isn't this an error? Why should an end-entity cert ever succeed when verified as a CA? (Or is that not what certificateUsageVerifyCA means? What does it mean?)

Comment 19

[Camilo Viecco (:cviecco)]

Comment on attachment 8399610 [details] [diff] [review]
update-tests-por-passing

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

::: security/manager/ssl/tests/unit/test_cert_trust.js
@@ +51,5 @@
>    check_cert_err_generic(ee_cert, useMozillaPKIX ? 0
>                                                   : SEC_ERROR_INADEQUATE_CERT_TYPE,
>                           certificateUsageObjectSigner); // expected
>    check_cert_err_generic(ee_cert, useMozillaPKIX ? SEC_ERROR_CA_CERT_INVALID
> +                                                 : 0,

Yes it means try to verify as a CA. In all of the cases for ee the test does NOT includes the basic constraint extension so InsanityPKIX (strictly following rfc 5280) says: This cert is not valid a as a CA for SSL, email or oject signing.

'Classic' nss does not enforce that, and since the ee cert does not include any EKU or KU then it claims it can be an CA.

Comment 20

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

Comment on attachment 8399610 [details] [diff] [review]
update-tests-por-passing

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

Thank you for clarifying. r=me with comments addressed.

::: security/manager/ssl/tests/unit/test_cert_trust.js
@@ +52,5 @@
>                                                   : SEC_ERROR_INADEQUATE_CERT_TYPE,
>                           certificateUsageObjectSigner); // expected
>    check_cert_err_generic(ee_cert, useMozillaPKIX ? SEC_ERROR_CA_CERT_INVALID
> +                                                 : 0,
> +                         certificateUsageVerifyCA); // expected. No bc in mozpkix

Maybe a more descriptive comment would be helpful. I'm thinking something like "mozilla::pkix enforces that certificates have a basic constraints extension with cA:true to be CA certificates, whereas classic does not."

@@ +75,5 @@
>                                                   : SEC_ERROR_INADEQUATE_CERT_TYPE,
>                           certificateUsageObjectSigner);
>    check_cert_err_generic(ee_cert, useMozillaPKIX ? SEC_ERROR_CA_CERT_INVALID
> +                                                 : 0,
> +                         certificateUsageVerifyCA); // expected. No bc in mozpkix

we probably only need a comment on the first one of these

Comment 21

[Camilo Viecco (:cviecco)]

 https://tbpl.mozilla.org/?tree=Try&rev=af0500f075c1

Comment 22

[Camilo Viecco (:cviecco)]

Attachment: update-tests-por-passing (v 1.1)

keeping r+ from keeler

Comment 23

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

https://hg.mozilla.org/mozilla-central/rev/311bb33950fd
https://hg.mozilla.org/mozilla-central/rev/276f44d9b36a
https://hg.mozilla.org/mozilla-central/rev/6dac076d25f7

Comment 24

[sciaticnerd]

(In reply to Camilo Viecco (:cviecco) from comment #15)
> (In reply to sciaticnerd from comment #14)
> > It's really encouraging to see this issue getting worked on.  Just a quick
> > question though.  Are any resources available, like a roadmap, that could be
> > kept up with that might highlight PSM and certificate related information? 
> > I did some digging, but the existing documentation seems to run out sometime
> > last year.  What am I missing?  Thank you!
> 
> Offtopic: you can take a look at our current 2014 q1 goals (defining q2 goals
> is our task for this
> week).https://wiki.mozilla.org/SecurityEngineering/2014/Q1Goals

Thank you so much! (even if it was off topic!  Where would a better place to ask that question have been, please?)

Comment 25

[sciaticnerd]

(In reply to Wes Kocher (:KWierso) from comment #23)
> https://hg.mozilla.org/mozilla-central/rev/311bb33950fd
> https://hg.mozilla.org/mozilla-central/rev/276f44d9b36a
> https://hg.mozilla.org/mozilla-central/rev/6dac076d25f7

So it looks like this will be fixed in v31?  Is that right?

Comment 26

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

(In reply to sciaticnerd from comment #25)
> (In reply to Wes Kocher (:KWierso) from comment #23)
> > https://hg.mozilla.org/mozilla-central/rev/311bb33950fd
> > https://hg.mozilla.org/mozilla-central/rev/276f44d9b36a
> > https://hg.mozilla.org/mozilla-central/rev/6dac076d25f7
> 
> So it looks like this will be fixed in v31?  Is that right?

Should be, yes.

Comment 27

[Camilo Viecco (:cviecco)]

(In reply to sciaticnerd from comment #25)
> (In reply to Wes Kocher (:KWierso) from comment #23)
> > https://hg.mozilla.org/mozilla-central/rev/311bb33950fd
> > https://hg.mozilla.org/mozilla-central/rev/276f44d9b36a
> > https://hg.mozilla.org/mozilla-central/rev/6dac076d25f7
> 
> So it looks like this will be fixed in v31?  Is that right?

Correct, Next step is to wait and make uplift requests for aurora and beta.

Comment 28

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

(In reply to sciaticnerd from comment #24)
> Thank you so much! (even if it was off topic!  Where would a better place to
> ask that question have been, please?)

IRC is a good place to ask questions: https://wiki.mozilla.org/IRC (particularly #security)

Comment 29

[Camilo Viecco (:cviecco)]

Attachment: fix-dod-import-final

Keeping r+ from keeler

Comment 30

[Camilo Viecco (:cviecco)]

Attachment: test-anycacanreturnvalid (v1.1)

Keeping r+t from keeler

Comment 31

[Camilo Viecco (:cviecco)]

Comment on attachment 8402693 [details] [diff] [review]
fix-dod-import-final

[Approval Request Comment]
Bug caused by (feature/regressing bug #): regression on Bug 891066 when we incorrectly optimized out the certificateUsageVerifyCA use case.
User impact if declined: Uses will not be able to import some CA files, in particular importing the DoD CA root file fails. 
Testing completed (on m-c, etc.):  Landed on MC on April 2 2014
Risk to taking this patch (and alternatives if risky): No risk (but the set of patches must be landed together, prefer a folded patch that includes the tests?)
String or IDL/UUID changes made by this patch: None.

Comment 32

[Camilo Viecco (:cviecco)]

Comment on attachment 8400253 [details] [diff] [review]
update-tests-por-passing (v 1.1)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): regression on Bug 891066 when we incorrectly optimized out the certificateUsageVerifyCA use case.
User impact if declined: Uses will not be able to import some CA files, in particular importing the DoD CA root file fails. 
Testing completed (on m-c, etc.):  Landed on MC on April 2 2014
Risk to taking this patch (and alternatives if risky): No risk (but the set of patches must be landed together, prefer a folded patch that includes the tests?)
String or IDL/UUID changes made by this patch: None.

Comment 33

[Camilo Viecco (:cviecco)]

Comment on attachment 8402694 [details] [diff] [review]
test-anycacanreturnvalid (v1.1)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): regression on Bug 891066 when we incorrectly optimized out the certificateUsageVerifyCA use case.
User impact if declined: Uses will not be able to import some CA files, in particular importing the DoD CA root file fails. 
Testing completed (on m-c, etc.):  Landed on MC on April 2 2014
Risk to taking this patch (and alternatives if risky): No risk (but the set of patches must be landed together, prefer a folded patch that includes the tests?)
String or IDL/UUID changes made by this patch: None.

Comment 34

[Camilo Viecco (:cviecco)]

Comment on attachment 8402693 [details] [diff] [review]
fix-dod-import-final

[Approval Request Comment]
Bug caused by (feature/regressing bug #): regression on Bug 891066 when we incorrectly optimized out the certificateUsageVerifyCA use case.
User impact if declined: Uses will not be able to import some CA files, in particular importing the DoD CA root file fails. 
Testing completed (on m-c, etc.):  Landed on MC on April 2 2014
Risk to taking this patch (and alternatives if risky): No risk (but the set of patches must be landed together, prefer a folded patch that includes the tests?)
String or IDL/UUID changes made by this patch: None.

Comment 35

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8402693 [details] [diff] [review]
fix-dod-import-final

Folded patch including tests is great

Comment 36

[Camilo Viecco (:cviecco)]

folded try aurora push:
https://tbpl.mozilla.org/?tree=Try&rev=c87cc3a9e6f4

Comment 37

[Nathan Toone [:toonetown]]

With this change applied, I can no longer compile firefox-30 for armv7.  I get the following error:

/Developer/Android/ndk/toolchains/arm-linux-androideabi-4.8/prebuilt/darwin-x86_64/bin/arm-linux-androideabi-g++ -o CertVerifier.o -c -fvisibility=hidden -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -DSTATIC_EXPORTABLE_JS_API -DNO_NSPR_10_SUPPORT -I/Users/ntoone/.gecko-dev/security/certverifier -I. -I/Users/ntoone/.gecko-dev/security/certverifier/../insanity/include -I../../dist/include -I/Users/ntoone/.gecko-dev/objdir-android.armv7/dist/include/nspr -I/Users/ntoone/.gecko-dev/objdir-android.armv7/dist/include/nss -I/Users/ntoone/.gecko-dev/objdir-android.armv7/dist/include -fPIC -idirafter /Developer/Android/ndk/platforms/android-9/arch-arm/usr/include -DMOZILLA_CLIENT -include ../../mozilla-config.h -MD -MP -MF .deps/CertVerifier.o.pp -idirafter /Developer/Android/ndk/platforms/android-9/arch-arm/usr/include -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Werror=int-to-pointer-cast -Wtype-limits -Wempty-body -Wsign-compare -Wno-invalid-offsetof -mandroid -fno-short-enums -fno-exceptions -Wno-psabi -march=armv7-a -mthumb -mfpu=vfpv3-d16 -mfloat-abi=softfp -mno-unaligned-access -isystem /Users/ntoone/.gecko-dev/build/stlport/stlport -isystem /Developer/Android/ndk/sources/cxx-stl/system/include -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -std=gnu++0x -pipe -DNDEBUG -DTRIMMED -g -Os -freorder-blocks -fno-reorder-functions -fomit-frame-pointer /Users/ntoone/.gecko-dev/security/certverifier/CertVerifier.cpp
/Users/ntoone/.gecko-dev/security/certverifier/CertVerifier.cpp: In member function 'SECStatus mozilla::psm::CertVerifier::VerifyCert(CERTCertificate*, const SECItem*, SECCertificateUsage, PRTime, void*, mozilla::psm::CertVerifier::Flags, insanity::pkix::ScopedCERTCertList*, SECOidTag*, CERTVerifyLog*)':
/Users/ntoone/.gecko-dev/security/certverifier/CertVerifier.cpp:173:22: warning: 'enumUsage' may be used uninitialized in this function [-Wmaybe-uninitialized]
     *validationChain = CERT_GetCertChainFromCert(cert, time, enumUsage);
                      ^
/Users/ntoone/.gecko-dev/security/certverifier/CertVerifier.cpp:127:16: note: 'enumUsage' was declared here
   SECCertUsage enumUsage;
                ^
/Users/ntoone/.gecko-dev/security/certverifier/CertVerifier.cpp: At global scope:
/Users/ntoone/.gecko-dev/security/certverifier/CertVerifier.cpp:852:3: fatal error: opening dependency file .deps/CertVerifier.o.pp: No such file or directory
 } } // namespace mozilla::psm
   ^
compilation terminated.
{standard input}: Assembler messages:
{standard input}:1224: Error: branch out of range

Comment 38

[Camilo Viecco (:cviecco)]

Nathan are you sure it is because of this? I do you have extra compilation values? Notice that I am not changing the delcaration of enumUsage, just adding a new value in its computation. also the try build was green. How are you compiling?

Comment 39

[Nathan Toone [:toonetown]]

When I roll back this change only, by build completes successfully.  Perhaps it is something in my build environment.

Comment 40

[Nathan Toone [:toonetown]]

Apparently, branch out of range means that the jump is too far away...maybe I'll play with some of the assembler options.

Comment 41

[Ryan VanderMeulen [:RyanVM]]

This was pushed to beta.

https://hg.mozilla.org/releases/mozilla-beta/rev/b192379d4d65

Comment 42

[Ryan VanderMeulen [:RyanVM]]

This was landed on Aurora nearly a week ago, though the bug was never marked as such.
https://hg.mozilla.org/releases/mozilla-aurora/rev/22251d6a3bb4

Comment 43

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

I've verified the fix using 29 Beta 9 (Build ID: 20140417185217), latest 30 Aurora (Build ID: 20140417004004) and latest 31 Nightly (Build ID: 20140418030202), on Win 7 x64, Win 8 x64, Mac OS X 10.92, and Ubuntu 12.10 x86.

When importing the .p7b file, all non-expired certificates are added without error (error shows only for expired ones).

Comment 44

[Nathan Toone [:toonetown]]

Looks like my issue is caused by this bug: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43961

Comment 45

[sciaticnerd]

Confirmed as working in v29!!! Thank you, team Mozilla!!!! Awesome work!

You guys ROCK.