Trustwords need rehashing of fingerprints #87

Closed
opened 3 months ago by fdik · 6 comments
fdik commented 3 months ago
Owner

Jonathan Rodríguez González points us to the problem that a MITM can easily find a pair of fingerprints Kma, Kmb, which are summing up with the original fingerprints of Alice Ka and Bob Kb to the same sum, respectively:

Kmb arbitrarily chosen.

Kma = Ka + Kmb + Kb

Ka + Kmb = S1
Kb + Kma = S1

So the Trustword check is depending on the security of the hash function, which calculates the fingerprint, because it depends on finding key data, which results in this fingerprint.

That requires fingerprints being quite huge to avoid Rainbow table attacks. And it needs to have a check, if Rainbow table attacks can be used for finding keys to make this an attack. In case it is possible we may need to replace XOR as the sum.

The rehashing may be required for Eliptic curve algos, because they may use the pubkey as “fingerprint” without rehashing. In this case pEp Trustwords must rehash.

Because It's easy to do so pEp Trustwords alays should rehash fingerprints.

Jonathan Rodríguez González points us to the problem that a MITM can easily find a pair of fingerprints Kma, Kmb, which are summing up with the original fingerprints of Alice Ka and Bob Kb to the same sum, respectively: Kmb arbitrarily chosen. Kma = Ka + Kmb + Kb Ka + Kmb = S1 Kb + Kma = S1 So the Trustword check is depending on the security of the hash function, which calculates the fingerprint, because it depends on finding key data, which results in this fingerprint. That requires fingerprints being quite huge to avoid Rainbow table attacks. And it needs to have a check, if Rainbow table attacks can be used for finding keys to make this an attack. In case it is possible we may need to replace XOR as the sum. The rehashing may be required for Eliptic curve algos, because they may use the pubkey as “fingerprint” without rehashing. In this case pEp Trustwords must rehash. Because It's easy to do so pEp Trustwords alays should rehash fingerprints.
positron was assigned by fdik 3 months ago
fdik commented 3 months ago
Poster
Owner

Luca, we need to make this a topic in EB, please.

Luca, we need to make this a topic in EB, please.
fdik commented 3 months ago
Poster
Owner

I think we'll change the sum function from XOR to a cryptographic hash, i.e. SHA256. This avoids the problem.

I think we'll change the sum function from XOR to a cryptographic hash, i.e. SHA256. This avoids the problem.
dirk commented 3 months ago

It looks like sequoia always hashes the complete contents of the public key (including any MPIs) for the fingerprint, also for EdDSA/curve 25519 and others. So a EdDSA fingerprint is not directly the key material, but the hashed version.

A public key:

9190e98331/openpgp/src/crypto/mpi.rs (L472)

Here the part for 'DJB's "Twisted" Edwards curve DSA public key':

9190e98331/openpgp/src/crypto/mpi.rs (L514)

The hash implmentation uses those MPIs (which is a mpi::PublicKey):

c4e6642f01/openpgp/src/crypto/hash.rs (L407)

And fingerprint uses that hash:

9190e98331/openpgp/src/packet/key.rs (L1158)

Edit: Removed nonsensical comment. Will verify in running code.

Update: Added some println! with the following result for a ecdh key:

ECDH { curve: Cv25519, q: 263 bits: 401C 324B 15AE A0DC 38E3 AFEB 35FE 3970 E867 5E12 C834 785C A816 2482 ED47 85DB 4C, hash: SHA256, sym: AES128 } -> Fingerprint("91779F21DA8241ACDDC91BC9CA7BD567BA73A1F9")

The fingerprint looks indeed very different from the actual key.

    pub fn fingerprint(&self) -> Fingerprint {
        let mut h = HashAlgorithm::SHA1.context().unwrap();

        self.hash(&mut h);

        let mut digest = vec![0u8; h.digest_size()];
        let _ = h.digest(&mut digest);
        let fpr = Fingerprint::from_bytes(digest.as_slice());
        println!("{:?} -> {:?}", self.mpis(), fpr);
        return fpr
    }
It looks like sequoia always hashes the complete contents of the public key (including any MPIs) for the fingerprint, also for EdDSA/curve 25519 and others. So a EdDSA fingerprint is not directly the key material, but the hashed version. A public key: https://gitlab.com/sequoia-pgp/sequoia/-/blob/9190e983315c9421ae4b6829997f161141a733c5/openpgp/src/crypto/mpi.rs#L472 Here the part for 'DJB's "Twisted" Edwards curve DSA public key': https://gitlab.com/sequoia-pgp/sequoia/-/blob/9190e983315c9421ae4b6829997f161141a733c5/openpgp/src/crypto/mpi.rs#L514 The hash implmentation uses those MPIs (which is a mpi::PublicKey): https://gitlab.com/sequoia-pgp/sequoia/-/blob/c4e6642f01ff45bfa4eea027b31c5418bfebb8b3/openpgp/src/crypto/hash.rs#L407 And fingerprint uses that hash: https://gitlab.com/sequoia-pgp/sequoia/-/blob/9190e983315c9421ae4b6829997f161141a733c5/openpgp/src/packet/key.rs#L1158 Edit: Removed nonsensical comment. Will verify in running code. Update: Added some println! with the following result for a ecdh key: ``` ECDH { curve: Cv25519, q: 263 bits: 401C 324B 15AE A0DC 38E3 AFEB 35FE 3970 E867 5E12 C834 785C A816 2482 ED47 85DB 4C, hash: SHA256, sym: AES128 } -> Fingerprint("91779F21DA8241ACDDC91BC9CA7BD567BA73A1F9") ``` The fingerprint looks indeed very different from the actual key. ``` pub fn fingerprint(&self) -> Fingerprint { let mut h = HashAlgorithm::SHA1.context().unwrap(); self.hash(&mut h); let mut digest = vec![0u8; h.digest_size()]; let _ = h.digest(&mut digest); let fpr = Fingerprint::from_bytes(digest.as_slice()); println!("{:?} -> {:?}", self.mpis(), fpr); return fpr } ```
sva added the bug enhancement labels 2 months ago
Owner

I am now satisfied with the state of the gitea-87 branch. After discussing the necessary adapter changes with @heck I want to merge this it into master. I may do some cosmetic refactoring, but the current state of the branch is essentially correct.

I am now satisfied with the state of the gitea-87 branch. After discussing the necessary adapter changes with @heck I want to merge this it into master. I may do some cosmetic refactoring, but the current state of the branch is essentially correct.
positron added reference gitea-87 1 month ago
Owner

At the time of writing, until I keep the gitea-87 branch existing (I plan to turn everything into one commit) the list of changes is here:
https://gitea.pep.foundation/pEp.foundation/pEpEngine/src/branch/gitea-87/NEWS-gitea-87

I am writing this here now in case there is no time for speaking to @heck later today; we usually meet on Thursdays.

Of couse we can speak about this another day, but it is better if I make things clear and do not forget them myself. I have taken days off from December 23 to December 27 included.

At the time of writing, until I keep the gitea-87 branch existing (I plan to turn everything into one commit) the list of changes is here: https://gitea.pep.foundation/pEp.foundation/pEpEngine/src/branch/gitea-87/NEWS-gitea-87 I am writing this here now in case there is no time for speaking to @heck later today; we usually meet on Thursdays. Of couse we can speak about this another day, but it is better if I make things clear and do not forget them myself. I have taken days off from December 23 to December 27 included.
Owner

Cherry-picking. gitea-87 is about to disappear.

Cherry-picking. gitea-87 is about to disappear.
positron closed this issue 1 month ago
positron changed reference from gitea-87 to master 1 month ago
Sign in to join this conversation.
No Milestone
No Assignees
3 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: pEp.foundation/pEpEngine#87
Loading…
There is no content yet.