Re: [Rpm-maint] [rpm-software-management/rpm] Validate self-signatures and require subkey bindings on PGP public keys (#1788)
@pmatilai commented on this pull request.
> if (pkttype == PGPTAG_SIGNATURE)
break;
+
+ if (alloced <= i) {
+ alloced *= 2;
To elaborate on that a bit, the suggested change is simply absurd when you
could simply place a simple upper bound and error out if exceeded.
--
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/1788#discussion_r727793623___
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 self-signatures and require subkey bindings on PGP public keys (#1788)
@pmatilai commented on this pull request. > + /* ignore unknown types */ + rc = 0; 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. -- 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/1788#discussion_r727789654___ 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 self-signatures and require subkey bindings on PGP public keys (#1788)
@pmatilai commented on this pull request.
> + 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)
+{
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.
--
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/1788#discussion_r727785633___
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 self-signatures and require subkey bindings on PGP public keys (#1788)
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. -- 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/1788#issuecomment-942014125___ 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 self-signatures and require subkey bindings on PGP public keys (#1788)
Closed #1788. -- 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/1788#event-5455103925___ 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 self-signatures and require subkey bindings on PGP public keys (#1788)
@DemiMarie requested changes on this pull request.
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.
> + /* ignore unknown types */
+ rc = 0;
```suggestion
/* reject unknown types */
```
> + 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.
> + 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)
+{
Edited
--
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/1788#pullrequestreview-777427981___
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 self-signatures and require subkey bindings on PGP public keys (#1788)
@pmatilai commented on this pull request. > } } - if (pgpPrtPkt(, digp)) + if (digp->tag == PGPTAG_PUBLIC_KEY && pkt->tag == PGPTAG_SIGNATURE) + selfsig = pgpDigParamsNew(pkt->tag); > 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. -- 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/1788#discussion_r726987199___ 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 self-signatures and require subkey bindings on PGP public keys (#1788)
@pmatilai commented on this pull request. > + if (sigalg->setmpi(sigalg, i, p)) + break; 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. -- 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/1788#discussion_r726977080___ 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 self-signatures and require subkey bindings on PGP public keys (#1788)
@pmatilai commented on this pull request.
> + 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)
+{
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.
--
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/1788#discussion_r726975184___
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 self-signatures and require subkey bindings on PGP public keys (#1788)
@pmatilai commented on this pull request.
> if (pkttype == PGPTAG_SIGNATURE)
break;
+
+ if (alloced <= i) {
+ alloced *= 2;
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.
--
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/1788#discussion_r726965337___
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 self-signatures and require subkey bindings on PGP public keys (#1788)
@DemiMarie requested changes on this pull request.
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.
> +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, [0], PGPTAG_PUBLIC_KEY);
+ if (!rc)
+ rc = hashKey(hash, [i-1], PGPTAG_PUBLIC_SUBKEY);
Presumably `i` must always be at least 1 here. If so, then I would add an
assertion and a comment explaining why that is the case.
> + hash = rpmDigestInit(selfsig->hash_algo, 0);
+ if (hash) {
+ rc = hashKey(hash, [0], PGPTAG_PUBLIC_KEY);
+ if (!rc)
+ rc = hashKey(hash, [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, [0], PGPTAG_PUBLIC_KEY);
+ if (!rc)
+ rc = hashUser(hash, [i-1], PGPTAG_USER_ID);
Same as above.
> +if (pkt->tag == exptag) {
+ uint8_t head[] = {
+ 0x99,
+ (pkt->blen >> 8),
+ (pkt->blen ),
```suggestion
if (pkt->tag == exptag && pkt->blen <= 0x) {
uint8_t head[] = {
0x99,
(pkt->blen >> 8),
(pkt->blen ),
```
If `pkt->blen` is greater than 0x the formula provided by the OpenPGP
specification for hashing a key makes no sense.
> + }
+ 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, [0], PGPTAG_PUBLIC_KEY);
+ if (!rc)
+ rc = hashUser(hash, [i-1], PGPTAG_USER_ID);
+ }
+ break;
+default:
+ /* ignore unknown types */
+ rc = 0;
I am 99% certain this should be an error condition; such signatures make no
sense.
> + 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.
> if (pkttype == PGPTAG_SIGNATURE)
break;
+
+ if (alloced <= i) {
+ alloced *= 2;
```suggestion
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.
> + 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)
+{
```suggestion
{
// The only caller of pgpVerifySelf always passes a number
// greater than 0
assert(i > 0 && "pgpVerifySelf passed invalid i");
```
> }
}
- if (pgpPrtPkt(, 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.
> 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.
--
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/1788#pullrequestreview-776261179___
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 self-signatures and require subkey bindings on PGP public keys (#1788)
@pmatilai commented on this pull request.
> + (pkt->blen >> 24),
+ (pkt->blen >> 16),
+ (pkt->blen >> 8),
+ (pkt->blen ),
+};
+rpmDigestUpdate(hash, head, 5);
+rpmDigestUpdate(hash, pkt->body, pkt->blen);
+}
+
+static int pgpVerifySelf(pgpDigParams key, pgpDigParams selfsig,
+ const struct pgpPkt *all, int i)
+{
+int xx = -1;
+DIGEST_CTX hash = rpmDigestInit(selfsig->hash_algo, 0);
+hashKey(hash, [0]);
+
Changed it to check for the types and more errors overall, userid lookup in the
CERT case still missing.
--
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/1788#discussion_r725986587___
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 self-signatures and require subkey bindings on PGP public keys (#1788)
@pmatilai pushed 3 commits. 46c45e9a25fe151e8f7dee055356d398e163fb69 Process MPI's from all kinds of signatures 26424121e1aee51d7acf25ade18caeb1c976f364 Refactor pgpDigParams construction to helper function 1dcfa3c00f61594763418da701bea194972f3fac Validate self-signatures and require subkey bindings 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/1788/files/5641a79576acf846e51254de491f9f4581985cc5..1dcfa3c00f61594763418da701bea194972f3fac ___ 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 self-signatures and require subkey bindings on PGP public keys (#1788)
@mlschroe commented on this pull request. > } } - if (pgpPrtPkt(, digp)) + if (digp->tag == PGPTAG_PUBLIC_KEY && pkt->tag == PGPTAG_SIGNATURE) + selfsig = pgpDigParamsNew(pkt->tag); 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) -- 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/1788#discussion_r725962536___ 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 self-signatures and require subkey bindings on PGP public keys (#1788)
@pmatilai commented on this pull request. > } } - if (pgpPrtPkt(, digp)) + if (digp->tag == PGPTAG_PUBLIC_KEY && pkt->tag == PGPTAG_SIGNATURE) + selfsig = pgpDigParamsNew(pkt->tag); I may be Monday dense here, but I fail to see the problem. -- 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/1788#discussion_r725910745___ 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 self-signatures and require subkey bindings on PGP public keys (#1788)
@pmatilai commented on this pull request.
> + (pkt->blen >> 24),
+ (pkt->blen >> 16),
+ (pkt->blen >> 8),
+ (pkt->blen ),
+};
+rpmDigestUpdate(hash, head, 5);
+rpmDigestUpdate(hash, pkt->body, pkt->blen);
+}
+
+static int pgpVerifySelf(pgpDigParams key, pgpDigParams selfsig,
+ const struct pgpPkt *all, int i)
+{
+int xx = -1;
+DIGEST_CTX hash = rpmDigestInit(selfsig->hash_algo, 0);
+hashKey(hash, [0]);
+
Oh, good point on the CERT verification (annoying as it may be...)
--
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/1788#discussion_r725905963___
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 self-signatures and require subkey bindings on PGP public keys (#1788)
@mlschroe commented on this pull request.
> + (pkt->blen >> 24),
+ (pkt->blen >> 16),
+ (pkt->blen >> 8),
+ (pkt->blen ),
+};
+rpmDigestUpdate(hash, head, 5);
+rpmDigestUpdate(hash, pkt->body, pkt->blen);
+}
+
+static int pgpVerifySelf(pgpDigParams key, pgpDigParams selfsig,
+ const struct pgpPkt *all, int i)
+{
+int xx = -1;
+DIGEST_CTX hash = rpmDigestInit(selfsig->hash_algo, 0);
+hashKey(hash, [0]);
+
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.
--
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/1788#discussion_r725892052___
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 self-signatures and require subkey bindings on PGP public keys (#1788)
@mlschroe commented on this pull request. > } } - if (pgpPrtPkt(, digp)) + if (digp->tag == PGPTAG_PUBLIC_KEY && pkt->tag == PGPTAG_SIGNATURE) + selfsig = pgpDigParamsNew(pkt->tag); Sure, but that's a problem as the signature checking code below is done for all signatures, but just the subkey binding one. -- 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/1788#discussion_r725886576___ 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 self-signatures and require subkey bindings on PGP public keys (#1788)
@pmatilai commented on this pull request.
> + (pkt->blen >> 24),
+ (pkt->blen >> 16),
+ (pkt->blen >> 8),
+ (pkt->blen ),
+};
+rpmDigestUpdate(hash, head, 5);
+rpmDigestUpdate(hash, pkt->body, pkt->blen);
+}
+
+static int pgpVerifySelf(pgpDigParams key, pgpDigParams selfsig,
+ const struct pgpPkt *all, int i)
+{
+int xx = -1;
+DIGEST_CTX hash = rpmDigestInit(selfsig->hash_algo, 0);
+hashKey(hash, [0]);
+
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...
--
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/1788#discussion_r725884882___
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 self-signatures and require subkey bindings on PGP public keys (#1788)
@pmatilai commented on this pull request.
> if (pkttype == PGPTAG_SIGNATURE)
break;
+
+ if (alloced < i) {
:facepalm:
Thanks for spotting, the '<' is a remnant from a different version where it was
the right thing, but wrong here of course.
--
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/1788#discussion_r725881007___
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 self-signatures and require subkey bindings on PGP public keys (#1788)
@pmatilai commented on this pull request. > break; + if (pkt->tag == PGPTAG_PUBLIC_SUBKEY) + expect = PGPTAG_SIGNATURE; It'd be good to differentiate between verified and non-verified somehow though. -- 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/1788#discussion_r725879198___ 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 self-signatures and require subkey bindings on PGP public keys (#1788)
@pmatilai commented on this pull request. > break; + if (pkt->tag == PGPTAG_PUBLIC_SUBKEY) + expect = PGPTAG_SIGNATURE; I considered that, but it'd be against the spec: > Immediately following each User ID packet, there are zero or more Signature > packets. -- 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/1788#discussion_r725878725___ 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 self-signatures and require subkey bindings on PGP public keys (#1788)
@pmatilai commented on this pull request. > } } - if (pgpPrtPkt(, digp)) + if (digp->tag == PGPTAG_PUBLIC_KEY && pkt->tag == PGPTAG_SIGNATURE) + selfsig = pgpDigParamsNew(pkt->tag); 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? -- 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/1788#discussion_r725877751___ 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 self-signatures and require subkey bindings on PGP public keys (#1788)
@mlschroe commented on this pull request. > } } - if (pgpPrtPkt(, digp)) + if (digp->tag == PGPTAG_PUBLIC_KEY && pkt->tag == PGPTAG_SIGNATURE) + selfsig = pgpDigParamsNew(pkt->tag); Maybe you should also check that the issuer id matches and ignore non-selfsigned sigs. -- 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/1788#discussion_r724043128___ 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 self-signatures and require subkey bindings on PGP public keys (#1788)
@mlschroe commented on this pull request. > break; + if (pkt->tag == PGPTAG_PUBLIC_SUBKEY) + expect = PGPTAG_SIGNATURE; Should we also enforce a self-sig on a User ID Packet? -- 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/1788#pullrequestreview-773704516___ 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 self-signatures and require subkey bindings on PGP public keys (#1788)
@mlschroe commented on this pull request.
> if (pkttype == PGPTAG_SIGNATURE)
break;
+
+ if (alloced < i) {
Shouldn't that be `alloced <= i`?
--
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/1788#pullrequestreview-773702277___
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 self-signatures and require subkey bindings on PGP public keys (#1788)
@mlschroe commented on this pull request. > } } - if (pgpPrtPkt(, 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? -- 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/1788#pullrequestreview-773700209___ 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 self-signatures and require subkey bindings on PGP public keys (#1788)
@mlschroe commented on this pull request.
> + (pkt->blen >> 24),
+ (pkt->blen >> 16),
+ (pkt->blen >> 8),
+ (pkt->blen ),
+};
+rpmDigestUpdate(hash, head, 5);
+rpmDigestUpdate(hash, pkt->body, pkt->blen);
+}
+
+static int pgpVerifySelf(pgpDigParams key, pgpDigParams selfsig,
+ const struct pgpPkt *all, int i)
+{
+int xx = -1;
+DIGEST_CTX hash = rpmDigestInit(selfsig->hash_algo, 0);
+hashKey(hash, [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.
--
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/1788#pullrequestreview-773697287___
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 self-signatures and require subkey bindings on PGP public keys (#1788)
@pmatilai pushed 1 commit. 5641a79576acf846e51254de491f9f4581985cc5 Validate self-signatures and require subkey bindings 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/1788/files/cf99101a05b1f63c828581f7c2229029278958f1..5641a79576acf846e51254de491f9f4581985cc5 ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
[Rpm-maint] [rpm-software-management/rpm] Validate self-signatures and require subkey bindings on PGP public keys (#1788)
All subkeys must 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: db7ab0a5a9580f493871aa52c73dcf1dae905d78 and e3dd156c8969c5298d56adc2a5673f0f336e04fa Fixes CVE-2021-3521 and more. You can view, comment on, or merge this pull request online at: https://github.com/rpm-software-management/rpm/pull/1788 -- Commit Summary -- * https://github.com/rpm-software-management/rpm/pull/1788/commits/db7ab0a5a9580f493871aa52c73dcf1dae905d78;>Process MPIs from all kinds of signatures * https://github.com/rpm-software-management/rpm/pull/1788/commits/e3dd156c8969c5298d56adc2a5673f0f336e04fa;>Refactor pgpDigParams construction to helper function * https://github.com/rpm-software-management/rpm/pull/1788/commits/cf99101a05b1f63c828581f7c2229029278958f1;>Validate self-signatures and require subkey bindings on PGP public keys -- File Changes -- M rpmio/rpmpgp.c (128) M tests/Makefile.am (4) M tests/rpmsigdig.at (36) -- Patch Links -- https://github.com/rpm-software-management/rpm/pull/1788.patch https://github.com/rpm-software-management/rpm/pull/1788.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/1788 ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
