298045 - Importing a PKCS#7 cert chain needs to be improved

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Steve Parkinson]

To distribute a new root, the most used method is to put the root certificate
and other certificates in the chain in a degenerate PKCS#7 file on a webserver,
and have the user download it with the mime-type x-x509-cert-chain. The user is
then presented with a dialog, and asked to trust one of the certificates in the
file.

The current implementation works just fine for the simple case with a single
certificate, but anything more complex, the user ends up being asked to
trust the 'wrong' certificate

For example:

1) If the PKCS#7 cert chain includes a Root R and a subordinate S, the user
will be asked to trust S. I would contend that the user should be asked to
trust the Root R, not S.

2) If the PKCS#7 cert chain includes a Root R and several subordinates S1, S2,
S3 of that Root, the user will be asked to trust whichever of S1, S2, S3 is the
last in the file. Again, the user should be asked to trust R.

The relevant code is here:
http://lxr.mozilla.org/mozilla/source/security/manager/ssl/src/nsNSSCertificateDB.cpp#257

There are several assumptions in the code which are mistaken:

A) The code assumes the user should be asked to trust the leaf, not the root.

This is not the right behaviour.  The user should be asked to trust the
'root-most' certificate in the PKCS#7 file.


B) The code assumes that the certs in the PKCS#7 file are a linear chain ordered
sequentially Root-first, or leaf first:

If a hierarchy looks like this:
R->A->B->C    it is usually represented in the file as either R,A,B,C or C,B,A,R

However, there are other types of topology. For example I have one large
customer for example with a Root, and 16+ subordinates of that root.  They
want to be able to
 - import the root and a subordinates into firefox in a single package
 - have the user trust the root

In other words, the topology looks like this:
  />S1
 R->S2
  \>S3
And this is represented in the file as R,S1,S2,S3.

The code in PSM currently compares R,S1, determines that this is a 'forward'
progressing list, and then asks the user to trust the cert at the end of the
list, S3.  (I tried to cobble together a PKCS#7 file that would 'work', with
the root duplicated: R,S1,S2,S3, but NSS goes into an infinite loop when trying
to encode this).

I would also prefer not to have any dependency on the root-most cert actually
being a root (i.e. self-signed, with IssuerName=SubjectName), however, the
code should still work in such a case.

I would much prefer not to have any dependency on the ordering of the certs
in the PKCS#7 file. Instead, the code should build a tree of the certs in
the file, then ask the user to trust the top-most cert in the tree. To do
this, it would:
 for i: each cert in the PKCS#7 file {
    for j: each of the other certs {
       if (j.issuer == i.subject ) add i to j->children
    }
 }

Other notes:
 - It would be nice to present to the user a tree of all the certs being trusted
 - The behaviour should be undefined if we would end up with multiple 'tree's
   of certs - i.e. if there is more than one candidate for the 'root-most'
   certificate


It turns out that my customer would get the behaviour that they want if we
just correct A) above, and this is very easy to do.

Comment 1

[Steve Parkinson]

Attachment: Patch to swap leaf/root - to solve problem A

Comment 2

[Steve Parkinson]

A PKCS#7 package which can be used to demonstrate the problem is
available at this location:

http://dodpki.c3pki.chamb.disa.mil/rootca.html

When you try to import it, it will ask you to trust CA number 8, which is the
last in the list.

When the patch is applied, it should ask you to trust the Root CA, the first
in the list.

Comment 3

[Kai Engert (:KaiE:)]

I'm fine with this patch in general.

But I want to brainstorm what other implications this change will have in the
multiple cert situation. While I agree the existing code isn't very smart, I at
least want to ensure we don't make it worse.

The old code will ask the user to trust the leaf and it considered its root as
optional information.

The new code will ask the user to trust the root. All subordinate certs will be
installed, too, but can considered kind of optional, because by trusting the
root, the subordinates are automatically trusted, too.

So for any existing cert installation packages on the web, the change will mean:
The user will be asked to confirm trust at a higher level. So far so good. I'm
fine with this idea, since we are clearly displaying the user what he is going
to install.


But are there cases where we could possible break existing cert installation
packages?


What will happen if the downloaded root certificate is already stored in the
browser?


Importing will probably stop with the message "certificate already present" and
the user will not be prompted to give trust.

That's fine if the certificate is already trusted. The subordinate cert, that
the old code would have installed in addition, will automatically be trusted anyway.


However, if the root certificate is currently stored as untrusted, the new code
will cause the user experience to differ. The user will no longer be given the
change to manually trust the subordinate cert, but simply get a message about
the root being already installed. The user will have to fix the situation
manually using certificate manager.

Is everbody ok with this change of behaviour?

I don't mind, if the NSS team doesn't mind.

Comment 4

[Kai Engert (:KaiE:)]

Comment on attachment 189246 [details] [diff] [review]
Patch to swap leaf/root - to solve problem A

If nobody objects to changing the behaviour, I'm fine with this patch.

r=kaie

Comment 5

[Wan-Teh Chang]

Comment on attachment 189246 [details] [diff] [review]
Patch to swap leaf/root - to solve problem A

If we want to display the root cert (instead of the
leaf cert) of the cert chain, this patch accomplishes
that.

However, I am not sure that displaying the root cert
is right, although I agree it is more correct to trust
the root cert than the leaf cert.

By displaying the leaf cert, the user can view the
cert chain and see what's in the downloaded cert package.
If we display the root cert, the user doesn't know what's
in the package other than the root cert.  I suspect this
may be why the current code displays the leaf cert.

Two other remarks.

1. The cert package cited in this bug does not contain
a linear cert chain.  It contains a "tree" of certs,
with the last cert in the list being the root cert.
So the first 2 certs don't have an issuer relation between
them, and therefore this patch still can't handle this
cert package (we take the "else" case in the code and
display the first cert).  I will attach two patches
that can handle this cert package next.

2. The cert package cited in this bug report can't be
imported by Internet Explorer on Windows XP SP2 either.
I get an error dialog:
    Invalid Public Key Security Object File
      This file is invalid for use as the following: Security Certificate

Comment 6

[Wan-Teh Chang]

Attachment: Patch to swap leaf/root - to solve problem A (v2) (in 1.9 alpha, 1.8b5)

Like Steve's patch, this patch also displays the root
cert instead of the leaf cert.

The improvement over Steve's patch is that this patch
can handle not only a linear cert chain but also a
partially ordered cert "tree".	Like the current code,
this patch assumes that the root cert is either the
first or the last cert in the cert list.

I verified that this patch can handle the cited cert
package.

Comment 7

[Kai Engert (:KaiE:)]

Comment on attachment 190173 [details] [diff] [review]
Patch to swap leaf/root - to solve problem A (v2) (in 1.9 alpha, 1.8b5)

r=kaie

Comment 8

[Wan-Teh Chang]

Attachment: Patch to swap leaf/root - to solve problems A and B

This patch is more general.  It still assumes the cert
package only has a root cert, but the root cert doesn't
need to be either the first or the last cert in the
list.

I implemented a worst case O(n^2) method to find the
topmost cert in a list.  There may be a better algorithm
to find the maximum element in a partially ordered set.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

A few thoughts about this.

Some time ago we had a high-publicity vulnerability where the act of importing
a set of certs could import certs that weren't verifiable and would "poison"
the cert DB (bug 249004).  So we decided to implement a policy that henceforth 
we will only import certs that are verifiable (chain to a trusted issuer), or 
are themselves roots.  Now that we've achieved that, we must not introduce any 
regressions there.  (I'm not suggesting this patch does or doesn't.  Just 
reminding of this policy, which doesn't seem to be written down anywhere.)

There are times when it is appropriate to ask the user to trust the root cert
(or cert closest to the root) in a chain, and there are other times when it 
is not appropriate to do so.  For example, when a user visits an https server
that serves up a cert chain leading to an unknown root, and the user decides
to override the error about unknown or untrusted issuer, and the user 
decides to override this, we want the override to be for the leaf, not the 
root.  Similarly for signed email messages.  We don't want the user to have
accepted a new root simply as a consequence of having visited a rogue SSL 
site, or of having received an email signed with a rogue email cert.  
I don't know what all places in PSM call the function being changed, but we 
need to be sure (IMO) that this change won't cause users to start to be 
asked to approve a root CA in places where that's not appropriate.  

I agree that the order of the certs in a PKCS#7 signature shouldn't matter 
for purposes of choosing which one to trust.  We should be finding the 
root-most cert, not the first or last.  So, I like that Wan-Teh's patch
does that.

Comment 10

[Wan-Teh Chang]

Comment on attachment 190173 [details] [diff] [review]
Patch to swap leaf/root - to solve problem A (v2) (in 1.9 alpha, 1.8b5)

The method this patch modifies, nsNSSCertificateDB::handleCACertDownload,
is only used to import CA certs (whose "type" is nsIX509Cert::CA_CERT or
PSMContentDownloader::X509_CA_CERT).  We don't use this method to import
a server cert, user cert, or email cert.

This method currently displays and asks the user to trust the leaf-most
CA in package of CA certs.  We want to change the method to display and
ask the user the trust the root-most CA instead.

This method uses a simplistic algorithm to determine which CA is
leaf-most.  I continue to use the simplistic algorithm and adapted
it to find the root-most CA.  I will address the algorithm issue in
a separate patch later.

Comment 11

[Simon Fraser [no longer active]]

Which patch do you want review on?

Comment 12

[Wan-Teh Chang]

Comment on attachment 190179 [details] [diff] [review]
Patch to swap leaf/root - to solve problems A and B

Simon, please review the patch
"Patch to swap leaf/root - to solve problem A (v2)"
(attachment 190173 [details] [diff] [review]) first.  That patch is easier to
understand and has a lower risk.

I will attach another patch that implements a more general
algorithm for finding the root-most cert in an array of
certs.

Comment 13

[Robert Relyea]

Comment on attachment 190173 [details] [diff] [review]
Patch to swap leaf/root - to solve problem A (v2) (in 1.9 alpha, 1.8b5)

r=relyea

Comment 14

[Wan-Teh Chang]

Attachment: Patch to swap leaf/root - to solve problems A and B (v2)

It turns out that there is an efficient robust algorithm
to find the root (or rootmost) CA cert in the cert package.
This patch implements that algorithm -- it is a version of
the algorithm that finds the maximum integer in an array of
integers.  This patch also makes sure that the cert we display
and ask the user to trust is indeed a CA cert.	The code
already makes sure that the other certs, if any, are CA certs
before importing them, but it doesn't check the cert that
we select to display to the user.

Comment 15

[Wan-Teh Chang]

Comment on attachment 196990 [details] [diff] [review]
Patch to swap leaf/root - to solve problems A and B (v2)

Kai will submit a patch that implements the error dialog
when the selected cert is not a CA cert.  In the interest
of fixing the bug, this patch merely makes the cert import
silently fail.

Comment 16

[Wan-Teh Chang]

Comment on attachment 196990 [details] [diff] [review]
Patch to swap leaf/root - to solve problems A and B (v2)

This linear search algorithm is wrong.	Sorry.

Comment 17

[Wan-Teh Chang]

Attachment: Make sure the displayed cert is a CA cert (simple version) (in 1.9 alpha, 1.8b5)

This patch implements the test to make sure the cert
we display and ask the user to trust is indeed a CA
cert.  It silently fails if the cert is not a CA cert.

Comment 18

[Robert Relyea]

You can use the following to help any validation you need:

  // get an nsNSSCertificate structure for nsIX509Cert. This is used all over PSM
  // at some point we should change this to use a query interface for an 
  // nsNSSCertificate, but for now this works.
  nsNSSCertificate *nssCert = NS_STATIC_CAST(nsNSSCertificate*, aCert);

  // get an NSS Certificate. This is a new reference, you we will need to 
  // destroy it  with CERT_DestroyCertificate() or use...
  CERTCertificate *cert = nssCert->GetCert();
  if (!cert) return NS_ERROR_FAILURE;

  // a cleaner to free it when it goes out of scope...
  CERTCertificateCleaner certCleaner(cert);

With these structures, you have a little more flexibility in checking to see if
the cert is a CACert or not, or using NSS to look up a potential parent.

Now your linear loop can check for the root most cert as follows (psuedo code):

     for () {
        if (!isCA(cert)) continue;
        caFound++;
        if (is self issued (issuer == subject)) {
            this is it;
            break;
        }
        getparent (find cert by subject());
        // no parent found
        if (parent == NULL ) {
           this is it;
           break;
        }
    }

    // at this point we know 1) if we have a cert "this is it;", or 2) there are
    // no ca certs 'cafound == 0', or 3) there were ca certs whose parents we
    // have already imported, or the certs form a circular chain.
    // case 3) is the only place we need to clean up. In that case we need to
    // call a more expensive and complicated algorithm to determine just what
    // needs to be done. Case 3) should be exceedingly rare in a normal
    // deployment (though not so rare that we can just punt).

bob

Comment 19

[Robert Relyea]

Comment on attachment 197042 [details] [diff] [review]
Make sure the displayed cert is a CA cert (simple version) (in 1.9 alpha, 1.8b5)

assuming a new dialog patch.

bob

Comment 20

[Wan-Teh Chang]

Comment on attachment 197042 [details] [diff] [review]
Make sure the displayed cert is a CA cert (simple version) (in 1.9 alpha, 1.8b5)

Simon, this simple patch validates the input file.
The content type of the file is "CA cert".  We make
sure the file indeed contains a CA cert.

In this simple patch, I didn't implement an error
dialog in the case of failure because I don't know
how to do that.  Kai will write a patch for that.
My goal is to ensure that we don't simply trust what
the web site says the file contains.

Comment 21

[Simon Fraser [no longer active]]

Comment on attachment 197042 [details] [diff] [review]
Make sure the displayed cert is a CA cert (simple version) (in 1.9 alpha, 1.8b5)

Side comment: why isn't CERT_IsCACert() exposed via nsIX509Cert ?

Comment 22

[Robert Relyea]

Because nsIX509Cert isn't anywhere near complete;). It just has those interfaces
that were needed by the mozilla psm ui code when it was frozen. (sigh). A new
nsIX509Cert2 needs to be defined, and several functions need to be added to it.

bob

Comment 23

[Wan-Teh Chang]

Attachment: A non-CA cert whose content type is CA cert

Comment 24

[Wan-Teh Chang]

Comment on attachment 190173 [details] [diff] [review]
Patch to swap leaf/root - to solve problem A (v2) (in 1.9 alpha, 1.8b5)

I checked in this patch on the Mozilla trunk (1.9 alpha).

Comment 25

[Wan-Teh Chang]

Comment on attachment 197042 [details] [diff] [review]
Make sure the displayed cert is a CA cert (simple version) (in 1.9 alpha, 1.8b5)

I checked in this patch on the Mozilla trunk (1.9 alpha),
after testing it with the DoD CA cert package and the
non-CA cert package I attached to this bug.

Comment 26

[Nelson Bolyard (seldom reads bugmail)]

Is this bug all fixed now?  If not, what remains to be done?

Comment 27

[Wan-Teh Chang]

Comment on attachment 190173 [details] [diff] [review]
Patch to swap leaf/root - to solve problem A (v2) (in 1.9 alpha, 1.8b5)

Requesting approval1.8b5.

This patch contains two changes.
1. When importing a file that contains CA certificates,
we display and ask the user to trust the CA that's closest
to the root (instead of the CA that's closest to the leaf).
2. We make sure that the file indeed contains CA certificates.

It has been reviewed by Kai Engert (a former PSM developer),
Bob Relyea (NSS developer), and superreviewed by Simon Fraser.
Nelson Bolyard (NSS developer) reviewed the design, not the
code.

The risk of the patch is low.  It is already on the Mozilla
trunk.

Comment 28

[Wan-Teh Chang]

The remaining work is arguably all optional.
1. Implement a more robust algorithm to find
the root most cert in a collection of CA certs.
I proposed a brute force patch in attachment
190179 [details] [diff] [review].  Bob made some suggestions in comment 18.
2. If the root most cert is not a CA cert, pop
up an error dialog.  Kai sent me a patch by email
that doesn't work.

Comment 29

[Kai Engert (:KaiE:)]

Attachment: Patch to display an error message, if cert is not a CA cert

This patch works, I have tested it with both a CA cert (no error box
displayed), and the example that Wan-Tah attached to this bug (error message
"This is not a CA Certificate").

Is the wording ok?

Comment 30

[Kai Engert (:KaiE:)]

Comment on attachment 197833 [details] [diff] [review]
Patch to display an error message, if cert is not a CA cert

Bob, could you please review?

I noticed we currently do not have a shared function to display an error box.
And to display one, it takes more than 20 lines of code...

I have converted the code in an existing function to a shared one.

As an optimization, it is possible to pass the instance of the nssComponent to
the function.

Comment 31

[Wan-Teh Chang]

Comment on attachment 190173 [details] [diff] [review]
Patch to swap leaf/root - to solve problem A (v2) (in 1.9 alpha, 1.8b5)

I checked in this patch on the MOZILLA_1_8_BRANCH
(1.8b5).  It will be in Firefox 1.5 Beta 2.

Comment 32

[Wan-Teh Chang]

Comment on attachment 197042 [details] [diff] [review]
Make sure the displayed cert is a CA cert (simple version) (in 1.9 alpha, 1.8b5)

I checked in this patch on MOZILLA_1_8_BRANCH (1.8b5).

Comment 33

[Robert Relyea]

Comment on attachment 197833 [details] [diff] [review]
Patch to display an error message, if cert is not a CA cert

r+, with one caveat:

Please change the dialog text from:
"This is not a CA Certificate"
to:
"The downloaded certificate is not a CA certificate".

Comment 34

[Simon Fraser [no longer active]]

I'd prefer the error message to supply an nsIX509Cert* to the UI layer, so that
the UI can decide to show more details about the certificate if it wants to (see
also bug 306290). This would require using a different interface than nsIPrompt
(which just shows a generic dialog); it would require a new method on
nsICertificateDialogs{2}, perhaps a generic "ShowCertificateMessage()" or somesuch.

Comment 35

[Kai Engert (:KaiE:)]

Simon, I like your idea, but can we track it in a separate RFE bug? Maybe bug
306290 can be extended to implement the new interface and the new generic
"message with cert" dialog. I guess there are more cases, at least the one here,
the one mentioned in 306290, and the proposed new message in bug 310446 could
make use of it, too.

Comment 36

[Simon Fraser [no longer active]]

(In reply to comment #35)
> Simon, I like your idea, but can we track it in a separate RFE bug?

Sure

Comment 37

[Kai Engert (:KaiE:)]

Attachment: UI message Patch v2

Updated patch as requested by rrelyea

Comment 38

[Kai Engert (:KaiE:)]

Comment on attachment 198024 [details] [diff] [review]
UI message Patch v2

sfraser, could you please sr ? I will work on your other suggestion shortly.

Comment 39

[Simon Fraser [no longer active]]

Comment on attachment 198024 [details] [diff] [review]
UI message Patch v2

>Index: security/manager/ssl/src/nsNSSComponent.cpp
>===================================================================

I like to see
/* static */ in front of static methods in the .cpp file so that it's obvious
that it is static.

>+void nsNSSComponent::DisplayAlertMessage(const char *stringID, nsINSSComponent *nssComponent)
>+{
>+  if (!stringID || !*stringID)
>+    return;
>+
>+  nsCOMPtr<nsINSSComponent> my_nssComponent;
>+  nsresult rv;
>   
>+  if (!nssComponent)
>+  {
>+    my_nssComponent = do_GetService(kNSSComponentCID, &rv);
>+    if (NS_FAILED(rv))
>+      return;
>+    
>+    nssComponent = my_nssComponent;
>+  }

Maybe slightly simpler as:

nsCOMPtr<nsINSSComponent> my_nssComponent = nssComponent;
if (!my_nssComponent)
{
  my_nssComponent = do_GetService(kNSSComponentCID, &rv);
  ...

Comment 40

[Kai Engert (:KaiE:)]

Please see bug 310446 comment 4.

I've produced a new patch that combines the various requests to give user
feedback with certificate details.

Comment 41

[Kai Engert (:KaiE:)]

Comment on attachment 198024 [details] [diff] [review]
UI message Patch v2

I'm obsoleting the patch, I'd now prefer to use the mentioned combined patch.

Comment 42

[Asa Dotzler [:asa]]

did "Patch to swap leaf/root - to solve problem A (v2) (in 1.9 alpha, 1.8b5)"
land on the 1.8 branch? We're closed for b5 and if this hasn't landed, we'll
have to consider it for rc.

Comment 43

[Wan-Teh Chang]

Asa: yes, that patch landed on the 1.8 branch.  See comment 31.

Comment 44

[Kai Engert (:KaiE:)]

reassign bug owner.
mass-update-kaie-20120918

Comment 45

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

This appears to have been fixed.