BUG/MEDIUM: acme: NUL terminate response buffer before PEM parsing

acme_res_certificate() passes the httpclient response buffer to
ssl_sock_load_pem_into_ckch(), which will then call BIO_new_mem_buf(buf, -1).
The "-1" flag will make the OpenSSL PEM parser determine the length by
using strlen(). However, the httpclient populates the response buffer with
__b_putblk() without writing a trailing NUL to it. The byte at area[data]
is whatever data previously resided there in the memory pool.

Thus, a malicious or compromised ACME CA can perform an arbitrary-length
out-of-bounds read until hitting the first NULL byte past the response
body. The OpenSSL PEM loader will try to iterate to load the chain
certificates, thus the PEM-looking garbage found in freed memory chunks
can be erroneously loaded as additional intermediate certificates. The
presence of a single NUL inside the valid response body will result in
silent truncation of the certificate.

Make sure that the area[data] contains a terminating NULL before passing
the buffer to the parser. Fail on insufficient room for the NUL terminator.

No backport required: The ACME client has been added in 3.x and this
code path didn't exist in 2.x.

diff --git a/src/acme.c b/src/acme.c
index 26ffc3ab8574ae29dd8588d4d410c131dca09823..6b4dcd2afd562c52ede9adb9cc71b65cdc765b23 100644 (file)
--- a/src/acme.c
+++ b/src/acme.c
@@ -1562,6 +1562,16 @@ int acme_res_certificate(struct task *task, struct acme_ctx *ctx, char **errmsg)
        key = ctx->store->data->key;
        ctx->store->data->key = NULL;
 
+       /* OpenSSL's BIO_new_mem_buf() expects a NUL-terminated string when
+        * passed -1. The httpclient buffer lacks this, so manually terminate
+        * it here to prevent an out-of-bounds heap read during PEM parsing.
+        */
+       if (b_room(&hc->res.buf) < 1) {
+               memprintf(errmsg, "ACME certificate response has no room for NUL terminator");
+               goto error;
+       }
+       hc->res.buf.area[hc->res.buf.data] = '\0';
+
        /* XXX: might need a function dedicated to this, which does not read a private key */
        if (ssl_sock_load_pem_into_ckch(ctx->store->path, hc->res.buf.area, ctx->store->data , errmsg) != 0)
                goto error;