Re: [Rpm-maint] [rpm-software-management/rpm] Validate and require subkey binding signatures on PGP public keys (#1795)
Merged #1795 into master. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1795#event-5482987935___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Validate and require subkey binding signatures on PGP public keys (#1795)
@pmatilai commented on this pull request. > + if (sigalg->setmpi(sigalg, i, p)) + break; Repeating yourself ad nauseum isn't helpful to the cause. Really. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1795#discussion_r731539255___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Validate and require subkey binding signatures on PGP public keys (#1795)
@DemiMarie requested changes on this pull request. > + 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. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1795#pullrequestreview-782356055___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Validate and require subkey binding signatures on PGP public keys (#1795)
@pmatilai commented on this pull request. > + if (sigalg->setmpi(sigalg, i, p)) + break; 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. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1795#discussion_r730708801___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Validate and require subkey binding signatures on PGP public keys (#1795)
@DemiMarie commented on this pull request. > + if (sigalg->setmpi(sigalg, i, p)) + break; 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. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1795#discussion_r729033785___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Validate and require subkey binding signatures on PGP public keys (#1795)
@pmatilai commented on this pull request. > + if (sigalg->setmpi(sigalg, i, p)) + break; 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. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1795#discussion_r729015479___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Validate and require subkey binding signatures on PGP public keys (#1795)
@DemiMarie commented on this pull request. > + if (sigalg->setmpi(sigalg, i, p)) + break; > 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 standardized signature types. > 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. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1795#discussion_r728970189___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Validate and require subkey binding signatures on PGP public keys (#1795)
@pmatilai commented on this pull request. > + if (sigalg->setmpi(sigalg, i, p)) + break; Yes, you've repeated this quite a few times now. The signature type information is there to tell the reader how to has 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 semi-random check from 20 years ago is hardly a security regression. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1795#discussion_r728947765___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Validate and require subkey binding signatures on PGP public keys (#1795)
@mlschroe commented on this pull request. > + 0x99, + (pkt->blen >> 8), + (pkt->blen ), 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. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1795#discussion_r728935271___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Validate and require subkey binding signatures on PGP public keys (#1795)
@DemiMarie requested changes on this pull request. This needs #1705 or equivalent to ensure that non-`PGPSIGTYPE_BINARY` signatures are not accepted as package signatures. > + 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. > + 0x99, + (pkt->blen >> 8), + (pkt->blen ), This is inconsistent (at best) for keys larger than 0x bytes. Not sure if such keys should just be rejected. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1795#pullrequestreview-778605073___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Validate and require subkey binding signatures on PGP public keys (#1795)
@pmatilai pushed 3 commits. 7b399fcb8f52566e6f3b4327197a85facd08db91 Process MPI's from all kinds of signatures 236b802a4aa48711823a191d1b7f753c82a89ec5 Refactor pgpDigParams construction to helper function e233fb844adda74a5199057d1fd7fa20d994564d Validate and require subkey binding signatures on PGP public keys -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/rpm-software-management/rpm/pull/1795/files/6a5ac9dd1330f304130985171666e261a31dd6c6..e233fb844adda74a5199057d1fd7fa20d994564d ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
[Rpm-maint] [rpm-software-management/rpm] Validate and require subkey binding signatures on PGP public keys (#1795)
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: 55d5811a10d5a4c5d965373f5841280a5f43d7ef and d2fcd5380fe3390e695a016727a695829a0a3610 You can view, comment on, or merge this pull request online at: https://github.com/rpm-software-management/rpm/pull/1795 -- Commit Summary -- * https://github.com/rpm-software-management/rpm/pull/1795/commits/55d5811a10d5a4c5d965373f5841280a5f43d7ef;>Only set MPIs for signature types we can handle * https://github.com/rpm-software-management/rpm/pull/1795/commits/d2fcd5380fe3390e695a016727a695829a0a3610;>Refactor pgpDigParams construction to helper function * https://github.com/rpm-software-management/rpm/pull/1795/commits/6a5ac9dd1330f304130985171666e261a31dd6c6;>Validate and require subkey binding signatures on PGP public keys -- File Changes -- M rpmio/rpmpgp.c (125) M tests/Makefile.am (3) A tests/data/keys/CVE-2021-3521-badbind.asc (25) A tests/data/keys/CVE-2021-3521-nosubsig-last.asc (25) A tests/data/keys/CVE-2021-3521-nosubsig.asc (37) M tests/rpmsigdig.at (28) -- Patch Links -- https://github.com/rpm-software-management/rpm/pull/1795.patch https://github.com/rpm-software-management/rpm/pull/1795.diff -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1795 ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
