From ec3d7cf259cdc591ed78242932589e7de1854569 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Mon, 21 Feb 2022 16:15:30 -0500 Subject: [PATCH] Ignore subkeys that cannot sign This ensures that a signature is only accepted if the key in question can actually be used for signing. Tests 265 and 266 verify that RPM completely ignores keys that are not capable of signing. A subkey is considered to be capable of signing if, and only if, its subkey binding signature has a hashed key flags subpacket that contains the flag 0x02. No signature, regardless of type, is permitted to have more than one hashed key flags subpacket. Unhashed key flags subpackets are ignored. The presence of a hashed key flags subpacket is specified by the flag PGPDIG_HAS_KEY_FLAGS in struct pgpDigParams_s::saved. A new `hashed` parameter is added to pgpPrtSubType() to allow hashed and unhashed subpackets to be distinguished. Since PGPDIG_HAS_KEY_FLAGS must not be inherited from a previous use of this `struct pgpDigParams_s`, it is cleared by pgpPrtSig() whenever it processes a new signature. All other saved flags, with the exception of PGPDIG_SAVED_ID and PGPDIG_SAVED_TIME, are also cleared. This allows for future changes to store per-key state without worrying that it will be inherited from a previously used key. The key usage flags are cleared too, although this is not strictly necessary. RFC4880 requires that a subkey binding signature be v4 if the subkey is capable of signing. Requiring a hashed key flags subpacket for such signatures enforces this requirement implicitly, as v3 signatures do not have subpackets. RFC4880 also requires that primary key binding signatures be present and checked. This is not implemented in this commit, but may be implemented in a future commit. To avoid a performance penalty, pgpPrtSig() is changed to ignore the MPIs of a signature if its `tag` parameter is 0. The only caller that sets `tag` to 0 is pgpPrtParamSubkeys(), which does not actually check any cryptographic signatures. The subkey binding signature has been checked earlier in pgpPrtParams(). Finally, use the new `hashed` parameter to pgpPrtSubType() to explicitly ignore *all* creation time subpackets in the unhashed section, even those that are of a length RPM does not know about. Fixes #1911. --- rpmio/digest.h | 2 + rpmio/rpmpgp.c | 70 +++++++++++++++++-- tests/Makefile.am | 1 + .../data/keys/rpm.org-ed25519-subkey-test.pub | 20 ++++++ tests/rpmsigdig.at | 57 ++++++++++++++- 5 files changed, 141 insertions(+), 9 deletions(-) create mode 100644 tests/data/keys/rpm.org-ed25519-subkey-test.pub diff --git a/rpmio/digest.h b/rpmio/digest.h index ec7f3392fc..6a326d20e4 100644 --- a/rpmio/digest.h +++ b/rpmio/digest.h @@ -27,6 +27,7 @@ struct pgpDigParams_s { uint8_t * hash; uint8_t tag; + uint8_t key_flags; /*!< key usage flags */ uint8_t version; /*!< version number. */ uint32_t time; /*!< key/signature creation time. */ uint8_t pubkey_algo; /*!< public key algorithm. */ @@ -41,6 +42,7 @@ struct pgpDigParams_s { #define PGPDIG_SAVED_TIME (1 << 0) #define PGPDIG_SAVED_ID (1 << 1) #define PGPDIG_SIG_HAS_CREATION_TIME (1 << 2) +#define PGPDIG_SIG_HAS_KEY_FLAGS (1 << 3) pgpDigAlg alg; }; diff --git a/rpmio/rpmpgp.c b/rpmio/rpmpgp.c index 59c80d7c43..e73a982876 100644 --- a/rpmio/rpmpgp.c +++ b/rpmio/rpmpgp.c @@ -437,7 +437,7 @@ int pgpSignatureType(pgpDigParams _digp) } static int pgpPrtSubType(const uint8_t *h, size_t hlen, pgpSigType sigtype, - pgpDigParams _digp) + pgpDigParams _digp, int hashed) { const uint8_t *p = h; size_t plen = 0, i; @@ -474,6 +474,8 @@ static int pgpPrtSubType(const uint8_t *h, size_t hlen, pgpSigType sigtype, pgpPrtVal(" ", pgpKeyServerPrefsTbl, p[i]); break; case PGPSUBTYPE_SIG_CREATE_TIME: /* signature creation time */ + if (!hashed) + break; /* RFC 4880 ยง5.2.3.4 creation time MUST be hashed */ if (plen-1 != sizeof(_digp->time)) break; /* other lengths not understood */ if (_digp->saved & PGPDIG_SIG_HAS_CREATION_TIME) @@ -498,6 +500,17 @@ static int pgpPrtSubType(const uint8_t *h, size_t hlen, pgpSigType sigtype, _digp->saved |= PGPDIG_SAVED_ID; memcpy(_digp->signid, p+1, sizeof(_digp->signid)); } + case PGPSUBTYPE_KEY_FLAGS: /* Key usage flags */ + /* Subpackets in the unhashed section cannot be trusted */ + if (!hashed) + break; + /* Reject duplicate key usage flags */ + if (_digp->saved & PGPDIG_SIG_HAS_KEY_FLAGS) + return 1; + impl = *p; + _digp->saved |= PGPDIG_SIG_HAS_KEY_FLAGS; + _digp->key_flags = plen >= 2 ? p[1] : 0; + break; case PGPSUBTYPE_EXPORTABLE_CERT: case PGPSUBTYPE_TRUST_SIG: case PGPSUBTYPE_REGEX: @@ -508,7 +521,6 @@ static int pgpPrtSubType(const uint8_t *h, size_t hlen, pgpSigType sigtype, case PGPSUBTYPE_PREFER_KEYSERVER: case PGPSUBTYPE_PRIMARY_USERID: case PGPSUBTYPE_POLICY_URL: - case PGPSUBTYPE_KEY_FLAGS: case PGPSUBTYPE_SIGNER_USERID: case PGPSUBTYPE_REVOKE_REASON: case PGPSUBTYPE_FEATURES: @@ -600,6 +612,7 @@ static int pgpPrtSig(pgpTag tag, const uint8_t *h, size_t hlen, /* Reset the saved flags */ _digp->saved &= PGPDIG_SAVED_TIME | PGPDIG_SAVED_ID; + _digp->key_flags = 0; if (pgpVersion(h, hlen, &version)) return rc; @@ -639,7 +652,7 @@ static int pgpPrtSig(pgpTag tag, const uint8_t *h, size_t hlen, } p = ((uint8_t *)v) + sizeof(*v); - rc = pgpPrtSigParams(tag, v->pubkey_algo, p, h, hlen, _digp); + rc = tag ? pgpPrtSigParams(tag, v->pubkey_algo, p, h, hlen, _digp) : 0; } break; case 4: { pgpPktSigV4 v = (pgpPktSigV4)h; @@ -666,7 +679,7 @@ static int pgpPrtSig(pgpTag tag, const uint8_t *h, size_t hlen, _digp->hashlen = sizeof(*v) + plen; _digp->hash = memcpy(xmalloc(_digp->hashlen), v, _digp->hashlen); } - if (pgpPrtSubType(p, plen, v->sigtype, _digp)) + if (pgpPrtSubType(p, plen, v->sigtype, _digp, 1)) return 1; p += plen; @@ -680,7 +693,7 @@ static int pgpPrtSig(pgpTag tag, const uint8_t *h, size_t hlen, if ((p + plen) > hend) return 1; - if (pgpPrtSubType(p, plen, v->sigtype, _digp)) + if (pgpPrtSubType(p, plen, v->sigtype, _digp, 0)) return 1; p += plen; @@ -701,7 +714,7 @@ static int pgpPrtSig(pgpTag tag, const uint8_t *h, size_t hlen, if (p > hend) return 1; - rc = pgpPrtSigParams(tag, v->pubkey_algo, p, h, hlen, _digp); + rc = tag ? pgpPrtSigParams(tag, v->pubkey_algo, p, h, hlen, _digp) : 0; } break; default: rpmlog(RPMLOG_WARNING, _("Unsupported version of signature: V%d\n"), version); @@ -1104,6 +1117,29 @@ static int pgpVerifySelf(pgpDigParams key, pgpDigParams selfsig, return rc; } +static int parseSubkeySig(const struct pgpPkt *pkt, uint8_t tag, + pgpDigParams *params_p) { + int rc = -1; /* assume failure */ + pgpDigParams params = *params_p; + + if (params == NULL) + params = *params_p = pgpDigParamsNew(tag); + + if (pkt->tag != PGPTAG_SIGNATURE) + goto fail; + + if (pgpPrtSig(tag, pkt->body, pkt->blen, params)) + goto fail; + + if (params->sigtype != PGPSIGTYPE_SUBKEY_BINDING && + params->sigtype != PGPSIGTYPE_SUBKEY_REVOKE) + goto fail; + + rc = 0; +fail: + return rc; +} + static const size_t RPM_MAX_OPENPGP_BYTES = 65535; /* max number of bytes in a key */ int pgpPrtParams(const uint8_t * pkts, size_t pktlen, unsigned int pkttype, @@ -1193,6 +1229,7 @@ int pgpPrtParamsSubkeys(const uint8_t *pkts, size_t pktlen, const uint8_t *p = pkts; const uint8_t *pend = pkts + pktlen; pgpDigParams *digps = NULL; + pgpDigParams params = NULL; int count = 0; int alloced = 10; struct pgpPkt pkt; @@ -1225,10 +1262,29 @@ int pgpPrtParamsSubkeys(const uint8_t *pkts, size_t pktlen, pgpDigParamsFree(digps[count]); continue; } - count++; + + if (decodePkt(p, pend - p, &pkt) || + parseSubkeySig(&pkt, 0, ¶ms)) + { + pgpDigParamsFree(digps[count]); + break; + } + + if (params->sigtype != PGPSIGTYPE_SUBKEY_BINDING || + !((params->saved & PGPDIG_SIG_HAS_KEY_FLAGS) && + (params->key_flags & 0x02))) { + /* Subkey is revoked or incapable of signing */ + pgpDigParamsFree(digps[count]); + } else { + digps[count]->key_flags = params->key_flags; + digps[count]->saved |= PGPDIG_SIG_HAS_KEY_FLAGS; + count++; + } + p += (pkt.body - pkt.head) + pkt.blen; } } rc = (p == pend) ? 0 : -1; + pgpDigParamsFree(params); if (rc == 0) { *subkeys = xrealloc(digps, count * sizeof(*digps)); diff --git a/tests/Makefile.am b/tests/Makefile.am index e58a9fd079..0bda8aa140 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -113,6 +113,7 @@ EXTRA_DIST += data/SOURCES/hello.c EXTRA_DIST += data/SPECS/hello-attr-buildid.spec EXTRA_DIST += data/SPECS/hello-config-buildid.spec EXTRA_DIST += data/SPECS/hello-cd.spec +EXTRA_DIST += data/keys/rpm.org-ed25519-subkey-test.pub EXTRA_DIST += data/keys/rpm.org-rsa-2048-test.pub EXTRA_DIST += data/keys/rpm.org-rsa-2048-test.secret EXTRA_DIST += data/keys/CVE-2021-3521-badbind.asc diff --git a/tests/data/keys/rpm.org-ed25519-subkey-test.pub b/tests/data/keys/rpm.org-ed25519-subkey-test.pub new file mode 100644 index 0000000000..1ad7ce5048 --- /dev/null +++ b/tests/data/keys/rpm.org-ed25519-subkey-test.pub @@ -0,0 +1,20 @@ +-----BEGIN PGP PUBLIC KEY BLOCK----- + +mDMEYhf4KRYJKwYBBAHaRw8BAQdAr/MfCGYyI1lNpG4h5Tvdw62G9CgDsn+nweQA +bYtC9bC0NXJwbS5vcmcgZWQyNTUxOSB0ZXN0a2V5IHdpdGggc3Via2V5IDxlZDI1 +NTE5QHJwbS5vcmc+iJoEExYKAEIWIQROGuLaBUTEjXgeGiLaH1/8yyIDHwUCYhf5 +UAIbAwUJA8JnAAULCQgHAgMiAgEGFQoJCAsCBBYCAwECHgcCF4AACgkQ2h9f/Msi +Ax/VxQEAm5tVLQRyLHzHpSXz8i8IZsK5kZHGsiDAlvgDubMD5d8A/3ig8MgOBlri +HdsUvvpZma/cJQgfyRHjfh9fkQOOg+cAuDgEYhf4KRIKKwYBBAGXVQEFAQEHQFNp +5tOJofXuDMVlzjnUC1FXp4La5BWkMH469jIbFlhAAwEIB4h4BBgWCgAgFiEEThri +2gVExI14Hhoi2h9f/MsiAx8FAmIX+CkCGwwACgkQ2h9f/MsiAx+kNAEA0eXCnGqX +jdaaPs1cKeGAkgzGwnwvut+YPD7+9H0r1eIA/RQw5yoTIJUButy55lx/mLGUPq0G +N1RhDNg9wosqxfkCuDMEYhf4ehYJKwYBBAHaRw8BAQdASWiPf9/Cd4yD3GtDZFI+ +pcPhdhfWb5Ndv1hP+ndTkSyI7wQYFgoAIBYhBE4a4toFRMSNeB4aItofX/zLIgMf +BQJiF/h6AhsCAIEJENofX/zLIgMfdiAEGRYKAB0WIQTc1zsT3tx9EJeerHXL9SUW +mxSfwgUCYhf4egAKCRDL9SUWmxSfwrjXAPwJf7n8qpEvee1/wCFr0XwObuhOZ+vB +ZeubWyEpWe/XqwEA1WCah/7cgAhYf4u1V7Ciel0kQskE0aAOvtJE/EZqCwix5wEA +14PxZWyO0PrU8AeKR2U/A9jM/81SpWjNMT7X52UYFmgBAL3GaEXFIC5STG9Qsopd +t1vfoExOCG5Ewg7CWOeLF9EB +=Fk+p +-----END PGP PUBLIC KEY BLOCK----- diff --git a/tests/rpmsigdig.at b/tests/rpmsigdig.at index 24c44c3b20..a9b5a14efb 100644 --- a/tests/rpmsigdig.at +++ b/tests/rpmsigdig.at @@ -247,8 +247,61 @@ UNW2iqnN3BA7guhOv6OMiROF1+I7Q5nWT63mQC7IgQ== gpg(rpm.org RSA testkey ) = 4:4344591e1964c5fc-58e63918 gpg(1964c5fc) = 4:4344591e1964c5fc-58e63918 gpg(4344591e1964c5fc) = 4:4344591e1964c5fc-58e63918 -gpg(f00650f8) = 4:185e6146f00650f8-58e63918 -gpg(185e6146f00650f8) = 4:185e6146f00650f8-58e63918 +], +[]) +AT_CLEANUP + +# ----------------------------------------- +# Import a public Ed25519 key with a subkey +AT_SETUP([rpmkeys --import ed25519 with subkey]) +AT_KEYWORDS([rpmkeys import]) +AT_CHECK([ +RPMDB_INIT + +runroot rpmkeys --import /data/keys/rpm.org-ed25519-subkey-test.pub +runroot rpm -qi gpg-pubkey-cb22031f-6217f950|grep -v Date|grep -v Version: +runroot rpm -q --provides gpg-pubkey +], +[0], +[Name : gpg-pubkey +Version : cb22031f +Release : 6217f950 +Architecture: (none) +Group : Public Keys +Size : 0 +License : pubkey +Signature : (none) +Source RPM : (none) +Build Host : localhost +Packager : rpm.org ed25519 testkey with subkey +Summary : rpm.org ed25519 testkey with subkey public key +Description : +-----BEGIN PGP PUBLIC KEY BLOCK----- + +mDMEYhf4KRYJKwYBBAHaRw8BAQdAr/MfCGYyI1lNpG4h5Tvdw62G9CgDsn+nweQA +bYtC9bC0NXJwbS5vcmcgZWQyNTUxOSB0ZXN0a2V5IHdpdGggc3Via2V5IDxlZDI1 +NTE5QHJwbS5vcmc+iJoEExYKAEIWIQROGuLaBUTEjXgeGiLaH1/8yyIDHwUCYhf5 +UAIbAwUJA8JnAAULCQgHAgMiAgEGFQoJCAsCBBYCAwECHgcCF4AACgkQ2h9f/Msi +Ax/VxQEAm5tVLQRyLHzHpSXz8i8IZsK5kZHGsiDAlvgDubMD5d8A/3ig8MgOBlri +HdsUvvpZma/cJQgfyRHjfh9fkQOOg+cAuDgEYhf4KRIKKwYBBAGXVQEFAQEHQFNp +5tOJofXuDMVlzjnUC1FXp4La5BWkMH469jIbFlhAAwEIB4h4BBgWCgAgFiEEThri +2gVExI14Hhoi2h9f/MsiAx8FAmIX+CkCGwwACgkQ2h9f/MsiAx+kNAEA0eXCnGqX +jdaaPs1cKeGAkgzGwnwvut+YPD7+9H0r1eIA/RQw5yoTIJUButy55lx/mLGUPq0G +N1RhDNg9wosqxfkCuDMEYhf4ehYJKwYBBAHaRw8BAQdASWiPf9/Cd4yD3GtDZFI+ +pcPhdhfWb5Ndv1hP+ndTkSyI7wQYFgoAIBYhBE4a4toFRMSNeB4aItofX/zLIgMf +BQJiF/h6AhsCAIEJENofX/zLIgMfdiAEGRYKAB0WIQTc1zsT3tx9EJeerHXL9SUW +mxSfwgUCYhf4egAKCRDL9SUWmxSfwrjXAPwJf7n8qpEvee1/wCFr0XwObuhOZ+vB +ZeubWyEpWe/XqwEA1WCah/7cgAhYf4u1V7Ciel0kQskE0aAOvtJE/EZqCwix5wEA +14PxZWyO0PrU8AeKR2U/A9jM/81SpWjNMT7X52UYFmgBAL3GaEXFIC5STG9Qsopd +t1vfoExOCG5Ewg7CWOeLF9EB +=Fk+p +-----END PGP PUBLIC KEY BLOCK----- + +gpg(rpm.org ed25519 testkey with subkey ) = 4:da1f5ffccb22031f-6217f950 +gpg(cb22031f) = 4:da1f5ffccb22031f-6217f950 +gpg(da1f5ffccb22031f) = 4:da1f5ffccb22031f-6217f950 +gpg(9b149fc2) = 4:cbf525169b149fc2-6217f87a +gpg(cbf525169b149fc2) = 4:cbf525169b149fc2-6217f87a ], []) AT_CLEANUP