fscoto commented 3 years ago

These are all hair-splitting things that I noticed (think: -Weverything), but you may want to disregard some or all of these things.

I hope at least some of these are helpful to you.

License header

Right now, the code says // --- 2-clause BSD licence (same as Monocypher's) follows ---. This may be misleading because Monocypher is dual-"licensed" CC-0 and 2-clause BSD. Monocypher under CC0 has no attribution requirements the way 2-clause BSD does, so this may be misinterpreted by hasty downstream consumers.

arc4random

I like arc4random. I like that you're encouraging arc4random(3). However, it is not portable. “On Linux, in <bsd/stdlib.h>” is inaccurate insofar as it requires the external libbsd library. Many common Linux distributions will ship it due to one reverse dependency or another, but it's not guaranteed to be there. Idiomatic Linux random number generation is via getrandom(2) instead. If those fail to be available, /dev/urandom is the next step. Similarly, Windows has no concept of arc4random(). Officially, you're meant to use BCryptGenRandom() (though libsodium uses the undocumented RtlGenRandom() instead to avoid pulling in too much CNG crypto context stuff).

“TODO: Make this work for Size other than 16, 32, 64.”

operator== should not work on sizes other than 16, 32 and 64. This was a deliberate design decision back in the day because a generic constant-time comparison function would get compiler output that's absurdly difficult to view for what it should be doing, see Loup-Vaillant/Monocypher#38.

If you're willing to do per-OS compatibility hacks, you may want to look into timingsafe_bcmp(3) on OpenBSD, but I'm not aware of this being an OS/libc-level thing anywhere else.

X25519 private keys

I don't know that much about C++, so this may be even more off the mark than the rest of my notes here, but would it be desireable to wipe the input key_bytes after copying it in on line 323 (explicit key(const void *key_bytes) {::memcpy(this->data(), key_bytes, 32);}), transferring “ownership” so to speak to the new object?

signature_t

It should be noted that POSIX reserves all identifiers ending in _t for the implementation, so using signature_t as an identifier may not be desirable.

snej commented 3 years ago

I'll fix the license text (in both places) to avoid comparisons with Monocypher's.

I misled myself about arc4random — the main project I work on uses it on Apple/Linux/Windows, so I thought it was built-in, but it turns out that's because we have a shim that implements it on platforms where it's not present. However, I've noticed that C++ has std::random_device which is defined as a nondeterministic RNG except on platforms where that doesn't exist (i.e. embedded systems, I guess.) I may consider using that instead.

Speaking of crypto_verifyNN, do you know why the implementation is so complicated? I would think that you could just XOR the arrays and then OR together the resulting bytes, and compare the result to 0. That easily works with arbitrary lengths. Or is the compiler "too smart" and figures out how to take shortcuts?

As for wiping key_bytes — it can't do that because they're passed through a const pointer. (Casting away the const would be evil, plus it could crash if someone put the key in read-only address space.) I could create an overload of the method that takes a void*, but I think it would be very unexpected for most users to have the function reach out and wipe the source data.