path: root/meta/recipes-connectivity/openssh/openssh/CVE-2023-38408-02.patch

From 92cebfbcc221c9ef3f6bbb78da3d7699c0ae56be Mon Sep 17 00:00:00 2001
From: "djm@openbsd.org" <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 <skulkarni@mvista.com>
---
 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 <string.h>
 #include <unistd.h>
 #include <errno.h>
+#include <limits.h>

 #include <openssl/ecdsa.h>
 #include <openssl/rsa.h>

-#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