Re: Forward secrecy in spiped

[Colin Percival <cperciva@tarsnap.com>]

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 <cperciva@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 <cperciva@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