verifying with certificateUsageVerifyCA always return failure

VERIFIED FIXED in Firefox 29

Status

()

defect
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: sciaticnerd, Assigned: cviecco)

Tracking

({regression, reproducible})

28 Branch
mozilla31
x86
Linux
Points:
---

Firefox Tracking Flags

(firefox29+ verified, firefox30+ verified, firefox31+ verified)

Details

Attachments

(4 attachments, 3 obsolete attachments)

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.
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
Component: Untriaged → Security: PSM
Product: Firefox → Core
An example of a group of trust chain certificates that no longer imports correctly has been attached per request.
I tried importing this on esr 24 at failed there too. Do you know the last version of firefox where importing succeded?
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → cviecco
v24 may have been the last time this worked (importing all non-expired from a PKCS7 formatted file).
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.
Posted patch fix-dod-import (obsolete) — Splinter Review
Posted patch test-anycacanreturnvalid (obsolete) — Splinter Review
Summary: Unable to import a group of root chain certificates → verifying with certificateUsageVerifyCA always return failure
Attachment #8398610 - Flags: review?(dkeeler)
Attachment #8398772 - Flags: review?(dkeeler)
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?
Attachment #8398610 - Flags: review?(dkeeler) → review+
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
Attachment #8398772 - Flags: review?(dkeeler) → review+
(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
(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.
Flags: needinfo?(cviecco)
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!
(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
Posted patch update-tests-por-passing (obsolete) — Splinter Review
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.
Attachment #8399610 - Flags: review?(dkeeler)
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?)
Attachment #8399610 - Flags: review?(dkeeler) → review-
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 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
Attachment #8399610 - Flags: review- → review+
keeping r+ from keeler
Attachment #8399610 - Attachment is obsolete: true
Attachment #8400253 - Flags: review+
(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?)
(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?
(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.
(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.
(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)
Keeping r+ from keeler
Attachment #8398610 - Attachment is obsolete: true
Attachment #8402693 - Flags: review+
Keeping r+t from keeler
Attachment #8398772 - Attachment is obsolete: true
Attachment #8402694 - Flags: review+
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.
Attachment #8402693 - Flags: approval-mozilla-aurora?
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.
Attachment #8400253 - Flags: approval-mozilla-aurora?
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.
Attachment #8402694 - Flags: approval-mozilla-aurora?
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.
Attachment #8402693 - Flags: approval-mozilla-beta?
Comment on attachment 8402693 [details] [diff] [review]
fix-dod-import-final

Folded patch including tests is great
Attachment #8402693 - Flags: approval-mozilla-beta?
Attachment #8402693 - Flags: approval-mozilla-beta+
Attachment #8402693 - Flags: approval-mozilla-aurora?
Attachment #8402693 - Flags: approval-mozilla-aurora+
Attachment #8402694 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8400253 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
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?
When I roll back this change only, by build completes successfully.  Perhaps it is something in my build environment.
Apparently, branch out of range means that the jump is too far away...maybe I'll play with some of the assembler options.
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
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).
Looks like my issue is caused by this bug: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43961
Confirmed as working in v29!!! Thank you, team Mozilla!!!! Awesome work!

You guys ROCK.
You need to log in before you can comment on or make changes to this bug.