@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
@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
@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);
+
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
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
@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;
@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
@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
@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);
+
@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
@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,
@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,
@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
@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
@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
@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,
@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,
@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
@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,
@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
@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
@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.
--
@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:
>
@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
@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
@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:
@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
@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,
@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:
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
30 matches
Mail list logo
