From 66de6b84beb1adaa027686f7d2861e525e5613e8 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 8 Jan 2019 10:50:50 +1030 Subject: [PATCH] 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 --- channeld/channeld.c | 30 ++++++++++---------- connectd/connectd.c | 18 ++++++++---- connectd/handshake.c | 41 ++++++++++++++------------- connectd/handshake.h | 3 +- connectd/test/run-initiator-success.c | 9 ++++-- connectd/test/run-responder-success.c | 9 ++++-- devtools/gossipwith.c | 7 +++-- hsmd/hsmd.c | 4 +-- 8 files changed, 69 insertions(+), 52 deletions(-) diff --git a/channeld/channeld.c b/channeld/channeld.c index 04d784994..3730a6838 100644 --- a/channeld/channeld.c +++ b/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, - struct secret *shared_secret) +static struct secret *get_shared_secret(const tal_t *ctx, + 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) { - /* Return an invalid shared secret. */ - memset(shared_secret, 0, sizeof(*shared_secret)); - return false; - } + if (!op) + return tal_free(secret); /* 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 !memeqzero(shared_secret, sizeof(*shared_secret)); + return 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); - get_shared_secret(htlc, htlc->shared_secret); + htlc->shared_secret = get_shared_secret(htlc, htlc); } static void handle_peer_feechange(struct peer *peer, const u8 *msg) @@ -1215,7 +1211,12 @@ static u8 *got_commitsig_msg(const tal_t *ctx, memcpy(a->onion_routing_packet, htlc->routing, sizeof(a->onion_routing_packet)); - *s = *htlc->shared_secret; + /* 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) { struct fulfilled_htlc *f; @@ -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); - get_shared_secret(htlc, htlc->shared_secret); + htlc->shared_secret = get_shared_secret(htlc, htlc); } } diff --git a/connectd/connectd.c b/connectd/connectd.c index 97a554178..8ccb76df9 100644 --- a/connectd/connectd.c +++ b/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; - if (!fromwire_hsm_ecdh_resp(resp, ss)) - return false; - return true; + return tal_free(secret); + + /* Note: hsmd will actually hang up on us if it can't ECDH: that implies + * 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 diff --git a/connectd/handshake.c b/connectd/handshake.c index d861e2424..617e7054f 100644 --- a/connectd/handshake.c +++ b/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))); diff --git a/connectd/handshake.h b/connectd/handshake.h index 20c51ecb9..970782eac 100644 --- a/connectd/handshake.h +++ b/connectd/handshake.h @@ -1,6 +1,7 @@ #ifndef LIGHTNING_CONNECTD_HANDSHAKE_H #define LIGHTNING_CONNECTD_HANDSHAKE_H #include "config.h" +#include #include 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 */ diff --git a/connectd/test/run-initiator-success.c b/connectd/test/run-initiator-success.c index 917c73418..eddd1ea05 100644 --- a/connectd/test/run-initiator-success.c +++ b/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, - ls_priv.secret.data) == 1; + struct secret *ss = tal(ctx, struct secret); + if (secp256k1_ecdh(secp256k1_ctx, ss->data, &point->pubkey, + ls_priv.secret.data) != 1) + return tal_free(ss); + return ss; } int main(void) diff --git a/connectd/test/run-responder-success.c b/connectd/test/run-responder-success.c index 843535f7d..8f2f3c9fe 100644 --- a/connectd/test/run-responder-success.c +++ b/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, - ls_priv.secret.data) == 1; + struct secret *ss = tal(ctx, struct secret); + if (secp256k1_ecdh(secp256k1_ctx, ss->data, &point->pubkey, + ls_priv.secret.data) != 1) + return tal_free(ss); + return ss; } int main(void) diff --git a/devtools/gossipwith.c b/devtools/gossipwith.c index 58a3eb4a6..e4f2a9808 100644 --- a/devtools/gossipwith.c +++ b/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 true; + return tal_free(ss); + return ss; } /* We don't want to discard *any* messages. */ diff --git a/hsmd/hsmd.c b/hsmd/hsmd.c index ac8752e5a..65491d60e 100644 --- a/hsmd/hsmd.c +++ b/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 - * fail (iff the point is invalid). */ + /*~ We simply use the secp256k1_ecdh function: if ss.data 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) {