Closed
Bug 943115
Opened 11 years ago
Closed 11 years ago
return early from CreateCertErrorRunnable for verification errors other than the ones we allow overrides for
Categories
(Core :: Security: PSM, defect)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla28
People
(Reporter: keeler, Assigned: keeler)
References
Details
(Whiteboard: [qa-])
Attachments
(1 file, 2 obsolete files)
5.47 KB,
patch
|
keeler
:
review+
bajaj
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
(Brian Smith (:briansmith, was :bsmith; Please NEEDINFO? me if you want a response) from bug 929617 comment 10) > Comment on attachment 8337993 [details] [diff] [review] > patch v2 > > Review of attachment 8337993 [details] [diff] [review]: > ----------------------------------------------------------------- > > Please make the blacklist/whitelist change I suggest below, unless that > breaks something. > > ::: security/manager/ssl/src/SSLServerCertVerification.cpp > @@ +456,5 @@ > > + defaultErrorCodeToReport == SEC_ERROR_OCSP_TRY_SERVER_LATER || > > + defaultErrorCodeToReport == SEC_ERROR_OCSP_REQUEST_NEEDS_SIG || > > + defaultErrorCodeToReport == SEC_ERROR_OCSP_UNAUTHORIZED_REQUEST || > > + defaultErrorCodeToReport == SEC_ERROR_OCSP_UNKNOWN_CERT || > > + defaultErrorCodeToReport == SEC_ERROR_OCSP_MALFORMED_RESPONSE) { > > Instead of having this be a blacklist, should we make it a whitelist of the > error codes that we will allow a cert error override for?: > > case SEC_ERROR_UNKNOWN_ISSUER: > case SEC_ERROR_CA_CERT_INVALID: > case SEC_ERROR_UNTRUSTED_ISSUER: > case SEC_ERROR_EXPIRED_ISSUER_CERTIFICATE: > case SEC_ERROR_UNTRUSTED_CERT: > case SEC_ERROR_INADEQUATE_KEY_USAGE: > case SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED: > case SSL_ERROR_BAD_CERT_DOMAIN: > case SEC_ERROR_EXPIRED_CERTIFICATE: > > It seems like the original call to CertVerifier::VerifyCert must have > returned one of these error codes in order for us to be able to successfully > allow the cert override. (And, if we wouldn't allow the cert override in the > first place, there's no reason to call CertVerifier::VerifyCert again.) I > think this would resolve your issue with SEC_ERROR_BAD_DATABASE more > generally. > > To answer your question more directly though, SEC_ERROR_BAD_DATABASE isn't > an overridable error, so we can short-circuit in that case, as you have done. > > Nit: If it is not too much trouble, this change should be done in its own > bug or at least its own patch. It doesn't need its own tests. (In reply to Brian Smith (:briansmith, was :bsmith; Please NEEDINFO? me if you want a response) from comment #10) > Comment on attachment 8337993 [details] [diff] [review] > patch v2 > > Review of attachment 8337993 [details] [diff] [review]: > ----------------------------------------------------------------- > > Please make the blacklist/whitelist change I suggest below, unless that > breaks something. > > ::: security/manager/ssl/src/SSLServerCertVerification.cpp > @@ +456,5 @@ > > + defaultErrorCodeToReport == SEC_ERROR_OCSP_TRY_SERVER_LATER || > > + defaultErrorCodeToReport == SEC_ERROR_OCSP_REQUEST_NEEDS_SIG || > > + defaultErrorCodeToReport == SEC_ERROR_OCSP_UNAUTHORIZED_REQUEST || > > + defaultErrorCodeToReport == SEC_ERROR_OCSP_UNKNOWN_CERT || > > + defaultErrorCodeToReport == SEC_ERROR_OCSP_MALFORMED_RESPONSE) { > > Instead of having this be a blacklist, should we make it a whitelist of the > error codes that we will allow a cert error override for?: > > case SEC_ERROR_UNKNOWN_ISSUER: > case SEC_ERROR_CA_CERT_INVALID: > case SEC_ERROR_UNTRUSTED_ISSUER: > case SEC_ERROR_EXPIRED_ISSUER_CERTIFICATE: > case SEC_ERROR_UNTRUSTED_CERT: > case SEC_ERROR_INADEQUATE_KEY_USAGE: > case SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED: > case SSL_ERROR_BAD_CERT_DOMAIN: > case SEC_ERROR_EXPIRED_CERTIFICATE: > > It seems like the original call to CertVerifier::VerifyCert must have > returned one of these error codes in order for us to be able to successfully > allow the cert override. (And, if we wouldn't allow the cert override in the > first place, there's no reason to call CertVerifier::VerifyCert again.) I > think this would resolve your issue with SEC_ERROR_BAD_DATABASE more > generally. > > To answer your question more directly though, SEC_ERROR_BAD_DATABASE isn't > an overridable error, so we can short-circuit in that case, as you have done. > > Nit: If it is not too much trouble, this change should be done in its own > bug or at least its own patch. It doesn't need its own tests.
Assignee | ||
Comment 1•11 years ago
|
||
Just making sure this is what you meant and that the formatting/method of implementing the whitelist is good.
Comment 2•11 years ago
|
||
Comment on attachment 8338108 [details] [diff] [review] patch Review of attachment 8338108 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/src/SSLServerCertVerification.cpp @@ +446,5 @@ > MOZ_ASSERT(infoObject); > MOZ_ASSERT(cert); > > + // We only allow overrides for certain errors. Return early if the error > + // is not one of them. Add a comment here, and in the switch statement below, saying that these two switch statements need to be kept in sync. Or, factor them into a common function somehow (if you do the latter, then re-request review).
Attachment #8338108 -
Flags: review?(brian) → review+
Assignee | ||
Comment 3•11 years ago
|
||
Good call - keeping two lists in sync is error-prone. I took a shot at refactoring this - let me know what you think.
Attachment #8338108 -
Attachment is obsolete: true
Attachment #8338670 -
Flags: review?(brian)
Comment 4•11 years ago
|
||
Comment on attachment 8338670 [details] [diff] [review] patch v2 Review of attachment 8338670 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/src/SSLServerCertVerification.cpp @@ +438,5 @@ > +// nsICertOverrideService::ERROR_MISMATCH, > +// nsICertOverrideService::ERROR_TIME > +// if the given error code is an overridable error. > +// If it is not, then 0 is returned. > +uint32_t If this is not in the unnamed namespace, then make it static. @@ +439,5 @@ > +// nsICertOverrideService::ERROR_TIME > +// if the given error code is an overridable error. > +// If it is not, then 0 is returned. > +uint32_t > +PRErrorCodeToOverrideType(PRErrorCode aErrorCode) Nit: In the new code in PSM, we (I) haven't been using the a prefix on parameters. IMO, it is better to avoid it. @@ +475,5 @@ > MOZ_ASSERT(infoObject); > MOZ_ASSERT(cert); > > + // We only allow overrides for certain errors. Return early if the error > + // is not one of them. I would add a comment specifically saying that this is necessary in order to avoid doing revocation fetching in the case of OCSP stapling and probably for other reasons. @@ +555,5 @@ > + return nullptr; > + } > + collected_errors |= overrideType; > + if (overrideType == nsICertOverrideService::ERROR_UNTRUSTED && > + errorCodeTrust == SECSuccess) { This was wrong in the original code. You should not be comparing to SECSuccess because SECSuccess is of type SECStatus. You should be comparing to zero because zero is a PRErrorCode. (3x here.) @@ +558,5 @@ > + if (overrideType == nsICertOverrideService::ERROR_UNTRUSTED && > + errorCodeTrust == SECSuccess) { > + errorCodeTrust = i_node->error; > + } > + if (overrideType == nsICertOverrideService::ERROR_MISMATCH && if you make these "else if" then you can add a: } else { MOZ_CRASH("unexpected return value from PRErrorCodeToOverrideType"); }
Attachment #8338670 -
Flags: review?(brian) → review+
Assignee | ||
Comment 5•11 years ago
|
||
Addressed comments (in particular, it turns out that function is in the unnamed namespace); carrying over r+.
Attachment #8338670 -
Attachment is obsolete: true
Attachment #8338828 -
Flags: review+
Assignee | ||
Comment 6•11 years ago
|
||
Comment on attachment 8338828 [details] [diff] [review] patch v2.1 Review of attachment 8338828 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/src/SSLServerCertVerification.cpp @@ +556,5 @@ > + return nullptr; > + } > + collected_errors |= overrideType; > + if (overrideType == nsICertOverrideService::ERROR_UNTRUSTED) { > + errorCodeTrust = (errorCodeTrust == 0 ? i_node->error : errorCodeTrust); To get the desired behavior, I had to move the setting of errorCode{Trust,Mismatch,Expired} inside the bodies of their respective conditionals - let me know if this is too clever or unclear.
Assignee | ||
Comment 7•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e781364ed4f8 https://hg.mozilla.org/integration/mozilla-inbound/rev/fc7b76d9da1e (pushed with the patch for bug 929617)
Comment 8•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e781364ed4f8
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Assignee | ||
Comment 9•11 years ago
|
||
Comment on attachment 8338828 [details] [diff] [review] patch v2.1 Turns out, this also needs to be uplifted if OCSP stapling telemetry is going to be uplifted to beta. [Approval Request Comment] Bug caused by (feature/regressing bug #): needed to uplift bug 887321 (OCSP stapling telemetry) to beta User impact if declined: no OCSP stapling telemetry on beta Testing completed (on m-c, etc.): landed in 28, no problems since then Risk to taking this patch (and alternatives if risky): low String or IDL/UUID changes made by this patch: none
Attachment #8338828 -
Flags: approval-mozilla-beta?
Comment 10•11 years ago
|
||
(In reply to David Keeler (:keeler) from comment #9) > Turns out, this also needs to be uplifted if OCSP stapling telemetry is > going to be uplifted to beta. I agree it is a good idea to uplift this. However, is it really necessary for uplifting the OCSP stapling telemetry? I think there must be a simple way to merge the OCSP telemetry in without this patch. > > [Approval Request Comment] > Bug caused by (feature/regressing bug #): needed to uplift bug 887321 (OCSP > stapling telemetry) to beta More importantly, this is a prerequisite for bug 929617. > User impact if declined: no OCSP stapling telemetry on beta And, more importantly, we can't uplift bug 929617 to beta. > Testing completed (on m-c, etc.): landed in 28, no problems since then We also have quite a few automated tests for the related functionality, to give us reasonable confidence that this patch doesn't cause new regressions or new bugs. > Risk to taking this patch (and alternatives if risky): low > String or IDL/UUID changes made by this patch: none I agree.
Updated•11 years ago
|
Attachment #8338828 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Assignee | ||
Comment 11•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/1786928ad7b2 (landed with bug 887321, bug 929617, bug 938805, bug 932519, bug 934327, and bug 930209)
status-firefox27:
--- → fixed
status-firefox28:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•