ghost
commented
7 years ago Loup, forget about an "imminent" release. We'll be here for a while and for good reason.
First of all, thank you very much for going through this. You spotted a lot of things that went flew over everybody else's heads.
About dictionary coders in general, does that leak still exist if the compressed data including decompressed length is encrypted?
Note I won't be mentioning dictionary coder stuff from here on out anymore.
Please put the TODO items in a new issue comment once we've sorted these out or it'll be completely impossible to figure out what has and what hasn't been done.
intro
Not related to intro.html but perhaps there should be a validation layer that checks things are aligned how they should be/things aren't overlapping when they shouldn't be/etc.
Alignment: Is there any standardized way to check for that? In other words, is there a way to check alignment that is not implementation dependent (such as __attribute__)?
Overlapping: Probably possible to check, but I'm not sure about the performance hit if you process many small messages. See https://blogs.msdn.microsoft.com/oldnewthing/20170927-00/?p=97095 for some notes about pointer arithmetic and range checks. This may or may not break pretty bad on some embedded platforms.
I'd love @secworks to chip in on this part in particular.
crypto_lock
Is it ok to use a counter for the nonce?
Should be answered in the argument documentation for nonce: "A 24-byte number, used only once with any given session key. It does not need to be secret or random, but it does have to be unique." A counter is a 24-byte number that is used only once with any given session key that is unique.
"That length is not authenticated." - I don't fully understand the problem or the solution here. You have to put the length in the authenticated data or it's not secure? Why doesn't monocypher do that for you? It's only 8 bytes.
If you need a length field in the plaintext, you need to put it into the authenticated data. Otherwise, an attacker could modify the length field and cause a buffer overflow because your code worked with a tampered length field.
Consider a protocol where all messages have a fixed length or where you have a known good trailing character (after you check the MAC, obviously, else there's no known good). You may not particularly care about the length, especially not eight bytes of length for many small messages where you could've just used one or two bytes, too. YMMV, though. Loup may likely disagree with me here, even.
In the in place encryption example the cipher_text variable is unused, except for the last comment where it is incorrect.
+1
Do you need to call crypto_wipe even when crypto_unlock fails? Can it partially decrypt your data?
The model for crypto_unlock is this:
- crypto_unlock()
- Check return value. a. if the return value is non-zero (-1, actually), the message MAC mismatched and the message should never be touched by code. crypto_unlock has not even started decrypting because it checks the MAC first. You still need to wipe your key, but there will be no plaintext to wipe. b. if the return value is zero, the message MAC checks out and the entire message has been decrypted. You can trust the decrypted data.
This is probably a bit unclear in the current text; fixes welcome.
crypto_argon2i
"It can be used to encrypt private keys or password databases." - "it" is the resulting key?
"It" is the argon2i function. That is, you can use it to derive a key from a passphrase for private keys in asymmetric crypto, think GPG passphrases, or for password storage. This part is not clear enough, I agree.
"malloc() should yield a suitable chunk of memory." - malloc will (not should) return 8 byte aligned addresses on any normal platform. Might be worth explicitly mentioning that.
glibc docs: "The address of a block returned by malloc or realloc in GNU systems is always a multiple of eight (or sixteen on 64-bit systems)" (emphasis mine)
OpenBSD malloc(3): "The allocated space is suitably aligned (after possible pointer coercion) for storage of any type of object."
Assuming any particular alignment from malloc is questionable. Assuming meaningful alignment makes sense only from a practical purpose (the devs of the OS have to eat their own dog food). The C standard makes no guarantees about alignment or suitability of a block of memory returned by malloc. The wording in this case is about as cautious as I'd expect.
"The version provided by Monocypher has no threading support, so the degree of parallelism is limited to 1. This is considered good enough for most purposes." - I don't know why you mention this
Readers may already be familiar with argon2i and thus expect to be able to tune the parallelism parameter. This needs to be clarified here because it's confusing when you expect the parallelism parameter to be there but Monocypher doesn't actually allow for that. Additionally, it's important to know what the value is set to for interoperability. Interoperability is a particular concern for particular password storage.
"This most likely has no practical application but is exposed for the sake of completeness." - TBH I would not expose it in that case
See https://github.com/P-H-C/phc-winner-argon2/issues/151 -- There are probably applications but they're fairly niche? I'm on the fence with this one. If someone comes up with a really clever application for it, I don't want to people to be in the situation of having to forgo it, on the other hand it'd simplify invocation by a lot.
"Use crypto_verify16(3monocypher), crypto_verify32(3monocypher) or crypto_verify64(3monocypher) to compare password hashes to prevent timing attacks, which are feasible against the weakest passwords." - maybe drop the bit after the comma
+1, the comparison speed really shouldn't matter that much.
"The hardness of the computation can be chosen thus:" and the two bullet points are oddly phrased. How about: [...]
The sentences are fairly run-on in the first paragraph. I also adjusted "100k" in the second paragraph to be 100000 again for copy-pasteability and being less surprising to non-native English speakers.
To select the nb_blocks and nb_iterations parameters, it should first be
decided how long the computation should take.
For user authentication, somewhere between half a second (convenient)
and several seconds (paranoid) is a good starting point.
The computation should use as much memory as can be spared.
Since parameter selection depends on your hardware, some trial and error
will be required in order to determine the ideal settings.
Three iterations and 100000 blocks (that is, one hundred megabytes of
memory) is a good starting point.
Adjust
.Fa nb_blocks
first.
If using all available memory is not slow enough, increase
.Fa nb_iterations .crypto_blake2b
"It can authenticate messages with a naive approach." - Naive approach = just calling the functions? Is it still a naive approach if it works and is the intended usage?
Indeed. The wording may be a bit questionable. Background: SHA-2 and other common hashing functions require a special construction called HMAC to use them as message authentication functions. However, BLAKE2b does not require that. Just calling the function with a suitably long key parameter turns it into a valid message authentication function.
"The output hash. Must have at least hash_size free bytes." - You don't normally say "Must have at least x free bytes".
* Must be a buffer of at least hash_size bytes?
"Use crypto_verify16(3monocypher), crypto_verify32(3monocypher), or crypto_verify64(3monocypher) to compare MACs created this way." - is it ok to use memcmp if you didn't use a key?
If you didn't use a key, it's not a MAC. You can use memcmp in this case. May need clarification.
"Message to hash. May overlap with the hash argument where present." - What do you mean by "where present"? The hash and message are always present
s/ where present//
Should probably also be double checked that it's "overlap", not just "point at the same address".
"the length of the message in bytes." - should start with a capital T for consistency (I went back and looked at the argon2i page, you stop using capitals half down there too. Presumably this comes up elsewhere too)
+1, good catch
"The direct interface has 2 functions" - I think it's considered good style to write two etc for small numbers. Doesn't really matter though
I just skimmed over mdoc(7) and Practical UNIX Manuals. I thought I'd read something about always writing digits, but I guess I was wrong.
Should be corrected to "two". "Small numbers" is generally one through twelve, inclusive. Should probably be ignored where we're talking about return values.
"crypto_blake2b(), provided for convenience (calling it is the same as calling crypto_blake2b_general() with a null key and a 64-byte hash)." - how about: [...]
Suggestion works for me.
crypto_chacha20_H
"The input in fills the space reserved for the nonce and counter. It does not have to be random." - huh?
This only makes sense if you're familiar with Chacha20 itself. Most likely needs rewording to clarify it's about the Chacha20 block setup.
"will be random iff the above holds" - capitalisation inconsistency again. Rather than "iff the above holds", you could say "iff key has enough entropy". It's not much longer
(Context: It's in the example)
+1 for the capitalization.
It's not much longer, but it'll be long enough that it'll be longer than 80 characters when displayed on a terminal, so you'd need to have a multi-line comment.
Should this function actually be exposed?
Design decision; deferring. IMO no. Not sure why it's there.
crypto_sign, crypto_poly1305
No block describing the arguments!
You're very right about that. Not sure if it really needs one. We'll probably be better off revisiting these once you're past the TODO.
crypto_key_exchange
I don't understand what crypto_x25519 is for. Is it an optimisation if you want to generate multiple keys? The old manual was more forceful about telling people to not use it unless they understand it.
It's the raw underlying primitive. crypto_key_exchange is what you get when you crypto_x25519 and then crypto_chacha20_H the result from crypto_x25519. We should probably tell people not to touch the thing in the normal case.
I think the randomised key exchange part needs to be explained better. Why crypto_lock instead of crypto_sign?
Because crypto_sign uses asymmetric cryptography. In asymmetric cryptography, you have a secret key (aka private key) and a public key. You publish the public key to everyone who should be able to verify your messages. But you keep the secret key a secret so that nobody but you can sign messages. Think of it like this: A government signs its laws with asymmetric cryptography. You want everybody to verify the laws being untampered, so everybody gets the public key. You want nobody but the government to sign the laws, so only the government gets the secret key. The crypto_key_exchange family of functions does the opposite of that: The result of a key exchange is a single, identical key between all participants in the key exchange.
crypto_lock uses symmetric cryptograhy. In symmetric cryptography, you have one secret key (ignoring details about authentication here). You do not want anybody to have the keys other than the communication participants, but the participants all need the same key to communicate.
Confusingly, you need an asymmetric key pair for crypto_key_exchange to generate a shared symmetric key.
Did that clear it up? Got any concrete idea how to improve the page with that new knowledge?
There should probably be a function like "crypto_is_dangerous_key". If you're randomly generating keys for each session it would be nice to be able to check the key you're sending isn't going to be rejected for being malicious.
There's a total of seven keys. That's odds of seven in 115792089237316195423570985008687907853269984665640564039457584007913129639936. These odds are negligible. In fact, if this random generation happens, whoever runs into it is the luckiest person alive, probably.
That said, you do raise a point: All-0 is one of the broken keys and the only way to get that is if your random number generator has brain damage (or /dev/urandom got symlinked to /dev/null). It's less a key validity check than a RNG compromise check, however. Not sure what to do about this.
crypto_memcmp
The functions have been removed, as it says on the manual page. However, the manual page itself has been kept around so that people who use an old version of Monocypher can (1) notice they're on an old version of Monocypher, and (2) have a clearly marked upgrade path.
Updating for constant time is not an option. #38 has proven fairly impossible to solve. OpenBSD exposes timingsafe_memcmp in the stdlib, but it's an outlier.
crypto_verify16
"Standard comparison functions tend to exit as soo as they find a difference" - "as soo" -> "as soon"
+1
how about: [...]
Not a fan of actively talking to the reader with "you". Rest of the changes to the first paragraph are okay.
"provide comparison functions whose timing is independent from the content of their input." - maybe: "provide comparison functions that run in constant time"
Technically, they're not constant-time. The execution time depends on the length parameter (16, 32, 64). Though given the length parameter is now hardcoded into the function, +1 for the change.
crypto_wipe
"Secrets and values derived from them should stay in memory for the shortest possible amount of time..." - how about: [...]
No objections other than maybe taking out the paragraph there, but that's very debatable and I have no strong feelings either way.
Size argument should maybe be called secret_size.
I have no strong feelings either way.
LoupVaillant
mikejsavage
I'm not done yet, but here are my notes so far. I hope this is readable!
intro.html
crypto_aead_lock.html crypto_aead_unlock.html crypto_lock.html crypto_unlock.html
crypto_argon2i.html
"The hardness of the computation can be chosen thus:" and the two bullet points are oddly phrased. How about:
crypto_blake2b.html crypto_blake2b_final.html crypto_blake2b_general.html crypto_blake2b_general_init.html crypto_blake2b_init.html crypto_blake2b_update.html
"crypto_blake2b(), provided for convenience (calling it is the same as calling crypto_blake2b_general() with a null key and a 64-byte hash)." - how about:
crypto_chacha20_H.html
crypto_chacha20_encrypt.html crypto_chacha20_init.html crypto_chacha20_set_ctr.html crypto_chacha20_stream.html crypto_chacha20_x_init.html
crypto_check.html crypto_sign.html crypto_sign_public_key.html
crypto_check_final.html crypto_check_init.html crypto_check_update.html crypto_sign_final.html crypto_sign_init_first_pass.html crypto_sign_init_second_pass.html crypto_sign_update.html
crypto_key_exchange.html crypto_x25519.html crypto_x25519_public_key.html
crypto_lock_auth.html crypto_lock_encrypt.html crypto_lock_final.html crypto_lock_init.html crypto_lock_update.html crypto_unlock_final.html crypto_unlock_update.html
crypto_memcmp.html crypto_zerocmp.html
crypto_poly1305.html crypto_poly1305_final.html crypto_poly1305_init.html crypto_poly1305_update.html
crypto_verify16.html crypto_verify32.html crypto_verify64.html
"Standard comparison functions tend to exit as soo as they find a difference" - "as soo" -> "as soon"
how about:
"provide comparison functions whose timing is independent from the content of their input." - maybe: "provide comparison functions that run in constant time"
crypto_wipe.html
"Secrets and values derived from them should stay in memory for the shortest possible amount of time..." - how about:
Size argument should maybe be called secret_size.