Skip to content

ssl_openssl: Fix some CRL mixups #844

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

ssl_openssl: Fix some CRL mixups #844

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 0 additions & 15 deletions src/openvpn/openssl_compat.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,21 +52,6 @@

/* Functionality missing in LibreSSL before 3.5 */
#if defined(LIBRESSL_VERSION_NUMBER) && LIBRESSL_VERSION_NUMBER < 0x3050000fL
/**
* Destroy a X509 object
*
* @param obj X509 object
*/
static inline void
X509_OBJECT_free(X509_OBJECT *obj)
{
if (obj)
{
X509_OBJECT_free_contents(obj);
OPENSSL_free(obj);
}
}

#define EVP_CTRL_AEAD_SET_TAG EVP_CTRL_GCM_SET_TAG
#define EVP_CTRL_AEAD_GET_TAG EVP_CTRL_GCM_GET_TAG
#endif
Expand Down
54 changes: 32 additions & 22 deletions src/openvpn/ssl_openssl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,8 @@ tls_ctx_free(struct tls_root_ctx *ctx)
ASSERT(NULL != ctx);
SSL_CTX_free(ctx->ctx);
ctx->ctx = NULL;
sk_X509_CRL_pop_free(ctx->crls, X509_CRL_free);
ctx->crls = NULL;
unload_xkey_provider(); /* in case it is loaded */
}

Expand Down Expand Up @@ -302,6 +304,22 @@ tls_ctx_set_tls_versions(struct tls_root_ctx *ctx, unsigned int ssl_flags)
return true;
}

static int
cert_verify_callback(X509_STORE_CTX *ctx, void *arg)
{
struct tls_session *session;
SSL *ssl;

ssl = X509_STORE_CTX_get_ex_data(ctx, SSL_get_ex_data_X509_STORE_CTX_idx());
ASSERT(ssl);
session = SSL_get_ex_data(ssl, mydata_index);
ASSERT(session);

/* Configure CRLs. */
X509_STORE_CTX_set0_crls(ctx, session->opt->ssl_ctx.crls);
return X509_verify_cert(ctx);
}

bool
tls_ctx_set_options(struct tls_root_ctx *ctx, unsigned int ssl_flags)
{
Expand Down Expand Up @@ -344,6 +362,7 @@ tls_ctx_set_options(struct tls_root_ctx *ctx, unsigned int ssl_flags)
verify_flags = SSL_VERIFY_PEER;
}
SSL_CTX_set_verify(ctx->ctx, verify_flags, verify_callback);
SSL_CTX_set_cert_verify_callback(ctx->ctx, cert_verify_callback, NULL);
Copy link
Contributor

@schwabe schwabe Oct 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The OpenSSL man page says:

In any case a viable verification result value must be reflected in the error member of x509_store_ctx, which can be done using X509_STORE_CTX_set_error(3).

I also don't see the X509_verify_cert calling this function, which is what OpenSSL also does when SSL_CTX_set_cert_verify_callback is not used.

The code looks good when compared with what OpenSSL internally does but does not do what the man page indicates it should do....

Copy link
Author

@davidben davidben Oct 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, that's because OpenSSL tends not to use their accessors internally and just writes into the struct directly. I'll send them a change to add a sentence saying that X509_verify_cert will do this for you. I think this documentation is if you were going to use this callback to swap out the verification process entirely.

Copy link
Author

@davidben davidben Oct 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

openssl/openssl#28960


SSL_CTX_set_info_callback(ctx->ctx, info_callback);

Expand Down Expand Up @@ -1318,27 +1337,16 @@ void
backend_tls_ctx_reload_crl(struct tls_root_ctx *ssl_ctx, const char *crl_file, bool crl_inline)
{
BIO *in = NULL;
STACK_OF(X509_CRL) *crls = NULL;

X509_STORE *store = SSL_CTX_get_cert_store(ssl_ctx->ctx);
if (!store)
{
crypto_msg(M_FATAL, "Cannot get certificate store");
}

/* Always start with a cleared CRL list, for that we
* we need to manually find the CRL object from the stack
* and remove it */
STACK_OF(X509_OBJECT) *objs = X509_STORE_get0_objects(store);
for (int i = 0; i < sk_X509_OBJECT_num(objs); i++)
{
X509_OBJECT *obj = sk_X509_OBJECT_value(objs, i);
ASSERT(obj);
if (X509_OBJECT_get_type(obj) == X509_LU_CRL)
{
sk_X509_OBJECT_delete(objs, i);
X509_OBJECT_free(obj);
}
}
sk_X509_CRL_pop_free(ssl_ctx->crls, X509_CRL_free);
ssl_ctx->crls = NULL;

X509_STORE_set_flags(store, X509_V_FLAG_CRL_CHECK | X509_V_FLAG_CRL_CHECK_ALL);

Expand All @@ -1354,7 +1362,13 @@ backend_tls_ctx_reload_crl(struct tls_root_ctx *ssl_ctx, const char *crl_file, b
if (in == NULL)
{
msg(M_WARN, "CRL: cannot read: %s", print_key_filename(crl_file, crl_inline));
goto end;
return;
}

crls = sk_X509_CRL_new_null();
if (crls == NULL)
{
crypto_msg(M_FATAL, "CRL: cannot create CRL list");
}

int num_crls_loaded = 0;
Expand All @@ -1380,18 +1394,14 @@ backend_tls_ctx_reload_crl(struct tls_root_ctx *ssl_ctx, const char *crl_file, b
break;
}

if (!X509_STORE_add_crl(store, crl))
if (!sk_X509_CRL_push(crls, crl))
{
X509_CRL_free(crl);
crypto_msg(M_WARN, "CRL: cannot add %s to store",
print_key_filename(crl_file, crl_inline));
break;
crypto_msg(M_FATAL, "CRL: cannot add CRL to list");
}
X509_CRL_free(crl);
num_crls_loaded++;
}
msg(M_INFO, "CRL: loaded %d CRLs from file %s", num_crls_loaded, crl_file);
end:
ssl_ctx->crls = crls;
BIO_free(in);
}

Expand Down
1 change: 1 addition & 0 deletions src/openvpn/ssl_openssl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ struct tls_root_ctx
SSL_CTX *ctx;
time_t crl_last_mtime;
off_t crl_last_size;
STACK_OF(X509_CRL) *crls;
};

struct key_state_ssl
Expand Down
18 changes: 1 addition & 17 deletions src/openvpn/ssl_verify_openssl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -783,23 +783,7 @@ tls_verify_crl_missing(const struct tls_options *opt)
return false;
}

X509_STORE *store = SSL_CTX_get_cert_store(opt->ssl_ctx.ctx);
if (!store)
{
crypto_msg(M_FATAL, "Cannot get certificate store");
}

STACK_OF(X509_OBJECT) *objs = X509_STORE_get0_objects(store);
for (int i = 0; i < sk_X509_OBJECT_num(objs); i++)
{
X509_OBJECT *obj = sk_X509_OBJECT_value(objs, i);
ASSERT(obj);
if (X509_OBJECT_get_type(obj) == X509_LU_CRL)
{
return false;
}
}
return true;
return opt->ssl_ctx.crls == NULL || sk_X509_CRL_num(opt->ssl_ctx.crls) == 0;
}

#endif /* defined(ENABLE_CRYPTO_OPENSSL) */
Loading