search

chokepoint / CryptHook

TCP/UDP symmetric encryption tunnel wrapper
MIT License
117 stars 41 forks source link

Security Audit #1

Open defuse opened 10 years ago

defuse commented 10 years ago

I have a hobby of auditing random crypto code I find on github, so I took a look at this one. There are two problems I found:

First, the way it checks if the key has already been generated will lead to unnecessary calls to PBKDF2, which could slow it down a lot:

    if (glob_key[0] == 0x00) // Generate key if its the first packet
            gen_key();

A random key will have the first byte 0 with probability 1 in 256. For such keys, PBKDF2 will be re-run for every call to encrypt_data.

Second, the same key is used for both directions (from the client to server, and server to client). This makes it possible to re-send one side of the connection's own packets back to itself and it will accept them as though the other client is sending them. Also, there are no sequence numbers, so packets can be re-ordered by the adversary and it will not be detected.

I suggest adding to the disclaimer section that it does not provide message authentication, since to claim message authentication those properties must be satisfied as well.

chokepoint commented 10 years ago

Thanks for checking out the code. I'll start working on these issues.

Since GCM mode uses a counter IV, could that not also be double tasked to track the sequence of packets? Just trying to find a way to keep the overhead to a minimum.

defuse commented 10 years ago

Yes, I think the IV (nonce) could be used to track the sequence of packets. I think it would be alright, but there's probably another attack I'm not thinking of, so I'm hesitant to say that would fix it.