Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
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 reject *all* creation time subpackets in the unhashed section, even those that are of a length RPM does not know about. Fixes rpm-software-management#1911.
- Loading branch information
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -437,7 +437,7 @@ int pgpSignatureType(pgpDigParams _digp) | |
| } | ||
|
|
||
| static int pgpPrtSubType(const uint8_t *h, size_t hlen, pgpSigType sigtype, | ||
| 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) | ||
| return 1; /* RFC 4880 §5.2.3.4 creation time MUST be hashed */ | ||
This comment has been minimized.
Sorry, something went wrong.
This comment has been minimized.
Sorry, something went wrong. 
DemiMarie
Author
Owner
|
||
| 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_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 = 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, 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, 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 = tag ? pgpPrtSigParams(tag, v->pubkey_algo, p, h, hlen, _digp) : 0; | ||
| } break; | ||
| default: | ||
| rpmlog(RPMLOG_WARNING, _("Unsupported version of signature: V%d\n"), version); | ||
|
|
@@ -1186,13 +1199,37 @@ int pgpPrtParams(const uint8_t * pkts, size_t pktlen, unsigned int pkttype, | |
| return rc; | ||
| } | ||
|
|
||
| static int parseSubkeyBindingSig(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) | ||
| goto fail; | ||
|
|
||
| rc = 0; | ||
| fail: | ||
| return rc; | ||
| } | ||
|
|
||
|
|
||
| int pgpPrtParamsSubkeys(const uint8_t *pkts, size_t pktlen, | ||
| pgpDigParams mainkey, pgpDigParams **subkeys, | ||
| int *subkeysCount) | ||
| { | ||
| 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,27 @@ int pgpPrtParamsSubkeys(const uint8_t *pkts, size_t pktlen, | |
| pgpDigParamsFree(digps[count]); | ||
| continue; | ||
| } | ||
|
|
||
| if (decodePkt(p, pend - p, &pkt) || | ||
| parseSubkeyBindingSig(&pkt, 0, ¶ms)) | ||
| { | ||
| pgpDigParamsFree(digps[count]); | ||
| break; | ||
| } | ||
|
|
||
| if (!((params->saved & PGPDIG_SIG_HAS_KEY_FLAGS) && | ||
| (params->key_flags & 0x02))) { | ||
| 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)); | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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----- |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -247,8 +247,61 @@ UNW2iqnN3BA7guhOv6OMiROF1+I7Q5nWT63mQC7IgQ== | |
| gpg(rpm.org RSA testkey <[email protected]>) = 4:4344591e1964c5fc-58e63918 | ||
| gpg(1964c5fc) = 4:4344591e1964c5fc-58e63918 | ||
| gpg(4344591e1964c5fc) = 4:4344591e1964c5fc-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 <[email protected]> | ||
| Summary : rpm.org ed25519 testkey with subkey <[email protected]> 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 <[email protected]>) = 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 | ||
|
|
||
I think this check is too strict. The subpacket needs to be hashed, yes, but if there is also an unhashed version, the latter should be ignored. So: s/return 1/continue.