search

keybase / saltpack

a modern crypto messaging format
https://saltpack.org/
BSD 3-Clause "New" or "Revised" License
989 stars 62 forks source link

Encryption sender secretbox payload is the sender KID (not raw key bytes) #79

Closed gabriel closed 4 years ago

gabriel commented 4 years ago

The encryption mode was assuming that the sender KID is the raw box key bytes. This is usually the case, but not always. If you don't use the raw box public key bytes as the KID, then decryption fails.

In other words, the sender secretbox payload is actually the sender KID. We rename senderKey to senderKID and remove the rawBoxKeyFromSlice call. LookupBoxPublicKey is called with KID bytes as before to get the BoxPublicKey.

In the test, where the sender KID bytes were being corrupted, this should result in the sender key not being found since the KID was changed and the subsequent call to LookupBoxPublicKey with the wrong bytes, wouldn't find a key.

Lmk if you need any more details or clarifications.

maxtaco commented 4 years ago

@AMarcedone can you take a look at this PR?

AMarcedone commented 4 years ago

Hi, thanks for the PR. I am not super familiar with this part of the codebase, so let me know if you disagree with me below, I am pretty open to feedback.

I agree that reading the code it seems that the LookupBoxPublicKey call should take a keyID and not the key itself. However, from the spec (https://saltpack.org/encryption-format-v2) it seems that: The sender secretbox is a crypto_secretbox containing the sender's long-term public key, encrypted with the payload key from below.. It mentions the key itself, not the key ID. So accepting this change would require editing the spec as well. I think we also do not impose any strict requirements on key IDs (they could not be unique for example), so it seems that encoding the public key itself is more robust than the ID, which makes me a bit reluctant to accept the change as is.

If you want to use a keyring where the keyID is not the key itself, the alternative solution I see would be to substitute the call to LookupBoxPublicKey with a reverse lookup call which ensures that the public key is present in the keyring.

What do you think?

gabriel commented 4 years ago

Ah right, the spec is clear that it should be the public key bytes, I should have checked that first.

Thanks for looking at this!