chokepoint/CryptHook

Security Audit

defuse opened this issue · 2 comments

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.

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.

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.