From 92cebfbcc221c9ef3f6bbb78da3d7699c0ae56be Mon Sep 17 00:00:00 2001 From: "djm@openbsd.org" Date: Wed, 19 Jul 2023 14:03:45 +0000 Subject: [PATCH 02/12] upstream: Separate ssh-pkcs11-helpers for each p11 module Make ssh-pkcs11-client start an independent helper for each provider, providing better isolation between modules and reliability if a single module misbehaves. This also implements reference counting of PKCS#11-hosted keys, allowing ssh-pkcs11-helper subprocesses to be automatically reaped when no remaining keys reference them. This fixes some bugs we have that make PKCS11 keys unusable after they have been deleted, e.g. https://bugzilla.mindrot.org/show_bug.cgi?id=3125 ok markus@ OpenBSD-Commit-ID: 0ce188b14fe271ab0568f4500070d96c5657244e Upstream-Status: Backport [https://github.com/openssh/openssh-portable/commit/099cdf59ce1e72f55d421c8445bf6321b3004755] CVE: CVE-2023-38408 Signed-off-by: Shubham Kulkarni --- ssh-pkcs11-client.c | 372 +++++++++++++++++++++++++++++++++----------- 1 file changed, 282 insertions(+), 90 deletions(-) diff --git a/ssh-pkcs11-client.c b/ssh-pkcs11-client.c index 41114c7..4f3c6ed 100644 --- a/ssh-pkcs11-client.c +++ b/ssh-pkcs11-client.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ssh-pkcs11-client.c,v 1.16 2020/01/25 00:03:36 djm Exp $ */ +/* $OpenBSD: ssh-pkcs11-client.c,v 1.18 2023/07/19 14:03:45 djm Exp $ */ /* * Copyright (c) 2010 Markus Friedl. All rights reserved. * Copyright (c) 2014 Pedro Martelletto. All rights reserved. @@ -30,12 +30,11 @@ #include #include #include +#include #include #include -#include "openbsd-compat/openssl-compat.h" - #include "pathnames.h" #include "xmalloc.h" #include "sshbuf.h" @@ -47,18 +46,140 @@ #include "ssh-pkcs11.h" #include "ssherr.h" +#include "openbsd-compat/openssl-compat.h" + /* borrows code from sftp-server and ssh-agent */ -static int fd = -1; -static pid_t pid = -1; +/* + * Maintain a list of ssh-pkcs11-helper subprocesses. These may be looked up + * by provider path or their unique EC/RSA METHOD pointers. + */ +struct helper { + char *path; + pid_t pid; + int fd; + RSA_METHOD *rsa_meth; + EC_KEY_METHOD *ec_meth; + int (*rsa_finish)(RSA *rsa); + void (*ec_finish)(EC_KEY *key); + size_t nrsa, nec; /* number of active keys of each type */ +}; +static struct helper **helpers; +static size_t nhelpers; + +static struct helper * +helper_by_provider(const char *path) +{ + size_t i; + + for (i = 0; i < nhelpers; i++) { + if (helpers[i] == NULL || helpers[i]->path == NULL || + helpers[i]->fd == -1) + continue; + if (strcmp(helpers[i]->path, path) == 0) + return helpers[i]; + } + return NULL; +} + +static struct helper * +helper_by_rsa(const RSA *rsa) +{ + size_t i; + const RSA_METHOD *meth; + + if ((meth = RSA_get_method(rsa)) == NULL) + return NULL; + for (i = 0; i < nhelpers; i++) { + if (helpers[i] != NULL && helpers[i]->rsa_meth == meth) + return helpers[i]; + } + return NULL; + +} + +static struct helper * +helper_by_ec(const EC_KEY *ec) +{ + size_t i; + const EC_KEY_METHOD *meth; + + if ((meth = EC_KEY_get_method(ec)) == NULL) + return NULL; + for (i = 0; i < nhelpers; i++) { + if (helpers[i] != NULL && helpers[i]->ec_meth == meth) + return helpers[i]; + } + return NULL; + +} + +static void +helper_free(struct helper *helper) +{ + size_t i; + int found = 0; + + if (helper == NULL) + return; + if (helper->path == NULL || helper->ec_meth == NULL || + helper->rsa_meth == NULL) + fatal("%s: inconsistent helper", __func__); + debug3("%s: free helper for provider %s", __func__ , helper->path); + for (i = 0; i < nhelpers; i++) { + if (helpers[i] == helper) { + if (found) + fatal("%s: helper recorded more than once", __func__); + found = 1; + } + else if (found) + helpers[i - 1] = helpers[i]; + } + if (found) { + helpers = xrecallocarray(helpers, nhelpers, + nhelpers - 1, sizeof(*helpers)); + nhelpers--; + } + free(helper->path); + EC_KEY_METHOD_free(helper->ec_meth); + RSA_meth_free(helper->rsa_meth); + free(helper); +} + +static void +helper_terminate(struct helper *helper) +{ + if (helper == NULL) { + return; + } else if (helper->fd == -1) { + debug3("%s: already terminated", __func__); + } else { + debug3("terminating helper for %s; " + "remaining %zu RSA %zu ECDSA", __func__, + helper->path, helper->nrsa, helper->nec); + close(helper->fd); + /* XXX waitpid() */ + helper->fd = -1; + helper->pid = -1; + } + /* + * Don't delete the helper entry until there are no remaining keys + * that reference it. Otherwise, any signing operation would call + * a free'd METHOD pointer and that would be bad. + */ + if (helper->nrsa == 0 && helper->nec == 0) + helper_free(helper); +} static void -send_msg(struct sshbuf *m) +send_msg(int fd, struct sshbuf *m) { u_char buf[4]; size_t mlen = sshbuf_len(m); int r; + if (fd == -1) + return; POKE_U32(buf, mlen); if (atomicio(vwrite, fd, buf, 4) != 4 || atomicio(vwrite, fd, sshbuf_mutable_ptr(m), @@ -69,12 +190,15 @@ send_msg(struct sshbuf *m) } static int -recv_msg(struct sshbuf *m) +recv_msg(int fd, struct sshbuf *m) { u_int l, len; u_char c, buf[1024]; int r; + sshbuf_reset(m); + if (fd == -1) + return 0; /* XXX */ if ((len = atomicio(read, fd, buf, 4)) != 4) { error("read from helper failed: %u", len); return (0); /* XXX */ @@ -83,7 +207,6 @@ recv_msg(struct sshbuf *m) if (len > 256 * 1024) fatal("response too long: %u", len); /* read len bytes into m */ - sshbuf_reset(m); while (len > 0) { l = len; if (l > sizeof(buf)) @@ -104,14 +227,17 @@ recv_msg(struct sshbuf *m) int pkcs11_init(int interactive) { - return (0); + return 0; } void pkcs11_terminate(void) { - if (fd >= 0) - close(fd); + size_t i; + + debug3("%s: terminating %zu helpers", __func__, nhelpers); + for (i = 0; i < nhelpers; i++) + helper_terminate(helpers[i]); } static int @@ -122,7 +248,11 @@ rsa_encrypt(int flen, const u_char *from, u_char *to, RSA *rsa, int padding) u_char *blob = NULL, *signature = NULL; size_t blen, slen = 0; int r, ret = -1; + struct helper *helper; + if ((helper = helper_by_rsa(rsa)) == NULL || helper->fd == -1) + fatal("%s: no helper for PKCS11 key", __func__); + debug3("%s: signing with PKCS11 provider %s", __func__, helper->path); if (padding != RSA_PKCS1_PADDING) goto fail; key = sshkey_new(KEY_UNSPEC); @@ -144,10 +274,10 @@ rsa_encrypt(int flen, const u_char *from, u_char *to, RSA *rsa, int padding) (r = sshbuf_put_string(msg, from, flen)) != 0 || (r = sshbuf_put_u32(msg, 0)) != 0) fatal("%s: buffer error: %s", __func__, ssh_err(r)); - send_msg(msg); + send_msg(helper->fd, msg); sshbuf_reset(msg); - if (recv_msg(msg) == SSH2_AGENT_SIGN_RESPONSE) { + if (recv_msg(helper->fd, msg) == SSH2_AGENT_SIGN_RESPONSE) { if ((r = sshbuf_get_string(msg, &signature, &slen)) != 0) fatal("%s: buffer error: %s", __func__, ssh_err(r)); if (slen <= (size_t)RSA_size(rsa)) { @@ -163,7 +293,26 @@ rsa_encrypt(int flen, const u_char *from, u_char *to, RSA *rsa, int padding) return (ret); } -#if defined(OPENSSL_HAS_ECC) && defined(HAVE_EC_KEY_METHOD_NEW) +static int +rsa_finish(RSA *rsa) +{ + struct helper *helper; + + if ((helper = helper_by_rsa(rsa)) == NULL) + fatal("%s: no helper for PKCS11 key", __func__); + debug3("%s: free PKCS11 RSA key for provider %s", __func__, helper->path); + if (helper->rsa_finish != NULL) + helper->rsa_finish(rsa); + if (helper->nrsa == 0) + fatal("%s: RSA refcount error", __func__); + helper->nrsa--; + debug3("%s: provider %s remaining keys: %zu RSA %zu ECDSA", __func__, + helper->path, helper->nrsa, helper->nec); + if (helper->nrsa == 0 && helper->nec == 0) + helper_terminate(helper); + return 1; +} + static ECDSA_SIG * ecdsa_do_sign(const unsigned char *dgst, int dgst_len, const BIGNUM *inv, const BIGNUM *rp, EC_KEY *ec) @@ -175,7 +324,11 @@ ecdsa_do_sign(const unsigned char *dgst, int dgst_len, const BIGNUM *inv, u_char *blob = NULL, *signature = NULL; size_t blen, slen = 0; int r, nid; + struct helper *helper; + if ((helper = helper_by_ec(ec)) == NULL || helper->fd == -1) + fatal("%s: no helper for PKCS11 key", __func__); + debug3("%s: signing with PKCS11 provider %s", __func__, helper->path); nid = sshkey_ecdsa_key_to_nid(ec); if (nid < 0) { error("%s: couldn't get curve nid", __func__); @@ -203,10 +356,10 @@ ecdsa_do_sign(const unsigned char *dgst, int dgst_len, const BIGNUM *inv, (r = sshbuf_put_string(msg, dgst, dgst_len)) != 0 || (r = sshbuf_put_u32(msg, 0)) != 0) fatal("%s: buffer error: %s", __func__, ssh_err(r)); - send_msg(msg); + send_msg(helper->fd, msg); sshbuf_reset(msg); - if (recv_msg(msg) == SSH2_AGENT_SIGN_RESPONSE) { + if (recv_msg(helper->fd, msg) == SSH2_AGENT_SIGN_RESPONSE) { if ((r = sshbuf_get_string(msg, &signature, &slen)) != 0) fatal("%s: buffer error: %s", __func__, ssh_err(r)); cp = signature; @@ -220,75 +373,110 @@ ecdsa_do_sign(const unsigned char *dgst, int dgst_len, const BIGNUM *inv, sshbuf_free(msg); return (ret); } -#endif /* OPENSSL_HAS_ECC && HAVE_EC_KEY_METHOD_NEW */ -static RSA_METHOD *helper_rsa; -#if defined(OPENSSL_HAS_ECC) && defined(HAVE_EC_KEY_METHOD_NEW) -static EC_KEY_METHOD *helper_ecdsa; -#endif /* OPENSSL_HAS_ECC && HAVE_EC_KEY_METHOD_NEW */ +static void +ecdsa_do_finish(EC_KEY *ec) +{ + struct helper *helper; + + if ((helper = helper_by_ec(ec)) == NULL) + fatal("%s: no helper for PKCS11 key", __func__); + debug3("%s: free PKCS11 ECDSA key for provider %s", __func__, helper->path); + if (helper->ec_finish != NULL) + helper->ec_finish(ec); + if (helper->nec == 0) + fatal("%s: ECDSA refcount error", __func__); + helper->nec--; + debug3("%s: provider %s remaining keys: %zu RSA %zu ECDSA", __func__, + helper->path, helper->nrsa, helper->nec); + if (helper->nrsa == 0 && helper->nec == 0) + helper_terminate(helper); +} /* redirect private key crypto operations to the ssh-pkcs11-helper */ static void -wrap_key(struct sshkey *k) +wrap_key(struct helper *helper, struct sshkey *k) { - if (k->type == KEY_RSA) - RSA_set_method(k->rsa, helper_rsa); -#if defined(OPENSSL_HAS_ECC) && defined(HAVE_EC_KEY_METHOD_NEW) - else if (k->type == KEY_ECDSA) - EC_KEY_set_method(k->ecdsa, helper_ecdsa); -#endif /* OPENSSL_HAS_ECC && HAVE_EC_KEY_METHOD_NEW */ - else + debug3("%s: wrap %s for provider %s", __func__, sshkey_type(k), helper->path); + if (k->type == KEY_RSA) { + RSA_set_method(k->rsa, helper->rsa_meth); + if (helper->nrsa++ >= INT_MAX) + fatal("%s: RSA refcount error", __func__); + } else if (k->type == KEY_ECDSA) { + EC_KEY_set_method(k->ecdsa, helper->ec_meth); + if (helper->nec++ >= INT_MAX) + fatal("%s: EC refcount error", __func__); + } else fatal("%s: unknown key type", __func__); + k->flags |= SSHKEY_FLAG_EXT; + debug3("%s: provider %s remaining keys: %zu RSA %zu ECDSA", __func__, + helper->path, helper->nrsa, helper->nec); } static int -pkcs11_start_helper_methods(void) +pkcs11_start_helper_methods(struct helper *helper) { - if (helper_rsa != NULL) - return (0); - -#if defined(OPENSSL_HAS_ECC) && defined(HAVE_EC_KEY_METHOD_NEW) - int (*orig_sign)(int, const unsigned char *, int, unsigned char *, + int (*ec_init)(EC_KEY *key); + int (*ec_copy)(EC_KEY *dest, const EC_KEY *src); + int (*ec_set_group)(EC_KEY *key, const EC_GROUP *grp); + int (*ec_set_private)(EC_KEY *key, const BIGNUM *priv_key); + int (*ec_set_public)(EC_KEY *key, const EC_POINT *pub_key); + int (*ec_sign)(int, const unsigned char *, int, unsigned char *, unsigned int *, const BIGNUM *, const BIGNUM *, EC_KEY *) = NULL; - if (helper_ecdsa != NULL) - return (0); - helper_ecdsa = EC_KEY_METHOD_new(EC_KEY_OpenSSL()); - if (helper_ecdsa == NULL) - return (-1); - EC_KEY_METHOD_get_sign(helper_ecdsa, &orig_sign, NULL, NULL); - EC_KEY_METHOD_set_sign(helper_ecdsa, orig_sign, NULL, ecdsa_do_sign); -#endif /* OPENSSL_HAS_ECC && HAVE_EC_KEY_METHOD_NEW */ - - if ((helper_rsa = RSA_meth_dup(RSA_get_default_method())) == NULL) + RSA_METHOD *rsa_meth; + EC_KEY_METHOD *ec_meth; + + if ((ec_meth = EC_KEY_METHOD_new(EC_KEY_OpenSSL())) == NULL) + return -1; + EC_KEY_METHOD_get_sign(ec_meth, &ec_sign, NULL, NULL); + EC_KEY_METHOD_set_sign(ec_meth, ec_sign, NULL, ecdsa_do_sign); + EC_KEY_METHOD_get_init(ec_meth, &ec_init, &helper->ec_finish, + &ec_copy, &ec_set_group, &ec_set_private, &ec_set_public); + EC_KEY_METHOD_set_init(ec_meth, ec_init, ecdsa_do_finish, + ec_copy, ec_set_group, ec_set_private, ec_set_public); + + if ((rsa_meth = RSA_meth_dup(RSA_get_default_method())) == NULL) fatal("%s: RSA_meth_dup failed", __func__); - if (!RSA_meth_set1_name(helper_rsa, "ssh-pkcs11-helper") || - !RSA_meth_set_priv_enc(helper_rsa, rsa_encrypt)) + helper->rsa_finish = RSA_meth_get_finish(rsa_meth); + if (!RSA_meth_set1_name(rsa_meth, "ssh-pkcs11-helper") || + !RSA_meth_set_priv_enc(rsa_meth, rsa_encrypt) || + !RSA_meth_set_finish(rsa_meth, rsa_finish)) fatal("%s: failed to prepare method", __func__); - return (0); + helper->ec_meth = ec_meth; + helper->rsa_meth = rsa_meth; + return 0; } -static int -pkcs11_start_helper(void) +static struct helper * +pkcs11_start_helper(const char *path) { int pair[2]; - char *helper, *verbosity = NULL; - - if (log_level_get() >= SYSLOG_LEVEL_DEBUG1) - verbosity = "-vvv"; - - if (pkcs11_start_helper_methods() == -1) { - error("pkcs11_start_helper_methods failed"); - return (-1); - } + char *prog, *verbosity = NULL; + struct helper *helper; + pid_t pid; + if (nhelpers >= INT_MAX) + fatal("%s: too many helpers", __func__); + debug3("%s: start helper for %s", __func__, path); if (socketpair(AF_UNIX, SOCK_STREAM, 0, pair) == -1) { error("socketpair: %s", strerror(errno)); - return (-1); + return NULL; + } + helper = xcalloc(1, sizeof(*helper)); + if (pkcs11_start_helper_methods(helper) == -1) { + error("pkcs11_start_helper_methods failed"); + goto fail; } if ((pid = fork()) == -1) { error("fork: %s", strerror(errno)); - return (-1); + fail: + close(pair[0]); + close(pair[1]); + RSA_meth_free(helper->rsa_meth); + EC_KEY_METHOD_free(helper->ec_meth); + free(helper); + return NULL; } else if (pid == 0) { if ((dup2(pair[1], STDIN_FILENO) == -1) || (dup2(pair[1], STDOUT_FILENO) == -1)) { @@ -297,18 +485,27 @@ pkcs11_start_helper(void) } close(pair[0]); close(pair[1]); - helper = getenv("SSH_PKCS11_HELPER"); - if (helper == NULL || strlen(helper) == 0) - helper = _PATH_SSH_PKCS11_HELPER; + prog = getenv("SSH_PKCS11_HELPER"); + if (prog == NULL || strlen(prog) == 0) + prog = _PATH_SSH_PKCS11_HELPER; + if (log_level_get() >= SYSLOG_LEVEL_DEBUG1) + verbosity = "-vvv"; debug("%s: starting %s %s", __func__, helper, verbosity == NULL ? "" : verbosity); - execlp(helper, helper, verbosity, (char *)NULL); - fprintf(stderr, "exec: %s: %s\n", helper, strerror(errno)); + execlp(prog, prog, verbosity, (char *)NULL); + fprintf(stderr, "exec: %s: %s\n", prog, strerror(errno)); _exit(1); } close(pair[1]); - fd = pair[0]; - return (0); + helper->fd = pair[0]; + helper->path = xstrdup(path); + helper->pid = pid; + debug3("%s: helper %zu for \"%s\" on fd %d pid %ld", __func__, nhelpers, + helper->path, helper->fd, (long)helper->pid); + helpers = xrecallocarray(helpers, nhelpers, + nhelpers + 1, sizeof(*helpers)); + helpers[nhelpers++] = helper; + return helper; } int @@ -322,9 +519,11 @@ pkcs11_add_provider(char *name, char *pin, struct sshkey ***keysp, size_t blen; u_int nkeys, i; struct sshbuf *msg; + struct helper *helper; - if (fd < 0 && pkcs11_start_helper() < 0) - return (-1); + if ((helper = helper_by_provider(name)) == NULL && + (helper = pkcs11_start_helper(name)) == NULL) + return -1; if ((msg = sshbuf_new()) == NULL) fatal("%s: sshbuf_new failed", __func__); @@ -332,10 +531,10 @@ pkcs11_add_provider(char *name, char *pin, struct sshkey ***keysp, (r = sshbuf_put_cstring(msg, name)) != 0 || (r = sshbuf_put_cstring(msg, pin)) != 0) fatal("%s: buffer error: %s", __func__, ssh_err(r)); - send_msg(msg); + send_msg(helper->fd, msg); sshbuf_reset(msg); - type = recv_msg(msg); + type = recv_msg(helper->fd, msg); if (type == SSH2_AGENT_IDENTITIES_ANSWER) { if ((r = sshbuf_get_u32(msg, &nkeys)) != 0) fatal("%s: buffer error: %s", __func__, ssh_err(r)); @@ -350,7 +549,7 @@ pkcs11_add_provider(char *name, char *pin, struct sshkey ***keysp, __func__, ssh_err(r)); if ((r = sshkey_from_blob(blob, blen, &k)) != 0) fatal("%s: bad key: %s", __func__, ssh_err(r)); - wrap_key(k); + wrap_key(helper, k); (*keysp)[i] = k; if (labelsp) (*labelsp)[i] = label; @@ -371,22 +570,15 @@ pkcs11_add_provider(char *name, char *pin, struct sshkey ***keysp, int pkcs11_del_provider(char *name) { - int r, ret = -1; - struct sshbuf *msg; - - if ((msg = sshbuf_new()) == NULL) - fatal("%s: sshbuf_new failed", __func__); - if ((r = sshbuf_put_u8(msg, SSH_AGENTC_REMOVE_SMARTCARD_KEY)) != 0 || - (r = sshbuf_put_cstring(msg, name)) != 0 || - (r = sshbuf_put_cstring(msg, "")) != 0) - fatal("%s: buffer error: %s", __func__, ssh_err(r)); - send_msg(msg); - sshbuf_reset(msg); - - if (recv_msg(msg) == SSH_AGENT_SUCCESS) - ret = 0; - sshbuf_free(msg); - return (ret); + struct helper *helper; + + /* + * ssh-agent deletes keys before calling this, so the helper entry + * should be gone before we get here. + */ + debug3("%s: delete %s", __func__, name); + if ((helper = helper_by_provider(name)) != NULL) + helper_terminate(helper); + return 0; } - #endif /* ENABLE_PKCS11 */ -- 2.41.0