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

pmatilai · 2021-10-13T07:35:24Z

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.

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:
55d5811 and d2fcd53

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. 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: 7b399fc and 236b802 Fixes CVE-2021-3521.

DemiMarie

This needs #1705 or equivalent to ensure that non-PGPSIGTYPE_BINARY signatures are not accepted as package signatures.

DemiMarie · 2021-10-13T13:48:38Z

rpmio/rpmpgp.c

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


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.

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.

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.

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.

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.

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.

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

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

DemiMarie · 2021-10-13T13:52:16Z

rpmio/rpmpgp.c

+	    0x99,
+	    (pkt->blen >> 8),
+	    (pkt->blen     ),


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

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.

DemiMarie · 2021-10-18T17:11:44Z

rpmio/rpmpgp.c

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


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

sshedi · 2022-09-14T06:59:34Z

Hi @DemiMarie,

I followed https://access.redhat.com/articles/3359321 to generate keys. But after this fix, I'm unable to import the keys.

root [ ~ ]# rpm --import foo.key 
error: foo.key: key 1 import failed.

root [ ~ ]# rpm -qi rpm
Name        : rpm
Version     : 4.16.1.3
Release     : 12.ph4
Architecture: x86_64
Install Date: Tue Sep 13 16:57:51 2022

Can you please assist me on generating the keys properly?

Here are my patches where I back ported this CVE fix to 4.16.x

https://github.com/vmware/photon/blob/4.0/SPECS/rpm/CVE-2021-3521-1.patch
https://github.com/vmware/photon/blob/4.0/SPECS/rpm/CVE-2021-3521-2.patch
https://github.com/vmware/photon/blob/4.0/SPECS/rpm/CVE-2021-3521-3.patch

DemiMarie · 2022-09-14T10:09:32Z

@sshedi GnuPG always generates subkey binding signatures by default.

DemiMarie · 2022-09-14T10:13:32Z

@sshedi you will also need to backport daeddb0

sshedi · 2022-09-14T10:18:55Z

But why the import failure? I saw some other discussions #1977

How can I generate gpg keys with sha512 algo?

I tried this:

$ cat .gnupg/gpg.conf 
personal-digest-preferences SHA512
cert-digest-algo SHA512
default-preference-list SHA512 SHA384 SHA256 SHA224 AES256 AES192 AES CAST5 ZLIB BZIP2 ZIP Uncompressed

but still I'm getting import failure.

pmatilai · 2022-09-14T10:25:18Z

A closed PR is hardly a place for assistance requests of any kind. Please open a discussion on the subject.

sshedi · 2022-09-14T10:29:52Z

Sorry about that @pmatilai.
We can continue here #2186

- CVE-2021-3521.patch taken from https://git.rockylinux.org/staging/rpms/rpm/-/blob/r8/SOURCES/rpm-4.14.3-validate-and-require-subkey-binding-sigs.patch - And add rpm upstream commit rpm-software-management/rpm@daeddb0 This is based on suggestion rpm-software-management/rpm#1795 (comment) Change-Id: Ie6222da4f001e36f4584844c83946dbec72bf283 Signed-off-by: Shreenidhi Shedi <[email protected]> Reviewed-on: http://photon-jenkins.eng.vmware.com:8082/17876 Reviewed-by: Tapas Kundu <[email protected]> Tested-by: Tapas Kundu <[email protected]>

- CVE-2021-3521.patch taken from https://git.rockylinux.org/staging/rpms/rpm/-/blob/r9/SOURCES/rpm-4.16.1.3-validate-and-require-subkey-binding-sigs.patch - And add rpm upstream commit rpm-software-management/rpm@daeddb0 This is based on suggestion rpm-software-management/rpm#1795 (comment) Change-Id: If30dfde170dcb2ee6496b632feb0597f2f7b13a1 Signed-off-by: Shreenidhi Shedi <[email protected]> Reviewed-on: http://photon-jenkins.eng.vmware.com:8082/17875 Reviewed-by: Tapas Kundu <[email protected]> Tested-by: Tapas Kundu <[email protected]>

- CVE-2021-3521.patch taken from https://git.rockylinux.org/staging/rpms/rpm/-/blob/r8/SOURCES/rpm-4.14.3-validate-and-require-subkey-binding-sigs.patch - And add rpm upstream commit rpm-software-management/rpm@daeddb0 This is based on suggestion rpm-software-management/rpm#1795 (comment) Change-Id: Id9e0fa045c70594bc16a7a4a0f206cb2789b8efa Signed-off-by: Shreenidhi Shedi <[email protected]> Reviewed-on: http://photon-jenkins.eng.vmware.com:8082/17877 Tested-by: gerrit-photon <[email protected]> Reviewed-by: Tapas Kundu <[email protected]>

pmatilai mentioned this pull request Oct 13, 2021

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

Closed

pmatilai added 3 commits October 13, 2021 11:56

Process MPI's from all kinds of signatures

7b399fc

No immediate effect but needed by the following commits.

Refactor pgpDigParams construction to helper function

236b802

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

pmatilai force-pushed the selfsig-binding-pr branch from 6a5ac9d to e233fb8 Compare October 13, 2021 08:57

DemiMarie suggested changes Oct 13, 2021

View reviewed changes

DemiMarie suggested changes Oct 18, 2021

View reviewed changes

pmatilai merged commit bd36c5d into rpm-software-management:master Oct 19, 2021

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

CamberLoid mentioned this pull request Nov 3, 2022

rpm: Several Vulnerabilites Regarding to version 4.17.0 AOSC-Dev/aosc-os-abbs#4283

Closed

willmurphyscode mentioned this pull request Jul 12, 2023

False Positive: CVE-2021-3521 reported against rpm-4.14.3-26.el8.x86_64 on Oracle Linux 8 anchore/grype#1362

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

pmatilai commented Oct 13, 2021

DemiMarie left a comment

DemiMarie Oct 13, 2021

pmatilai Oct 14, 2021 •

edited

Loading

DemiMarie Oct 14, 2021 •

edited

Loading

pmatilai Oct 14, 2021

DemiMarie Oct 14, 2021

pmatilai Oct 18, 2021

DemiMarie Oct 18, 2021

pmatilai Oct 19, 2021

DemiMarie Oct 13, 2021

mlschroe Oct 14, 2021

DemiMarie Oct 18, 2021

sshedi commented Sep 14, 2022

DemiMarie commented Sep 14, 2022

DemiMarie commented Sep 14, 2022

sshedi commented Sep 14, 2022

pmatilai commented Sep 14, 2022

sshedi commented Sep 14, 2022

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

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

Conversation

pmatilai commented Oct 13, 2021

DemiMarie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmatilai Oct 14, 2021 • edited Loading

Choose a reason for hiding this comment

DemiMarie Oct 14, 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

Choose a reason for hiding this comment

sshedi commented Sep 14, 2022

DemiMarie commented Sep 14, 2022

DemiMarie commented Sep 14, 2022

sshedi commented Sep 14, 2022

pmatilai commented Sep 14, 2022

sshedi commented Sep 14, 2022

pmatilai Oct 14, 2021 •

edited

Loading

DemiMarie Oct 14, 2021 •

edited

Loading