Certveifier can return SECSuccess when FLAG_NO_DV_FALLBACK_FOR_EV is used even tough the verification was a failure.

RESOLVED FIXED in mozilla29

Status

()

Core
Security: PSM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cviecco, Assigned: cviecco)

Tracking

unspecified
mozilla29
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 8 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 8364008 [details]
ensure-certverify-returns-success-correctly

When certverify returns SECSuccess the server must have a successful verification (given flags) currently this condition is not true.
(Assignee)

Comment 1

5 years ago
Created attachment 8364009 [details] [diff] [review]
ensure-certverify-returns-success-correctly
Assignee: nobody → cviecco
Attachment #8364008 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #8364009 - Flags: review?(dkeeler)
Comment on attachment 8364009 [details] [diff] [review]
ensure-certverify-returns-success-correctly

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

I'm concerned that an appropriate error won't be set by the libraries (see below).

::: security/manager/ssl/src/CertVerifier.cpp
@@ +157,5 @@
>    SECOidTag evPolicy = SEC_OID_UNKNOWN;
>  
>  #ifdef NSS_NO_LIBPKIX
>    if (flags & FLAG_NO_DV_FALLBACK_FOR_EV) {
> +    return SECFailure;

We need to set an error here (PR_SetError or whatever it's called), because otherwise it will just end up being whatever it was last, which isn't guaranteed to be the same each time (or even related to this function call at all).

@@ +321,5 @@
>  
>    // If we're here, PKIX EV verification failed.
>    // If requested, don't do DV fallback.
>    if (flags & FLAG_NO_DV_FALLBACK_FOR_EV) {
> +    return SECFailure;

Similarly, I'm not convinced libpkix is going to have set the same error each time - wouldn't it depend on what order it tried possible certificate paths? We should probably set an error that means something like "couldn't fetch revocation information".
Attachment #8364009 - Flags: review?(dkeeler) → review-
(Assignee)

Comment 3

5 years ago
(In reply to David Keeler (:keeler) from comment #2)
> Comment on attachment 8364009 [details] [diff] [review]
> ensure-certverify-returns-success-correctly
> 
> Review of attachment 8364009 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm concerned that an appropriate error won't be set by the libraries (see
> below).
> 
> ::: security/manager/ssl/src/CertVerifier.cpp
> @@ +157,5 @@
> >    SECOidTag evPolicy = SEC_OID_UNKNOWN;
> >  
> >  #ifdef NSS_NO_LIBPKIX
> >    if (flags & FLAG_NO_DV_FALLBACK_FOR_EV) {
> > +    return SECFailure;
Agreed.
> 
> We need to set an error here (PR_SetError or whatever it's called), because
> otherwise it will just end up being whatever it was last, which isn't
> guaranteed to be the same each time (or even related to this function call
> at all).
> 
> @@ +321,5 @@
> >  
> >    // If we're here, PKIX EV verification failed.
> >    // If requested, don't do DV fallback.
> >    if (flags & FLAG_NO_DV_FALLBACK_FOR_EV) {
> > +    return SECFailure;
> 
> Similarly, I'm not convinced libpkix is going to have set the same error
> each time - wouldn't it depend on what order it tried possible certificate
> paths? 
No, libpkix is deterministic given a static database. 
We should probably set an error that means something like "couldn't
> fetch revocation information".
I made some changes so that the libpkix error is kept (see next patch version)
(Assignee)

Comment 4

5 years ago
Created attachment 8364358 [details] [diff] [review]
ensure-certverify-returns-success-correctly (v2)
Attachment #8364009 - Attachment is obsolete: true
(Assignee)

Comment 5

5 years ago
Created attachment 8364359 [details] [diff] [review]
rename-opt-to-MUST-BE-EV

A better name for the option.
(Assignee)

Comment 6

5 years ago
Created attachment 8364361 [details] [diff] [review]
ensure-certverify-returns-success-correctly (v2.1)
Attachment #8364358 - Attachment is obsolete: true
(Assignee)

Comment 7

5 years ago
Created attachment 8364364 [details] [diff] [review]
rename-opt-to-MUST-BE-EV (v2)
Attachment #8364359 - Attachment is obsolete: true
(Assignee)

Comment 8

5 years ago
Created attachment 8364367 [details] [diff] [review]
ensure-certverify-returns-success-correctly (v2.2)
Attachment #8364361 - Attachment is obsolete: true
(Assignee)

Comment 9

5 years ago
Created attachment 8364368 [details] [diff] [review]
rename-opt-to-MUST-BE-EV (v2.1)
Attachment #8364364 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #8364367 - Flags: review?(dkeeler)
(Assignee)

Comment 10

5 years ago
Comment on attachment 8364368 [details] [diff] [review]
rename-opt-to-MUST-BE-EV (v2.1)

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

This is a better name for the option as used. (less decoding of what we are doing)
Attachment #8364368 - Flags: feedback?(dkeeler)
Comment on attachment 8364367 [details] [diff] [review]
ensure-certverify-returns-success-correctly (v2.2)

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

This looks good - r=me with comments addressed.

::: security/manager/ssl/src/CertVerifier.cpp
@@ +189,5 @@
>        // Do not setup EV verification params
>        evPolicy = SEC_OID_UNKNOWN;
>      }
> +    if (evPolicy == SEC_OID_UNKNOWN && (flags & FLAG_NO_DV_FALLBACK_FOR_EV)) {
> +        PORT_SetError(SEC_ERROR_UNKNOWN_ISSUER);

Maybe SEC_ERROR_INVALID_ARGS? I'm not sure "unknown issuer" is an accurate description of this case. Is there an error code more like "this certificate doesn't have the thing we're looking for"?

@@ +291,5 @@
>  
>      cvin[i].type = cert_pi_end;
>  
>      rv = CERT_PKIXVerifyCert(cert, usage, cvin, cvout, pinArg);
> +    PRErrorCode savedErrorCode = PORT_GetError();

I don't think PR_LOG can set an error, so is savedErrorCode necessary?

::: security/manager/ssl/tests/unit/test_ev_certs.js
@@ +199,5 @@
>    ocspResponder.stop(run_next_test);
>  }
>  
>  add_test(function() {
> +  check_no_ocsp_requests("ev-valid", SEC_ERROR_REVOKED_CERTIFICATE);

A good test (that I should have added originally) would be to do this:
1. check that "ev-valid" doesn't verify as EV with NO_DV_FALLBACK and LOCAL_ONLY set
2. check that it does verify as EV with the normal flags
3. without clearing the OCSP cache, check that it verifies as EV with the flags set again
Attachment #8364367 - Flags: review?(dkeeler) → review+
Comment on attachment 8364368 [details] [diff] [review]
rename-opt-to-MUST-BE-EV (v2.1)

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

Looks good. r=me with the uuid regenerated.

::: security/manager/ssl/public/nsIX509CertDB.idl
@@ +305,5 @@
>    const uint32_t FLAG_LOCAL_ONLY = 1 << 0;
>    // Do not fall back to DV verification after attempting EV validation.
>    // Actually does prevent network traffic, but can cause a valid EV
>    // certificate to not be considered valid.
> +  const uint32_t MUST_BE_EV = 1 << 1;

I think we'll need to rev the uuid again.
Attachment #8364368 - Flags: feedback?(dkeeler) → review+
(Assignee)

Comment 13

5 years ago
(In reply to David Keeler (:keeler) from comment #11)
> Comment on attachment 8364367 [details] [diff] [review]
> ensure-certverify-returns-success-correctly (v2.2)
> 
> Review of attachment 8364367 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good - r=me with comments addressed.
> 
> ::: security/manager/ssl/src/CertVerifier.cpp
> @@ +189,5 @@
> >        // Do not setup EV verification params
> >        evPolicy = SEC_OID_UNKNOWN;
> >      }
> > +    if (evPolicy == SEC_OID_UNKNOWN && (flags & FLAG_NO_DV_FALLBACK_FOR_EV)) {
> > +        PORT_SetError(SEC_ERROR_UNKNOWN_ISSUER);
> 
> Maybe SEC_ERROR_INVALID_ARGS? I'm not sure "unknown issuer" is an accurate
> description of this case. Is there an error code more like "this certificate
> doesn't have the thing we're looking for"?

Actually there are 4 cases here
1. There is no EV policy in the cert
2. There is a EV policy but is not one known by the mozilla root store
3. There is an EV policy that is one known by the mozilla cert, but the issuer is not part of the root store
4. There is an EV policy that is eventually signed by a default trusted mozilla root, but the user has removed the trust bits.

At this point in the code I cannot distinguish between cases 1 and 2 and between 3 and 4 (actually with some extra code cases 3 and 4 could be distinguished in most cases). The error as currently returned for cases 3 and 4 is SEC_ERROR_UNKNOWN_ISSUER so I took the liberty of also using it for cases 1 and 2. For these cases (1,2) we could return SEC_ERROR_INADEQUATE_CERT_TYPE.  

> @@ +291,5 @@
> >  
> >      cvin[i].type = cert_pi_end;
> >  
> >      rv = CERT_PKIXVerifyCert(cert, usage, cvin, cvout, pinArg);
> > +    PRErrorCode savedErrorCode = PORT_GetError();
> 
> I don't think PR_LOG can set an error, so is savedErrorCode necessary?
> 
> ::: security/manager/ssl/tests/unit/test_ev_certs.js
> @@ +199,5 @@
> >    ocspResponder.stop(run_next_test);
> >  }
> >  
> >  add_test(function() {
> > +  check_no_ocsp_requests("ev-valid", SEC_ERROR_REVOKED_CERTIFICATE);
> 
> A good test (that I should have added originally) would be to do this:
> 1. check that "ev-valid" doesn't verify as EV with NO_DV_FALLBACK and
> LOCAL_ONLY set
> 2. check that it does verify as EV with the normal flags
> 3. without clearing the OCSP cache, check that it verifies as EV with the
> flags set again

I like that test, will add something similar.
(Assignee)

Comment 16

5 years ago
Created attachment 8366043 [details] [diff] [review]
ensure-certverify-returns-success-correctly

joined and tested patch. Keeping r+ from keeler.
Attachment #8364367 - Attachment is obsolete: true
Attachment #8364368 - Attachment is obsolete: true
Attachment #8366043 - Flags: review+
(Assignee)

Comment 17

5 years ago
landing of Bug 891066 caused mayor un-rebaseable changes. 
new push:
 https://tbpl.mozilla.org/?tree=Try&rev=99328c129d17
https://hg.mozilla.org/mozilla-central/rev/f693e9ad7bc5
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Comment on attachment 8366043 [details] [diff] [review]
ensure-certverify-returns-success-correctly

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

::: security/manager/ssl/public/nsIX509CertDB.idl
@@ +305,5 @@
>    const uint32_t FLAG_LOCAL_ONLY = 1 << 0;
>    // Do not fall back to DV verification after attempting EV validation.
>    // Actually does prevent network traffic, but can cause a valid EV
>    // certificate to not be considered valid.
> +  const uint32_t FLAG_MUST_BE_EV = 1 << 1;

NOTE: Since we're shipping Firefox 28 with FLAG_NO_DV_FALLBACK_FOR_EV, this change has some compatibility issues. It is good that you changed the UUID. However, it is also important to avoid re-using a flag value for something with different semantics.

In this particular case, it isn't a big deal and we don't need to make any changes. Just be aware of this in the future, when writing and reviewing patches.
Depends on: 967299
You need to log in before you can comment on or make changes to this bug.