From ae356661b86e48000f8c20a1977ecca19ae6f26d Mon Sep 17 00:00:00 2001
From: Demi Marie Obenour <demi@invisiblethingslab.com>
Date: Fri, 1 Apr 2022 20:55:33 -0400
Subject: [PATCH] Restrict packets following a subkey

According to RFC 4880, subkeys are supposed to only be followed by
binding signatures, revocation signatures, and other subkeys.  In
practice, the most likely reason to get something else is a revocation
signature of a type that RPM does not implement.  This could include a
revocation of a subkey binding signature or a revocation made by a
designated revoker.  In this case, it is not safe to import the key, as
RPM might accept a signature made by a subkey whose secret portion has
been compromised.  Therefore, return an error in this case.
---
 rpmio/rpmpgp_internal.c | 35 ++++++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 13 deletions(-)

diff --git a/rpmio/rpmpgp_internal.c b/rpmio/rpmpgp_internal.c
index f36c28f339..98b6878afa 100644
--- a/rpmio/rpmpgp_internal.c
+++ b/rpmio/rpmpgp_internal.c
@@ -927,8 +927,12 @@ static int parseSubkeySig(const struct pgpPkt *pkt, uint8_t tag,
 			  pgpDigParams *params_p) {
     pgpDigParams params = *params_p = NULL; /* assume failure */
 
-    if (pkt->tag != PGPTAG_SIGNATURE)
+    if (pkt->tag != PGPTAG_SIGNATURE) {
+	rpmlog(RPMLOG_WARNING, "Key had an unexpected packet (type %u) "
+		               "where a subkey signature was expected",
+	       (unsigned int)pkt->tag);
 	goto fail;
+    }
 
     params = pgpDigParamsNew(tag);
 
@@ -938,6 +942,12 @@ static int parseSubkeySig(const struct pgpPkt *pkt, uint8_t tag,
     if (params->sigtype != PGPSIGTYPE_SUBKEY_BINDING &&
 	params->sigtype != PGPSIGTYPE_SUBKEY_REVOKE)
     {
+	/* It isn't safe to ignore this, because it could be a
+	 * revocation of a binding signature or something else that
+	 * should cause the subkey to be ignored.  Return an error
+	 * here, but log a warning so that users know what is going on. */
+	rpmlog(RPMLOG_WARNING, _("Unsupported subkey signature type %u\n"),
+	       (unsigned int)params->sigtype);
 	goto fail;
     }
 
@@ -970,7 +980,6 @@ int pgpPrtParams(const uint8_t * pkts, size_t pktlen, unsigned int pkttype,
     const uint8_t *p = pkts;
     const uint8_t *pend = pkts + pktlen;
     pgpDigParams digp = NULL;
-    pgpDigParams selfsig = NULL;
     int i = 0;
     int alloced = 16; /* plenty for normal cases */
     int rc = -1; /* assume failure */
@@ -994,24 +1003,25 @@ int pgpPrtParams(const uint8_t * pkts, size_t pktlen, unsigned int pkttype,
 	    }
 	}
 
-	if (expect) {
-	    if (pkt->tag != expect)
-		break;
-	    selfsig = pgpDigParamsNew(pkt->tag);
-	}
-
-	if (pgpPrtPkt(pkt, selfsig ? selfsig : digp))
+	if (expect && pkt->tag != expect)
 	    break;
+	expect = 0;
 
-	if (selfsig) {
+	/* Do we need to verify a self-signature? */
+	if (prevtag == PGPTAG_PUBLIC_SUBKEY &&
+	    pkt->tag != PGPTAG_PUBLIC_SUBKEY)
+	{
+	    pgpDigParams selfsig = NULL;
+	    if (parseSubkeySig(pkt, pkt->tag, &selfsig))
+		break;
 	    /* subkeys must be followed by binding or revocation signature */
 	    int xx = pgpVerifySelf(digp, selfsig, all, i, prevtag);
 
 	    selfsig = pgpDigParamsFree(selfsig);
 	    if (xx)
 		break;
-	    expect = 0;
-	}
+	} else if (pgpPrtPkt(pkt, digp))
+	    break;
 
 	if (pkt->tag == PGPTAG_PUBLIC_SUBKEY)
 	    expect = PGPTAG_SIGNATURE;
@@ -1032,7 +1042,6 @@ int pgpPrtParams(const uint8_t * pkts, size_t pktlen, unsigned int pkttype,
     rc = (digp && (p == pend) && expect == 0) ? 0 : -1;
 
     free(all);
-    selfsig = pgpDigParamsFree(selfsig);
     if (ret && rc == 0) {
 	*ret = digp;
     } else {