672811 - When security.use_libpkix_verification=true, Certificates usage in Certificate Viewer is always shown as "could not verify this certificate for unknown reasons"

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

+++ This bug was initially created as a clone of Bug #670454 +++

Start Firefox with a new Profile just to be sure
Set preference security.use_libpkix_verification=true
Restart Firefox
Open Menu Preferences 
Select  Advanced | Encryption , View Certificates
Select tab certificates, scroll down to Thawte
Select first Thawte Certificate, click "view..." button 

(Actually the problem is not specific to this single certificate. I tried many more built-in root certificates and they all had the same issue.)



Actual results:

Certificate Viewer opens, it says "could not verify this certificate for unknown reasons"


Expected results:

Certificate Viewer opens, it should say "this certificates has been verified for the following uses" and a list of uses.

Comment 1

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

We need to clarify the semantics of (and test, if there isn't a test) CERT_PKIXVerifyCert regarding certificateUsageCheckAllUsages, to see which error code it returns when the certificate matches some but not all usages and there was no other failure. 

In particular, if the function failed because of SEC_ERROR_NO_MEMORY or other error other than "the certificate isn't valid for all the requested usages", we shouldn't silently ignore the error returned.

Comment 2

[Kai Engert (:KaiE:)]

I probably had picked certificateUsageCheckAllUsages as the usage for the call to CERT_PKIXVerifyCert without doublechecking that it works. It seems it doesn't.

I haven't done a full trace through libpkix (because that's tricky), but maybe there's no special handling for value certificateUsageCheckAllUsages (zero).

What I can say now, when called with certificateUsageCheckAllUsages/zero, the function quickly returns with "untrusted issue".

However, if I use the full list of potential usages (just as we call the classic API when we want to learn the allowed uses), then I get different results - our UI says "currently verifying", waiting for the async results - but no results come in, the UI remains in that waiting state...


With regards to return values and error codes, when given a list of usages on the parameter line, I suspect people expect identical behaviour as the classic API. Maybe Alexei and Bob have thoughts here?


What's definitively confusing, that this API allows to provide usages both as an immediate parameter, and nested inside CERTValInParam. This should get some clarifications and comments in the headers.

Comment 3

[Kai Engert (:KaiE:)]

(In reply to comment #2)
> However, if I use the full list of potential usages (just as we call the
> classic API when we want to learn the allowed uses), then I get different
> results - our UI says "currently verifying", waiting for the async results -
> but no results come in, the UI remains in that waiting state...

Ok, it remains in that state, because my experimental code crashed...
It crashes, because of an assertion in PKIX_PL_Cert_VerifyCertAndKeyType that requires only a single usage bit is set.

Comment 4

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

Kai, it is helpful to look at the source code for CERT_VerifyCertificate to understand what needs to be done. AFAICT, we need to have a version of CERT_VerifyCertificate that always internally calls PKIX_PKIXVerifyCert instead of cert_VerifyCertChain.

Perhaps a simpler alternative would be to simply remove the certificate usage display from the general tab of the certificate dialog. A user interested in the allowed usages could always inspect the details tab. Alex, do you think this would be acceptable?

Comment 5

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

(In reply to comment #4)
> Perhaps a simpler alternative would be to simply remove the certificate
> usage display from the general tab of the certificate dialog. A user
> interested in the allowed usages could always inspect the details tab. Alex,
> do you think this would be acceptable?

Never mind. We still want to display whether the certificate has been revoked or not, at least.

Comment 6

[Kai Engert (:KaiE:)]

(In reply to comment #4)
> Kai, it is helpful to look at the source code for CERT_VerifyCertificate to
> understand what needs to be done. AFAICT, we need to have a version of
> CERT_VerifyCertificate that always internally calls PKIX_PKIXVerifyCert
> instead of cert_VerifyCertChain.

When writing the patch for bug 479393, I had assumed we already have this feature in libpkix, because it has been clearly defined in the interface description of CERT_PKIXVerifyCert.

Unfortunately it seems it hasn't been implemented yet. I filed bug 672871.

Comment 7

[Kai Engert (:KaiE:)]

Not a blocker at this time, as pkix is not yet the default.

Comment 8

[LegNeato]

Ok, not on by default so we won't track for 6. People (enterprises?) want to turn this on though?

Comment 9

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

We will switch it on by default in bug 651246.

Comment 10

[LegNeato]

Ok, not tracking for 6 as this is not on by default. Please let us know if people would run with this pref enabled.

Comment 11

[Kai Engert (:KaiE:)]

At this point only developers, or people explicitly testing this feature, would set this pref.

It's even a hidden pref that you must add explicitly, so people cannot discover it in about:config

Comment 12

[LegNeato]

Perfect, thanks for the info!

Comment 13

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

I think that we can fix this without fixing bug 672871, by simply looping through each usage, verifying each one, and then ORing the successfully-validated ones together. That would make this part of the dialog update slightly slower, but I think that is acceptable. AFAICT, previously PSM used to work exactly like that, but it was changed to work the current way because it was taking too long for the dialog box to pop up, as the certificate chain was validated before showing the dialog. However, that concern doesn't apply anymore because we now validate the certificate chain asynchronously after/while showing the dialog box.

Comment 14

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

See bug 387024, which proposed something similar to what I proposed in comment 13, but in NSS instead of in PSM. IMO, it is better to just do it in PSM.

Comment 15

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

Attachment: Fix cert viewer usage display when using libpkix

Kai, this patch seems to fix the issue using the approach described above.

One concern I have is that libpkix does not return the same error codes in the same situations as the older verification library. This means that more errors have to be considered equivalent to "not valid for this usage." We may need to add more error codes to the error mapping in the future. But, this seems to work.

Comment 16

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

Attachment: Fix cert viewer usage display when using libpkix [v2]

Comment 17

[Kai Engert (:KaiE:)]

Comment on attachment 596510 [details] [diff] [review]
Fix cert viewer usage display when using libpkix [v2]

r- because you change the non-libpkix code path to call CERT_VerifyCertificateNow multiple times.

This non-libpkix code path should remain unchanged.

Comment 18

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

Kai, I agree with you. I will update the patch. Do you see any other problems with the patch, especially in the libpkix-based code path?

Comment 19

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

Attachment: Fix cert viewer usage display when using libpkix [v3]

This addresses your review comment by leaving the non-libpkix implementation almost completely unchanged.

Comment 20

[Kai Engert (:KaiE:)]

Comment on attachment 644100 [details] [diff] [review]
Fix cert viewer usage display when using libpkix [v3]

Brian, based on your recent statements, I think you should implement automated tests to assure that your code produces the correct output.

Comment 21

[Camilo Viecco (:cviecco)]

Attachment: test (a subset of cases)

Comment 22

[Camilo Viecco (:cviecco)]

hopefully this will work 

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

Comment 23

[Camilo Viecco (:cviecco)]

Attachment: tests (v2) (a subset of cases)

Comment 24

[Camilo Viecco (:cviecco)]

Attachment: Fix cert viewer usage display when using libpkix [v 3.1] 3 + bitrrot fixes

Comment 25

[Camilo Viecco (:cviecco)]

Attachment: Fix cert viewer usage display when using libpkix [v 3.2] 3 + bitrrot fixes+ spacecleanup

Comment 26

[Camilo Viecco (:cviecco)]

Comment on attachment 729020 [details] [diff] [review]
tests (v2) (a subset of cases)

I reviewed this patch before the bitrot and after

Comment 27

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

Comment on attachment 729020 [details] [diff] [review]
tests (v2) (a subset of cases)

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

Deal with the trailing whitespace and fix the missing spaces between tokens through the patches.

::: security/manager/ssl/tests/unit/test_certificate_usages/generate.pl
@@ +20,5 @@
> +my $db=$tmpdir;
> +
> +my @base_usages=("",
> +                 "certSigning,crlSigning",
> +                 "digitalSignature,nonRepudiation,keyEncipherment,dataEncipherment,keyAgreement,certSigning,crlSigning");

Add a entry that has an explicit key usage *without* certSigning.

@@ +24,5 @@
> +                 "digitalSignature,nonRepudiation,keyEncipherment,dataEncipherment,keyAgreement,certSigning,crlSigning");
> +
> +my @ee_usages=("",
> +               "digitalSignature,keyEncipherment,dataEncipherment",
> +               "digitalSignature,nonRepudiation,keyEncipherment,dataEncipherment,keyAgreement,certSigning,crlSigning");

Add a case for certSigning without any other key usages.

Comment 28

[Camilo Viecco (:cviecco)]

so after the two additions there is divergence on the values of the results using 'classic' vs 'pkix'.  In particular, when the CA that does NOT has certSigning :

ca usages: classic: Client,Server,Sign,Encrypt,Status Responder
           pkix:    Status Responder

ee_usages: classic  Client,Server,Sign,Encrypt(x3), ''
           pkix:    '' (x3)

clearly classic is broken: so the question shall we comment out the broken tests?

Comment 29

[Camilo Viecco (:cviecco)]

Attachment: test usages v3

Comment 30

[Camilo Viecco (:cviecco)]

Attachment: test usages v3.1

Comment 31

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Comment on attachment 731991 [details] [diff] [review]
test usages v3.1

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

Looks great - just a couple of comments. Probably not ready for prime-time, so I'll just mark feedback+.

::: security/manager/ssl/tests/unit/test_certificate_usages.js
@@ +1,2 @@
> +"use strict";
> +var Cc = Components.classes;

let instead of var

@@ +5,5 @@
> +var Cr = Components.results;
> +
> +/* To regenerate the certificates and apps for this test:
> +
> +        cd security/manager/ssl/tests/unit/test_certificate_usages

I think just security/manager/ssl/tests/unit, right?

@@ +22,5 @@
> +
> +// XXX from prio.h
> +const PR_RDWR        = 0x04;
> +const PR_CREATE_FILE = 0x08;
> +const PR_TRUNCATE    = 0x20;

it doesn't look like you use these

@@ +29,5 @@
> +Cu.import("resource://gre/modules/NetUtil.jsm", tempScope);
> +let NetUtil = tempScope.NetUtil;
> +
> +Cu.import("resource://gre/modules/FileUtils.jsm"); // XXX: tempScope?
> +Cu.import("resource://gre/modules/Services.jsm");  // XXX: tempScope?

it doesn't look like you use FileUtils or Services

@@ +107,5 @@
> +                ];
> +
> +
> +function run_test() {
> +  //ca's are one based!

Why not just make everything 0-based? Otherwise it will be confusing when you have to add/subtract 1, making it easy to make mistakes.

@@ +131,5 @@
> +      var ee_name = "ee-" + j + "-ca-" + i;
> +      var ee_filename =ee_name + ".der";
> +      //do_print("ee_filename" + ee_filename);
> +      var ee_cert_der =
> +        do_get_file("test_certificate_usages/"+ee_filename, false);

maybe spaces around the "+"

@@ +138,5 @@
> +      var ee_cert;
> +      ee_cert=certdb.findCertByNickname(null, ee_name);
> +      var verified = {};
> +      var usages = {};
> +      ee_cert.getUsagesString(true, verified,usages);

space here before "usages"

::: security/manager/ssl/tests/unit/test_certificate_usages/generate.pl
@@ +1,1 @@
> +#!/usr/bin/perl

I don't know perl that well, but I'll take a shot at looking at this.

@@ +11,5 @@
> +use strict;
> +
> +
> +my $srcdir=getcwd();
> +#my $tmpdir="/tmp/foo2/"; #$TMP/test_temp_dir

probably don't need this commented-out line anymore

@@ +12,5 @@
> +
> +
> +my $srcdir=getcwd();
> +#my $tmpdir="/tmp/foo2/"; #$TMP/test_temp_dir
> +my $tmpdir = tempdir( CLEANUP => 1 );

Later, you remove and then create this directory. a) is that the right thing to do? (if it exists, do you want to clobber it?) b) does CLEANUP => 1 not automatically remove the file when you're done with it?

@@ +18,5 @@
> +my $passwordfile=$tmpdir."/passwordfile";
> +my $ca_responses=$srcdir."/ca_responses";
> +my $ee_responses=$srcdir."/ee_responses";
> +
> +my $db=$tmpdir;

Why have both db and tmpdir if they're the same?

@@ +42,5 @@
> +    or die "system @args failed: $?";
> +}
> +
> +sub generate_certs(){
> +   for (my $i=1;$i<scalar(@base_usages)+1;$i++){

maybe space things out (e.g. $i = 1; $i < scalar(@base_usages) + 1;, etc.)

Comment 32

[Camilo Viecco (:cviecco)]

Comment on attachment 731991 [details] [diff] [review]
test usages v3.1

clear review request on bsmith to address issues raised by keeler.

Comment 33

[Camilo Viecco (:cviecco)]

Attachment: test usages v4

Comment 34

[Camilo Viecco (:cviecco)]

Attachment: test usages v4.1

forgot to fold fixes to pl script

Comment 35

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

Comment on attachment 732964 [details] [diff] [review]
test usages v4.1

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

::: security/manager/ssl/tests/unit/test_certificate_usages.js
@@ +37,5 @@
> +  fstream.init(file, -1, 0, 0);
> +  let data = NetUtil.readInputStreamToString(fstream, fstream.available());
> +  fstream.close();
> +  return data;
> +}

We should avoid duplicating code, even this test code. (We have had a lot of problems modifying tests in netwerk/ because of all the copy-pasting we did). Please put all the code duplicated between this test and the packaged app signing code in cert_head.js and reference cert_head.js in the xpcshell.ini head=cert_head.js.

@@ +44,5 @@
> +                 'SSL CA',
> +                 'Client,Server,Sign,Encrypt,SSL CA,Status Responder',
> +                 'Status Responder'];
> +/* XXX
> +  When using PKIX the usage for the fourth CA (key usage= all but certsigning) the

Instead of saying "fourth CA", put this comment directly above the problematic test case. That way, we don't have to keep this comment in sync as we insert/remove items in the array ahead of the fourth element.

@@ +55,5 @@
> +
> +  But the tests include -> basic CA asserted and Keyusage NOT asserted.
> +  In this case, the 'classic' mode uses the 'union' of the capabilites ->
> +  the cert is considered a CA. libpkix uses the 'intersection' of
> +  capabilites, the cert is NOT considered a CA.

Look to see if there is an existing bug against the classic code for this in the NSS product and if not, file one.

@@ +105,5 @@
> +
> +function run_test() {
> +  //ca's are one based!
> +  for (var i = 0; i < ca_usages.length; i++) {
> +    if ( i == 3) {

This is brittle. I would rather that we just put the wrong values the array above, with a comment saying what the correct values should be.

@@ +118,5 @@
> +    certdb.addCert(der, "CTu,CTu,CTu", ca_name);
> +
> +    do_print("ca_name=" + ca_name);
> +    var cert;
> +    cert=certdb.findCertByNickname(null, ca_name);

whitespace.

@@ +129,5 @@
> +
> +    //now the ee, names also one based
> +    for (var j = 0; j < ee_usages[i].length; j++) {
> +      var ee_name = "ee-" + (j + 1) + "-" + ca_name;
> +      var ee_filename =ee_name + ".der";

whitespace.

::: security/manager/ssl/tests/unit/test_certificate_usages/generate.pl
@@ +41,5 @@
> +   for (my $i = 1; $i < scalar(@base_usages) + 1; $i++) {
> +     my $ca_name = "ca-$i";
> +     my $ca_key_usage = $base_usages[$i - 1];
> +     if (length($ca_key_usage) > 1) {
> +       $ca_key_usage=" --keyUsage $ca_key_usage,critical";

whitespace.

Comment 36

[Camilo Viecco (:cviecco)]

post reviews:
  https://tbpl.mozilla.org/?tree=Try&rev=301026c823a1

Comment 37

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9c2a43214d74