Fix bounds checks in public key parsing

If a public key was too short for the curve ID, the code would
previously perform out-of-bounds pointer arithmetic, which is undefined
behavior in C.  Check that the packet is long enough to contain the
curve ID before bumping `se` past the curve ID.

Furthermore, if a public key is too short to even contain the fixed-size
header, an out-of-bounds pointer would be created, which is also
undefined behavior.  Fix this by returning early if the buffer is too
short.

Finally, return early if the public key algorithm or curve ID is
invalid, rather than relying in processMpis() to fail.  While
processMpis() will error out, bailing out explicitly is much clearer.

1 files changed, 20 insertions, 21 deletions

diff --git a/rpmio/rpmpgp.c b/rpmio/rpmpgp.c
index 35603286f..015c15a5c 100644
--- a/rpmio/rpmpgp.c
+++ b/rpmio/rpmpgp.c
@@ -834,28 +834,27 @@ int pgpPubkeyFingerprint(const uint8_t *h, size_t hlen,
       {	pgpPktKeyV4 v = (pgpPktKeyV4) (h);
 	int mpis = -1;
 
-	/* Packet must be larger than v to have room for the required MPIs */
-	if (hlen > sizeof(*v)) {
-	    switch (v->pubkey_algo) {
-	    case PGPPUBKEYALGO_RSA:
-		mpis = 2;
-		break;
-	    case PGPPUBKEYALGO_DSA:
-		mpis = 4;
-		break;
-	    case PGPPUBKEYALGO_EDDSA:
-		mpis = 1;
-		break;
-	    }
-	}
-
+	/* Packet must be strictly larger than v to have room for the
+	 * required MPIs and (for EdDSA) the curve ID */
+	if (hlen < sizeof(*v) + sizeof(uint8_t))
+	    return rc;
 	se = (uint8_t *)(v + 1);
-	/* EdDSA has a curve id before the MPIs */
-	if (v->pubkey_algo == PGPPUBKEYALGO_EDDSA) {
-	    if (se < pend && se[0] != 0x00 && se[0] != 0xff)
-		se += 1 + se[0];
-	    else
-		se = pend;      /* error out when reading the MPI */
+	switch (v->pubkey_algo) {
+	case PGPPUBKEYALGO_EDDSA:
+	    /* EdDSA has a curve id before the MPIs */
+	    if (se[0] == 0x00 || se[0] == 0xff || pend - se < 1 + se[0])
+		return rc;
+	    se += 1 + se[0];
+	    mpis = 1;
+	    break;
+	case PGPPUBKEYALGO_RSA:
+	    mpis = 2;
+	    break;
+	case PGPPUBKEYALGO_DSA:
+	    mpis = 4;
+	    break;
+	default:
+	    return rc;
 	}
 
 	/* Does the size and number of MPI's match our expectations? */