Justus/openpgp fixes #1813

teythoon · 2021-10-27T09:17:31Z

No description provided.

mlschroe · 2021-10-27T09:59:07Z

rpmio/digest_libgcrypt.c

@@ -422,8 +422,6 @@ static int pgpVerifySigEDDSA(pgpDigAlg pgpkey, pgpDigAlg pgpsig, uint8_t *hash,
 	return rc;
    if (pgpkey->curve != PGPCURVE_ED25519)
 	return rc;
-    if (hash_algo != PGPHASHALGO_SHA256)
-	return rc;


I think the check was from a time where we used a different way to set up the sexp, and we just forgot to remove it. Thanks for spotting this.

pmatilai · 2021-10-27T14:10:50Z

rpmio/rpmpgp.c

@@ -503,6 +500,9 @@ static int pgpPrtSubType(const uint8_t *h, size_t hlen, pgpSigType sigtype,
 	case PGPSUBTYPE_REVOKE_REASON:
 	case PGPSUBTYPE_FEATURES:
 	case PGPSUBTYPE_EMBEDDED_SIG:
+	    pgpPrtHex("", p+1, plen-1);
+	    break;
+	case PGPSUBTYPE_NOTATION:


I fail to see how notations are any different from all the other stuff in the above that we don't handle. I mean, if "recognizing" is a matter of being in a switch-case then PGPSUBTYPE_NOTATION is just as "recognized" as, say, PGPSUBTYPE_REVOKE_KEY. And if not, then most of these should be in the "not recognized" category, which is basically what my "implemented" flag did.

PGPSUBTYPE_NOTATION is definitely not recognized unless the notation name is recognized.

The difference is that you made an conscious decision to ignore a subpacket like the features subpacket, whereas you did not make a conscious decision to ignore the notation with the name "[email protected]".

Okay, I can see the semantic difference in that but this all sounds like a whole lot of hairsplitting me. I can't possibly know for sure, but I'm inclined to think the person who wrote this parser 20 years ago did not consider such subtleties, just like I think these are just "unimplemented" with no consideration to possible side-effects.

Mind you, I'm not arguing against the change as such, just noting that such subtleties are likely lost on the average person looking at this parser (eg myself). Which is why I'd prefer the simpler and practical "we don't parse this so it's unrecognized" interpretation, which my "implemented" flag did. The "implemented" name was not spec language based BTW but rather my engineering view based, I didn't even realize the spec has such subtle division between implemented and recognized. None of this is likely to matter in rpm context anyhow.

This was marked resolved but I'm not sure it is.

What I'm trying to say here is that the only safe assumption wrt these tags falling straight through to default case is to consider them not recognized. I think with the semantics explained above, everything downwards of (and including) PGPSUBTYPE_EXPORTABLE_CERT should be flagged not recognized, not just notations.

PGP certificate canonicalization

What is this, and how could it be useful in the context of RPM? Serious question.

Before a OpenPGP certificate can be used, it has to be canonicalized. Failure to do so properly leads to catastrophic failure of the protocol. It is at the heart of every PGP implementation. In Sequoia, this is roughly a fifth of the core library.

Among other things, you need to check that the signatures binding the cert's components to the primary key are valid. Then, you need a way to reason about the certificate, e.g. given a policy and a time, which (sub)key is eligible to make signatures?

I see. What policy do you suggest for RPM?

At the bare minimum, reject unsafe algorithms and key sizes. But, "It is not RPM's place to impose such a policy".

For inspiration, please see https://docs.rs/sequoia-openpgp/latest/sequoia_openpgp/policy/struct.StandardPolicy.html

The RPM developers made it abundantly clear that PGP certificate canonicalization is not needed for RPM. They also explained to me how OpenPGP works, and how the IETF working group works, and that me spending hours every week working on the revision of the spec is nothing new relative to what Werner did. All of that made it a lot less exciting to work on RPM.

Apologies from my behalf if that's the impression you got. That's not at all what I think, just that we're looking at the thing from very different perspectives: the rpm implementation strives to do the bare minimum to get by, and when you look at a spec through such glasses, I guess things will look very different compared to somebody working on a full implementation.

Also, when I started looking into RPM's implementation, I thought it would be salvageable. I'm no longer convinced.

Then, I started refactoring RPM with the idea of making the PGP implementation plugable. That turned out to be very messy and immediately break RPM's public interfaces, therefore it cannot be an incremental improvement. Not fun. Lingers in my branch waiting to be picked up again. May happen someday.

Sorry for dumping. I'm not going to fix RPM's PGP implementation.

Quite understandable. Thanks for looking into it and the patches provided!

rpmio/rpmpgp.c

Mask out the critical bit before switching over the subpacket type.

The question is not whether a subpacket type is implemented or acted upon, the question is whether it is recognized or not. Section 5.2.3.1. of RFC4880 says: Bit 7 of the subpacket type is the "critical" bit. If set, it denotes that the subpacket is one that is critical for the evaluator of the signature to recognize. If a subpacket is encountered that is marked critical but is unknown to the evaluating software, the evaluator SHOULD consider the signature to be in error. An evaluator may "recognize" a subpacket, but not implement it. The purpose of the critical bit is to allow the signer to tell an evaluator that it would prefer a new, unknown feature to generate an error than be ignored.

struct pgpDigParams_s keeps a copy of the verbatim key material for hashing. The length of this data is kept in 'hashlen' which previously was a uint8_t. However, the size of the signature's hashed subpacket area can be up to 2^16 bytes, and one needs to hash some of the signature packet's fields on top of that. Hence, 'hashlen' must be at least a uint32_t. This overflow happens in practice as soon as the signature's hashed subpacket area contains an embedded signature. See section 11.1 of RFC4880: Each Subkey packet MUST be followed by one Signature packet, which should be a subkey binding signature issued by the top-level key. For subkeys that can issue signatures, the subkey binding signature MUST contain an Embedded Signature subpacket with a primary key binding signature (0x19) issued by the subkey on the top-level key. While the embedded signature may be in the unhashed subpacket area because it is self-authenticating, it is more robust to put it in the hashed area.

In OpenPGP, hashing is done independently of the signing algorithm, hence they may be freely combined.

Since we don't recognize any notations, critical notation subpackets should invalidate the signature.

The hash context is duplicated unconditionally, but there is an execution path exiting the function without it being finalized.

pmatilai · 2021-11-10T10:35:31Z

I merged the non-controversial parts manually to move this forward.
Thanks for the work + patches so far!

DemiMarie

This change is not safe; some new features need to be implemented in RPM.

DemiMarie · 2021-11-10T10:21:02Z

rpmio/rpmpgp.c

@@ -444,7 +444,7 @@ static int pgpPrtSubType(const uint8_t *h, size_t hlen, pgpSigType sigtype,
    int rc = 0;

    while (hlen > 0 && rc == 0) {
-	int impl = 0;
+	int recognized = 1;


This makes new values default to being recognized, which is unsafe.

A new value would get zeroed in the default case of the switch below, but I totally agree it's safer and far more obvious to have the default be non-recognized.

DemiMarie · 2021-11-10T10:40:32Z

rpmio/rpmpgp.c

@@ -503,6 +500,9 @@ static int pgpPrtSubType(const uint8_t *h, size_t hlen, pgpSigType sigtype,
 	case PGPSUBTYPE_REVOKE_REASON:
 	case PGPSUBTYPE_FEATURES:
 	case PGPSUBTYPE_EMBEDDED_SIG:
+	    pgpPrtHex("", p+1, plen-1);
+	    break;
+	case PGPSUBTYPE_NOTATION:


RPM must implement the following:

Signature creation time

Signature expiration time

Key expiration time

Key flags

In these cases, ignoring that a subpacket is marked critical would be a serious error, as it could cause RPM to accept a package that it must not. In all four of these cases, the correct fix is for RPM to actually implement the corresponding subpacket, not to ignore it.

RPM can safely ignore the following, even if marked critical, at least for now:

exportable certification

trust signature

regular expression

revocable

revocation key

primary userid

embedded signature

preferred symmetric key

preferred hash algorithm

preferred compression algorithm

key server preferences

policy URL

For notation subpackets, the default must be to reject unknown critical notations, but some notations may be ignorable. If RPM gets support for revocation or a web of trust, the list of safely ignoreable subpackets will shrink. Note that unlike subkey binding signatures, primary key binding signatures do not need to be supported yet. My understanding is that primary key binding signatures ensure Eve cannot claim to have made a signature actually made by Alice, which is not an issue for RPM at this time.

pmatilai · 2022-01-24T12:10:06Z

Closing as per #1813 (comment), thanks anyway!

mlschroe reviewed Oct 27, 2021

View reviewed changes

pmatilai reviewed Oct 27, 2021

View reviewed changes

pmatilai reviewed Oct 29, 2021

View reviewed changes

rpmio/rpmpgp.c Outdated Show resolved Hide resolved

teythoon added 7 commits November 1, 2021 15:50

Fix signature subpacket type handling

00bb218

Mask out the critical bit before switching over the subpacket type.

Fix typo

3ea0677

Fix Ed25519 signature verification using libgcrypt

9113999

In OpenPGP, hashing is done independently of the signing algorithm, hence they may be freely combined.

Fix handling of signature notations

57bdf2b

Since we don't recognize any notations, critical notation subpackets should invalidate the signature.

Fix hash context leak

a91d13a

The hash context is duplicated unconditionally, but there is an execution path exiting the function without it being finalized.

teythoon force-pushed the justus/openpgp-fixes branch from ec211d6 to a91d13a Compare November 1, 2021 14:50

DemiMarie suggested changes Nov 10, 2021

View reviewed changes

pmatilai closed this Jan 24, 2022

teythoon mentioned this pull request Apr 22, 2022

Distinguish between trusted and untrusted signatures and keys. #1993

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Justus/openpgp fixes #1813

Justus/openpgp fixes #1813

teythoon commented Oct 27, 2021

mlschroe Oct 27, 2021

pmatilai Oct 27, 2021

DemiMarie Oct 27, 2021

teythoon Oct 27, 2021

pmatilai Oct 28, 2021

pmatilai Nov 3, 2021

DemiMarie Jan 21, 2022

teythoon Jan 21, 2022

DemiMarie Jan 21, 2022

teythoon Jan 21, 2022

pmatilai Jan 24, 2022

pmatilai commented Nov 10, 2021 •

edited

Loading

DemiMarie left a comment

DemiMarie Nov 10, 2021

pmatilai Nov 10, 2021

DemiMarie Nov 10, 2021

pmatilai commented Jan 24, 2022

Justus/openpgp fixes #1813

Justus/openpgp fixes #1813

Conversation

teythoon commented Oct 27, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmatilai commented Nov 10, 2021 • edited Loading

DemiMarie left a 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

pmatilai commented Jan 24, 2022

pmatilai commented Nov 10, 2021 •

edited

Loading