Sorry about the delay, I've been triaging more urgent vs. less urgent emails...
On 04/30/14 16:41, Frederick Akalin wrote: > On 4/29/14, Colin Percival <cperc...@tarsnap.com> wrote: >> Let's add a new option instead: >> -g Require perfect forward secrecy by dropping connections if the >> other host is using the -f option. > > Done. Also enforce that -f and -g aren't both used at the same time. > Not sure if there's a better way to express that in the usage blurb, > though. I think showing it as alternatives in the synopsis is good enough. If someone gets confused and offers improved documentation I can always accept it later. >> This is wrong. We need to detect 1; we don't need to detect 0. (A validly >> signed 0 implies that someone who has the shared key is not following the >> protocol, in which case we've already lost.) > > Done, although I still have a question about this (below). > > On Wed, Apr 30, 2014 at 1:33 AM, Colin Percival <cperc...@tarsnap.com> wrote: >> There's lots of "impossible" values -- all quadratic non-residues, for >> example >> -- but there's no point checking for all of them. There's always going to be >> ways that a participant can deliberately sabotage the protocol (by revealing >> the negotiated keys, if nothing else); the point of the protocol is to >> protect >> compliant hosts from non-participants. Even the -g option isn't about any >> level >> of cryptographic security; it's purely about detecting misconfigurations. > > Fair enough. But you already sanity-check the values to be < the > modulus. Why is that okay but checking for 0 is not? Checking for 0 > can be considered a sanity-check, too. Well... the sanity checking for y < N isn't really about detecting harmless protocol noncompliance. That's me being paranoid about the possibility of breakage in crypto libraries -- it *should* be fine, but OpenSSL demonstrates that assuming that crypto libraries get anything at all right is not a very good idea. >> Also, I'd prefer to avoid the early return by accumulating: >> size_t i; >> char y; >> >> for (i = 0, y = 0; i < len - 1; i++) >> y |= x[i]; >> return (y | (x[len - 1] - 1)); >> and making the function "is_not_one". > > Done. I thought you might prefer a constant-time check. I was > originally going to go with that, but I noticed that > crypto_dh_sanitycheck was already non-constant time (since it uses > memcmp). Was that deliberate? Good point. I guess I was being less paranoid when I wrote _dh_sanitycheck; either that or I just got lazy. Considering that this is a value which goes out over the wire, it's not really a problem. A few more issues with the patch, which I've fixed: > +static int is_not_one(const uint8_t *x, size_t len) There should be a line break after 'static int'; function implementations always have their names at the start of a line (unlike function declarations) so that "grep -R ^functionname .' will find them. Spaces on both sides of the '*' for consistency with other spiped code. > + return ((requirefps != 0) && ! is_not_one(&yh_r[0], CRYPTO_DH_PUBLEN)); This is confusingly complicated -- I've broken this out to /* If necessary, enforce that the diffie-hellman value is != 1. */ if (requirefps) { if (! is_not_one(&yh_r[0], CRYPTO_DH_PUBLEN)) return (1); } /* Everything is good. */ return (0); > + * secrecy. If ${requirefps} is non-zero, drop the connection if the other > end > + * attempts to do a "weak" handshake. The shared protocol secret is ${K}. For consistency with my pretentious language, s/do/perform/. > + * don't use perfect forward secrecy. if ${requirefps} is non-zero, drop s/if/If/. > -[\-fj] > +[\-{f | g}j] This should be [-f | -g] [-j], since braces are used only to offer a mandatory choice (as with spiped {-e | -d}). You forgot to update the README files. > case 'f': > - if (opt_f) > + if (opt_f || opt_g) > usage(); Checking for contradictory options goes under "Sanity-check options", after the end of the getopt loop. The tests inside the loop are just to catch specifying the same option multiple times. Committed, thanks! -- Colin Percival Security Officer Emeritus, FreeBSD | The power to serve Founder, Tarsnap | www.tarsnap.com | Online backups for the truly paranoid