Closed Bug 945349 Opened 6 years ago Closed 6 years ago

CertVerifier should check early for bad usages

Categories

(Core :: Security: PSM, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28
Tracking Status
firefox28 --- fixed

People

(Reporter: cviecco, Assigned: cviecco)

Details

(Whiteboard: [qa-])

Attachments

(1 file, 3 obsolete files)

The verifycert call should be aware of valid usages and return early when any of the potential verification calls is uncapable of verifying a particular usage.
Assignee: nobody → cviecco
Summary: Cervirifier should check early for bad usages → CertVerifier should check early for bad usages
Attached patch certverifier-check-bad-usages (obsolete) — Splinter Review
Things on my mind:
1. Are those all the usages that we support (running through the tests, nothing failed).
2. Made use of an static inline to keep main verifycert clean. OK?
3. For the tests of more than one usage I used the fact that a multiply will do carry operations (and with only one bit there will be no carry) or do you prefer to actually count the number of ones?
Attachment #8341305 - Flags: feedback?(brian)
Comment on attachment 8341305 [details] [diff] [review]
certverifier-check-bad-usages

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

::: security/manager/ssl/src/CertVerifier.cpp
@@ +120,5 @@
>    return rv;
>  }
>  
> +static inline
> +bool usagesToVerifyValid(const SECCertificateUsage usage) {

static inline bool should all be together on its own line.
The opening brace of a function should be on its own line.

@@ +121,5 @@
>  }
>  
> +static inline
> +bool usagesToVerifyValid(const SECCertificateUsage usage) {
> +  const SECCertificateUsage validFlags = (

no need for parentheses, and this should be static.

@@ +135,5 @@
> +   PR_LOG(gPIPNSSLog, PR_LOG_DEBUG,
> +          ("UsagesToVerifyValid: bad usage usage = 0x%x valid=0x%x \n", usage, validFlags));
> +   return false;
> +  }
> +  //and now check only one issue was sent

s/issue/usage/?

If only one usage should be given, then why not just do this?:

switch (usage) {
   case certificateUsageSSLClient: 
   case certificateUsageSSLServer:
   case certificateUsageSSLCA:
   case certificateUsageEmailSigner: 
   case certificateUsageEmailRecipient:
   case certificateUsageObjectSigner:
   case certificateUsageStatusResponder:
     return true;
   default:
     return false;
}

Also, since only one usage is allowed for CertVerifier::VerifyCert, this limitation should be mentioned in the header file comment for this member variable.

@@ +146,5 @@
> +                               (usage * certificateUsageStatusResponder);
> +  if (usage * validFlags !=  testVal) {
> +   PR_LOG(gPIPNSSLog, PR_LOG_DEBUG,
> +          ("UsagesToVerifyValid: bad usage usage(2) = 0x%x valid = 0x%x "
> +           "mult = 0x%x testval = 0x%x\n", usage, validFlags, usage * validFlags, testVal));

I think that this logging is a little bit overkill; see my next comment.

@@ +166,5 @@
>    if (!cert) {
>      PORT_SetError(SEC_ERROR_INVALID_ARGS);
>      return SECFailure;
>    }
> +  if (!usagesToVerifyValid(usage)) {

I recommend using NS_WARN_IF:

if (NS_WARN_IF(!usageToVerifyValid(usage))) {
  ...
}

This will log an error to the console when an invalid usage is passed. (This is the part of the replacement for NS_ENSURE_*.)
Attachment #8341305 - Flags: feedback?(brian) → feedback-
Attachment #8341305 - Attachment is obsolete: true
Attachment #8341979 - Attachment is obsolete: true
Comment on attachment 8341980 [details] [diff] [review]
certverifier-check-bad-usages-alt

made it a switch (as suggested) but made it inline.
Attachment #8341980 - Flags: review?(brian)
Comment on attachment 8341980 [details] [diff] [review]
certverifier-check-bad-usages-alt

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

::: security/manager/ssl/src/CertVerifier.cpp
@@ +143,5 @@
>  
> +  switch(usage){
> +    case certificateUsageSSLClient:
> +    case certificateUsageSSLServer:
> +    case certificateUsageSSLServerWithStepUp:

We shouldn't be validating for step-up. If we are, then we should stop doing so; please file a follow-up bug in that case.

@@ +151,5 @@
> +    case certificateUsageObjectSigner:
> +    case certificateUsageStatusResponder:
> +      break;
> +    default:
> +      NS_WARNING("Calling VerifyCert with invalid Usage");

s/Usage/usage/.

::: security/manager/ssl/src/CertVerifier.h
@@ +26,5 @@
>  
>    // *evOidPolicy == SEC_OID_UNKNOWN means the cert is NOT EV
>    SECStatus VerifyCert(CERTCertificate * cert,
> +                       const SECCertificateUsage usage, // Only one usage per
> +                                                        // verification

Please put this comment in the "doc comment" above the declaration of the function.
Attachment #8341980 - Flags: review?(brian) → review+
Fixes nit from bsmith.
Attachment #8341980 - Attachment is obsolete: true
Attachment #8343992 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/5fda7fbcc45b
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.