Index | Thread | Search

From:
Brent Cook <busterb@gmail.com>
Subject:
Re: [PATCH] Use tls_set_errorx when errno isn't set on failure
To:
Theo Buehler <tb@theobuehler.org>
Cc:
Michael Forney <mforney@mforney.org>, tech@openbsd.org, jsing@openbsd.org, bcook@openbsd.org
Date:
Wed, 15 Apr 2026 20:39:31 -0500

Download raw body.

Thread
On Wed, Apr 15, 2026 at 5:05 PM Theo Buehler <tb@theobuehler.org> wrote:
>
> On Wed, Apr 15, 2026 at 11:55:21AM -0700, Michael Forney wrote:
> > In these cases, the current errno value may not be relevant.
> >
> > A few calls with TLS_ERROR_OUT_OF_MEMORY were normalized to setx/errorx
> > since that was the most common throughout the codebase.
> >
> > One malloc error path was changed to use TLS_ERROR_OUT_OF_MEMORY
> > to match the others.
> > ---
> > I noticed a couple of these, so went looking for others. Some are
> > pretty clear they should use the x variant (e.g. tls_ocsp_asn1_parse_time
> > errors), but I wasn't sure about something like BIO_new_mem_buf,
> > which probably does set errno = ENOMEM on failure. I changed those
> > to tls_set_errorx to match the other BIO_new_mem_buf error paths.
>
> Thanks.
>
> Changing to the x variant after BIO_new_mem_buf() looks like the right
> thing to do to me.
>
> I think there are three places (one in tls_keypair_load_cert() and
> two in tls_signer_add_keypair_mem()) where the size passed to
> BIO_new_mem_buf() could in principle be larger than INT_MAX. This can
> overflow to be negative or be truncated to remain positive. In the first
> case we'd have no relevant errno set.
>
> We should fix that at least by adding errors for length > INT_MAX
> before BIO_new_mem_buf(), and perhaps we should also consider
> not loading files of such preposterous size in the first place.
>
> > Please disregard any of these I may have gotten wrong if they are
> > correct as-is.
>
> I agree with all these changes and while they are cosmetic, I think
> they would be nice to have in the upcoming release -- especially if
> people want to update libretls to support OpenSSL 4 without carrying
> unnecessary patches on top of libressl 4.3's libtls.
>
> I will need an ok or someone can commit with mine (probably want
> an additional ok from deraadt since the release lock is imminent).
>
> Diff re-inlined for convenience.

This looks correct to me. ok bcook@

>
>  src/lib/libtls/tls.c         |  2 +-
>  src/lib/libtls/tls_client.c  |  2 +-
>  src/lib/libtls/tls_config.c  |  2 +-
>  src/lib/libtls/tls_keypair.c |  6 +++---
>  src/lib/libtls/tls_ocsp.c    | 10 +++++-----
>  src/lib/libtls/tls_server.c  |  8 ++++----
>  src/lib/libtls/tls_signer.c  |  6 +++---
>  7 files changed, 18 insertions(+), 18 deletions(-)
>
> diff --git a/src/lib/libtls/tls.c b/src/lib/libtls/tls.c
> index 41bb06d85..fd0e9929e 100644
> --- a/src/lib/libtls/tls.c
> +++ b/src/lib/libtls/tls.c
> @@ -686,7 +686,7 @@ tls_configure_ssl_verify(struct tls *ctx, SSL_CTX *ssl_ctx, int verify)
>                         if (xi->crl == NULL)
>                                 continue;
>                         if (!X509_STORE_add_crl(store, xi->crl)) {
> -                               tls_set_error(ctx, TLS_ERROR_UNKNOWN,
> +                               tls_set_errorx(ctx, TLS_ERROR_UNKNOWN,
>                                     "failed to add crl");
>                                 goto err;
>                         }
> diff --git a/src/lib/libtls/tls_client.c b/src/lib/libtls/tls_client.c
> index 97e1d4021..5763eab6f 100644
> --- a/src/lib/libtls/tls_client.c
> +++ b/src/lib/libtls/tls_client.c
> @@ -115,7 +115,7 @@ tls_connect_servername(struct tls *ctx, const char *host, const char *port,
>                         hints.ai_family = AF_UNSPEC;
>                         hints.ai_flags = AI_ADDRCONFIG;
>                         if ((s = getaddrinfo(h, p, &hints, &res0)) != 0) {
> -                               tls_set_error(ctx, TLS_ERROR_UNKNOWN,
> +                               tls_set_errorx(ctx, TLS_ERROR_UNKNOWN,
>                                     "%s", gai_strerror(s));
>                                 goto err;
>                         }
> diff --git a/src/lib/libtls/tls_config.c b/src/lib/libtls/tls_config.c
> index 848117a91..16d896685 100644
> --- a/src/lib/libtls/tls_config.c
> +++ b/src/lib/libtls/tls_config.c
> @@ -65,7 +65,7 @@ tls_config_load_file(struct tls_error *error, const char *filetype,
>                 goto err;
>         *len = (size_t)st.st_size;
>         if ((*buf = malloc(*len)) == NULL) {
> -               tls_error_set(error, TLS_ERROR_UNKNOWN,
> +               tls_error_setx(error, TLS_ERROR_OUT_OF_MEMORY,
>                     "failed to allocate buffer for %s file",
>                     filetype);
>                 goto err;
> diff --git a/src/lib/libtls/tls_keypair.c b/src/lib/libtls/tls_keypair.c
> index ffda91df8..a6f555063 100644
> --- a/src/lib/libtls/tls_keypair.c
> +++ b/src/lib/libtls/tls_keypair.c
> @@ -144,13 +144,13 @@ tls_keypair_load_cert(struct tls_keypair *keypair, struct tls_error *error,
>         *cert = NULL;
>
>         if (keypair->cert_mem == NULL) {
> -               tls_error_set(error, TLS_ERROR_UNKNOWN,
> +               tls_error_setx(error, TLS_ERROR_UNKNOWN,
>                     "keypair has no certificate");
>                 goto err;
>         }
>         if ((cert_bio = BIO_new_mem_buf(keypair->cert_mem,
>             keypair->cert_len)) == NULL) {
> -               tls_error_set(error, TLS_ERROR_UNKNOWN,
> +               tls_error_setx(error, TLS_ERROR_UNKNOWN,
>                     "failed to create certificate bio");
>                 goto err;
>         }
> @@ -158,7 +158,7 @@ tls_keypair_load_cert(struct tls_keypair *keypair, struct tls_error *error,
>             NULL)) == NULL) {
>                 if ((ssl_err = ERR_peek_error()) != 0)
>                         errstr = ERR_error_string(ssl_err, NULL);
> -               tls_error_set(error, TLS_ERROR_UNKNOWN,
> +               tls_error_setx(error, TLS_ERROR_UNKNOWN,
>                     "failed to load certificate: %s", errstr);
>                 goto err;
>         }
> diff --git a/src/lib/libtls/tls_ocsp.c b/src/lib/libtls/tls_ocsp.c
> index c65911920..1cc167eaa 100644
> --- a/src/lib/libtls/tls_ocsp.c
> +++ b/src/lib/libtls/tls_ocsp.c
> @@ -85,7 +85,7 @@ tls_ocsp_fill_info(struct tls *ctx, int response_status, int cert_status,
>         ctx->ocsp->ocsp_result = NULL;
>
>         if ((info = calloc(1, sizeof (struct tls_ocsp_result))) == NULL) {
> -               tls_set_error(ctx, TLS_ERROR_OUT_OF_MEMORY, "out of memory");
> +               tls_set_errorx(ctx, TLS_ERROR_OUT_OF_MEMORY, "out of memory");
>                 return -1;
>         }
>         info->response_status = response_status;
> @@ -102,19 +102,19 @@ tls_ocsp_fill_info(struct tls *ctx, int response_status, int cert_status,
>         info->revocation_time = info->this_update = info->next_update = -1;
>         if (revtime != NULL &&
>             tls_ocsp_asn1_parse_time(ctx, revtime, &info->revocation_time) != 0) {
> -               tls_set_error(ctx, TLS_ERROR_UNKNOWN,
> +               tls_set_errorx(ctx, TLS_ERROR_UNKNOWN,
>                     "unable to parse revocation time in OCSP reply");
>                 goto err;
>         }
>         if (thisupd != NULL &&
>             tls_ocsp_asn1_parse_time(ctx, thisupd, &info->this_update) != 0) {
> -               tls_set_error(ctx, TLS_ERROR_UNKNOWN,
> +               tls_set_errorx(ctx, TLS_ERROR_UNKNOWN,
>                     "unable to parse this update time in OCSP reply");
>                 goto err;
>         }
>         if (nextupd != NULL &&
>             tls_ocsp_asn1_parse_time(ctx, nextupd, &info->next_update) != 0) {
> -               tls_set_error(ctx, TLS_ERROR_UNKNOWN,
> +               tls_set_errorx(ctx, TLS_ERROR_UNKNOWN,
>                     "unable to parse next update time in OCSP reply");
>                 goto err;
>         }
> @@ -305,7 +305,7 @@ tls_ocsp_process_response_internal(struct tls *ctx, const unsigned char *respons
>         if (resp == NULL) {
>                 tls_ocsp_free(ctx->ocsp);
>                 ctx->ocsp = NULL;
> -               tls_set_error(ctx, TLS_ERROR_UNKNOWN,
> +               tls_set_errorx(ctx, TLS_ERROR_UNKNOWN,
>                     "unable to parse OCSP response");
>                 return -1;
>         }
> diff --git a/src/lib/libtls/tls_server.c b/src/lib/libtls/tls_server.c
> index 42a697327..47c711421 100644
> --- a/src/lib/libtls/tls_server.c
> +++ b/src/lib/libtls/tls_server.c
> @@ -242,12 +242,12 @@ tls_configure_server_ssl(struct tls *ctx, SSL_CTX **ssl_ctx,
>
>         if (SSL_CTX_set_tlsext_servername_callback(*ssl_ctx,
>             tls_servername_cb) != 1) {
> -               tls_set_error(ctx, TLS_ERROR_UNKNOWN,
> +               tls_set_errorx(ctx, TLS_ERROR_UNKNOWN,
>                     "failed to set servername callback");
>                 goto err;
>         }
>         if (SSL_CTX_set_tlsext_servername_arg(*ssl_ctx, ctx) != 1) {
> -               tls_set_error(ctx, TLS_ERROR_UNKNOWN,
> +               tls_set_errorx(ctx, TLS_ERROR_UNKNOWN,
>                     "failed to set servername callback arg");
>                 goto err;
>         }
> @@ -298,7 +298,7 @@ tls_configure_server_ssl(struct tls *ctx, SSL_CTX **ssl_ctx,
>                 SSL_CTX_clear_options(*ssl_ctx, SSL_OP_NO_TICKET);
>                 if (!SSL_CTX_set_tlsext_ticket_key_cb(*ssl_ctx,
>                     tls_server_ticket_cb)) {
> -                       tls_set_error(ctx, TLS_ERROR_UNKNOWN,
> +                       tls_set_errorx(ctx, TLS_ERROR_UNKNOWN,
>                             "failed to set the TLS ticket callback");
>                         goto err;
>                 }
> @@ -306,7 +306,7 @@ tls_configure_server_ssl(struct tls *ctx, SSL_CTX **ssl_ctx,
>
>         if (SSL_CTX_set_session_id_context(*ssl_ctx, ctx->config->session_id,
>             sizeof(ctx->config->session_id)) != 1) {
> -               tls_set_error(ctx, TLS_ERROR_UNKNOWN,
> +               tls_set_errorx(ctx, TLS_ERROR_UNKNOWN,
>                     "failed to set session id context");
>                 goto err;
>         }
> diff --git a/src/lib/libtls/tls_signer.c b/src/lib/libtls/tls_signer.c
> index 2573803ec..70160cd98 100644
> --- a/src/lib/libtls/tls_signer.c
> +++ b/src/lib/libtls/tls_signer.c
> @@ -137,7 +137,7 @@ tls_signer_add_keypair_mem(struct tls_signer *signer, const uint8_t *cert,
>         }
>
>         if ((skey = calloc(1, sizeof(*skey))) == NULL) {
> -               tls_error_set(&signer->error, TLS_ERROR_OUT_OF_MEMORY,
> +               tls_error_setx(&signer->error, TLS_ERROR_OUT_OF_MEMORY,
>                     "out of memory");
>                 goto err;
>         }
> @@ -223,7 +223,7 @@ tls_sign_rsa(struct tls_signer *signer, struct tls_signer_key *skey,
>                 return (-1);
>         }
>         if ((signature = calloc(1, rsa_size)) == NULL) {
> -               tls_error_set(&signer->error, TLS_ERROR_OUT_OF_MEMORY,
> +               tls_error_setx(&signer->error, TLS_ERROR_OUT_OF_MEMORY,
>                     "out of memory");
>                 return (-1);
>         }
> @@ -271,7 +271,7 @@ tls_sign_ecdsa(struct tls_signer *signer, struct tls_signer_key *skey,
>                 return (-1);
>         }
>         if ((signature = calloc(1, signature_len)) == NULL) {
> -               tls_error_set(&signer->error, TLS_ERROR_OUT_OF_MEMORY,
> +               tls_error_setx(&signer->error, TLS_ERROR_OUT_OF_MEMORY,
>                     "out of memory");
>                 return (-1);
>         }
> --
> 2.49.0
>