Well, it's about 2 screefuls of very simple code. Eg for the PK part, if you ctrl-f for publicKey and remotePublicKey, you see there are no usages that could plausibly be doing public key crypto or PK key generation based on crypto 101 from school.
Not sure how to respond about gatekeeping. What do you think would be a good way to write a comment pointing out these kinds of security problems? Or do you think it's unfair to do it at all?
I’m not sure what you mean by there being no public key cryptography here. In the Crypto constructor, a random secret key is generated, and then the generator for Curve25519 is scalar multiplied on the elliptic curve by this secret key to generate a public key. That’s the standard process for X25519 key generation, and it’s what Libsodium does to generate a secret key/public key pair [1].
To establish a shared secret session key, the standard ECDH procedure is used, as seen in Crypto::setRemotePublicKey, where we multiply the other party’s public key by our secret key on curve25519 to establish a shared secret. This shared secret is then used for encryption, since symmetric encryption is generally cheaper + more secure than asymmetric.
I absolutely agree that this project needs a more formal cryptographic approach, and should be using a higher level construct (e.g: crypto_secretstream, also from libsodium), but as far as I can tell at first glance, this is a working implementation of X25519.
The reason I "rolled my own" key exchange is that I'd want to use the IETF variant of Chacha20-Poly1305 in my protocol and there doesn't seem to be helper functions for key exchange for this algo.
Unless I'm misunderstanding you, the crypto_secretstream construction [1] that I (and others) have recommended uses ChaCha20Poly1305-IETF as its symmetric cipher. There wouldn't be a key exchange for this algorithm, since it's symmetric. However, the crypto_kx functions [2] generate a 256 bit shared secret, which should work for a ChaCha20Poly1305 key I believe?
Probably should have used higher-level functions. Maybe next time :). I don't think I can change the protocol and break compatibility at this time lol.
The crypto_kx functions seem to generate 2 symmetric keys for 2 directions. That creates complications in the code and that's why I didn't use it initially..
Yeahhh, that's a fair point about unnecessary complexity. The docs say if only one symmetric key is necessary, you can just set one of tx/rx to null, and the other will still be the requisite 256 bits, but I appreciate why that's a bit ugly.
Just point out the attack vectors, with some reference on solving those.
No need to "you're implementing things yourself. bad. use someone else's library". That is gatekeeping indeed, and a rather insulting way to express your concerns.
If you simply demonstrate the complexity of dealing with all security concerns, they'll realize what they need. Prepackaged "always do this" does not teach well.
Not sure how to respond about gatekeeping. What do you think would be a good way to write a comment pointing out these kinds of security problems? Or do you think it's unfair to do it at all?