[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Forward secrecy in spiped
On 04/29/14 04:38, Frederick Akalin wrote:
> On Thu, Apr 24, 2014 at 4:04 PM, Colin Percival <cperciva@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