Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validate and require subkey binding signatures on PGP public keys #1795

Merged
merged 3 commits into from
Oct 19, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
121 changes: 104 additions & 17 deletions rpmio/rpmpgp.c
Original file line number Diff line number Diff line change
Expand Up @@ -543,7 +543,7 @@ pgpDigAlg pgpDigAlgFree(pgpDigAlg alg)
return NULL;
}

static int pgpPrtSigParams(pgpTag tag, uint8_t pubkey_algo, uint8_t sigtype,
static int pgpPrtSigParams(pgpTag tag, uint8_t pubkey_algo,
const uint8_t *p, const uint8_t *h, size_t hlen,
pgpDigParams sigp)
{
Expand All @@ -556,10 +556,8 @@ static int pgpPrtSigParams(pgpTag tag, uint8_t pubkey_algo, uint8_t sigtype,
int mpil = pgpMpiLen(p);
if (pend - p < mpil)
break;
if (sigtype == PGPSIGTYPE_BINARY || sigtype == PGPSIGTYPE_TEXT) {
if (sigalg->setmpi(sigalg, i, p))
break;
}
if (sigalg->setmpi(sigalg, i, p))
break;
Comment on lines +559 to +560
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This requires a corresponding change in the package signature checking code to ensure that package signatures are PGPSIGTYPE_BINARY. #1705 is one implementation, and I can replace it with a better one that uses proper accessor functions.

Copy link
Member Author

@pmatilai pmatilai Oct 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you've repeated this quite a few times now.

The signature type information is there to tell the reader how to hash the material for correct results. We ignore the byte anyhow for the package hashing purposes because it's just not that intersting for our purposes.

A better implementation would do things differently in many ways, but removing that misplaced semi-random check from 20 years ago is hardly a security regression.

Copy link
Contributor

@DemiMarie DemiMarie Oct 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The signature type information is there to tell the reader how to hash the material for correct results. We ignore the byte anyhow for the package hashing purposes because it's just not that intersting for our purposes.

It also provides cryptographic domain separation between different types of signatures, which prevents a signature of a public key, a certification signature, or a revocation signature from being accepted as a signature of a document. That is where the security aspect comes from. In the case of RPM, this is somewhat mitigated since the data being signed must start with 0x8e, which means it cannot collide data being signed in any of the other currently standardized signature types. However, there is no guarantee that new signature types will not be added that make this a problem.

A better implementation would do things differently in many ways, but removing that misplaced semi-random check from 20 years ago is hardly a security regression.

See above: in the case of RPM it may not be exploitable, but it could become exploitable if future changes are made to the OpenPGP standard. Best to add the check now as a precaution.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really even disagree - optimally we should check for it someplace.
It's just that the check doesn't really fit anywhere nicely and meanwhile arguing over a relatively petty issue is just delaying getting the silly CVE fixed. Silly, because if you get an admin to import a key file you have access to, you don't need to pull off stunts like fiddle with subkey binding signatures.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if I made a good quality PR that fixed the problem, either directly or on to your branch? #1705 got NAK’d on the grounds that it added “another struct pgpDigParams direct access when we're trying to eliminate those.” I can instead add a proper accessor function (is pgpDigParamsSigType okay?) and use it.

Silly, because if you get an admin to import a key file you have access to, you don't need to pull off stunts like fiddle with subkey binding signatures.

The main worry is if someone does something like:

$ gpg --export 'some trusted fingerprint'

and their /usr/bin/gpg doesn’t bother to check subkey binding signatures when exporting.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not about just accessors.

The point is, these worries are so far out there when there are a thousand more practical ways to exploit that an inordinate time has already been spent on this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pmatilai good point. In fact, I would argue that not adding the check would make this PR a regression security wise. Would it be possible to include #1705 in this PR? It is tiny and passes the regression test suite, and can be replaced with a better solution later.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Repeating yourself ad nauseum isn't helpful to the cause. Really.

p += mpil;
}

Expand Down Expand Up @@ -619,7 +617,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, v->sigtype, p, h, hlen, _digp);
rc = pgpPrtSigParams(tag, v->pubkey_algo, p, h, hlen, _digp);
} break;
case 4:
{ pgpPktSigV4 v = (pgpPktSigV4)h;
Expand Down Expand Up @@ -678,7 +676,7 @@ static int pgpPrtSig(pgpTag tag, const uint8_t *h, size_t hlen,
if (p > hend)
return 1;

rc = pgpPrtSigParams(tag, v->pubkey_algo, v->sigtype, p, h, hlen, _digp);
rc = pgpPrtSigParams(tag, v->pubkey_algo, p, h, hlen, _digp);
} break;
default:
rpmlog(RPMLOG_WARNING, _("Unsupported version of signature: V%d\n"), version);
Expand Down Expand Up @@ -1057,38 +1055,128 @@ unsigned int pgpDigParamsAlgo(pgpDigParams digp, unsigned int algotype)
return algo;
}

static pgpDigParams pgpDigParamsNew(uint8_t tag)
{
pgpDigParams digp = xcalloc(1, sizeof(*digp));
digp->tag = 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 ),
Comment on lines +1070 to +1072
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is inconsistent (at best) for keys larger than 0xFFFF bytes. Not sure if such keys should just be rejected.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That may be, but it's what the spec says. It's not a security problem because the complete package is hashed anyway. V5 keys hash the complete length, not just the lowest 16 bits.

};

rpmDigestUpdate(hash, head, 3);
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;
default:
/* ignore types we can't handle */
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;
int prevtag = 0;

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

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

if (pgpPrtPkt(&pkt, digp))
if (expect) {
if (pkt->tag != expect)
break;
selfsig = pgpDigParamsNew(pkt->tag);
}

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

p += (pkt.body - pkt.head) + pkt.blen;
if (selfsig) {
/* subkeys must be followed by binding signature */
if (prevtag == PGPTAG_PUBLIC_SUBKEY) {
if (selfsig->sigtype != PGPSIGTYPE_SUBKEY_BINDING)
break;
}

int xx = pgpVerifySelf(digp, selfsig, all, i);

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

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

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 Expand Up @@ -1123,8 +1211,7 @@ int pgpPrtParamsSubkeys(const uint8_t *pkts, size_t pktlen,
digps = xrealloc(digps, alloced * sizeof(*digps));
}

digps[count] = xcalloc(1, sizeof(**digps));
digps[count]->tag = PGPTAG_PUBLIC_SUBKEY;
digps[count] = pgpDigParamsNew(PGPTAG_PUBLIC_SUBKEY);
/* Copy UID from main key to subkey */
digps[count]->userid = xstrdup(mainkey->userid);

Expand Down
3 changes: 3 additions & 0 deletions tests/Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,9 @@ 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-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-----

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-----

28 changes: 28 additions & 0 deletions tests/rpmsigdig.at
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,34 @@ 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-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