Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

openpgp: prevent panic for missing self-signature #99

Merged
merged 2 commits into from Apr 7, 2022

Conversation

bitfehler
Copy link
Contributor

There can be public keys where an identity has no self-signature. Since
the order of identities as tested against shouldPreferIdentity is
unstable (due to being based on a map iteration), such a situation leads
to a panic in a certain percentage of runs.

Avoid the panic by preferring the potential new identity. This can still
lead to unusable results, but they will returned as a proper error,
allowing an application to handle it.

@twiss
Copy link
Member

twiss commented Apr 4, 2022

Hey 👋 Thanks for catching this!

I agree this is an issue, that was introduced by not considering revocation signatures as self-signatures anymore (and storing them separately), but still accepting User IDs with only a revocation signature; breaking the assumption that all Identities have a SelfSignature.

However, I think there are a few other places that have this assumption - for example, in EncryptionKey() and SigningKeyById() (which are called by most functions that use the key), the PrimaryIdentity()'s SelfSignature is also used without checking whether it exists. Probably checks should be introduced there as well? Or did you already test this?

@bitfehler
Copy link
Contributor Author

Thanks for the feedback! I did not check this, I was in fact mostly trying to mitigate an issue I was encountering in the wild, I wasn't aware of the background. I can look into this and put a similar check in place in other places where I find this.

@twiss
Copy link
Member

twiss commented Apr 4, 2022

👍 Thanks! That would be great 🙏

Since revocation signatures are not considered self-signatures anymore,
but identities can potentially only have a revocation signature, there
can be identities without self-signature (i.e. `id.SelfSignature ==
nil`).

Hence, guard against null pointer dereference in the various places
where the self-signature is accessed.
openpgp/keys.go Outdated Show resolved Hide resolved
Co-authored-by: Conrad Hoffmann <1226676+bitfehler@users.noreply.github.com>
@twiss twiss merged commit a948124 into ProtonMail:master Apr 7, 2022
artoj pushed a commit to artoj/meta.sr.ht that referenced this pull request Mar 30, 2023
It contains a fix [1] that will prevent a panic that we observed
happening in production.

[1] ProtonMail/go-crypto#99
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants