rnpgp / rnp

RNP: high performance C++ OpenPGP library used by Mozilla Thunderbird
https://www.rnpgp.org
Other
191 stars 54 forks source link

Handle unknown pubkey algorithms safely (set `RNP_LOAD_SAVE_PERMISSIVE` by default?) #2204

Open dkg opened 3 months ago

dkg commented 3 months ago

The rnp-sop implementation used for the Interop Test Suite fails to ignore a subkey with an unknown algorithm. There is a request to make rnp-sop set RNP_LOAD_SAVE_PERMISSIVE by default, but it's not clear that it will be adopted, as it might not reflect the default behavior of a naive user of librnp. This makes it look like RNP is currently a blocker toward the ecosystem being able to adopt and distribute new pubkey algorithms, even if they are adopted in a backward-compatible way (i.e., widely-adopted algorithms are available in the same certificate as newer algorithms).

I see at least three ways to address this:

dkg commented 3 months ago

Presumably a rigorous stateful OpenPGP implementation would want to do something like this on certificate import (ignoring questions about third-party certifications for the purposes of this discussion):

But the RNP_LOAD_SAVE_PERMISSIVE semantics are a strange combination that doesn't really match the above reasoning that i can tell, whether it is set or cleared.

ni4 commented 3 months ago

RNP library is designed with backward compatibility in mind, and from this point of view changing the default behaviour is wrong. That's actually why this flag was added at some point. So if implementation which use RNP library want to fail on unknown keys then it should not set this flag, and set otherwise. We do not dictate desired behaviour, and give a way to adobt any policy.

So if SOP implementers do not want to set this flag it's not a problem of RNP, but their intended behaviour.

dkg commented 3 months ago

This discussion reminds me of the discussion we had around #1820 , where backward compatibility was cited as the reason for not changing the default, even though the default was actually blocking ecosystem evolution (because of the behavior when an unknown signature was attached). I'm grateful that you ended up resolving that bug by changing the default in #1896 !

I am not claiming that the exact right answer here is to set the RNP_LOAD_SAVE_PERMISSIVE flag by default. maybe there are some more subtle semantics that would be a preferable default. But the basic shape of the problem is the same as #1820: rnp encounters an OpenPGP artifact that has some stuff it understands, and some stuff it doesn't understand. It rejects the entire artifact by default, unless the caller knows the right way to invoke it to get compatible, interoperable behavior.

The default should be compatible, interoperable behavior. whether that means ignoring and dropping the stuff it doesn't understand; or ignoring and saving the stuff it doesn't understand, i don't have much of a preference (though i think most rnp users might appreciate having the extra stuff stored as long as it has been cryptographically confirmed, so that they can use it after an rnp upgrade). But rejecting the stuff that rnp does understand means that no one else can introduce new OpenPGP material without worrying that it will break things for users of librnp.

kaie commented 3 months ago

Related Thunderbird discussion: https://bugzilla.mozilla.org/show_bug.cgi?id=1884337