channeld: use pointer for shared secret.

It's more natural than using a zero-secret when something goes wrong.

Also note that the HSM will actually kill the connection if the ECDH
fails, which is fortunately statistically unlikely.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

channeld/channeld.c

@@ -527,29 +527,26 @@ static void handle_peer_announcement_signatures(struct peer *peer, const u8 *msg
 	channel_announcement_negotiate(peer);
 }
 
-static bool get_shared_secret(const struct htlc *htlc,
+static struct secret *get_shared_secret(const tal_t *ctx,
-			      struct secret *shared_secret)
+					const struct htlc *htlc)
 {
 	struct pubkey ephemeral;
 	struct onionpacket *op;
+	struct secret *secret = tal(ctx, struct secret);
 	const u8 *msg;
 
 	/* We unwrap the onion now. */
 	op = parse_onionpacket(tmpctx, htlc->routing, TOTAL_PACKET_SIZE);
-	if (!op) {
+	if (!op)
-		/* Return an invalid shared secret. */
+		return tal_free(secret);
-		memset(shared_secret, 0, sizeof(*shared_secret));
-		return false;
-	}
 
 	/* Because wire takes struct pubkey. */
 	ephemeral.pubkey = op->ephemeralkey;
 	msg = hsm_req(tmpctx, towire_hsm_ecdh_req(tmpctx, &ephemeral));
-	if (!fromwire_hsm_ecdh_resp(msg, shared_secret))
+	if (!fromwire_hsm_ecdh_resp(msg, secret))
 		status_failed(STATUS_FAIL_HSM_IO, "Reading ecdh response");
 
-	/* Gives all-zero shares_secret if it was invalid. */
+	return secret;
-	return !memeqzero(shared_secret, sizeof(*shared_secret));
 }
 
 static void handle_peer_add_htlc(struct peer *peer, const u8 *msg)
@@ -581,8 +578,7 @@ static void handle_peer_add_htlc(struct peer *peer, const u8 *msg)
 
 	/* If this is wrong, we don't complain yet; when it's confirmed we'll
 	 * send it to the master which handles all HTLC failures. */
-	htlc->shared_secret = tal(htlc, struct secret);
+	htlc->shared_secret = get_shared_secret(htlc, htlc);
-	get_shared_secret(htlc, htlc->shared_secret);
 }
 
 static void handle_peer_feechange(struct peer *peer, const u8 *msg)
@@ -1215,6 +1211,11 @@ static u8 *got_commitsig_msg(const tal_t *ctx,
 			memcpy(a->onion_routing_packet,
 			       htlc->routing,
 			       sizeof(a->onion_routing_packet));
+			/* Invalid shared secret gets set to all-zero: our
+			 * code generator can't make arrays of optional values */
+			if (!htlc->shared_secret)
+				memset(s, 0, sizeof(*s));
+			else
 				*s = *htlc->shared_secret;
 		} else if (htlc->state == RCVD_REMOVE_COMMIT) {
 			if (htlc->r) {
@@ -2589,8 +2590,7 @@ static void init_shared_secrets(struct channel *channel,
 			continue;
 
 		htlc = channel_get_htlc(channel, REMOTE, htlcs[i].id);
-		htlc->shared_secret = tal(htlc, struct secret);
+		htlc->shared_secret = get_shared_secret(htlc, htlc);
-		get_shared_secret(htlc, htlc->shared_secret);
 	}
 }
 

connectd/connectd.c

@@ -1445,18 +1445,24 @@ static struct io_plan *recv_req(struct io_conn *conn,
  * knowledge of the HSM, but also at one stage I made a hacky gossip vampire
  * tool which used the handshake code, so it's nice to keep that
  * standalone. */
-bool hsm_do_ecdh(struct secret *ss, const struct pubkey *point)
+struct secret *hsm_do_ecdh(const tal_t *ctx, const struct pubkey *point)
 {
 	u8 *req = towire_hsm_ecdh_req(tmpctx, point), *resp;
+	struct secret *secret = tal(ctx, struct secret);
 
 	if (!wire_sync_write(HSM_FD, req))
-		return false;
+		return tal_free(secret);
 	resp = wire_sync_read(req, HSM_FD);
 	if (!resp)
-		return false;
+		return tal_free(secret);
-	if (!fromwire_hsm_ecdh_resp(resp, ss))
+
-		return false;
+	/* Note: hsmd will actually hang up on us if it can't ECDH: that implies
-	return true;
+	 * that our node private key is invalid, and we shouldn't have made
+	 * it this far.  */
+	if (!fromwire_hsm_ecdh_resp(resp, secret))
+		return tal_free(secret);
+
+	return secret;
 }
 
 /*~ UNUSED is defined to an __attribute__ for GCC; at one stage we tried to use

connectd/handshake.c

@@ -159,7 +159,7 @@ struct handshake {
 	struct secret temp_k;
 	struct sha256 h;
 	struct keypair e;
-	struct secret ss;
+	struct secret *ss;
 
 	/* Used between the Acts */
 	struct pubkey re;
@@ -473,10 +473,11 @@ static struct io_plan *act_three_initiator(struct io_conn *conn,
 	 *     * where `re` is the ephemeral public key of the responder
 	 *
 	 */
-	if (!hsm_do_ecdh(&h->ss, &h->re))
+	h->ss = hsm_do_ecdh(h, &h->re);
+	if (!h->ss)
 		return handshake_failed(conn, h);
 
-	SUPERVERBOSE("# ss=0x%s", tal_hexstr(tmpctx, &h->ss, sizeof(h->ss)));
+	SUPERVERBOSE("# ss=0x%s", tal_hexstr(tmpctx, h->ss, sizeof(*h->ss)));
 
 	/* BOLT #8:
 	 *
@@ -484,7 +485,7 @@ static struct io_plan *act_three_initiator(struct io_conn *conn,
 	 *     * The final intermediate shared secret is mixed into the running
 	 *       chaining key.
 	 */
-	hkdf_two_keys(&h->ck, &h->temp_k, &h->ck, &h->ss, sizeof(h->ss));
+	hkdf_two_keys(&h->ck, &h->temp_k, &h->ck, h->ss, sizeof(*h->ss));
 	SUPERVERBOSE("# ck,temp_k3=0x%s,0x%s",
 		     tal_hexstr(tmpctx, &h->ck, sizeof(h->ck)),
 		     tal_hexstr(tmpctx, &h->temp_k, sizeof(h->temp_k)));
@@ -549,11 +550,11 @@ static struct io_plan *act_two_initiator2(struct io_conn *conn,
 	 * 5. `ss = ECDH(re, e.priv)`
 	 *    * where `re` is the responder's ephemeral public key
 	 */
-	if (!secp256k1_ecdh(secp256k1_ctx, h->ss.data, &h->re.pubkey,
+	if (!secp256k1_ecdh(secp256k1_ctx, h->ss->data, &h->re.pubkey,
 			    h->e.priv.secret.data))
 		return handshake_failed(conn, h);
 
-	SUPERVERBOSE("# ss=0x%s", tal_hexstr(tmpctx, &h->ss, sizeof(h->ss)));
+	SUPERVERBOSE("# ss=0x%s", tal_hexstr(tmpctx, h->ss, sizeof(*h->ss)));
 
 	/* BOLT #8:
 	 *
@@ -561,7 +562,7 @@ static struct io_plan *act_two_initiator2(struct io_conn *conn,
 	 *     * A new temporary encryption key is generated, which is
 	 *       used to generate the authenticating MAC.
 	 */
-	hkdf_two_keys(&h->ck, &h->temp_k, &h->ck, &h->ss, sizeof(h->ss));
+	hkdf_two_keys(&h->ck, &h->temp_k, &h->ck, h->ss, sizeof(*h->ss));
 	SUPERVERBOSE("# ck,temp_k2=0x%s,0x%s",
 		     tal_hexstr(tmpctx, &h->ck, sizeof(h->ck)),
 		     tal_hexstr(tmpctx, &h->temp_k, sizeof(h->temp_k)));
@@ -639,11 +640,12 @@ static struct io_plan *act_one_initiator(struct io_conn *conn,
 	 *     * The initiator performs an ECDH between its newly generated
 	 *       ephemeral key and the remote node's static public key.
 	 */
-	if (!secp256k1_ecdh(secp256k1_ctx, h->ss.data,
+	h->ss = tal(h, struct secret);
+	if (!secp256k1_ecdh(secp256k1_ctx, h->ss->data,
 			    &h->their_id.pubkey, h->e.priv.secret.data))
 		return handshake_failed(conn, h);
 
-	SUPERVERBOSE("# ss=0x%s", tal_hexstr(tmpctx, h->ss.data, sizeof(h->ss.data)));
+	SUPERVERBOSE("# ss=0x%s", tal_hexstr(tmpctx, h->ss->data, sizeof(h->ss->data)));
 
 	/* BOLT #8:
 	 *
@@ -651,7 +653,7 @@ static struct io_plan *act_one_initiator(struct io_conn *conn,
 	 *     * A new temporary encryption key is generated, which is
 	 *       used to generate the authenticating MAC.
 	 */
-	hkdf_two_keys(&h->ck, &h->temp_k, &h->ck, &h->ss, sizeof(h->ss));
+	hkdf_two_keys(&h->ck, &h->temp_k, &h->ck, h->ss, sizeof(*h->ss));
 	SUPERVERBOSE("# ck,temp_k1=0x%s,0x%s",
 		     tal_hexstr(tmpctx, &h->ck, sizeof(h->ck)),
 		     tal_hexstr(tmpctx, &h->temp_k, sizeof(h->temp_k)));
@@ -740,16 +742,16 @@ static struct io_plan *act_three_responder2(struct io_conn *conn,
 	 * 6. `ss = ECDH(rs, e.priv)`
 	 *      * where `e` is the responder's original ephemeral key
 	 */
-	if (!secp256k1_ecdh(secp256k1_ctx, h->ss.data, &h->their_id.pubkey,
+	if (!secp256k1_ecdh(secp256k1_ctx, h->ss->data, &h->their_id.pubkey,
 			    h->e.priv.secret.data))
 		return handshake_failed(conn, h);
 
-	SUPERVERBOSE("# ss=0x%s", tal_hexstr(tmpctx, &h->ss, sizeof(h->ss)));
+	SUPERVERBOSE("# ss=0x%s", tal_hexstr(tmpctx, h->ss, sizeof(*h->ss)));
 
 	/* BOLT #8:
 	 * 7. `ck, temp_k3 = HKDF(ck, ss)`
 	 */
-	hkdf_two_keys(&h->ck, &h->temp_k, &h->ck, &h->ss, sizeof(h->ss));
+	hkdf_two_keys(&h->ck, &h->temp_k, &h->ck, h->ss, sizeof(*h->ss));
 	SUPERVERBOSE("# ck,temp_k3=0x%s,0x%s",
 		     tal_hexstr(tmpctx, &h->ck, sizeof(h->ck)),
 		     tal_hexstr(tmpctx, &h->temp_k, sizeof(h->temp_k)));
@@ -815,10 +817,10 @@ static struct io_plan *act_two_responder(struct io_conn *conn,
 	 *      * where `re` is the ephemeral key of the initiator, which was
 	 *        received during Act One
 	 */
-	if (!secp256k1_ecdh(secp256k1_ctx, h->ss.data, &h->re.pubkey,
+	if (!secp256k1_ecdh(secp256k1_ctx, h->ss->data, &h->re.pubkey,
 			    h->e.priv.secret.data))
 		return handshake_failed(conn, h);
-	SUPERVERBOSE("# ss=0x%s", tal_hexstr(tmpctx, &h->ss, sizeof(h->ss)));
+	SUPERVERBOSE("# ss=0x%s", tal_hexstr(tmpctx, h->ss, sizeof(*h->ss)));
 
 	/* BOLT #8:
 	 *
@@ -826,7 +828,7 @@ static struct io_plan *act_two_responder(struct io_conn *conn,
 	 *     * A new temporary encryption key is generated, which is
 	 *       used to generate the authenticating MAC.
 	 */
-	hkdf_two_keys(&h->ck, &h->temp_k, &h->ck, &h->ss, sizeof(h->ss));
+	hkdf_two_keys(&h->ck, &h->temp_k, &h->ck, h->ss, sizeof(*h->ss));
 	SUPERVERBOSE("# ck,temp_k2=0x%s,0x%s",
 		     tal_hexstr(tmpctx, &h->ck, sizeof(h->ck)),
 		     tal_hexstr(tmpctx, &h->temp_k, sizeof(h->temp_k)));
@@ -902,10 +904,11 @@ static struct io_plan *act_one_responder2(struct io_conn *conn,
 	 *    * The responder performs an ECDH between its static private key and
 	 *      the initiator's ephemeral public key.
 	 */
-	if (!hsm_do_ecdh(&h->ss, &h->re))
+	h->ss = hsm_do_ecdh(h, &h->re);
+	if (!h->ss)
 		return handshake_failed(conn, h);
 
-	SUPERVERBOSE("# ss=0x%s", tal_hexstr(tmpctx, &h->ss, sizeof(h->ss)));
+	SUPERVERBOSE("# ss=0x%s", tal_hexstr(tmpctx, h->ss, sizeof(*h->ss)));
 
 	/* BOLT #8:
 	 *
@@ -913,7 +916,7 @@ static struct io_plan *act_one_responder2(struct io_conn *conn,
 	 *    * A new temporary encryption key is generated, which will
 	 *      shortly be used to check the authenticating MAC.
 	 */
-	hkdf_two_keys(&h->ck, &h->temp_k, &h->ck, &h->ss, sizeof(h->ss));
+	hkdf_two_keys(&h->ck, &h->temp_k, &h->ck, h->ss, sizeof(*h->ss));
 	SUPERVERBOSE("# ck,temp_k1=0x%s,0x%s",
 		     tal_hexstr(tmpctx, &h->ck, sizeof(h->ck)),
 		     tal_hexstr(tmpctx, &h->temp_k, sizeof(h->temp_k)));

connectd/handshake.h

@@ -1,6 +1,7 @@
 #ifndef LIGHTNING_CONNECTD_HANDSHAKE_H
 #define LIGHTNING_CONNECTD_HANDSHAKE_H
 #include "config.h"
+#include <ccan/tal/tal.h>
 #include <ccan/typesafe_cb/typesafe_cb.h>
 
 struct crypto_state;
@@ -52,5 +53,5 @@ struct io_plan *responder_handshake_(struct io_conn *conn,
 				     void *cbarg);
 
 /* helper which is defined in connect.c */
-bool hsm_do_ecdh(struct secret *ss, const struct pubkey *point);
+struct secret *hsm_do_ecdh(const tal_t *ctx, const struct pubkey *point);
 #endif /* LIGHTNING_CONNECTD_HANDSHAKE_H */

connectd/test/run-initiator-success.c

@@ -190,10 +190,13 @@ static struct io_plan *success(struct io_conn *conn UNUSED,
 	exit(0);
 }
 
-bool hsm_do_ecdh(struct secret *ss, const struct pubkey *point)
+struct secret *hsm_do_ecdh(const tal_t *ctx, const struct pubkey *point)
 {
-	return secp256k1_ecdh(secp256k1_ctx, ss->data, &point->pubkey,
+	struct secret *ss = tal(ctx, struct secret);
-			      ls_priv.secret.data) == 1;
+	if (secp256k1_ecdh(secp256k1_ctx, ss->data, &point->pubkey,
+			   ls_priv.secret.data) != 1)
+		return tal_free(ss);
+	return ss;
 }
 
 int main(void)

connectd/test/run-responder-success.c

@@ -187,10 +187,13 @@ static struct io_plan *success(struct io_conn *conn UNUSED,
 	exit(0);
 }
 
-bool hsm_do_ecdh(struct secret *ss, const struct pubkey *point)
+struct secret *hsm_do_ecdh(const tal_t *ctx, const struct pubkey *point)
 {
-	return secp256k1_ecdh(secp256k1_ctx, ss->data, &point->pubkey,
+	struct secret *ss = tal(ctx, struct secret);
-			      ls_priv.secret.data) == 1;
+	if (secp256k1_ecdh(secp256k1_ctx, ss->data, &point->pubkey,
+			   ls_priv.secret.data) != 1)
+		return tal_free(ss);
+	return ss;
 }
 
 int main(void)

devtools/gossipwith.c

@@ -72,12 +72,13 @@ void peer_failed_connection_lost(void)
 	exit(0);
 }
 
-bool hsm_do_ecdh(struct secret *ss, const struct pubkey *point)
+struct secret *hsm_do_ecdh(const tal_t *ctx, const struct pubkey *point)
 {
+	struct secret *ss = tal(ctx, struct secret);
 	if (secp256k1_ecdh(secp256k1_ctx, ss->data, &point->pubkey,
 			   notsosecret.data) != 1)
-		errx(1, "ECDH failed");
+		return tal_free(ss);
-	return true;
+	return ss;
 }
 
 /* We don't want to discard *any* messages. */

hsmd/hsmd.c

@@ -554,8 +554,8 @@ static struct io_plan *handle_ecdh(struct io_conn *conn,
 	if (!fromwire_hsm_ecdh_req(msg_in, &point))
 		return bad_req(conn, c, msg_in);
 
-	/*~ We simply use the secp256k1_ecdh function, which really shouldn't
+	/*~ We simply use the secp256k1_ecdh function: if ss.data is invalid,
-	 * fail (iff the point is invalid). */
+	 * we kill them for bad randomness (~1 in 2^127 if ss.data is random) */
 	node_key(&privkey, NULL);
 	if (secp256k1_ecdh(secp256k1_ctx, ss.data, &point.pubkey,
 			   privkey.secret.data) != 1) {