Closed
Bug 1060112
Opened 10 years ago
Closed 9 years ago
Incorrect error value when OCSP responser is of the wrong certificate (but correct signer)
Categories
(Core :: Security: PSM, defect)
Tracking
()
RESOLVED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: cviecco, Assigned: ttaubert)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
keeler
:
review+
|
Details | Diff | Splinter Review |
SEC_ERROR_OCSP_UNKNOWN_CERT is returned from VerifyEncodedOCSPResponse when processing an ocsp response of a valid ocsp response for a different host (correct signer).
This happens becase we initialize Context to CertStatus::Unknown and we never find the match for the certificate and no other error is found and we conflate the error returned with the possible ans1 error codes in the Context object.
Reporter | ||
Comment 1•10 years ago
|
||
Attachment #8499175 -
Flags: feedback?(dkeeler)
Comment on attachment 8499175 [details] [diff] [review]
ocsp-matching-fixup
Review of attachment 8499175 [details] [diff] [review]:
-----------------------------------------------------------------
I think this is a good approach. See second comment in particular for a suggestion for an improvement.
::: security/pkix/lib/pkixocsp.cpp
@@ +70,5 @@
> CertStatus certStatus;
> Time* thisUpdate;
> Time* validThrough;
> bool expired;
> + bool matchFound;
We should probably include a quick documentation comment.
@@ +583,5 @@
> static_cast<uint8_t>(CertStatus::Unknown));
> if (rv != Success) {
> return rv;
> }
> + context.matchFound = true;
Rather than having this line three times in this function, I think we can just have it once, after line 542.
Attachment #8499175 -
Flags: feedback?(dkeeler) → feedback+
Updated•10 years ago
|
Assignee: dougt → rlb
Blocks: 1029832
(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from bug 1120418 comment #6)
> One possible improvement here would be to use a different error code for the
> case where the OCSP response included some CertID, but not the CertID for
> the cert in question, e.g. ERROR_OCSP_RESPONSE_FOR_WRONG_CERT.
Perhaps rather than using Result::ERROR_OCSP_SERVER_ERROR, we should introduce the above as a new error.
Assignee | ||
Comment 5•9 years ago
|
||
Assignee: rlb → ttaubert
Attachment #8499175 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8608855 -
Flags: review?(dkeeler)
Comment 6•9 years ago
|
||
Comment on attachment 8608855 [details] [diff] [review]
0001-Bug-1060112-Don-t-treat-OCSP-responses-omitting-the-.patch
Review of attachment 8608855 [details] [diff] [review]:
-----------------------------------------------------------------
::: security/pkix/include/pkix/Result.h
@@ +181,5 @@
> SEC_ERROR_UNSUPPORTED_EC_POINT_FORM) \
> MOZILLA_PKIX_MAP(ERROR_SIGNATURE_ALGORITHM_MISMATCH, 48, \
> MOZILLA_PKIX_ERROR_SIGNATURE_ALGORITHM_MISMATCH) \
> + MOZILLA_PKIX_MAP(ERROR_OCSP_RESPONSE_FOR_WRONG_CERT, 49, \
> + MOZILLA_PKIX_ERROR_OCSP_RESPONSE_FOR_WRONG_CERT) \
"FOR_WRONG_CERT" implies that there was at least one status for a different cert. But, I guess there might also an OCSP response that doesn't contain any statuses for any cert. In that case, "WRONG_CERT" would be misleading. Perhaps "NOT_FOR_CERT" would be better.
Assignee | ||
Comment 7•9 years ago
|
||
Yeah, that's a good point. Can change that.
Assignee | ||
Comment 8•9 years ago
|
||
Renamed it to ERROR_OCSP_RESPONSE_FOR_CERT_MISSING, hope that's a little clearer.
Attachment #8608855 -
Attachment is obsolete: true
Attachment #8608855 -
Flags: review?(dkeeler)
Attachment #8608882 -
Flags: review?(dkeeler)
Comment on attachment 8608882 [details] [diff] [review]
0001-Bug-1060112-Don-t-treat-OCSP-responses-omitting-the-.patch
Review of attachment 8608882 [details] [diff] [review]:
-----------------------------------------------------------------
Great! I just had a couple of comments.
r=me.
::: security/manager/locales/en-US/chrome/pipnss/nsserrors.properties
@@ +317,5 @@
> MOZILLA_PKIX_ERROR_V1_CERT_USED_AS_CA=An X.509 version 1 certificate that is not a trust anchor was used to issue the server's certificate. X.509 version 1 certificates are deprecated and should not be used to sign other certificates.
> MOZILLA_PKIX_ERROR_NOT_YET_VALID_CERTIFICATE=The server presented a certificate that is not yet valid.
> MOZILLA_PKIX_ERROR_NOT_YET_VALID_ISSUER_CERTIFICATE=A certificate that is not yet valid was used to issue the server's certificate.
> MOZILLA_PKIX_ERROR_SIGNATURE_ALGORITHM_MISMATCH=The signature algorithm in the signature field of the certificate does not match the algorithm in its signatureAlgorithm field.
> +MOZILLA_PKIX_ERROR_OCSP_RESPONSE_FOR_CERT_MISSING=The OCSP response does not include a status for the certificate we're interested in.
Reflexively, I think we should avoid personal pronouns. Perhaps "... for the certificate in question."? or "... for the certificate being verified."?
::: security/manager/ssl/tests/unit/head_psm.js
@@ +72,5 @@
> const MOZILLA_PKIX_ERROR_INADEQUATE_KEY_SIZE = MOZILLA_PKIX_ERROR_BASE + 2; // -16382
> const MOZILLA_PKIX_ERROR_V1_CERT_USED_AS_CA = MOZILLA_PKIX_ERROR_BASE + 3;
> const MOZILLA_PKIX_ERROR_NOT_YET_VALID_CERTIFICATE = MOZILLA_PKIX_ERROR_BASE + 5;
> const MOZILLA_PKIX_ERROR_NOT_YET_VALID_ISSUER_CERTIFICATE = MOZILLA_PKIX_ERROR_BASE + 6;
> +const MOZILLA_PKIX_ERROR_SIGNATURE_ALGORITHM_MISMATCH = MOZILLA_PKIX_ERROR_BASE + 7;
In general, we're not including error codes that don't actually appear in any xpcshell tests, so I don't think we need to add this.
::: security/pkix/lib/pkixnss.cpp
@@ +198,5 @@
> "The signature algorithm in the signature field of the certificate does "
> "not match the algorithm in its signatureAlgorithm field." },
> + { "MOZILLA_PKIX_ERROR_OCSP_RESPONSE_FOR_CERT_MISSING",
> + "The OCSP response does not include a status for the certificate we're "
> + "interested in." },
Reminder to update this if the string in nsserrors.properties changes.
Attachment #8608882 -
Flags: review?(dkeeler) → review+
Comment 10•9 years ago
|
||
Comment 11•9 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in
before you can comment on or make changes to this bug.
Description
•