Okay, replies inline, followed by updated patch.
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.
> 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.
> 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.)
Done. Sorry, meant to copy surrounding style, but forgot.
> 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?
> This will all need to be revised to use the new -g / 'requirefps' option
> instead of overloading the -f / 'nofps' option.
Done.
diff --git a/proto/proto_conn.c b/proto/proto_conn.c
index f657544..f01d667 100644
--- a/proto/proto_conn.c
+++ b/proto/proto_conn.c
@@ -20,6 +20,7 @@ struct conn_state {
struct sock_addr ** sas;
int decr;
int nofps;
+ int requirefps;
int nokeepalive;
const struct proto_secret * K;
double timeo;
@@ -56,7 +57,7 @@ starthandshake(struct conn_state * C, int s, int decr)
/* Start the handshake. */
if ((C->handshake_cookie = proto_handshake(s, decr, C->nofps,
- C->K, callback_handshake_done, C)) == NULL)
+ C->requirefps, C->K, callback_handshake_done, C)) == NULL)
goto err1;
/* Success! */
@@ -153,21 +154,23 @@ dropconn(struct conn_state * C)
}
/**
- * proto_conn_create(s, sas, decr, nofps, nokeepalive, K, timeo,
+ * proto_conn_create(s, sas, decr, nofps, requirefps, nokeepalive, K, timeo,
* callback_dead, cookie):
* Create a connection with one end at ${s} and the other end connecting to
* the target addresses ${sas}. If ${decr} is 0, encrypt the outgoing data;
* if ${decr} is nonzero, decrypt the outgoing data. If ${nofps} is non-zero,
- * don't use perfect forward secrecy. Enable transport layer keep-alives (if
- * applicable) on both sockets if and only if ${nokeepalive} is zero. Drop the
- * connection if the handshake or connecting to the target takes more than
- * ${timeo} seconds. When the connection is dropped, invoke
- * ${callback_dead}(${cookie}). Free ${sas} once it is no longer needed.
+ * don't use perfect forward secrecy. if ${requirefps} is non-zero, drop
+ * the connection if the other end tries to disable perfect forward secrecy.
+ * Enable transport layer keep-alives (if applicable) on both sockets if and
+ * only if ${nokeepalive} is zero. Drop the connection if the handshake or
+ * connecting to the target takes more than ${timeo} seconds. When the
+ * connection is dropped, invoke ${callback_dead}(${cookie}). Free ${sas}
+ * once it is no longer needed.
*/
int
proto_conn_create(int s, struct sock_addr ** sas, int decr, int nofps,
- int nokeepalive, const struct proto_secret * K, double timeo,
- int (* callback_dead)(void *), void * cookie)
+ int requirefps, int nokeepalive, const struct proto_secret * K,
+ double timeo, int (* callback_dead)(void *), void * cookie)
{
struct conn_state * C;
@@ -179,6 +182,7 @@ proto_conn_create(int s, struct sock_addr ** sas,
int decr, int nofps,
C->sas = sas;
C->decr = decr;
C->nofps = nofps;
+ C->requirefps = requirefps;
C->nokeepalive = nokeepalive;
C->K = K;
C->timeo = timeo;
diff --git a/proto/proto_conn.h b/proto/proto_conn.h
index 5030d1d..f1fa42a 100644
--- a/proto/proto_conn.h
+++ b/proto/proto_conn.h
@@ -6,18 +6,20 @@ struct proto_secret;
struct sock_addr;
/**
- * proto_conn_create(s, sas, decr, nofps, nokeepalive, K, timeo,
+ * proto_conn_create(s, sas, decr, nofps, requirefps, nokeepalive, K, timeo,
* callback_dead, cookie):
* Create a connection with one end at ${s} and the other end connecting to
* the target addresses ${sas}. If ${decr} is 0, encrypt the outgoing data;
* if ${decr} is nonzero, decrypt the outgoing data. If ${nofps} is non-zero,
- * don't use perfect forward secrecy. Enable transport layer keep-alives (if
- * applicable) on both sockets if and only if ${nokeepalive} is zero. Drop the
- * connection if the handshake or connecting to the target takes more than
- * ${timeo} seconds. When the connection is dropped, invoke
- * ${callback_dead}(${cookie}). Free ${sas} once it is no longer needed.
+ * don't use perfect forward secrecy. if ${requirefps} is non-zero, drop
+ * the connection if the other end tries to disable perfect forward secrecy.
+ * Enable transport layer keep-alives (if applicable) on both sockets if and
+ * only if ${nokeepalive} is zero. Drop the connection if the handshake or
+ * connecting to the target takes more than ${timeo} seconds. When the
+ * connection is dropped, invoke ${callback_dead}(${cookie}). Free ${sas}
+ * once it is no longer needed.
*/
-int proto_conn_create(int, struct sock_addr **, int, int, int,
+int proto_conn_create(int, struct sock_addr **, int, int, int, int,
const struct proto_secret *, double, int (*)(void *), void *);
#endif /* !_CONN_H_ */
diff --git a/proto/proto_crypt.c b/proto/proto_crypt.c
index d674bfc..8a70147 100644
--- a/proto/proto_crypt.c
+++ b/proto/proto_crypt.c
@@ -145,14 +145,32 @@ void proto_crypt_dhmac(const struct proto_secret * K,
}
/**
- * proto_crypt_dh_validate(yh_r, dhmac_r):
+ * is_not_one(x, len):
+ * Returns non-zero if the big-endian value stored at (${x}, ${len}) is not
+ * equal to 1.
+ */
+static int is_not_one(const uint8_t *x, size_t len)
+{
+ size_t i;
+ char y;
+
+ for (i = 0, y = 0; i < len - 1; i++) {
+ y |= x[i];
+ }
+
+ return (y | (x[len - 1] - 1));
+}
+
+/**
+ * proto_crypt_dh_validate(yh_r, dhmac_r, requirefps):
* 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
+ * ${requirefps} is non-zero and the included y 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 requirefps)
{
uint8_t hbuf[32];
@@ -165,7 +183,11 @@ 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);
+
+ /* Enforce that the diffie-hellman value != 1 if necessary. */
+ return ((requirefps != 0) && ! is_not_one(&yh_r[0], CRYPTO_DH_PUBLEN));
}
/**
diff --git a/proto/proto_crypt.h b/proto/proto_crypt.h
index 8801aa7..be1d3dc 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, requirefps):
* 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
+ * ${requirefps} is non-zero and the included y 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..d34591d 100644
--- a/proto/proto_handshake.c
+++ b/proto/proto_handshake.c
@@ -16,6 +16,7 @@ struct handshake_cookie {
int s;
int decr;
int nofps;
+ int requirefps;
const struct proto_secret * K;
uint8_t nonce_local[PCRYPT_NONCE_LEN];
uint8_t nonce_remote[PCRYPT_NONCE_LEN];
@@ -60,18 +61,20 @@ handshakefail(struct handshake_cookie * H)
}
/**
- * proto_handshake(s, decr, nofps, K, callback, cookie):
+ * proto_handshake(s, decr, nofps, requirefps, K, callback, cookie):
* Perform a protocol handshake on socket ${s}. If ${decr} is non-zero we are
* at the receiving end of the connection; otherwise at the sending end. If
* ${nofps} is non-zero, perform a "weak" handshake without forward perfect
- * secrecy. The shared protocol secret is ${K}. Upon completion, invoke
- * ${callback}(${cookie}, f, r) where f contains the keys needed for the
- * forward direction and r contains the keys needed for the reverse direction;
- * or w = r = NULL if the handshake failed. Return a cookie which can be
- * passed to proto_handshake_cancel to cancel the handshake.
+ * 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}.
+ * Upon completion, invoke ${callback}(${cookie}, f, r) where f contains the
+ * keys needed for the forward direction and r contains the keys needed for
+ * the reverse direction; or w = r = NULL if the handshake failed. Return a
+ * cookie which can be passed to proto_handshake_cancel to cancel the
handshake.
*/
void *
-proto_handshake(int s, int decr, int nofps, const struct proto_secret * K,
+proto_handshake(int s, int decr, int nofps, int requirefps,
+ const struct proto_secret * K,
int (* callback)(void *, struct proto_keys *, struct proto_keys *),
void * cookie)
{
@@ -85,6 +88,7 @@ proto_handshake(int s, int decr, int nofps, const
struct proto_secret * K,
H->s = s;
H->decr = decr;
H->nofps = nofps;
+ H->requirefps = requirefps;
H->K = K;
/* Generate a 32-byte connection nonce. */
@@ -209,7 +213,8 @@ 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->requirefps))
return (handshakefail(H));
/*
diff --git a/proto/proto_handshake.h b/proto/proto_handshake.h
index 80d3813..925ad97 100644
--- a/proto/proto_handshake.h
+++ b/proto/proto_handshake.h
@@ -6,17 +6,18 @@ struct proto_keys;
struct proto_secret;
/**
- * proto_handshake(s, decr, nofps, K, callback, cookie):
+ * proto_handshake(s, decr, nofps, requirefps, K, callback, cookie):
* Perform a protocol handshake on socket ${s}. If ${decr} is non-zero we are
* at the receiving end of the connection; otherwise at the sending end. If
* ${nofps} is non-zero, perform a "weak" handshake without forward perfect
- * secrecy. The shared protocol secret is ${K}. Upon completion, invoke
- * ${callback}(${cookie}, f, r) where f contains the keys needed for the
- * forward direction and r contains the keys needed for the reverse direction;
- * or w = r = NULL if the handshake failed. Return a cookie which can be
- * passed to proto_handshake_cancel to cancel the handshake.
+ * 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}.
+ * Upon completion, invoke ${callback}(${cookie}, f, r) where f contains the
+ * keys needed for the forward direction and r contains the keys needed for
+ * the reverse direction; or w = r = NULL if the handshake failed. Return a
+ * cookie which can be passed to proto_handshake_cancel to cancel the
handshake.
*/
-void * proto_handshake(int, int, int, const struct proto_secret *,
+void * proto_handshake(int, int, int, int, const struct proto_secret *,
int (*)(void *, struct proto_keys *, struct proto_keys *), void *);
/**
diff --git a/spipe/main.c b/spipe/main.c
index 960f421..e55304b 100644
--- a/spipe/main.c
+++ b/spipe/main.c
@@ -32,7 +32,7 @@ usage(void)
{
fprintf(stderr, "usage: spipe -t <target socket> -k <key file>"
- " [-fj] [-o <connection timeout>]\n");
+ " [-{f | g}j] [-o <connection timeout>]\n");
exit(1);
}
@@ -47,6 +47,7 @@ main(int argc, char * argv[])
{
/* Command-line parameters. */
int opt_f = 0;
+ int opt_g = 0;
int opt_j = 0;
const char * opt_k = NULL;
double opt_o = 0.0;
@@ -61,13 +62,18 @@ main(int argc, char * argv[])
WARNP_INIT;
/* Parse the command line. */
- while ((ch = getopt(argc, argv, "fjk:o:t:")) != -1) {
+ while ((ch = getopt(argc, argv, "fgjk:o:t:")) != -1) {
switch (ch) {
case 'f':
- if (opt_f)
+ if (opt_f || opt_g)
usage();
opt_f = 1;
break;
+ case 'g':
+ if (opt_f || opt_g)
+ usage();
+ opt_g = 1;
+ break;
case 'j':
if (opt_j)
usage();
@@ -142,7 +148,7 @@ main(int argc, char * argv[])
}
/* Set up a connection. */
- if (proto_conn_create(s[1], sas_t, 0, opt_f, opt_j, K, opt_o,
+ if (proto_conn_create(s[1], sas_t, 0, opt_f, opt_g, opt_j, K, opt_o,
callback_conndied, NULL)) {
warnp("Could not set up connection");
exit(1);
diff --git a/spipe/spipe.1 b/spipe/spipe.1
index 5086d6b..f6708a5 100644
--- a/spipe/spipe.1
+++ b/spipe/spipe.1
@@ -28,7 +28,7 @@ spipe \- spiped client utility
.B spipe
\-t <target socket>
\-k <key file>
-[\-fj]
+[\-{f | g}j]
[\-o <connection timeout>]
.SH OPTIONS
.TP
@@ -43,6 +43,10 @@ Use fast/weak handshaking: This reduces the CPU
time spent in the
initial connection setup, at the expense of losing perfect forward
secrecy.
.TP
+.B \-g
+Require perfect forward secrecy by dropping connections if the other
+host is using the -f option.
+.TP
.B \-j
Disable transport layer keep-alives.
(By default they are enabled.)
diff --git a/spiped/dispatch.c b/spiped/dispatch.c
index e21540b..8316298 100644
--- a/spiped/dispatch.c
+++ b/spiped/dispatch.c
@@ -20,6 +20,7 @@ struct accept_state {
double rtime;
int decr;
int nofps;
+ int requirefps;
int nokeepalive;
const struct proto_secret * K;
size_t nconn;
@@ -124,8 +125,8 @@ callback_gotconn(void * cookie, int s)
goto err1;
/* Create a new connection. */
- if (proto_conn_create(s, sas, A->decr, A->nofps, A->nokeepalive,
- A->K, A->timeo, callback_conndied, A)) {
+ if (proto_conn_create(s, sas, A->decr, A->nofps, A->requirefps,
+ A->nokeepalive, A->K, A->timeo, callback_conndied, A)) {
warnp("Failure setting up new connection");
goto err2;
}
@@ -148,22 +149,23 @@ err0:
}
/**
- * dispatch_accept(s, tgt, rtime, sas, decr, nofps, nokeepalive, K, nconn_max,
- * timeo):
+ * dispatch_accept(s, tgt, rtime, sas, decr, nofps, requirefps, nokeepalive, K,
+ * nconn_max, timeo):
* Start accepting connections on the socket ${s}. Connect to the target
* ${tgt}, re-resolving it every ${rtime} seconds if ${rtime} > 0; on address
* resolution failure use the most recent successfully obtained addresses, or
* the addresses ${sas}. If ${decr} is 0, encrypt the outgoing connections; if
* ${decr} is non-zero, decrypt the incoming connections. Don't accept more
* than ${nconn_max} connections. If ${nofps} is non-zero, don't use perfect
- * forward secrecy. Enable transport layer keep-alives (if applicable) if and
- * only if ${nokeepalive} is zero. Drop connections if the handshake or
+ * forward secrecy. if {$requirefps} is non-zero, require that both ends use
+ * perfect forward secrecy. Enable transport layer keep-alives (if applicable)
+ * if and only if ${nokeepalive} is zero. Drop connections if the handshake or
* connecting to the target takes more than ${timeo} seconds.
*/
int
dispatch_accept(int s, const char * tgt, double rtime, struct sock_addr ** sas,
- int decr, int nofps, int nokeepalive, const struct proto_secret * K,
- size_t nconn_max, double timeo)
+ int decr, int nofps, int requirefps, int nokeepalive,
+ const struct proto_secret * K, size_t nconn_max, double timeo)
{
struct accept_state * A;
@@ -176,6 +178,7 @@ dispatch_accept(int s, const char * tgt, double
rtime, struct sock_addr ** sas,
A->rtime = rtime;
A->decr = decr;
A->nofps = nofps;
+ A->requirefps = requirefps;
A->nokeepalive = nokeepalive;
A->K = K;
A->nconn = 0;
diff --git a/spiped/dispatch.h b/spiped/dispatch.h
index e2a1680..4bd8d04 100644
--- a/spiped/dispatch.h
+++ b/spiped/dispatch.h
@@ -8,19 +8,20 @@ struct proto_secret;
struct sock_addr;
/**
- * dispatch_accept(s, tgt, rtime, sas, decr, nofps, nokeepalive, K, nconn_max,
- * timeo):
+ * dispatch_accept(s, tgt, rtime, sas, decr, nofps, requirefps, nokeepalive, K,
+ * nconn_max, timeo):
* Start accepting connections on the socket ${s}. Connect to the target
* ${tgt}, re-resolving it every ${rtime} seconds if ${rtime} > 0; on address
* resolution failure use the most recent successfully obtained addresses, or
* the addresses ${sas}. If ${decr} is 0, encrypt the outgoing connections; if
* ${decr} is non-zero, decrypt the incoming connections. Don't accept more
* than ${nconn_max} connections. If ${nofps} is non-zero, don't use perfect
- * forward secrecy. Enable transport layer keep-alives (if applicable) if and
- * only if ${nokeepalive} is zero. Drop connections if the handshake or
+ * forward secrecy. if {$requirefps} is non-zero, require that both ends use
+ * perfect forward secrecy. Enable transport layer keep-alives (if applicable)
+ * if and only if ${nokeepalive} is zero. Drop connections if the handshake or
* connecting to the target takes more than ${timeo} seconds.
*/
int dispatch_accept(int, const char *, double, struct sock_addr **, int, int,
- int, const struct proto_secret *, size_t, double);
+ int, int, const struct proto_secret *, size_t, double);
#endif /* !_DISPATCH_H_ */
diff --git a/spiped/main.c b/spiped/main.c
index 3cbf51c..070805d 100644
--- a/spiped/main.c
+++ b/spiped/main.c
@@ -19,7 +19,7 @@ usage(void)
fprintf(stderr, "usage: spiped {-e | -d} -s <source socket> "
"-t <target socket> -k <key file>\n"
- " [-DfFj] [-n <max # connections>] "
+ " [-D{f | g}Fj] [-n <max # connections>] "
"[-o <connection timeout>] [-p <pidfile>]\n"
" [{-r <rtime> | -R}]\n");
exit(1);
@@ -39,6 +39,7 @@ main(int argc, char * argv[])
int opt_D = 0;
int opt_e = 0;
int opt_f = 0;
+ int opt_g = 0;
int opt_F = 0;
int opt_j = 0;
const char * opt_k = NULL;
@@ -60,7 +61,7 @@ main(int argc, char * argv[])
WARNP_INIT;
/* Parse the command line. */
- while ((ch = getopt(argc, argv, "dDefFjk:n:o:r:Rp:s:t:")) != -1) {
+ while ((ch = getopt(argc, argv, "dDefFgjk:n:o:r:Rp:s:t:")) != -1) {
switch (ch) {
case 'd':
if (opt_d || opt_e)
@@ -78,7 +79,7 @@ main(int argc, char * argv[])
opt_e = 1;
break;
case 'f':
- if (opt_f)
+ if (opt_f || opt_g)
usage();
opt_f = 1;
break;
@@ -87,6 +88,11 @@ main(int argc, char * argv[])
usage();
opt_F = 1;
break;
+ case 'g':
+ if (opt_f || opt_g)
+ usage();
+ opt_g = 1;
+ break;
case 'j':
if (opt_j)
usage();
@@ -238,7 +244,7 @@ main(int argc, char * argv[])
/* Start accepting connections. */
if (dispatch_accept(s, opt_t, opt_R ? 0.0 : opt_r, sas_t, opt_d,
- opt_f, opt_j, K, opt_n, opt_o)) {
+ opt_f, opt_g, opt_j, K, opt_n, opt_o)) {
warnp("Failed to initialize connection acceptor");
exit(1);
}
diff --git a/spiped/spiped.1 b/spiped/spiped.1
index 09d5abe..3fba263 100644
--- a/spiped/spiped.1
+++ b/spiped/spiped.1
@@ -30,7 +30,7 @@ spiped \- secure pipe daemon
\-t <target socket>
\-k <key file>
.br
-[\-DfFj]
+[\-D{f | g}Fj]
[\-n <max # connections>]
[\-o <connection timeout>]
[\-p <pidfile>]
@@ -75,6 +75,10 @@ Use fast/weak handshaking: This reduces the CPU
time spent in the
initial connection setup, at the expense of losing perfect forward
secrecy.
.TP
+.B \-g
+Require perfect forward secrecy by dropping connections if the other
+host is using the -f option.
+.TP
.B \-F
Run in foreground. This can be useful with systems like daemontools.
.TP
