[Rpm-maint] [rpm-software-management/rpm] Distinguish between trusted and untrusted signatures and keys. (PR #1993)

[Demi Marie Obenour]

@DemiMarie commented on this pull request.

The complexity of the internal parser scares me too, but I suspect most of that complexity is essential.  I wonder how GRUB’s parser fares.

>  	}
     }
+end:

Good point.  It will be a while before I can fix this, though.

> +cHJvY2Vzc2luZwAKCRDRFb6b693SZDcaAP9ObjfoUbWoAZCk5YHDRJM46iIm3Rso
+7PM6YiQyZj+IsAD/Vy+GyFI6HSjseRRyv5CRYAFGrooE8zZiEKKYy36mpAWI7wQY
+FgoAIBYhBMnkWFpdAgGpoHfOYNEVvpvr3dJkBQJiP49bAhsCAIEJENEVvpvr3dJk
+diAEGRYKAB0WIQTOb01Xo6PinL6M2XkCZ0a0bI6plQUCYj+PWwAKCRACZ0a0bI6p
+lb06AQC9L1W6jk5afsbeqsVnmUeqFZOPoIXpRv4I4Weeset2RQEA65JuqlX2Rwnx
+O+Ks6i6ProtEJc9PJfurmqTkw9Husw3QmAEAy9wakg7Q/SCKaGCOEM/hk4dbvlWr
+dRKzP4R71bNh5mYBAOjg0WqQhhZpe2ocrT5Q0UnGFykd4W3Uw6vYuWRSbRgAuDME
+Yj+PqxYJKwYBBAHaRw8BAQdAiarMGXScnHY1b3kTHmKFEN8zZi/lCpp5rgFyYbCe
+sCeI7wQYFgoAIBYhBMnkWFpdAgGpoHfOYNEVvpvr3dJkBQJiP4+rAhsCAIEJENEV
+vpvr3dJkdiAEGRYKAB0WIQRXTUwdJeAh19V7FuDLLiTHrB+/PQUCYj+PqwAKCRDL
+LiTHrB+/PemvAQD98K824iCRReNI9fa6tKoJIOT++2ju/LaS6Ux4NiuQ6AEAsTZW
+Fz+SBgUEYUo/IG3DfYdE12QG8mosjwlSn++2iQGZCwEA75OsfeW4x8ltyduRAj5h
+5nFoswUnFC2C9SpJeiZDLSgBAO5h6mRI9iafOref8xhCf6qAtnXD3J195VMX1zCU
+chEM
+=MpKy
+-----END PGP PUBLIC KEY BLOCK-----

That indeed should be done, sorry about that.  I will be busy for a while with Qubes tasks.

> @@ -1179,7 +1179,11 @@ rpmRC pgpVerifySignature(pgpDigParams key, pgpDigParams sig, DIGEST_CTX hashctx)
 	pgpDigAlg ka = key->alg;
 	if (sa && sa->verify) {
 	    if (sa->verify(ka, sa, hash, hashlen, sig->hash_algo) == 0) {
-		res = RPMRC_OK;
+		if ((key->saved & PGPDIG_UNTRUSTED) ||
+		    (sig->saved & PGPDIG_UNTRUSTED))
+		    res = RPMRC_NOTTRUSTED;
+		else
+		    res = RPMRC_OK;

I’m fine with using `RPMRC_NOKEY` instead.

> @@ -335,20 +331,42 @@ static int pgpPrtSubType(const uint8_t *h, size_t hlen, pgpSigType sigtype,
 	    _digp->saved |= PGPDIG_SIG_HAS_KEY_FLAGS;
 	    _digp->key_flags = plen >= 2 ? p[1] : 0;
 	    break;
-	case PGPSUBTYPE_EXPORTABLE_CERT:
-	case PGPSUBTYPE_TRUST_SIG:
-	case PGPSUBTYPE_REGEX:
+	case PGPSUBTYPE_SIGNER_USERID: /* RFC4880 §5.2.3.22 Signer's User ID */
+	    impl = *p;
+	    break;
+	case PGPSUBTYPE_EMBEDDED_SIG: /* embedded signature */
+	    impl = *p;
+	    break;

That is correct.  RPM has never checked that signature, but it really should.  It isn’t a security problem in the case of checking a signature immediately before installing the package, though, as the primary key is trusted to not have signed a subkey that doesn’t belong to it.  I didn’t want to add even more complexity to this code, but feel free to add such a check.

> @@ -335,20 +331,42 @@ static int pgpPrtSubType(const uint8_t *h, size_t hlen, pgpSigType sigtype,
 	    _digp->saved |= PGPDIG_SIG_HAS_KEY_FLAGS;
 	    _digp->key_flags = plen >= 2 ? p[1] : 0;
 	    break;
-	case PGPSUBTYPE_EXPORTABLE_CERT:
-	case PGPSUBTYPE_TRUST_SIG:
-	case PGPSUBTYPE_REGEX:
+	case PGPSUBTYPE_SIGNER_USERID: /* RFC4880 §5.2.3.22 Signer's User ID */
+	    impl = *p;
+	    break;
+	case PGPSUBTYPE_EMBEDDED_SIG: /* embedded signature */
+	    impl = *p;
+	    break;
+	case PGPSUBTYPE_REVOKE_REASON: /* RFC4880 §5.2.3.23 Reason for Revocation */
+	    if (plen < 2)
+		return 1; /* missing reason code */
+	    if (sigtype == PGPSIGTYPE_SUBKEY_REVOKE)
+		impl = *p;

RPM doesn’t implement those kinds of revocations.  My assumption is that keys imported into RPM are a result of `gpg2 --export --armor --export-options=export-minimal -- <TRUSTED_FINGERPRINT>` or similar.

> @@ -999,12 +1006,8 @@ int pgpPrtParams(const uint8_t * pkts, size_t pktlen, unsigned int pkttype,
 	    break;
 
 	if (selfsig) {
-	    /* subkeys must be followed by binding signature */
-	    int xx = 1; /* assume failure */
-
-	    if (!(prevtag == PGPTAG_PUBLIC_SUBKEY &&
-		  selfsig->sigtype != PGPSIGTYPE_SUBKEY_BINDING))
-		xx = pgpVerifySelf(digp, selfsig, all, i);
+	    /* subkeys must be followed by binding or revocation signature */
+	    int xx = pgpVerifySelf(digp, selfsig, all, i, prevtag);
 

See above comment about assumptions: GPG’s `--export --export-options=export-minimal` should strip such signatures off, and RPM doesn’t have the machinery needed to judge if a given packet can be safely ignored.  The Sequoia backend does and would handle this properly.

-- 
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1993#pullrequestreview-943510884
You are receiving this because you are subscribed to this thread.

Message ID: <rpm-software-management/rpm/pull/1993/review/943510884 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rpm.org/pipermail/rpm-maint/attachments/20220415/dd680c50/attachment-0001.html>