On 04/29/14 04:38, Frederick Akalin wrote: > On Thu, Apr 24, 2014 at 4:04 PM, Colin Percival <cperc...@tarsnap.com> wrote: >> Want to send me a patch? > > Okay, here is my first attempt.
Code review follows. If you prefer I can fix things myself, but since you did the first draft I figure I should give you the option. :-) > I found it to be simplest to mandate > that both sides use -f to turn off forward secrecy, but I don't know > if that rules out valid use cases. Thoughts? Can't do that -- it would break backwards compatibility. (I don't know if anyone is running with -f on one endpoint and not on the other, and if they are it's probably a mistake... but we still have to avoid any possibility that upgrading to a newer version of spiped will turn a working setup into a non-working setup.) Let's add a new option instead: -g Require perfect forward secrecy by dropping connections if the other host is using the -f option. > + * is_zero_or_one(x, len): > + * Returns non-zero if the big-endian value stored at (${x}, ${len}) is equal > + * to either 0 or 1. 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.) > + */ > +static int is_zero_or_one(const uint8_t *x, size_t len) { > + for (size_t i = 0; i < len - 1; i++) { > + if (x[i] != 0) > + return (0); > + } > + > + return ((x[len - 1] == 0) || (x[len - 1] == 1)); > +} Style is not wrong -- the opening '{' of the function should be on a separate line, and 'size_t i' should be the first line of the function, followed by an empty line. (Even if you prefer a different style, you should be consistent with spiped's style rules when working on spiped code.) 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". > +/** > + * proto_crypt_dh_validate(yh_r, dhmac_r, nofps): > * Return non-zero if the value ${yh_r} received from the remote party is not > * correctly MACed using the diffie-hellman parameter MAC key ${dhmac_r}, or > - * if the included y value is >= the diffie-hellman group modulus. > + * if the included y value is >= the diffie-hellman group modulus, or if > + * "${nofps} is zero" does not equal "the included value is > 1". > */ > int > proto_crypt_dh_validate(const uint8_t yh_r[PCRYPT_YH_LEN], > - const uint8_t dhmac_r[PCRYPT_DHMAC_LEN]) > + const uint8_t dhmac_r[PCRYPT_DHMAC_LEN], int nofps) > { > uint8_t hbuf[32]; > > @@ -165,7 +180,15 @@ proto_crypt_dh_validate(const uint8_t > yh_r[PCRYPT_YH_LEN], > return (1); > > /* Sanity-check the diffie-hellman value. */ > - return (crypto_dh_sanitycheck(&yh_r[0])); > + if (crypto_dh_sanitycheck(&yh_r[0])) > + return (1); > + > + /* Check that forward perfect secrecy is used if and only if the > + * diffie-hellman value is > 1. */ > + if ((nofps != 0) != (is_zero_or_one(&yh_r[0], CRYPTO_DH_PUBLEN) != 0)) > + return (1); > + > + return (0); > } > > /** > diff --git a/proto/proto_crypt.h b/proto/proto_crypt.h > index 8801aa7..cd8fbcb 100644 > --- a/proto/proto_crypt.h > +++ b/proto/proto_crypt.h > @@ -39,13 +39,14 @@ void proto_crypt_dhmac(const struct proto_secret *, > uint8_t[PCRYPT_DHMAC_LEN], uint8_t[PCRYPT_DHMAC_LEN], int); > > /** > - * proto_crypt_dh_validate(yh_r, dhmac_r): > + * proto_crypt_dh_validate(yh_r, dhmac_r, nofps): > * Return non-zero if the value ${yh_r} received from the remote party is not > * correctly MACed using the diffie-hellman parameter MAC key ${dhmac_r}, or > - * if the included y value is >= the diffie-hellman group modulus. > + * if the included y value is >= the diffie-hellman group modulus, or if > + * "${nofps} is zero" does not equal "the included value is > 1". > */ > int proto_crypt_dh_validate(const uint8_t[PCRYPT_YH_LEN], > - const uint8_t[PCRYPT_DHMAC_LEN]); > + const uint8_t[PCRYPT_DHMAC_LEN], int); > > /** > * proto_crypt_dh_generate(yh_l, x, dhmac_l, nofps): > diff --git a/proto/proto_handshake.c b/proto/proto_handshake.c > index 0c87caa..c677e07 100644 > --- a/proto/proto_handshake.c > +++ b/proto/proto_handshake.c > @@ -209,7 +209,7 @@ callback_dh_read(void * cookie, ssize_t len) > return (handshakefail(H)); > > /* Is the value we read valid? */ > - if (proto_crypt_dh_validate(H->yh_remote, H->dhmac_remote)) > + if (proto_crypt_dh_validate(H->yh_remote, H->dhmac_remote, H->nofps)) > return (handshakefail(H)); > > /* > diff --git a/spipe/spipe.1 b/spipe/spipe.1 > index 5086d6b..c39f2bf 100644 > --- a/spipe/spipe.1 > +++ b/spipe/spipe.1 > @@ -41,7 +41,7 @@ Use the provided key file to authenticate and encrypt. > .B \-f > Use fast/weak handshaking: This reduces the CPU time spent in the > initial connection setup, at the expense of losing perfect forward > -secrecy. > +secrecy. The other party must also use this flag. > .TP > .B \-j > Disable transport layer keep-alives. > diff --git a/spiped/spiped.1 b/spiped/spiped.1 > index 09d5abe..82ca7b3 100644 > --- a/spiped/spiped.1 > +++ b/spiped/spiped.1 > @@ -73,7 +73,7 @@ finished launching it will be ready to create pipes. > .B \-f > Use fast/weak handshaking: This reduces the CPU time spent in the > initial connection setup, at the expense of losing perfect forward > -secrecy. > +secrecy. The other party must also use this flag. > .TP > .B \-F > Run in foreground. This can be useful with systems like daemontools. This will all need to be revised to use the new -g / 'requirefps' option instead of overloading the -f / 'nofps' option. -- Colin Percival Security Officer Emeritus, FreeBSD | The power to serve Founder, Tarsnap | www.tarsnap.com | Online backups for the truly paranoid