SecurityInnovation / PGPy

Pretty Good Privacy for Python
BSD 3-Clause "New" or "Revised" License
317 stars 98 forks source link

`PGPKey.verify`: dubious logic about ignoring collision-resistance for self-signatures #422

Open dkg opened 1 year ago

dkg commented 1 year ago

in PGPKey.verify we see:

                if self_verifying:
                    signature_issues &= ~SecurityIssues.hashFunctionNotCollisionResistant

I believe the argument for this logic is that it's ok to issue a self-signature without worrying about collision-resistance, because the issuer controls the entire message being signed, so an attacker cannot supply one side of a collision in some attacker-controlled part.

While i think i agree with that logic, it does not follow that it is safe to consume what appears to be a self-signature without worrying about collision-resistance. For example, consider an unsafe issuer, who is willing to issue non-self-sigs using an algorithm that has inadequate collision resistance. An attacker can supply an object to be signed that is one side of a collision with a self-signature. When the issuer creates the signature for the attacker, the attacker can strip it and attach the signature to the other side of the collision.

The result is that any consumer who accepts self-sigs made over algorithms that are not collision resistant is vulnerable to such an attack. I recommend removing this check, or perhaps making it more nuanced (e.g. by algorithm, by date, like: MD5-based signatures are unacceptable no matter what, and SHA1-based self-signatures are acceptable as long as they were made before 2023)

Commod0re commented 1 year ago

We should be strict in what we emit and lenient in what we take in. We can emit a warning or something but the library isn't actually useful if it refuses to function for seemingly arbitrary reasons, especially if it offers the user no way to accept the warning.

dkg commented 1 year ago

I don't think that "postel's law" (strict on produce, lenient on consume) is necessarily the right approach for a security tool. In particular, it can lead to security failures, like accepting signatures that it should not accept.

There are many reasons that a signature verification might fail -- not least of which that the algorithm is now known to be bad. For example a signature could fail because the signing key was revoked. It's not wrong for signature verification to fail -- that's the safe thing to do.

Put another way, if we want to preserve Postel's law, we can look at the tooling as "something that produces signature verifications". Is it strict to produce a :+1: verification when we know that the signature is dubious? being strict on what we produce makes it so that implementations can actually trust the output.

If you're looking for a way for the user to accept the warning, or to proactively mark certain kinds of signature verification problems as acceptable (e.g. the user for some reason doesn't care about collision resistance?) then i'm happy to talk about an interface for such a bypass mechanism. But i think the default should be robust and strict verification.

Commod0re commented 1 year ago

Critical misunderstanding: this is software that is meant to be used. It's not a "security tool" it is a "library" for enabling software to interoperate with other software using the OpenPGP standard.

Can it be used for security tooling? Yes. But first and foremost it absolutely must function or else why would anyone us it?

We can nudge people towards making good choices with strong defaults but the more artificial roadblocks are in place, the less likely users are to keep using the library when nobody else gets in the way so insistently.

I designed PGPy's API to be natural to use because I want people to use it, and I want it to induce minimal frustration. Refusing to work for arcane/arbitrary reasons induces unnecessary frustration and I'm totally against that.

Commod0re commented 1 year ago

not least of which that the algorithm is now known to be bad.

Absolutely untrue. A valid signature will never fail to verify due to the algorithm being "known to be bad" unless we arbitrarily block its use. The danger there is for a forged signature to validate when it should not.

While that is a valid and potentially relevant danger it's not absolute and I won't pretend it is. People should be able to use PGPy to validate any old signature they have lying around for example. Interoperation with legacy systems is a thing and fragmenting users into old or modified versions of PGPy to make them work is (imo) worse. Pushing them away to other libraries is arguably worse too

Commod0re commented 1 year ago

For typical usage, all of the important security decisions are made during (sub)key creation so if anything should be restricted for "improves security" reasons it should be that part of the interface.

Some users of PGPy don't have the luxury of generating a new key for their purposes at all and the idea of excluding them from usable updates with ivory tower justifications just doesn't sit right with me

dkg commented 1 year ago

I hear your concerns, @Commod0re, and i can also understand why you'd be reluctant to make PGPy be the first implementation that rejects a signature that other implementations might accept.

The ecosystem is moving, though, and known-bad algorithms are going to be rejected by other implementations (e.g. no other modern implementation i'm aware of produces or accepts signatures over MD5) In some cases, that may make it look like PGPy is a more dangerous implementation, because it's willing to accept signatures that other implementations would reject. In practice, most users of an OpenPGP implementation don't want to have to manually understand or decide on matters like "what algorithm was this made by again?" -- they just want to know that if their library says :+1: then the signature is somethng they can rely on. That puts more weight on the implementation, to be sure :grimacing:.

The upcoming revision for RFC 4880 contains explicit guidance about what kinds of signatures are not acceptable in the modern cryptographic context (searching the doc for "must not validate" yields several instances). This is hardly an "ivory tower" justification, it's very much rooted in practical reliability. In particular, someone who can only make signatures using known-weak digest algorithms has no business issuing updates for systems that are being updated in 2023.

Anyway, if you're interested in talking about how users or administrators can set their own policy (rather than relying on the default policy of PGPy), i'd be happy to explore the API space there with you too. (i know that RedHat, for example, has a system-wide cryptography policy about algorithm deprecation that they want all their tools to follow; i don't know how well that would map into PGPy itself)