Validate self-signatures and require subkey bindings on PGP public keys #1788

pmatilai · 2021-09-30T07:17:59Z

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.

mlschroe · 2021-10-07T10:00:03Z

rpmio/rpmpgp.c

+    int xx = -1;
+    DIGEST_CTX hash = rpmDigestInit(selfsig->hash_algo, 0);
+    hashKey(hash, &all[0]);
+


You might want to verify the package types of the package you're hashing, i.e. all[i-1] should be of the required type.

Does it actually matter though? I mean, if the type is wrong then the hash will also be wrong and simply fail to pass? Okay, as a belt and suspenders kind of thing I suppose...

I also think that it does not matter, but it's the right thing to do for security relevant code ;-)

Anyway, for the _CERT verification you can't use all[i-1] as there may be multiple signatures.

Oh, good point on the CERT verification (annoying as it may be...)

Changed it to check for the types and more errors overall, userid lookup in the CERT case still missing.

mlschroe · 2021-10-07T10:02:52Z

rpmio/rpmpgp.c

 	    }
 	}

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


The code assumes that the self-sig always comes first if there are multiple signatures. Does the standard say something about this?

Maybe you should also check that the issuer id matches and ignore non-selfsigned sigs.

https://datatracker.ietf.org/doc/html/rfc4880#section-11.1 says:

Each Subkey packet MUST be followed by one Signature packet, which should be a subkey binding signature issued by the top-level key.

Surely a signature with a different issuer will simply not pass?

Sure, but that's a problem as the signature checking code below is done for all signatures, not just the subkey binding one.

I'm probably Monday dense here, but I fail to see the problem.

Maybe it's me ;)

My point is that selfsig will also be set for signatures that use a different key (i.e. not self-signed) and the code below will call pgpVerifySelf on them and then break the loop as the verification will fail.

(Of course I'm talking about _CERT sigs, not the subkey binding one)

I am fine with assuming and requiring that the self-signature comes first, provided that doing so is interoperable. Keys with non-self-signatures obviously need to be accepted, but I am fine with RPM ignoring the extra signatures and omitting them from the parsed result.

My point is that selfsig will also be set for signatures that use a different key (i.e. not self-signed) and the code below will call pgpVerifySelf on them and then break the loop as the verification will fail.

Right, got it. I think.

This is all a surprisingly nasty tangle of logic. Perhaps I should split the _CERT part out of this to simplify, that's not part of the CVE anyway and that's what I want to get out of the way.

rpmio/rpmpgp.c

mlschroe · 2021-10-07T10:07:13Z

rpmio/rpmpgp.c

 	    break;

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


Should we also enforce a self-sig on a User ID Packet?

I considered that, but it'd be against the spec:

Immediately following each User ID packet, there are zero or more Signature packets.

It'd be good to differentiate between verified and non-verified somehow though.

I believe GPG ignores User ID packets that have no self-signatures. RPM should either reject such packets or ignore them; processing them as if they were verified is a bad idea.

No immediate effect but needed by the following commits.

No functional changes, just to reduce code duplication and needed by the following commits.

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.

DemiMarie

At a minimum, there needs to be a check for signature type in the code that verifies package signatures, now that such signatures will no longer be automatically rejected.

DemiMarie · 2021-10-11T14:09:20Z

rpmio/rpmpgp.c

+	if (sigalg->setmpi(sigalg, i, p))
+	    break;


With this change, RPM needs to check that package signatures are in fact signatures of binary documents. I am not aware of an actual exploit, but it is the Right Thing To Do, since a security patch should not regress security elsewhere.

Well, instead of removing the check maybe we should check for the kinds that we actually support in the PGP parser. We dont do TEXT anyway.

A certification or binding signature isn’t a valid package signature and should not be accepted where a package signature is required. The parser doesn’t currently know what kind of signature is expected and so can’t make that decision. That said, another option would be to add a new API function that only parses signatures, and which expects the type of the signature as an argument.

DemiMarie · 2021-10-11T14:13:10Z

rpmio/rpmpgp.c

 	if (pkttype == PGPTAG_SIGNATURE)
 	    break;
+
+	if (alloced <= i) {
+	    alloced *= 2;


Suggested change

alloced *= 2;

if (alloced > INT_MAX / 2 || alloced > SIZE_MAX / (4 * sizeof(*all)))

abort();

alloced *= 2;

This is a last-ditch check against memory corruption due to integer overflow. Such long keys should be rejected earlier in any case.

No. Just no. I've told you before, we wont be littering the code-base with stuff like this. If we did, nobody could find the actual code from underneath.

To elaborate on that a bit, the suggested change is simply absurd when you could simply place an upper bound and error out if exceeded.

DemiMarie · 2021-10-11T14:18:06Z

rpmio/rpmpgp.c

 	    }
 	}

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


I am fine with assuming and requiring that the self-signature comes first, provided that doing so is interoperable. Keys with non-self-signatures obviously need to be accepted, but I am fine with RPM ignoring the extra signatures and omitting them from the parsed result.

DemiMarie · 2021-10-11T14:18:29Z

rpmio/rpmpgp.c

 	    break;

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


I believe GPG ignores User ID packets that have no self-signatures. RPM should either reject such packets or ignore them; processing them as if they were verified is a bad idea.

DemiMarie · 2021-10-11T14:20:33Z

rpmio/rpmpgp.c

+
+static int pgpVerifySelf(pgpDigParams key, pgpDigParams selfsig,
+			const struct pgpPkt *all, int i)
+{


Suggested change

{

{

assert(i > 0 && "pgpVerifySelf passed invalid i");

You're suggesting to add two lines of a comment to explain what an assert() is? Come on.
Review of the PGP matters is totally welcome, but material like this is only good for raising my blood pressure.

The point is that we don't sprinkle material like this around. This is just redundant clutter in the codebase which makes it unreadable. Whenever you feel the need to add a comment or an assert, it's more likely because the code in question is dumb and could be written in a better way. Such as here.

DemiMarie

Package signatures need to be checked to be of PGPSIGTYPE_BINARY, and keys with third-party certifications must not be rejected. I believe nonsensical signature types should be rejected.

DemiMarie · 2021-10-12T14:01:43Z

rpmio/rpmpgp.c

+
+static int pgpVerifySelf(pgpDigParams key, pgpDigParams selfsig,
+			const struct pgpPkt *all, int i)
+{


DemiMarie · 2021-10-12T14:06:13Z

rpmio/rpmpgp.c

+	if (sigalg->setmpi(sigalg, i, p))
+	    break;


A certification or binding signature isn’t a valid package signature and should not be accepted where a package signature is required. The parser doesn’t currently know what kind of signature is expected and so can’t make that decision. That said, another option would be to add a new API function that only parses signatures, and which expects the type of the signature as an argument.

DemiMarie · 2021-10-12T14:11:06Z

rpmio/rpmpgp.c

+	/* ignore unknown types */
+	rc = 0;


Suggested change

/* ignore unknown types */

rc = 0;

/* reject unknown types */

No, rejecting types we cannot handle would only cause us to fail on perfectly legitimate keys. IIRC the PGP spec quite specifically tells you to ignore what you don't know, which generally is the key to future expandable standards.

pmatilai · 2021-10-13T07:37:19Z

The subkey binding part simplified a bit and split to #1795, the user certification is more involved and has all manner of strange open questions, I don't have time to deal with that now. Thanks for the feedback so far.

soig · 2022-02-05T09:51:48Z

Hi
Can you backport those fixes to rpm-4.16.x?
We need them for fixing the CVE in Mageia 8 (and probably other distributions).
Thanks
b5e8bc7 & 9f03f42 apply cleanly.
But not bd36c5d since there's been refactoring between 4.16 & 4.17

DemiMarie · 2022-02-05T17:17:23Z

Hi
Can you backport those fixes to rpm-4.16.x?
We need them for fixing the CVE in Mageia 8 (and probably other distributions).
Thanks
b5e8bc7 & 9f03f42 apply cleanly.
But not bd36c5d since there's been refactoring between 4.16 & 4.17

The OpenPGP packet parser has had a lot of hardening between 4.16 and 4.17. daeddb0 adds a check that b5e8bc7 removed, and there are other commits that fix various undefined behaviors. I suggest backporting all of the commits in https://github.com/rpm-software-management/rpm/commits/714e606558b4517bd2d02d03a0ddad7da79a58c6/rpmio/rpmpgp.c.

pmatilai force-pushed the selfsig-pr branch from cf99101 to 5641a79 Compare September 30, 2021 07:21

mlschroe reviewed Oct 7, 2021

View reviewed changes

rpmio/rpmpgp.c Outdated Show resolved Hide resolved

mlschroe reviewed Oct 7, 2021

View reviewed changes

pmatilai added 3 commits October 11, 2021 13:17

Process MPI's from all kinds of signatures

46c45e9

No immediate effect but needed by the following commits.

Refactor pgpDigParams construction to helper function

2642412

No functional changes, just to reduce code duplication and needed by the following commits.

pmatilai force-pushed the selfsig-pr branch from 5641a79 to 1dcfa3c Compare October 11, 2021 10:17

DemiMarie suggested changes Oct 11, 2021

View reviewed changes

DemiMarie suggested changes Oct 12, 2021

View reviewed changes

pmatilai closed this Oct 13, 2021

pmatilai deleted the selfsig-pr branch February 10, 2022 13:43

pmatilai restored the selfsig-pr branch April 7, 2022 11:12

pmatilai deleted the selfsig-pr branch April 7, 2022 11:13

breskeby mentioned this pull request Apr 14, 2022

rpm --import https://artifacts.elastic.co/GPG-KEY-elasticsearch not working on centOS stream. gives a key import error. elastic/elasticsearch#85876

Closed

Validate self-signatures and require subkey bindings on PGP public keys #1788

Validate self-signatures and require subkey bindings on PGP public keys #1788

Conversation

pmatilai commented Sep 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlschroe Oct 11, 2021 • edited Loading

Choose a reason for hiding this comment

pmatilai Oct 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DemiMarie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmatilai Oct 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DemiMarie Oct 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DemiMarie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmatilai commented Oct 13, 2021

soig commented Feb 5, 2022

DemiMarie commented Feb 5, 2022 • edited Loading

pmatilai commented Sep 30, 2021 •

edited

Loading

mlschroe Oct 11, 2021 •

edited

Loading

pmatilai Oct 11, 2021 •

edited

Loading

pmatilai Oct 13, 2021 •

edited

Loading

DemiMarie Oct 11, 2021 •

edited

Loading

DemiMarie commented Feb 5, 2022 •

edited

Loading