Skip to content

Commit

Permalink
Validate self-signatures and require subkey bindings on PGP public keys
Browse files Browse the repository at this point in the history
All subkeys must be followed by a binding signature by the primary key
as per the OpenPGP RFC, enforce the presence and validity in the parser.
In addition, validate user ID certification signatures when present.

The implementation is as kludgey as they come to work around our
simple-minded parser structure without touching API, to maximise
backportability. Store all the raw packets internally as we decode them
to be able to access previous elements at will, needed to validate ordering
and access the actual data. Add testcases for manipulated keys whose
import previously would succeed.

Depends on the two previous commits:
db7ab0a and
e3dd156

Fixes CVE-2021-3521 and more.
  • Loading branch information
pmatilai committed Oct 11, 2021
1 parent 2642412 commit 1dcfa3c
Show file tree
Hide file tree
Showing 7 changed files with 275 additions and 7 deletions.
124 changes: 117 additions & 7 deletions rpmio/rpmpgp.c
Original file line number Diff line number Diff line change
Expand Up @@ -1062,37 +1062,147 @@ static pgpDigParams pgpDigParamsNew(uint8_t tag)
return digp;
}

static int hashKey(DIGEST_CTX hash, const struct pgpPkt *pkt, int exptag)
{
int rc = -1;
if (pkt->tag == exptag) {
uint8_t head[] = {
0x99,
(pkt->blen >> 8),
(pkt->blen ),
};

rpmDigestUpdate(hash, head, 3);
rpmDigestUpdate(hash, pkt->body, pkt->blen);
rc = 0;
}
return rc;
}

static int hashUser(DIGEST_CTX hash, const struct pgpPkt *pkt, int exptag)
{
int rc = -1;
if (pkt->tag == exptag) {
uint8_t head[] = {
0xb4,
(pkt->blen >> 24),
(pkt->blen >> 16),
(pkt->blen >> 8),
(pkt->blen ),
};
rpmDigestUpdate(hash, head, 5);
rpmDigestUpdate(hash, pkt->body, pkt->blen);
rc = 0;
}
return rc;
}

static int pgpVerifySelf(pgpDigParams key, pgpDigParams selfsig,
const struct pgpPkt *all, int i)
{
int rc = -1;
DIGEST_CTX hash = NULL;

switch (selfsig->sigtype) {
case PGPSIGTYPE_SUBKEY_BINDING:
hash = rpmDigestInit(selfsig->hash_algo, 0);
if (hash) {
rc = hashKey(hash, &all[0], PGPTAG_PUBLIC_KEY);
if (!rc)
rc = hashKey(hash, &all[i-1], PGPTAG_PUBLIC_SUBKEY);
}
break;
case PGPSIGTYPE_GENERIC_CERT:
case PGPSIGTYPE_PERSONA_CERT:
case PGPSIGTYPE_CASUAL_CERT:
case PGPSIGTYPE_POSITIVE_CERT:
hash = rpmDigestInit(selfsig->hash_algo, 0);
if (hash) {
rc = hashKey(hash, &all[0], PGPTAG_PUBLIC_KEY);
if (!rc)
rc = hashUser(hash, &all[i-1], PGPTAG_USER_ID);
}
break;
default:
/* ignore unknown types */
rc = 0;
break;
}

if (hash && rc == 0)
rc = pgpVerifySignature(key, selfsig, hash);

rpmDigestFinal(hash, NULL, NULL, 0);

return rc;
}

int pgpPrtParams(const uint8_t * pkts, size_t pktlen, unsigned int pkttype,
pgpDigParams * ret)
{
const uint8_t *p = pkts;
const uint8_t *pend = pkts + pktlen;
pgpDigParams digp = NULL;
struct pgpPkt pkt;
pgpDigParams selfsig = NULL;
int i = 0;
int alloced = 16; /* plenty for normal cases */
struct pgpPkt *all = xmalloc(alloced * sizeof(*all));
int rc = -1; /* assume failure */
int expect = 0;

while (p < pend) {
if (decodePkt(p, (pend - p), &pkt))
struct pgpPkt *pkt = &all[i];
if (decodePkt(p, (pend - p), pkt))
break;

if (pkt->tag == PGPTAG_PUBLIC_SUBKEY)
expect = PGPTAG_SIGNATURE;

if (digp == NULL) {
if (pkttype && pkt.tag != pkttype) {
if (pkttype && pkt->tag != pkttype) {
break;
} else {
digp = pgpDigParamsNew(pkt.tag);
digp = pgpDigParamsNew(pkt->tag);
}
}

if (pgpPrtPkt(&pkt, digp))
if (digp->tag == PGPTAG_PUBLIC_KEY && pkt->tag == PGPTAG_SIGNATURE)
selfsig = pgpDigParamsNew(pkt->tag);

if (pgpPrtPkt(pkt, selfsig ? selfsig : digp))
break;

p += (pkt.body - pkt.head) + pkt.blen;
/* subkeys must be followed by binding signature */
if (i > 0 && all[i-1].tag == PGPTAG_PUBLIC_SUBKEY)
if (!(selfsig && selfsig->sigtype == PGPSIGTYPE_SUBKEY_BINDING))
break;

if (selfsig) {
int xx = -1;

if (i > 0)
xx = pgpVerifySelf(digp, selfsig, all, i);

selfsig = pgpDigParamsFree(selfsig);
if (xx)
break;
expect = 0;
}

i++;
p += (pkt->body - pkt->head) + pkt->blen;
if (pkttype == PGPTAG_SIGNATURE)
break;

if (alloced <= i) {
alloced *= 2;
all = xrealloc(all, alloced * sizeof(*all));
}
}

rc = (digp && (p == pend)) ? 0 : -1;
rc = (digp && (p == pend) && expect == 0) ? 0 : -1;

free(all);
if (ret && rc == 0) {
*ret = digp;
} else {
Expand Down
4 changes: 4 additions & 0 deletions tests/Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,10 @@ EXTRA_DIST += data/SPECS/hello-config-buildid.spec
EXTRA_DIST += data/SPECS/hello-cd.spec
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
EXTRA_DIST += data/keys/CVE-2021-3521-baduser.asc
EXTRA_DIST += data/keys/CVE-2021-3521-nosubsig.asc
EXTRA_DIST += data/keys/CVE-2021-3521-nosubsig-last.asc
EXTRA_DIST += data/macros.testfile
EXTRA_DIST += data/macros.debug
EXTRA_DIST += data/SOURCES/foo.c
Expand Down
25 changes: 25 additions & 0 deletions tests/data/keys/CVE-2021-3521-badbind.asc
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
-----BEGIN PGP PUBLIC KEY BLOCK-----
Version: rpm-4.17.90 (NSS-3)

mQENBFjmORgBCAC7TMEk6wnjSs8Dr4yqSScWdU2pjcqrkTxuzdWvowcIUPZI0w/g
HkRqGd4apjvY2V15kjL10gk3QhFP3pZ/9p7zh8o8NHX7aGdSGDK7NOq1eFaErPRY
91LW9RiZ0lbOjXEzIL0KHxUiTQEmdXJT43DJMFPyW9fkCWg0OltiX618FUdWWfI8
eySdLur1utnqBvdEbCUvWK2RX3vQZQdvEBODnNk2pxqTyV0w6VPQ96W++lF/5Aas
7rUv3HIyIXxIggc8FRrnH+y9XvvHDonhTIlGnYZN4ubm9i4y3gOkrZlGTrEw7elQ
1QeMyG2QQEbze8YjpTm4iLABCBrRfPRaQpwrABEBAAG0IXJwbS5vcmcgUlNBIHRl
c3RrZXkgPHJzYUBycG0ub3JnPokBNwQTAQgAIQUCWOY5GAIbAwULCQgHAgYVCAkK
CwIEFgIDAQIeAQIXgAAKCRBDRFkeGWTF/MxxCACnjqFL+MmPh9W9JQKT2DcLbBzf
Cqo6wcEBoCOcwgRSk8dSikhARoteoa55JRJhuMyeKhhEAogE9HRmCPFdjezFTwgB
BDVBpO2dZ023mLXDVCYX3S8pShOgCP6Tn4wqCnYeAdLcGg106N4xcmgtcssJE+Pr
XzTZksbZsrTVEmL/Ym+R5w5jBfFnGk7Yw7ndwfQsfNXQb5AZynClFxnX546lcyZX
fEx3/e6ezw57WNOUK6WT+8b+EGovPkbetK/rGxNXuWaP6X4A/QUm8O98nCuHYFQq
+mvNdsCBqGf7mhaRGtpHk/JgCn5rFvArMDqLVrR9hX0LdCSsH7EGE+bR3r7wuQEN
BFjmORgBCACk+vDZrIXQuFXEYToZVwb2attzbbJJCqD71vmZTLsW0QxuPKRgbcYY
zp4K4lVBnHhFrF8MOUOxJ7kQWIJZMZFt+BDcptCYurbD2H4W2xvnWViiC+LzCMzz
iMJT6165uefL4JHTDPxC2fFiM9yrc72LmylJNkM/vepT128J5Qv0gRUaQbHiQuS6
Dm/+WRnUfx3i89SV4mnBxb/Ta93GVqoOciWwzWSnwEnWYAvOb95JL4U7c5J5f/+c
KnQDHsW7sIiIdscsWzvgf6qs2Ra1Zrt7Fdk4+ZS2f/adagLhDO1C24sXf5XfMk5m
L0OGwZSr9m5s17VXxfspgU5ugc8kBJfzABEBAAE=
=WCfs
-----END PGP PUBLIC KEY BLOCK-----

31 changes: 31 additions & 0 deletions tests/data/keys/CVE-2021-3521-baduser.asc
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
-----BEGIN PGP PUBLIC KEY BLOCK-----
Version: rpm-4.17.90 (NSS-3)

mQENBFjmORgBCAC7TMEk6wnjSs8Dr4yqSScWdU2pjcqrkTxuzdWvowcIUPZI0w/g
HkRqGd4apjvY2V15kjL10gk3QhFP3pZ/9p7zh8o8NHX7aGdSGDK7NOq1eFaErPRY
91LW9RiZ0lbOjXEzIL0KHxUiTQEmdXJT43DJMFPyW9fkCWg0OltiX618FUdWWfI8
eySdLur1utnqBvdEbCUvWK2RX3vQZQdvEBODnNk2pxqTyV0w6VPQ96W++lF/5Aas
7rUv3HIyIXxIggc8FRrnH+y9XvvHDonhTIlGnYZN4ubm9i4y3gOkrZlGTrEw7elQ
1QeMyG2QQEbze8YjpTm4iLABCBrRfPRaQpwrABEBAAG0IWYwMGYucmcgUlNBIHRl
c3RrZXkgPHJzYUBycG0ub3JnPokBNwQTAQgAIQUCWOY5GAIbAwULCQgHAgYVCAkK
CwIEFgIDAQIeAQIXgAAKCRBDRFkeGWTF/MxxCACnjqFL+MmPh9W9JQKT2DcLbBzf
Cqo6wcEBoCOcwgRSk8dSikhARoteoa55JRJhuMyeKhhEAogE9HRmCPFdjezFTwgB
BDVBpO2dZ023mLXDVCYX3S8pShOgCP6Tn4wqCnYeAdLcGg106N4xcmgtcssJE+Pr
XzTZksbZsrTVEmL/Ym+R5w5jBfFnGk7Yw7ndwfQsfNXQb5AZynClFxnX546lcyZX
fEx3/e6ezw57WNOUK6WT+8b+EGovPkbetK/rGxNXuWaP6X4A/QUm8O98nCuHYFQq
+mvNdsCBqGf7mhaRGtpHk/JgCn5rFvArMDqLVrR9hX0LdCSsH7EGE+bR3r7wuQEN
BFjmORgBCACk+vDZrIXQuFXEYToZVwb2attzbbJJCqD71vmZTLsW0QxuPKRgbcYY
zp4K4lVBnHhFrF8MOUOxJ7kQWIJZMZFt+BDcptCYurbD2H4W2xvnWViiC+LzCMzz
iMJT6165uefL4JHTDPxC2fFiM9yrc72LmylJNkM/vepT128J5Qv0gRUaQbHiQuS6
Dm/+WRnUfx3i89SV4mnBxb/Ta93GVqoOciWwzWSnwEnWYAvOb95JL4U7c5J5f/+c
KnQDHsW7sIiIdscsWzvgf6qs2Ra1Zrt7Fdk4+ZS2f/adagLhDO1C24sXf5XfMk5m
L0OGwZSr9m5s17VXxfspgU5ugc8kBJfzABEBAAGJAR8EGAEIAAkFAljmORgCGwwA
CgkQQ0RZHhlkxfzwDQf/Y5on5o+s/xD3tDyRYa6SErfT44lEArdCD7Yi+cygJFox
3jyM8ovtJAkwRegwyxcaLN7zeG1p1Sk9ZAYWQEJT6qSU4Ppu+CVGHgxgnTcfUiu6
EZZQE6srvua53IMY1lT50M7vx0T5VicHFRWBFV2C/Mc32p7cEE6nn45nEZgUXQNl
ySEyvoRlsAJq6gFsfqucVz2vMJDTMVczUtq1CjvUqFbif8JVL36EoZCf1SeRw6d6
s1Kp3AA33Rjd+Uw87HJ4EIB75zMFQX2H0ggAVdYTQcqGXHP5MZK1jJrHfxJyMi3d
UNW2iqnN3BA7guhOv6OMiROF1+I7Q5nWT63mQC7IgQ==
=sE8L
-----END PGP PUBLIC KEY BLOCK-----

25 changes: 25 additions & 0 deletions tests/data/keys/CVE-2021-3521-nosubsig-last.asc
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
-----BEGIN PGP PUBLIC KEY BLOCK-----
Version: rpm-4.17.90 (NSS-3)

mQENBFjmORgBCAC7TMEk6wnjSs8Dr4yqSScWdU2pjcqrkTxuzdWvowcIUPZI0w/g
HkRqGd4apjvY2V15kjL10gk3QhFP3pZ/9p7zh8o8NHX7aGdSGDK7NOq1eFaErPRY
91LW9RiZ0lbOjXEzIL0KHxUiTQEmdXJT43DJMFPyW9fkCWg0OltiX618FUdWWfI8
eySdLur1utnqBvdEbCUvWK2RX3vQZQdvEBODnNk2pxqTyV0w6VPQ96W++lF/5Aas
7rUv3HIyIXxIggc8FRrnH+y9XvvHDonhTIlGnYZN4ubm9i4y3gOkrZlGTrEw7elQ
1QeMyG2QQEbze8YjpTm4iLABCBrRfPRaQpwrABEBAAG0IXJwbS5vcmcgUlNBIHRl
c3RrZXkgPHJzYUBycG0ub3JnPokBNwQTAQgAIQUCWOY5GAIbAwULCQgHAgYVCAkK
CwIEFgIDAQIeAQIXgAAKCRBDRFkeGWTF/MxxCACnjqFL+MmPh9W9JQKT2DcLbBzf
Cqo6wcEBoCOcwgRSk8dSikhARoteoa55JRJhuMyeKhhEAogE9HRmCPFdjezFTwgB
BDVBpO2dZ023mLXDVCYX3S8pShOgCP6Tn4wqCnYeAdLcGg106N4xcmgtcssJE+Pr
XzTZksbZsrTVEmL/Ym+R5w5jBfFnGk7Yw7ndwfQsfNXQb5AZynClFxnX546lcyZX
fEx3/e6ezw57WNOUK6WT+8b+EGovPkbetK/rGxNXuWaP6X4A/QUm8O98nCuHYFQq
+mvNdsCBqGf7mhaRGtpHk/JgCn5rFvArMDqLVrR9hX0LdCSsH7EGE+bR3r7wuQEN
BFjmORgBCACk+vDZrIXQuFXEYToZVwb2attzbbJJCqD71vmZTLsW0QxuPKRgbcYY
zp4K4lVBnHhFrF8MOUOxJ7kQWIJZMZFt+BDcptCYurbD2H4W2xvnWViiC+LzCMzz
iMJT6165uefL4JHTDPxC2fFiM9yrc72LmylJNkM/vepT128J5Qv0gRUaQbHiQuS6
Dm/+WRnUfx3i89SV4mnBxb/Ta93GVqoOciWwzWSnwEnWYAvOb95JL4U7c5J5f/+c
KnQDHsW7sIiIdscsWzvgf6qs2Ra1Zrt7Fdk4+ZS2f/adagLhDO1C24sXf5XfMk5m
L0OGwZSr9m5s17VXxfspgU5ugc8kBJfzABEBAAE=
=WCfs
-----END PGP PUBLIC KEY BLOCK-----

37 changes: 37 additions & 0 deletions tests/data/keys/CVE-2021-3521-nosubsig.asc
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
-----BEGIN PGP PUBLIC KEY BLOCK-----
Version: rpm-4.17.90 (NSS-3)

mQENBFjmORgBCAC7TMEk6wnjSs8Dr4yqSScWdU2pjcqrkTxuzdWvowcIUPZI0w/g
HkRqGd4apjvY2V15kjL10gk3QhFP3pZ/9p7zh8o8NHX7aGdSGDK7NOq1eFaErPRY
91LW9RiZ0lbOjXEzIL0KHxUiTQEmdXJT43DJMFPyW9fkCWg0OltiX618FUdWWfI8
eySdLur1utnqBvdEbCUvWK2RX3vQZQdvEBODnNk2pxqTyV0w6VPQ96W++lF/5Aas
7rUv3HIyIXxIggc8FRrnH+y9XvvHDonhTIlGnYZN4ubm9i4y3gOkrZlGTrEw7elQ
1QeMyG2QQEbze8YjpTm4iLABCBrRfPRaQpwrABEBAAG0IXJwbS5vcmcgUlNBIHRl
c3RrZXkgPHJzYUBycG0ub3JnPokBNwQTAQgAIQUCWOY5GAIbAwULCQgHAgYVCAkK
CwIEFgIDAQIeAQIXgAAKCRBDRFkeGWTF/MxxCACnjqFL+MmPh9W9JQKT2DcLbBzf
Cqo6wcEBoCOcwgRSk8dSikhARoteoa55JRJhuMyeKhhEAogE9HRmCPFdjezFTwgB
BDVBpO2dZ023mLXDVCYX3S8pShOgCP6Tn4wqCnYeAdLcGg106N4xcmgtcssJE+Pr
XzTZksbZsrTVEmL/Ym+R5w5jBfFnGk7Yw7ndwfQsfNXQb5AZynClFxnX546lcyZX
fEx3/e6ezw57WNOUK6WT+8b+EGovPkbetK/rGxNXuWaP6X4A/QUm8O98nCuHYFQq
+mvNdsCBqGf7mhaRGtpHk/JgCn5rFvArMDqLVrR9hX0LdCSsH7EGE+bR3r7wuQEN
BFjmORgBCACk+vDZrIXQuFXEYToZVwb2attzbbJJCqD71vmZTLsW0QxuPKRgbcYY
zp4K4lVBnHhFrF8MOUOxJ7kQWIJZMZFt+BDcptCYurbD2H4W2xvnWViiC+LzCMzz
iMJT6165uefL4JHTDPxC2fFiM9yrc72LmylJNkM/vepT128J5Qv0gRUaQbHiQuS6
Dm/+WRnUfx3i89SV4mnBxb/Ta93GVqoOciWwzWSnwEnWYAvOb95JL4U7c5J5f/+c
KnQDHsW7sIiIdscsWzvgf6qs2Ra1Zrt7Fdk4+ZS2f/adagLhDO1C24sXf5XfMk5m
L0OGwZSr9m5s17VXxfspgU5ugc8kBJfzABEBAAG5AQ0EWOY5GAEIAKT68NmshdC4
VcRhOhlXBvZq23NtskkKoPvW+ZlMuxbRDG48pGBtxhjOngriVUGceEWsXww5Q7En
uRBYglkxkW34ENym0Ji6tsPYfhbbG+dZWKIL4vMIzPOIwlPrXrm558vgkdMM/ELZ
8WIz3KtzvYubKUk2Qz+96lPXbwnlC/SBFRpBseJC5LoOb/5ZGdR/HeLz1JXiacHF
v9Nr3cZWqg5yJbDNZKfASdZgC85v3kkvhTtzknl//5wqdAMexbuwiIh2xyxbO+B/
qqzZFrVmu3sV2Tj5lLZ/9p1qAuEM7ULbixd/ld8yTmYvQ4bBlKv2bmzXtVfF+ymB
Tm6BzyQEl/MAEQEAAYkBHwQYAQgACQUCWOY5GAIbDAAKCRBDRFkeGWTF/PANB/9j
mifmj6z/EPe0PJFhrpISt9PjiUQCt0IPtiL5zKAkWjHePIzyi+0kCTBF6DDLFxos
3vN4bWnVKT1kBhZAQlPqpJTg+m74JUYeDGCdNx9SK7oRllATqyu+5rncgxjWVPnQ
zu/HRPlWJwcVFYEVXYL8xzfantwQTqefjmcRmBRdA2XJITK+hGWwAmrqAWx+q5xX
Pa8wkNMxVzNS2rUKO9SoVuJ/wlUvfoShkJ/VJ5HDp3qzUqncADfdGN35TDzscngQ
gHvnMwVBfYfSCABV1hNByoZcc/kxkrWMmsd/EnIyLd1Q1baKqc3cEDuC6E6/o4yJ
E4XX4jtDmdZPreZALsiB
=rRop
-----END PGP PUBLIC KEY BLOCK-----

36 changes: 36 additions & 0 deletions tests/rpmsigdig.at
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,42 @@ gpg(185e6146f00650f8) = 4:185e6146f00650f8-58e63918
[])
AT_CLEANUP

AT_SETUP([rpmkeys --import invalid keys])
AT_KEYWORDS([rpmkeys import])
RPMDB_INIT

AT_CHECK([
runroot rpmkeys --import /data/keys/CVE-2021-3521-badbind.asc
],
[1],
[],
[error: /data/keys/CVE-2021-3521-badbind.asc: key 1 import failed.]
)
AT_CHECK([
runroot rpmkeys --import /data/keys/CVE-2021-3521-baduser.asc
],
[1],
[],
[error: /data/keys/CVE-2021-3521-baduser.asc: key 1 import failed.]
)

AT_CHECK([
runroot rpmkeys --import /data/keys/CVE-2021-3521-nosubsig.asc
],
[1],
[],
[error: /data/keys/CVE-2021-3521-nosubsig.asc: key 1 import failed.]
)

AT_CHECK([
runroot rpmkeys --import /data/keys/CVE-2021-3521-nosubsig-last.asc
],
[1],
[],
[error: /data/keys/CVE-2021-3521-nosubsig-last.asc: key 1 import failed.]
)
AT_CLEANUP

# ------------------------------
# Test pre-built package verification
AT_SETUP([rpmkeys -K <signed> 1])
Expand Down

0 comments on commit 1dcfa3c

Please sign in to comment.