upstream: remove vestigal support for KRL signatures

When the KRL format was originally defined, it included support for
signing of KRL objects. However, the code to sign KRLs and verify KRL
signatues was never completed in OpenSSH.

Now, some years later, we have SSHSIG support in ssh-keygen that is
more general, well tested and actually works. So this removes the
semi-finished KRL signing/verification support from OpenSSH and
refactors the remaining code to realise the benefit - primarily, we
no longer need to perform multiple parsing passes over KRL objects.

ok markus@

OpenBSD-Commit-ID: 517437bab3d8180f695c775410c052340e038804

4 files changed, 36 insertions, 179 deletions

diff --git a/PROTOCOL.krl b/PROTOCOL.krl
index f4213156..1b59c76b 100644
--- a/PROTOCOL.krl
+++ b/PROTOCOL.krl
@@ -193,6 +193,10 @@ The "extension_contents" contains the body of the extension.
 
 6. KRL signature sections
 
+Note: KRL signatures are not supported by OpenSSH. OpenSSH >= 9.4 will
+refuse to load KRLs that contain signatures. We recommend the use
+of SSHSIG (`ssh-keygen -Y sign ...`) style signatures for KRLs instead.
+
 The KRL_SECTION_SIGNATURE section serves a different purpose to the
 preceding ones: to provide cryptographic authentication of a KRL that
 is retrieved over a channel that does not provide integrity protection.
@@ -215,4 +219,4 @@ Implementations that retrieve KRLs over untrusted channels must verify
 signatures. Signature sections are optional for KRLs distributed by
 trusted means.
 
-$OpenBSD: PROTOCOL.krl,v 1.6 2023/07/17 03:57:21 djm Exp $
+$OpenBSD: PROTOCOL.krl,v 1.7 2023/07/17 04:01:10 djm Exp $
diff --git a/krl.c b/krl.c
index f04ea27d..c53fdd6e 100644
--- a/krl.c
+++ b/krl.c
@@ -14,7 +14,7 @@
  * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
  */
 
-/* $OpenBSD: krl.c,v 1.56 2023/07/17 03:57:21 djm Exp $ */
+/* $OpenBSD: krl.c,v 1.57 2023/07/17 04:01:10 djm Exp $ */
 
 #include "includes.h"
 
@@ -729,15 +729,13 @@ revoked_certs_generate(struct revoked_certs *rc, struct sshbuf *buf)
 }
 
 int
-ssh_krl_to_blob(struct ssh_krl *krl, struct sshbuf *buf,
-    struct sshkey **sign_keys, u_int nsign_keys)
+ssh_krl_to_blob(struct ssh_krl *krl, struct sshbuf *buf)
 {
 	int r = SSH_ERR_INTERNAL_ERROR;
 	struct revoked_certs *rc;
 	struct revoked_blob *rb;
 	struct sshbuf *sect;
 	u_char *sblob = NULL;
-	size_t slen, i;
 
 	if (krl->generated_date == 0)
 		krl->generated_date = time(NULL);
@@ -801,22 +799,7 @@ ssh_krl_to_blob(struct ssh_krl *krl, struct sshbuf *buf,
 		    (r = sshbuf_put_stringb(buf, sect)) != 0)
 			goto out;
 	}
-
-	for (i = 0; i < nsign_keys; i++) {
-		KRL_DBG(("sig key %s", sshkey_ssh_name(sign_keys[i])));
-		if ((r = sshbuf_put_u8(buf, KRL_SECTION_SIGNATURE)) != 0 ||
-		    (r = sshkey_puts(sign_keys[i], buf)) != 0)
-			goto out;
-		/* XXX support sk-* keys */
-		if ((r = sshkey_sign(sign_keys[i], &sblob, &slen,
-		    sshbuf_ptr(buf), sshbuf_len(buf), NULL, NULL,
-		    NULL, 0)) != 0)
-			goto out;
-		KRL_DBG(("signature sig len %zu", slen));
-		if ((r = sshbuf_put_string(buf, sblob, slen)) != 0)
-			goto out;
-	}
-
+	/* success */
 	r = 0;
  out:
 	free(sblob);
@@ -1057,45 +1040,39 @@ extension_section(struct sshbuf *sect, struct ssh_krl *krl)
 	return r;
 }
 
-/* Attempt to parse a KRL, checking its signature (if any) with sign_ca_keys. */
+/* Attempt to parse a KRL */
 int
-ssh_krl_from_blob(struct sshbuf *buf, struct ssh_krl **krlp,
-    const struct sshkey **sign_ca_keys, size_t nsign_ca_keys)
+ssh_krl_from_blob(struct sshbuf *buf, struct ssh_krl **krlp)
 {
 	struct sshbuf *copy = NULL, *sect = NULL;
 	struct ssh_krl *krl = NULL;
 	char timestamp[64];
-	int r = SSH_ERR_INTERNAL_ERROR, sig_seen;
-	struct sshkey *key = NULL, **ca_used = NULL, **tmp_ca_used;
+	int r = SSH_ERR_INTERNAL_ERROR;
 	u_char type;
-	const u_char *blob;
-	size_t i, j, sig_off, sects_off, blen, nca_used;
 	u_int format_version;
 
-	nca_used = 0;
 	*krlp = NULL;
-	if (sshbuf_len(buf) < sizeof(KRL_MAGIC) - 1 ||
-	    memcmp(sshbuf_ptr(buf), KRL_MAGIC, sizeof(KRL_MAGIC) - 1) != 0) {
-		debug3_f("not a KRL");
-		return SSH_ERR_KRL_BAD_MAGIC;
-	}
 
-	/* Take a copy of the KRL buffer so we can verify its signature later */
-	if ((copy = sshbuf_fromb(buf)) == NULL) {
-		r = SSH_ERR_ALLOC_FAIL;
-		goto out;
+	/* KRL must begin with magic string */
+	if ((r = sshbuf_cmp(buf, 0, KRL_MAGIC, sizeof(KRL_MAGIC) - 1)) != 0) {
+		debug2_f("bad KRL magic header");
+		return r;
 	}
-	if ((r = sshbuf_consume(copy, sizeof(KRL_MAGIC) - 1)) != 0)
-		goto out;
 
 	if ((krl = ssh_krl_init()) == NULL) {
 		error_f("alloc failed");
 		goto out;
 	}
-
-	if ((r = sshbuf_get_u32(copy, &format_version)) != 0)
+	/* Don't modify buffer */
+	if ((copy = sshbuf_fromb(buf)) == NULL) {
+		r = SSH_ERR_ALLOC_FAIL;
+		goto out;
+	}
+	if ((r = sshbuf_consume(copy, sizeof(KRL_MAGIC) - 1)) != 0 ||
+	    (r = sshbuf_get_u32(copy, &format_version)) != 0)
 		goto out;
 	if (format_version != KRL_FORMAT_VERSION) {
+		error_f("unsupported KRL format version %u", format_version);
 		r = SSH_ERR_INVALID_FORMAT;
 		goto out;
 	}
@@ -1103,106 +1080,23 @@ ssh_krl_from_blob(struct sshbuf *buf, struct ssh_krl **krlp,
 	    (r = sshbuf_get_u64(copy, &krl->generated_date)) != 0 ||
 	    (r = sshbuf_get_u64(copy, &krl->flags)) != 0 ||
 	    (r = sshbuf_skip_string(copy)) != 0 ||
-	    (r = sshbuf_get_cstring(copy, &krl->comment, NULL)) != 0)
+	    (r = sshbuf_get_cstring(copy, &krl->comment, NULL)) != 0) {
+		error_fr(r, "parse KRL header");
 		goto out;
-
+	}
 	format_timestamp(krl->generated_date, timestamp, sizeof(timestamp));
 	debug("KRL version %llu generated at %s%s%s",
 	    (long long unsigned)krl->krl_version, timestamp,
 	    *krl->comment ? ": " : "", krl->comment);
 
-	/*
-	 * 1st pass: verify signatures, if any. This is done to avoid
-	 * detailed parsing of data whose provenance is unverified.
-	 */
-	sig_seen = 0;
-	if (sshbuf_len(buf) < sshbuf_len(copy)) {
-		/* Shouldn't happen */
-		r = SSH_ERR_INTERNAL_ERROR;
-		goto out;
-	}
-	sects_off = sshbuf_len(buf) - sshbuf_len(copy);
-	while (sshbuf_len(copy) > 0) {
-		if ((r = sshbuf_get_u8(copy, &type)) != 0 ||
-		    (r = sshbuf_get_string_direct(copy, &blob, &blen)) != 0)
-			goto out;
-		KRL_DBG(("first pass, section 0x%02x", type));
-		if (type != KRL_SECTION_SIGNATURE) {
-			if (sig_seen) {
-				error("KRL contains non-signature section "
-				    "after signature");
-				r = SSH_ERR_INVALID_FORMAT;
-				goto out;
-			}
-			/* Not interested for now. */
-			continue;
-		}
-		sig_seen = 1;
-		/* First string component is the signing key */
-		if ((r = sshkey_from_blob(blob, blen, &key)) != 0) {
-			r = SSH_ERR_INVALID_FORMAT;
-			goto out;
-		}
-		if (sshbuf_len(buf) < sshbuf_len(copy)) {
-			/* Shouldn't happen */
-			r = SSH_ERR_INTERNAL_ERROR;
-			goto out;
-		}
-		sig_off = sshbuf_len(buf) - sshbuf_len(copy);
-		/* Second string component is the signature itself */
-		if ((r = sshbuf_get_string_direct(copy, &blob, &blen)) != 0) {
-			r = SSH_ERR_INVALID_FORMAT;
-			goto out;
-		}
-		/* Check signature over entire KRL up to this point */
-		if ((r = sshkey_verify(key, blob, blen,
-		    sshbuf_ptr(buf), sig_off, NULL, 0, NULL)) != 0)
-			goto out;
-		/* Check if this key has already signed this KRL */
-		for (i = 0; i < nca_used; i++) {
-			if (sshkey_equal(ca_used[i], key)) {
-				error("KRL signed more than once with "
-				    "the same key");
-				r = SSH_ERR_INVALID_FORMAT;
-				goto out;
-			}
-		}
-		/* Record keys used to sign the KRL */
-		tmp_ca_used = recallocarray(ca_used, nca_used, nca_used + 1,
-		    sizeof(*ca_used));
-		if (tmp_ca_used == NULL) {
-			r = SSH_ERR_ALLOC_FAIL;
-			goto out;
-		}
-		ca_used = tmp_ca_used;
-		ca_used[nca_used++] = key;
-		key = NULL;
-	}
-
-	if (sshbuf_len(copy) != 0) {
-		/* Shouldn't happen */
-		r = SSH_ERR_INTERNAL_ERROR;
-		goto out;
-	}
-
-	/*
-	 * 2nd pass: parse and load the KRL, skipping the header to the point
-	 * where the section start.
-	 */
-	sshbuf_free(copy);
-	if ((copy = sshbuf_fromb(buf)) == NULL) {
-		r = SSH_ERR_ALLOC_FAIL;
-		goto out;
-	}
-	if ((r = sshbuf_consume(copy, sects_off)) != 0)
-		goto out;
+	/* Parse and load the KRL sections. */
 	while (sshbuf_len(copy) > 0) {
 		sshbuf_free(sect);
 		sect = NULL;
 		if ((r = sshbuf_get_u8(copy, &type)) != 0 ||
 		    (r = sshbuf_froms(copy, &sect)) != 0)
 			goto out;
-		KRL_DBG(("second pass, section 0x%02x", type));
+		KRL_DBG(("section 0x%02x", type));
 
 		switch (type) {
 		case KRL_SECTION_CERTIFICATES:
@@ -1247,51 +1141,12 @@ ssh_krl_from_blob(struct sshbuf *buf, struct ssh_krl **krlp,
 		}
 	}
 
-	/* Check that the key(s) used to sign the KRL weren't revoked */
-	sig_seen = 0;
-	for (i = 0; i < nca_used; i++) {
-		if (ssh_krl_check_key(krl, ca_used[i]) == 0)
-			sig_seen = 1;
-		else {
-			sshkey_free(ca_used[i]);
-			ca_used[i] = NULL;
-		}
-	}
-	if (nca_used && !sig_seen) {
-		error("All keys used to sign KRL were revoked");
-		r = SSH_ERR_KEY_REVOKED;
-		goto out;
-	}
-
-	/* If we have CA keys, then verify that one was used to sign the KRL */
-	if (sig_seen && nsign_ca_keys != 0) {
-		sig_seen = 0;
-		for (i = 0; !sig_seen && i < nsign_ca_keys; i++) {
-			for (j = 0; j < nca_used; j++) {
-				if (ca_used[j] == NULL)
-					continue;
-				if (sshkey_equal(ca_used[j], sign_ca_keys[i])) {
-					sig_seen = 1;
-					break;
-				}
-			}
-		}
-		if (!sig_seen) {
-			r = SSH_ERR_SIGNATURE_INVALID;
-			error("KRL not signed with any trusted key");
-			goto out;
-		}
-	}
-
+	/* Success */
 	*krlp = krl;
 	r = 0;
  out:
 	if (r != 0)
 		ssh_krl_free(krl);
-	for (i = 0; i < nca_used; i++)
-		sshkey_free(ca_used[i]);
-	free(ca_used);
-	sshkey_free(key);
 	sshbuf_free(copy);
 	sshbuf_free(sect);
 	return r;
@@ -1425,7 +1280,7 @@ ssh_krl_file_contains_key(const char *path, const struct sshkey *key)
 		oerrno = errno;
 		goto out;
 	}
-	if ((r = ssh_krl_from_blob(krlbuf, &krl, NULL, 0)) != 0)
+	if ((r = ssh_krl_from_blob(krlbuf, &krl)) != 0)
 		goto out;
 	debug2_f("checking KRL %s", path);
 	r = ssh_krl_check_key(krl, key);
diff --git a/krl.h b/krl.h
index d0f46987..eb244767 100644
--- a/krl.h
+++ b/krl.h
@@ -14,7 +14,7 @@
  * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
  */
 
-/* $OpenBSD: krl.h,v 1.9 2023/07/17 03:57:21 djm Exp $ */
+/* $OpenBSD: krl.h,v 1.10 2023/07/17 04:01:10 djm Exp $ */
 
 #ifndef _KRL_H
 #define _KRL_H
@@ -57,10 +57,8 @@ int ssh_krl_revoke_key_explicit(struct ssh_krl *krl, const struct sshkey *key);
 int ssh_krl_revoke_key_sha1(struct ssh_krl *krl, const u_char *p, size_t len);
 int ssh_krl_revoke_key_sha256(struct ssh_krl *krl, const u_char *p, size_t len);
 int ssh_krl_revoke_key(struct ssh_krl *krl, const struct sshkey *key);
-int ssh_krl_to_blob(struct ssh_krl *krl, struct sshbuf *buf,
-    struct sshkey **sign_keys, u_int nsign_keys);
-int ssh_krl_from_blob(struct sshbuf *buf, struct ssh_krl **krlp,
-    const struct sshkey **sign_ca_keys, size_t nsign_ca_keys);
+int ssh_krl_to_blob(struct ssh_krl *krl, struct sshbuf *buf);
+int ssh_krl_from_blob(struct sshbuf *buf, struct ssh_krl **krlp);
 int ssh_krl_check_key(struct ssh_krl *krl, const struct sshkey *key);
 int ssh_krl_file_contains_key(const char *path, const struct sshkey *key);
 int krl_dump(struct ssh_krl *krl, FILE *f);
diff --git a/ssh-keygen.c b/ssh-keygen.c
index 93c3ff70..9ccea624 100644
--- a/ssh-keygen.c
+++ b/ssh-keygen.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssh-keygen.c,v 1.469 2023/07/14 05:31:44 djm Exp $ */
+/* $OpenBSD: ssh-keygen.c,v 1.470 2023/07/17 04:01:10 djm Exp $ */
 /*
  * Author: Tatu Ylonen <ylo@cs.hut.fi>
  * Copyright (c) 1994 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland
@@ -2223,7 +2223,7 @@ load_krl(const char *path, struct ssh_krl **krlp)
 	if ((r = sshbuf_load_file(path, &krlbuf)) != 0)
 		fatal_r(r, "Unable to load KRL %s", path);
 	/* XXX check sigs */
-	if ((r = ssh_krl_from_blob(krlbuf, krlp, NULL, 0)) != 0 ||
+	if ((r = ssh_krl_from_blob(krlbuf, krlp)) != 0 ||
 	    *krlp == NULL)
 		fatal_r(r, "Invalid KRL file %s", path);
 	sshbuf_free(krlbuf);
@@ -2466,7 +2466,7 @@ do_gen_krl(struct passwd *pw, int updating, const char *ca_key_path,
 
 	if ((kbuf = sshbuf_new()) == NULL)
 		fatal("sshbuf_new failed");
-	if (ssh_krl_to_blob(krl, kbuf, NULL, 0) != 0)
+	if (ssh_krl_to_blob(krl, kbuf) != 0)
 		fatal("Couldn't generate KRL");
 	if ((r = sshbuf_write_file(identity_file, kbuf)) != 0)
 		fatal("write %s: %s", identity_file, strerror(errno));