CERT_SortCBValidity should prefer a root certificate to a "newer" non-root certificate

ASSIGNED
Assigned to

Status

NSS
Libraries
P1
normal
ASSIGNED
4 years ago
4 years ago

People

(Reporter: Rob Stradling, Assigned: Rob Stradling)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

4 years ago
Created attachment 8435008 [details] [diff] [review]
In CERT_SortCBValidity(), prefer a root certificate to a "newer" non-root certificate

Some (newer) BuiltIn root certificates are cross-certified by other (older) BuiltIn root certificates.  In some cases a cross-certificate has a more recent notBefore date than the associated root certificate, and in other cases the reverse is true.

In order to minimize the number of OCSP lookups and certificate signature verifications that need to be performed, NSS should ideally select the shortest candidate chain that terminates with a BuiltIn.  However, since this would require switching from depth-first search to breadth-first search, a simpler approach which should work in most cases (although see bug 1020237 comment 3) would be to modify CERT_SortCBValidity() so that it prefers a root certificate to a "newer" non-root certificate.

The attached patch, which does just that, was originally attached to bug 1020237.

If this patch looks OK, then maybe the code it adds should actually be moved to the top of the CERT_SortCBValidity() function.

However, I'm not sure that this patch is OK.  In some cases, selecting a longer chain that includes 1 or more cross-certificates is likely to be more useful.  For example, when deciding which intermediate certs to attach to an S/MIME signed email, the more certs the better, because the sender won't know which root certificates are trusted by the recipient.  So it might be better to have different sorting rules for different contexts.
(Assignee)

Updated

4 years ago
Blocks: 1020237
(Assignee)

Updated

4 years ago
Attachment #8435008 - Flags: review?(brian)
Assignee: nobody → rob
Comment on attachment 8435008 [details] [diff] [review]
In CERT_SortCBValidity(), prefer a root certificate to a "newer" non-root certificate

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

Rob, thanks for the patch. I agree that this is a good idea.

The comments that describes what this function does would be misleading/incomplete with your change, because they don't describe the preference of root certificates over non-root certificates. Please update the comments in cert.h and replace the comment in certdh.c with something like "/* See the documentation in cert.h". In your update, please describe the desired priority.

Fortunately, CERT_SortCBValidity is not exported. That means we could also give it a more descriptive name of we wanted to. But, I think the existing name is OK if the documentation is updated.

My expectation is that we want to prefer certificates where the given time is within the certificate's validity period over certificates where the given time is not within the certificate's validity period. When the given time is within both cert's validity periods or not within both, we want to prefer root certificates over non-root certificates. And, then we break ties based on which certificates are newer according to the times in notAfter and notBefore. Is that right?

If my understanding is correct, then shouldn't you put your (certa->isRoot != certb->isRoot) check right after the (aNotValid && !bNotValid) check? If not, could you please explain why we need to do these two checks first?

    /* a and b are either valid or not valid */
    if ( newerbefore && newerafter ) {
	return(PR_TRUE);
    }
    
    if ( ( !newerbefore ) && ( !newerafter ) ) {
	return(PR_FALSE);
    }
Attachment #8435008 - Flags: review?(brian) → feedback+
Rob, is it important that this change be included in Firefox 31, which will be the first version of Firefox that uses mozilla::pkix? Or, is it OK to defer it to Firefox 32? I think NSS 3.16.2, which will be the version of NSS used in Firefox 31, will be finalized very soon, so it is important to know this ASAP.
(Assignee)

Comment 3

4 years ago
(In reply to Brian Smith from comment #2)
> Rob, is it important that this change be included in Firefox 31, which will
> be the first version of Firefox that uses mozilla::pkix? Or, is it OK to
> defer it to Firefox 32? I think NSS 3.16.2, which will be the version of NSS
> used in Firefox 31, will be finalized very soon, so it is important to know
> this ASAP.

This change can wait until Firefox 32.  It's an optimization, not a bugfix.  So I'd rather take the time to do this right, rather than risk introducing a bug!
(Assignee)

Comment 4

4 years ago
(In reply to Brian Smith from comment #1)
<snip>
> Rob, thanks for the patch. I agree that this is a good idea.
> 
> The comments that describes what this function does would be
> misleading/incomplete with your change, because they don't describe the
> preference of root certificates over non-root certificates.

I agree.  That patch was intended as a quick proof-of-concept / discussion starter, because I wasn't at all sure it was the correct approach to take.  (I suppose I should've marked it "feedback?" instead of "review?" - sorry about that).

Thanks for indicating that you're happy with the basic idea of the patch.

> Please update
> the comments in cert.h and replace the comment in certdh.c with something
> like "/* See the documentation in cert.h". In your update, please describe
> the desired priority.

I'll address this in the next version of the patch.

> Fortunately, CERT_SortCBValidity is not exported.

Either I don't agree or I don't understand what you mean by "exported".  I see...

$ grep CERT_SortCBValidity nss/lib/certdb/cert.h -A 2
CERT_SortCBValidity(CERTCertificate *certa,
                    CERTCertificate *certb,
                    void *arg);

$ objdump -t libnss3.so | grep CERT_SortCBValidity
00065ae8 l     F .text  000001b3              CERT_SortCBValidity

> That means we could also
> give it a more descriptive name of we wanted to. But, I think the existing
> name is OK if the documentation is updated.

OK.  So it doesn't matter whether or not CERT_SortCBValidity() is exported.

> My expectation is that we want to prefer certificates where the given time
> is within the certificate's validity period over certificates where the
> given time is not within the certificate's validity period. When the given
> time is within both cert's validity periods or not within both, we want to
> prefer root certificates over non-root certificates. And, then we break ties
> based on which certificates are newer according to the times in notAfter and
> notBefore. Is that right?

Yes, that makes sense.

> If my understanding is correct, then shouldn't you put your (certa->isRoot
> != certb->isRoot) check right after the (aNotValid && !bNotValid) check? If
> not, could you please explain why we need to do these two checks first?
> 
>     /* a and b are either valid or not valid */
>     if ( newerbefore && newerafter ) {
> 	return(PR_TRUE);
>     }
>     
>     if ( ( !newerbefore ) && ( !newerafter ) ) {
> 	return(PR_FALSE);
>     }

Yes, my check should be done before these two checks.  I'll address this in the next version of the patch.
(Assignee)

Comment 5

4 years ago
(In reply to Rob Stradling from comment #4)
> (In reply to Brian Smith from comment #1)
<snip>
> > ...could you please explain why we need to do these two checks first?
> > 
> >     /* a and b are either valid or not valid */
> >     if ( newerbefore && newerafter ) {
> > 	return(PR_TRUE);
> >     }
> >     
> >     if ( ( !newerbefore ) && ( !newerafter ) ) {
> > 	return(PR_FALSE);
> >     }
> 
> Yes, my check should be done before these two checks.  I'll address this in
> the next version of the patch.

Actually, these two checks (quoted above) are completely redundant, because the function's final check (below) achieves the same result.  And there is actually no case where newerafter influences the outcome.

    if ( newerbefore ) {
	/* cert A was issued after cert B, but expires sooner */
	return(PR_TRUE);
    } else {
	/* cert B was issued after cert A, but expires sooner */
	return(PR_FALSE);
    }

Also, the "a and b are either valid or not valid" comment is a bit confusing - it describes the current state rather than the nature of the next check.

I'll have a go at tidying up this function in my next patch.
(Assignee)

Comment 6

4 years ago
Created attachment 8435807 [details] [diff] [review]
CERT_SortCBValidity patch v2

Brian, this patch addresses all of your review comments.  I've also tidied up the CERT_SortCBValidity function a bit - the only additional functional change is a final tie-breaker that prefers a later notAfter date.
Attachment #8435008 - Attachment is obsolete: true
Attachment #8435807 - Flags: review?(brian)
Comment on attachment 8435807 [details] [diff] [review]
CERT_SortCBValidity patch v2

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

Bob, do you see any reason why we shouldn't do this? It makes a lot of sense, especially when we're doing OCSP fetching for intermediates, because shorter chains mean less fetching and thus better performance.

::: security/nss/lib/certdb/cert.h
@@ +1255,5 @@
> + *   - certs where the given time "arg" is within the cert's validity period
> + * are preferred to certs where this is not the case.
> + *   - then, root certs are preferred to non-root certs.
> + *   - then, newer notBefore dates are preferred.
> + *   - to break ties, newer notAfter dates are preferred.

Nit: capitalize the first word in each sentence: Callback, Certs, Then, Then, To.

::: security/nss/lib/certdb/certdb.c
@@ +2627,5 @@
>  {
>      return CERT_AddCertToListHeadWithData(certs, cert, NULL);
>  }
>  
> +/* See the documentation in cert.h

Nit: full stop at the end of this sentence.

@@ +2650,4 @@
>      rv = CERT_GetCertTimes(certb, &notBeforeB, &notAfterB);
>      if ( rv != SECSuccess ) {
>  	return(PR_TRUE);
>      }

It would be good to move these calls to CERT_GetCertTimes to be after your (certa->isRoot != certb->isRoot) check, to make the code even more clear and for better performance.

@@ +2670,4 @@
>      }
>  
> +    /* A or B (but not both) is a root - prefer the root */
> +    if ( certa->isRoot != certb->isRoot ) {

Since PRBool is an integer where any non-zero value means "true", it would be better to write "!!certa->isRoot != !!certb->isRoot" or "!certa->isRoot != !certb->isRoot"
Attachment #8435807 - Flags: superreview?(rrelyea)
Attachment #8435807 - Flags: review?(brian)
Attachment #8435807 - Flags: review+
(Assignee)

Comment 8

4 years ago
Created attachment 8439159 [details] [diff] [review]
CERT_SortCBValidity patch v3

This v3 patch addresses Brian's comments on the v2 patch (see comment 7).
Attachment #8435807 - Attachment is obsolete: true
Attachment #8435807 - Flags: superreview?(rrelyea)
Attachment #8439159 - Flags: superreview?(rrelyea)
Attachment #8439159 - Flags: review?(brian)

Comment 9

4 years ago
> Bob, do you see any reason why we shouldn't do this? It makes a lot of sense, especially when
> we're doing OCSP fetching for intermediates, because shorter chains mean less fetching and
> thus better performance.

The one issue is it may make some chains fail. The better check is to prefer a trusted certificate.

The issue with the root cert is the case where someone has deployed a root cert that we don't trust, but we have an intermediate cert that chains to a cert we do trust. We will now fail a validation that would otherwise succeed. If we select the trusted root, then we always win (shorter chains and we know it validates). It also makes the sense for leaf certs (a self signed leaf that is trusted is preferable to a chained leaf, otherwise we would prefer the chained leaf).

bob
(Assignee)

Comment 10

4 years ago
(In reply to Robert Relyea from comment #9)
<snip>
> The one issue is it may make some chains fail. The better check is to prefer
> a trusted certificate.

That makes sense to me.

> The issue with the root cert is the case where someone has deployed a root
> cert that we don't trust, but we have an intermediate cert that chains to a
> cert we do trust. We will now fail a validation that would otherwise
> succeed.

This shouldn't be a problem for mozilla::pkix, because it tries every candidate chain before it gives up.  But I presume you're talking about other use(r)s of this NSS code.

Isn't this already an issue with the current NSS code?  What if someone has deployed a root cert that you don't trust, and that root cert has a newer notBefore date than an intermediate cert that chains to a cert you do trust?  The current NSS code would presumably "fail a validation that would otherwise succeed", wouldn't it?

If so, then FWIW I'm not sure that my v3 patch necessarily makes the current situation much worse.

> If we select the trusted root, then we always win (shorter chains
> and we know it validates). It also makes the sense for leaf certs (a self
> signed leaf that is trusted is preferable to a chained leaf, otherwise we
> would prefer the chained leaf).

I'll happily prepare a v4 patch that does this, if you wouldn't mind giving me some pointers.

I'm guessing...
Call CERT_GetCertTrust(), then call SEC_GET_TRUST_FLAGS() for each of trustSSL, trustEmail and trustObjectSigning.  If any of the 3 flag bitmasks have the CERTDB_TRUSTED_CA bit set, then treat it as a trusted certificate.
Does that sound about right?
Status: NEW → ASSIGNED
Flags: needinfo?(rrelyea)
(In reply to Rob Stradling from comment #10)
> > The issue with the root cert is the case where someone has deployed a root
> > cert that we don't trust, but we have an intermediate cert that chains to a
> > cert we do trust. We will now fail a validation that would otherwise
> > succeed.
> 
> This shouldn't be a problem for mozilla::pkix, because it tries every
> candidate chain before it gives up.  But I presume you're talking about
> other use(r)s of this NSS code.

Let's just change (in the other bug) the mozilla::pkix code to loop through the candidate issuers twice instead of once: Trying all trust anchors first, and then trying all non-trust-anchors. Then we don't have to worry about about breaking the legacy CERT_Verify* and similar functions with this change.

> Isn't this already an issue with the current NSS code?  What if someone has
> deployed a root cert that you don't trust, and that root cert has a newer
> notBefore date than an intermediate cert that chains to a cert you do trust?
> The current NSS code would presumably "fail a validation that would
> otherwise succeed", wouldn't it?

Yes, I agree it is already an issue.

> If so, then FWIW I'm not sure that my v3 patch necessarily makes the current
> situation much worse.

I agree.

> > If we select the trusted root, then we always win (shorter chains
> > and we know it validates). It also makes the sense for leaf certs (a self
> > signed leaf that is trusted is preferable to a chained leaf, otherwise we
> > would prefer the chained leaf).
> 
> I'll happily prepare a v4 patch that does this, if you wouldn't mind giving
> me some pointers.
> 
> I'm guessing...
> Call CERT_GetCertTrust(), then call SEC_GET_TRUST_FLAGS() for each of
> trustSSL, trustEmail and trustObjectSigning.  If any of the 3 flag bitmasks
> have the CERTDB_TRUSTED_CA bit set, then treat it as a trusted certificate.
> Does that sound about right?

IMO, it is not a good idea to do that in this NSS function. If we want to check the cert's trust, then we should check the trust bit that is relevant for the current verification. However, in this NSS function, that information isn't available. However, in mozilla::pkix, it is possible to do it correctly because you can just call TrustDomain::GetCertTrust() and look at the trustLevel output parameter. In Gecko, NSSCertDBTrustDomain::GetCertTrust knows which trust bit to check and checks the right one automatically.

Comment 12

4 years ago
> Does that sound about right?

yes.
Flags: needinfo?(rrelyea)
Priority: -- → P1
Target Milestone: --- → 3.16.3

Updated

4 years ago
Target Milestone: 3.16.3 → 3.17
Comment on attachment 8439159 [details] [diff] [review]
CERT_SortCBValidity patch v3

This change looks good to me, but I don't want to override rrelyea's objections. IMO, it is better to do it this way then to test the wrong trust bits for the ranking.

In the case of Firefox, I now recommend that you solve the problem in NSSCertDBTrustDomain::FindIssuer instead of and/or in addition to this. Let's discuss that in the Gecko bug that depends on this bug if you want to do that.
Attachment #8439159 - Flags: review?(brian) → review+
(Assignee)

Comment 14

4 years ago
(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #13)
> Comment on attachment 8439159 [details] [diff] [review]
> CERT_SortCBValidity patch v3
> 
> This change looks good to me, but I don't want to override rrelyea's
> objections. IMO, it is better to do it this way then to test the wrong trust
> bits for the ranking.

Is the non-yet-written v4 patch envisaged by comments 10, 11 and 12 needed?
Or shall we mark this bug WONTFIX?

> In the case of Firefox, I now recommend that you solve the problem in
> NSSCertDBTrustDomain::FindIssuer instead of and/or in addition to this.
> Let's discuss that in the Gecko bug that depends on this bug if you want to
> do that.

I just attached a patch that does this to bug 1020237.
You need to log in before you can comment on or make changes to this bug.