From: djm Date: Wed, 19 Jul 2023 14:03:45 +0000 (+0000) Subject: Separate ssh-pkcs11-helpers for each p11 module X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=c3ac409df09ae014b818304db263419b15cd187f;p=openbsd 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@ --- diff --git a/usr.bin/ssh/ssh-pkcs11-client.c b/usr.bin/ssh/ssh-pkcs11-client.c index a230abe308c..d4221644ca9 100644 --- a/usr.bin/ssh/ssh-pkcs11-client.c +++ b/usr.bin/ssh/ssh-pkcs11-client.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ssh-pkcs11-client.c,v 1.17 2020/10/18 11:32:02 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. @@ -24,6 +24,7 @@ #include #include #include +#include #include #include @@ -41,16 +42,136 @@ /* 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 -send_msg(struct sshbuf *m) +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_f("inconsistent helper"); + debug3_f("free helper for provider %s", helper->path); + for (i = 0; i < nhelpers; i++) { + if (helpers[i] == helper) { + if (found) + fatal_f("helper recorded more than once"); + 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_f("already terminated"); + } else { + debug3_f("terminating helper for %s; " + "remaining %zu RSA %zu ECDSA", + 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(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), @@ -61,12 +182,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 */ @@ -75,7 +199,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)) @@ -96,14 +219,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_f("terminating %zu helpers", nhelpers); + for (i = 0; i < nhelpers; i++) + helper_terminate(helpers[i]); } static int @@ -114,7 +240,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_f("no helper for PKCS11 key"); + debug3_f("signing with PKCS11 provider %s", helper->path); if (padding != RSA_PKCS1_PADDING) goto fail; key = sshkey_new(KEY_UNSPEC); @@ -136,10 +266,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_fr(r, "compose"); - 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_fr(r, "parse"); if (slen <= (size_t)RSA_size(rsa)) { @@ -155,6 +285,26 @@ rsa_encrypt(int flen, const u_char *from, u_char *to, RSA *rsa, int padding) return (ret); } +static int +rsa_finish(RSA *rsa) +{ + struct helper *helper; + + if ((helper = helper_by_rsa(rsa)) == NULL) + fatal_f("no helper for PKCS11 key"); + debug3_f("free PKCS11 RSA key for provider %s", helper->path); + if (helper->rsa_finish != NULL) + helper->rsa_finish(rsa); + if (helper->nrsa == 0) + fatal_f("RSA refcount error"); + helper->nrsa--; + debug3_f("provider %s remaining keys: %zu RSA %zu ECDSA", + 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) @@ -166,7 +316,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_f("no helper for PKCS11 key"); + debug3_f("signing with PKCS11 provider %s", helper->path); nid = sshkey_ecdsa_key_to_nid(ec); if (nid < 0) { error_f("couldn't get curve nid"); @@ -194,10 +348,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_fr(r, "compose"); - 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_fr(r, "parse"); cp = signature; @@ -212,67 +366,109 @@ ecdsa_do_sign(const unsigned char *dgst, int dgst_len, const BIGNUM *inv, return (ret); } -static RSA_METHOD *helper_rsa; -static EC_KEY_METHOD *helper_ecdsa; +static void +ecdsa_do_finish(EC_KEY *ec) +{ + struct helper *helper; + + if ((helper = helper_by_ec(ec)) == NULL) + fatal_f("no helper for PKCS11 key"); + debug3_f("free PKCS11 ECDSA key for provider %s", helper->path); + if (helper->ec_finish != NULL) + helper->ec_finish(ec); + if (helper->nec == 0) + fatal_f("ECDSA refcount error"); + helper->nec--; + debug3_f("provider %s remaining keys: %zu RSA %zu ECDSA", + 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); - else if (k->type == KEY_ECDSA) - EC_KEY_set_method(k->ecdsa, helper_ecdsa); - else + debug3_f("wrap %s for provider %s", sshkey_type(k), helper->path); + if (k->type == KEY_RSA) { + RSA_set_method(k->rsa, helper->rsa_meth); + if (helper->nrsa++ >= INT_MAX) + fatal_f("RSA refcount error"); + } else if (k->type == KEY_ECDSA) { + EC_KEY_set_method(k->ecdsa, helper->ec_meth); + if (helper->nec++ >= INT_MAX) + fatal_f("EC refcount error"); + } else fatal_f("unknown key type"); + k->flags |= SSHKEY_FLAG_EXT; + debug3_f("provider %s remaining keys: %zu RSA %zu ECDSA", + helper->path, helper->nrsa, helper->nec); } static int -pkcs11_start_helper_methods(void) +pkcs11_start_helper_methods(struct helper *helper) { - if (helper_ecdsa != NULL) - return (0); - - 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); - - 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_f("RSA_meth_dup failed"); - 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_f("failed to prepare method"); - 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_f("too many helpers"); + debug3_f("start helper for %s", path); if (socketpair(AF_UNIX, SOCK_STREAM, 0, pair) == -1) { - error("socketpair: %s", strerror(errno)); - return (-1); + error_f("socketpair: %s", strerror(errno)); + return NULL; + } + helper = xcalloc(1, sizeof(*helper)); + if (pkcs11_start_helper_methods(helper) == -1) { + error_f("pkcs11_start_helper_methods failed"); + goto fail; } if ((pid = fork()) == -1) { - error("fork: %s", strerror(errno)); - return (-1); + error_f("fork: %s", strerror(errno)); + 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)) { @@ -281,18 +477,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; - debug_f("starting %s %s", 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_f("starting %s %s", prog, 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_f("helper %zu for \"%s\" on fd %d pid %ld", nhelpers, + helper->path, helper->fd, (long)helper->pid); + helpers = xrecallocarray(helpers, nhelpers, + nhelpers + 1, sizeof(*helpers)); + helpers[nhelpers++] = helper; + return helper; } int @@ -306,9 +511,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_f("sshbuf_new failed"); @@ -316,10 +523,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_fr(r, "compose"); - 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_fr(r, "parse nkeys"); @@ -333,7 +540,7 @@ pkcs11_add_provider(char *name, char *pin, struct sshkey ***keysp, fatal_fr(r, "parse key"); if ((r = sshkey_from_blob(blob, blen, &k)) != 0) fatal_fr(r, "decode key"); - wrap_key(k); + wrap_key(helper, k); (*keysp)[i] = k; if (labelsp) (*labelsp)[i] = label; @@ -354,20 +561,14 @@ 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_f("sshbuf_new failed"); - 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_fr(r, "compose"); - 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_f("delete %s", name); + if ((helper = helper_by_provider(name)) != NULL) + helper_terminate(helper); + return 0; }