Closed Bug 672811 Opened 9 years ago Closed 7 years ago

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

Categories

(Core :: Security: PSM, defect)

6 Branch
All
Other
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23
Tracking Status
firefox6 - ---

People

(Reporter: briansmith, Assigned: briansmith)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 9 obsolete files)

13.95 KB, patch
briansmith
: review+
cviecco
: review+
Details | Diff | Splinter Review
41.30 KB, patch
briansmith
: review+
Details | Diff | Splinter Review
+++ 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.
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.
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.
(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.
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?
(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.
(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.
Depends on: 672871
Not a blocker at this time, as pkix is not yet the default.
Severity: blocker → normal
Ok, not on by default so we won't track for 6. People (enterprises?) want to turn this on though?
We will switch it on by default in bug 651246.
Ok, not tracking for 6 as this is not on by default. Please let us know if people would run with this pref enabled.
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
Perfect, thanks for the info!
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.
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.
See Also: → 387024
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.
Assignee: nobody → bsmith
Attachment #596506 - Flags: review?(kaie)
Attachment #596506 - Attachment is obsolete: true
Attachment #596510 - Flags: review?(kaie)
Attachment #596506 - Flags: review?(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.
Attachment #596510 - Flags: review?(kaie) → review-
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?
This addresses your review comment by leaving the non-libpkix implementation almost completely unchanged.
Attachment #644100 - Flags: review?(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.
Attachment #644100 - Flags: review?(kaie) → review-
Attachment #596510 - Attachment is obsolete: true
No longer blocks: 813418
Depends on: 813418
Blocks: 813418
No longer depends on: 813418
Attached patch tests (v2) (a subset of cases) (obsolete) — Splinter Review
Attachment #728483 - Attachment is obsolete: true
Attachment #729020 - Flags: review?(bsmith)
Attachment #730191 - Flags: review?(bsmith)
Comment on attachment 729020 [details] [diff] [review]
tests (v2) (a subset of cases)

I reviewed this patch before the bitrot and after
Attachment #729020 - Flags: review+
Attachment #729020 - Flags: review+
Attachment #730191 - Flags: review+
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.
Attachment #729020 - Flags: review?(bsmith)
Attachment #730191 - Flags: review?(bsmith) → review+
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?
Attached patch test usages v3 (obsolete) — Splinter Review
Attachment #729020 - Attachment is obsolete: true
Attached patch test usages v3.1 (obsolete) — Splinter Review
Attachment #731977 - Attachment is obsolete: true
Attachment #731991 - Flags: review?(bsmith)
Attachment #731991 - Flags: review?(dkeeler)
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.)
Attachment #731991 - Flags: review?(dkeeler) → feedback+
Comment on attachment 731991 [details] [diff] [review]
test usages v3.1

clear review request on bsmith to address issues raised by keeler.
Attachment #731991 - Flags: review?(bsmith)
Attached patch test usages v4 (obsolete) — Splinter Review
Attachment #731991 - Attachment is obsolete: true
Attached patch test usages v4.1Splinter Review
forgot to fold fixes to pl script
Attachment #732960 - Attachment is obsolete: true
Attachment #732964 - Flags: review?(bsmith)
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.
Attachment #732964 - Flags: review?(bsmith) → review+
https://hg.mozilla.org/mozilla-central/rev/9c2a43214d74
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.