1060112 - Incorrect error value when OCSP responser is of the wrong certificate (but correct signer)

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Camilo Viecco (:cviecco)]

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.

Comment 1

[Camilo Viecco (:cviecco)]

Attachment: ocsp-matching-fixup

Comment 2

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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.

Comment 4

[Dana Keeler (she/her) (use needinfo) [:keeler]]

(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.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1060112-Don-t-treat-OCSP-responses-omitting-the-.patch

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Yeah, that's a good point. Can change that.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1060112-Don-t-treat-OCSP-responses-omitting-the-.patch

Renamed it to ERROR_OCSP_RESPONSE_FOR_CERT_MISSING, hope that's a little clearer.

Comment 9

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/18252e340c73

Comment 11

[Ekanan Ketunuti]

https://hg.mozilla.org/mozilla-central/rev/18252e340c73