603761 - Certificate verification should return SECFailure and set a PORT_SetError when errors are logged

Status: UNCONFIRMED

(NSS :: Libraries, defect, P5)

Description

[Christophe Ravel]

When logging errors, cert_VerifyCertChainOld can return SECSuccess even if errors were logged.

cert_VerifyCertChainOld does a number of validations on the given cert. We can either stop at the first error (not logging) or go through all the validations and log each error.

When we don't log errors, at the first error we "goto loser" where "rv = SECFailure".

When we do log errors, the code doesn't necessarily "goto loser" if the last verification is successful.

We need to make sure that at the end "rv = SECFailure" if log->count > 0.

Comment 1

[Christophe Ravel]

Attachment: Returns SECFailure when log->count > 0

Comment 2

[Christophe Ravel]

There are 4 functions using LOG_ERROR_OR_EXIT than may have the same issue:
- cert_VerifyCertChainOld
- CERT_VerifyCACertForUsage
- CERT_VerifyCertificate
- CERT_VerifyCert

They all use a different logic to track the return status.
CERT_VerifyCertificate and CERT_VerifyCert seem to do the tracking correctly and return SECFailure whenever an error is logged.

CERT_VerifyCACertForUsage probably has the same issue as cert_VerifyCertChainOld and should be fixed too.
Should I open a separate bug for CERT_VerifyCACertForUsage or just change the title of this bug to include this second function ?

Comment 3

[Christophe Ravel]

Attachment: Avoid overriding SECWouldBlock if set

Change rv to SECFailure only if it was SECSuccess and log->count > 0

Comment 4

[Wan-Teh Chang]

Comment on attachment 482700 [details] [diff] [review]
Avoid overriding SECWouldBlock if set

Thanks for the patch.  This patch looks like a band-aid rather than a fix.  We should turn this into an assertion:

+    if ((log != NULL) && (log->count > 0)) {
+       PORT_Assert(rv != SECSuccess);
+    }

and use the assertion failures to track down where the bugs are.  I am worried that we may return SECSuccess incorrectly when 'log' is NULL.

For cert_VerifyCertChainOld, it seems that the proper fix is to set rvFinal = SECFailure before every LOG_ERROR_OR_EXIT call.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

In the course of attempting to validate a cert, libPKIX may construct 
numerous trial chains that do not validate for some reason, such as not supporting the requested cert policies, yet the final result may be a 
successful validation.  

I vaguely recall that the error log attempts to record something about those
alternative chains, but I don't recall the details.

So, my initial inclination would NOT be to force an error to be returned 
when errors are logged.  It MIGHT be to discard the error log if the 
return is SECSuccess.  I would definitely take the time (if I still worked
on this) to test the hypothesis that the error log records unsuccessful 
paths (chains).

Comment 6

[Christophe Ravel]

(In reply to comment #4)
> I am worried that we may return SECSuccess incorrectly when 'log' is NULL.

When 'log' is NULL, LOG_ERROR_OR_EXIT does 'goto loser' which sets 'rv = SECFailure'.

Comment 7

[Christophe Ravel]

(In reply to comment #5)
If I am not mistaken, cert_VerifyCertChainOld doesn't use libPKIX. cert_VerifyCertChainPkix does.

Comment 8

[Alexei Volkov]

Comment on attachment 482700 [details] [diff] [review]
Avoid overriding SECWouldBlock if set

This functions not only validates, but it also builds a chain. During built process it may visit multiple branches that would be rejected and logged. However, this function deals only with a single branch since CERT_FindCertIssuer returns only a single issuer. 

log is optional parameter in this function, but if function is called without log, rv is set to SECFailure at the looser label when a first error is encountered. So anything wrong that can happen with a cert will be logged.

There for I think it is ok to have this fix.

Comment 9

[Christophe Ravel]

Attachment: Returns SECFailure and set error code when failing

Theses changes make cert verification behave the same when logging and not logging: return an error and set an error code when failing.
The behavior when not logging is unchanged.

This patch addresses Wan-Teh's comments (from this bug and also from bug 518551):
- Set rvFinal = SECFailure for each error encountered (instead of looking at the log->count).
- Set errFinal = PORT_GetError for the first error encountered when logging
- Set PORT_SetError(errFinal) if SECFailure before returning when logging

I added a PORT_Assert(errFinal != 0) when SECFailure to make sure I could catch any bug when running all the tests. This could go away in the final patch.

Comment 10

[BugBot [:suhaib / :marco]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.