Skip to content

Commit

Permalink
Use SHA256 as default digest for OCSP signing (#2038)
Browse files Browse the repository at this point in the history
OpenSSL apparently sets a default digest when consuming the
`EVP_DigestSign*` APIs. We avoid this and emit a relevant error when
this happens. Ruby 3.2's double downed on this on their consumption
of OCSP however: ruby/ruby@4d6a293 
We don't want to introduce a "default digest" to all calls to
`EVP_DigestSign*`, so I've chosen to imitate OpenSSL's behavior in only
the places necessary in this case (OCSP). This fixes the Ruby 3.2 OCSP
tests we were failing before.

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license and the ISC license.
  • Loading branch information
samuel40791765 authored Dec 9, 2024
1 parent 2ab1f4d commit 6c58a0f
Show file tree
Hide file tree
Showing 4 changed files with 42 additions and 4 deletions.
11 changes: 10 additions & 1 deletion crypto/ocsp/internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ extern "C" {

// OCSPResponseStatus does not have a status assigned to the value 4.
//
// See Reason Code RFC: https://datatracker.ietf.org/doc/html/rfc6960#section-4.2.1
// See Reason Code RFC:
// https://datatracker.ietf.org/doc/html/rfc6960#section-4.2.1
#define OCSP_UNASSIGNED_RESPONSE_STATUS 4

// OCSP Request ASN.1 specification:
Expand Down Expand Up @@ -240,6 +241,14 @@ DECLARE_ASN1_FUNCTIONS(OCSP_SIGNATURE)
DECLARE_ASN1_FUNCTIONS(OCSP_RESPBYTES)
DECLARE_ASN1_FUNCTIONS(OCSP_REVOKEDINFO)

// OCSP_get_default_digest sets the default digest according to |signer|.
// This exists because OpenSSL sets the default to |EVP_sha256| when passing
// NULL for |type| in |EVP_DigestSignInit| when using certain key types. We wish
// to avoid this general behavior for all |EVP_DigestSign*| operations, so we
// only set the default digest from the OCSP layer. |dgst| represents the user's
// self-defined digest type, if it's non-NULL, |dgst| is directly returned.
const EVP_MD *OCSP_get_default_digest(const EVP_MD *dgst, EVP_PKEY *signer);

// Try exchanging request and response via HTTP on (non-)blocking BIO in rctx.
OPENSSL_EXPORT int OCSP_REQ_CTX_nbio(OCSP_REQ_CTX *rctx);

Expand Down
8 changes: 7 additions & 1 deletion crypto/ocsp/ocsp_client.c
Original file line number Diff line number Diff line change
Expand Up @@ -86,10 +86,16 @@ int OCSP_request_sign(OCSP_REQUEST *req, X509 *signer, EVP_PKEY *key,
OPENSSL_PUT_ERROR(OCSP, OCSP_R_PRIVATE_KEY_DOES_NOT_MATCH_CERTIFICATE);
goto err;
}
const EVP_MD *init_dgst = OCSP_get_default_digest(dgst, key);
if (init_dgst == NULL) {
OPENSSL_PUT_ERROR(OCSP, EVP_R_NO_DEFAULT_DIGEST);
goto err;
}

if (!ASN1_item_sign(ASN1_ITEM_rptr(OCSP_REQINFO),
req->optionalSignature->signatureAlgorithm, NULL,
req->optionalSignature->signature, req->tbsRequest, key,
dgst)) {
init_dgst)) {
goto err;
}
}
Expand Down
21 changes: 19 additions & 2 deletions crypto/ocsp/ocsp_server.c
Original file line number Diff line number Diff line change
Expand Up @@ -172,20 +172,37 @@ static int OCSP_basic_sign_ctx(OCSP_BASICRESP *resp, X509 *signer,
return 1;
}

const EVP_MD *OCSP_get_default_digest(const EVP_MD *dgst, EVP_PKEY *signer) {
if (dgst != NULL) {
return dgst;
}
int pkey_nid = EVP_PKEY_id(signer);
if (pkey_nid == EVP_PKEY_EC || pkey_nid == EVP_PKEY_RSA ||
pkey_nid == EVP_PKEY_DSA) {
return EVP_sha256();
}
return NULL;
}

int OCSP_basic_sign(OCSP_BASICRESP *resp, X509 *signer, EVP_PKEY *key,
const EVP_MD *dgst, STACK_OF(X509) *certs,
unsigned long flags) {
GUARD_PTR(resp);
GUARD_PTR(signer);
GUARD_PTR(key);
GUARD_PTR(dgst);

const EVP_MD *init_dgst = OCSP_get_default_digest(dgst, key);
if (init_dgst == NULL) {
OPENSSL_PUT_ERROR(OCSP, EVP_R_NO_DEFAULT_DIGEST);
return 0;
}

EVP_MD_CTX *ctx = EVP_MD_CTX_new();
if (ctx == NULL) {
return 0;
}

if (!EVP_DigestSignInit(ctx, NULL, dgst, NULL, key)) {
if (!EVP_DigestSignInit(ctx, NULL, init_dgst, NULL, key)) {
EVP_MD_CTX_free(ctx);
return 0;
}
Expand Down
6 changes: 6 additions & 0 deletions crypto/ocsp/ocsp_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -973,6 +973,8 @@ static const OCSPRequestTestVector kRequestTestVectors[] = {
"rsa_key", EVP_sha1()},
{"ocsp_request", OCSP_REQUEST_PARSE_SUCCESS, OCSP_SIGN_SUCCESS, "rsa_cert",
"rsa_key", EVP_sha256()},
{"ocsp_request", OCSP_REQUEST_PARSE_SUCCESS, OCSP_SIGN_SUCCESS, "rsa_cert",
"rsa_key", nullptr},
{"ocsp_request_attached_cert", OCSP_REQUEST_PARSE_SUCCESS, OCSP_SIGN_ERROR,
"rsa_cert", "rsa_key", nullptr},
{"ocsp_request_no_nonce", OCSP_REQUEST_PARSE_SUCCESS, OCSP_SIGN_SUCCESS,
Expand All @@ -988,6 +990,8 @@ static const OCSPRequestTestVector kRequestTestVectors[] = {
"ecdsa_cert", "ecdsa_key", EVP_sha1()},
{"ocsp_request", OCSP_REQUEST_PARSE_SUCCESS, OCSP_SIGN_SUCCESS,
"ecdsa_cert", "ecdsa_key", EVP_sha256()},
{"ocsp_request", OCSP_REQUEST_PARSE_SUCCESS, OCSP_SIGN_SUCCESS,
"ecdsa_cert", "ecdsa_key", nullptr},
{"ocsp_request_no_nonce", OCSP_REQUEST_PARSE_SUCCESS, OCSP_SIGN_SUCCESS,
"rsa_cert", "rsa_key", EVP_sha256()},
{"ocsp_request_no_nonce", OCSP_REQUEST_PARSE_SUCCESS, OCSP_SIGN_SUCCESS,
Expand Down Expand Up @@ -1137,10 +1141,12 @@ static const OCSPResponseSignTestVector kOCSPResponseSignTestVectors[] = {
{OCSP_SIGN_SUCCESS, "rsa_cert", "rsa_key", EVP_sha1()},
{OCSP_SIGN_SUCCESS, "rsa_cert", "rsa_key", EVP_sha256()},
{OCSP_SIGN_SUCCESS, "rsa_cert", "rsa_key", EVP_sha512()},
{OCSP_SIGN_SUCCESS, "rsa_cert", "rsa_key", nullptr},
// Test signing with ECDSA certs and keys.
{OCSP_SIGN_SUCCESS, "ecdsa_cert", "ecdsa_key", EVP_sha1()},
{OCSP_SIGN_SUCCESS, "ecdsa_cert", "ecdsa_key", EVP_sha256()},
{OCSP_SIGN_SUCCESS, "ecdsa_cert", "ecdsa_key", EVP_sha512()},
{OCSP_SIGN_SUCCESS, "ecdsa_cert", "ecdsa_key", nullptr},
// Test certificate type mismatch.
{OCSP_SIGN_ERROR, "rsa_cert", "ecdsa_key", EVP_sha256()},
{OCSP_SIGN_ERROR, "ecdsa_cert", "rsa_key", EVP_sha256()},
Expand Down

0 comments on commit 6c58a0f

Please sign in to comment.