From 917f78a4f823c56c1ee12616ef38190b80f2a787 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Sun, 3 Jan 2021 12:32:43 +0100 Subject: [PATCH] lightningd: group hsm_secret encryption key derivation This avoids duplication of both logic and error-prone values, such as the salt. Grouping all hsm encryption logic into a public API will also allow us to fuzz it. Signed-off-by: Antoine Poinsot --- common/Makefile | 1 + common/hsm_encryption.c | 38 +++++++++++++++++++++++++++ common/hsm_encryption.h | 22 ++++++++++++++++ hsmd/Makefile | 1 + hsmd/hsmd.c | 10 +++---- lightningd/Makefile | 1 + lightningd/lightningd.c | 3 ++- lightningd/options.c | 30 ++++++--------------- lightningd/test/run-find_my_abspath.c | 3 +++ tools/Makefile | 2 +- tools/hsmtool.c | 26 +++++++++--------- 11 files changed, 93 insertions(+), 44 deletions(-) create mode 100644 common/hsm_encryption.c create mode 100644 common/hsm_encryption.h diff --git a/common/Makefile b/common/Makefile index 8c2d6e87b..71dc75e91 100644 --- a/common/Makefile +++ b/common/Makefile @@ -32,6 +32,7 @@ COMMON_SRC_NOGEN := \ common/gossip_store.c \ common/hash_u5.c \ common/hmac.c \ + common/hsm_encryption.c \ common/htlc_state.c \ common/htlc_trim.c \ common/htlc_tx.c \ diff --git a/common/hsm_encryption.c b/common/hsm_encryption.c new file mode 100644 index 000000000..d936fad2a --- /dev/null +++ b/common/hsm_encryption.c @@ -0,0 +1,38 @@ +#include +#include +#include + + +char *hsm_secret_encryption_key(const char *pass, struct secret *key) +{ + u8 salt[16] = "c-lightning\0\0\0\0\0"; + + /* Don't swap the encryption key ! */ + if (sodium_mlock(key->data, sizeof(key->data)) != 0) + return "Could not lock hsm_secret encryption key memory."; + + /* Check bounds. */ + if (strlen(pass) < crypto_pwhash_argon2id_PASSWD_MIN) + return "Password too short to be able to derive a key from it."; + if (strlen(pass) > crypto_pwhash_argon2id_PASSWD_MAX) + return "Password too long to be able to derive a key from it."; + + /* Now derive the key. */ + if (crypto_pwhash(key->data, sizeof(key->data), pass, strlen(pass), salt, + /* INTERACTIVE needs 64 MiB of RAM, MODERATE needs 256, + * and SENSITIVE needs 1024. */ + crypto_pwhash_argon2id_OPSLIMIT_MODERATE, + crypto_pwhash_argon2id_MEMLIMIT_MODERATE, + crypto_pwhash_ALG_ARGON2ID13) != 0) + return "Could not derive a key from the password."; + + return NULL; +} + +void discard_key(struct secret *key TAKES) +{ + /* sodium_munlock() also zeroes the memory. */ + sodium_munlock(key->data, sizeof(key->data)); + if (taken(key)) + tal_free(key); +} diff --git a/common/hsm_encryption.h b/common/hsm_encryption.h new file mode 100644 index 000000000..5239ba31e --- /dev/null +++ b/common/hsm_encryption.h @@ -0,0 +1,22 @@ +#ifndef LIGHTNING_COMMON_HSM_ENCRYPTION_H +#define LIGHTNING_COMMON_HSM_ENCRYPTION_H +#include "config.h" +#include +#include +#include + + +/** Derive the hsm_secret encryption key from a passphrase. + * @pass: the passphrase string. + * @encryption_key: the output key derived from the passphrase. + * + * On success, NULL is returned. On error, a human-readable error is. + */ +char *hsm_secret_encryption_key(const char *pass, struct secret *encryption_key); + +/** Unlock and zeroize the encryption key memory after use. + * @key: the encryption key. If taken, it will be tal_free'd + */ +void discard_key(struct secret *key TAKES); + +#endif /* LIGHTNING_COMMON_HSM_ENCRYPTION_H */ diff --git a/hsmd/Makefile b/hsmd/Makefile index 397f732b1..dadbe280a 100644 --- a/hsmd/Makefile +++ b/hsmd/Makefile @@ -26,6 +26,7 @@ HSMD_COMMON_OBJS := \ common/derive_basepoints.o \ common/status_wiregen.o \ common/hash_u5.o \ + common/hsm_encryption.o \ common/key_derive.o \ common/memleak.o \ common/msg_queue.o \ diff --git a/hsmd/hsmd.c b/hsmd/hsmd.c index 0acd0bf53..d7dfd7bef 100644 --- a/hsmd/hsmd.c +++ b/hsmd/hsmd.c @@ -31,6 +31,7 @@ #include #include #include +#include #include #include #include @@ -779,12 +780,9 @@ static struct io_plan *init_hsm(struct io_conn *conn, maybe_create_new_hsm(hsm_encryption_key, true); load_hsm(hsm_encryption_key); - /*~ We don't need the hsm_secret encryption key anymore. - * Note that sodium_munlock() also zeroes the memory. */ - if (hsm_encryption_key) { - sodium_munlock(hsm_encryption_key, sizeof(*hsm_encryption_key)); - tal_free(hsm_encryption_key); - } + /*~ We don't need the hsm_secret encryption key anymore. */ + if (hsm_encryption_key) + discard_key(take(hsm_encryption_key)); /*~ We tell lightning our node id and (public) bip32 seed. */ node_key(NULL, &key); diff --git a/lightningd/Makefile b/lightningd/Makefile index 91ac15355..f9b176996 100644 --- a/lightningd/Makefile +++ b/lightningd/Makefile @@ -92,6 +92,7 @@ LIGHTNINGD_COMMON_OBJS := \ common/gossip_rcvd_filter.o \ common/hash_u5.o \ common/hmac.o \ + common/hsm_encryption.o \ common/htlc_state.o \ common/htlc_trim.o \ common/htlc_wire.o \ diff --git a/lightningd/lightningd.c b/lightningd/lightningd.c index 26f2694a9..c7f7f2e60 100644 --- a/lightningd/lightningd.c +++ b/lightningd/lightningd.c @@ -59,6 +59,7 @@ #include #include #include +#include #include #include #include @@ -878,7 +879,7 @@ int main(int argc, char *argv[]) /*~ If hsm_secret is encrypted, we don't need its encryption key * anymore. Note that sodium_munlock() also zeroes the memory.*/ if (ld->config.keypass) - sodium_munlock(ld->config.keypass->data, sizeof(ld->config.keypass->data)); + discard_key(take(ld->config.keypass)); /*~ Our default color and alias are derived from our node id, so we * can only set those now (if not set by config options). */ diff --git a/lightningd/options.c b/lightningd/options.c index 246e3d676..63afe2e7a 100644 --- a/lightningd/options.c +++ b/lightningd/options.c @@ -13,6 +13,7 @@ #include #include #include +#include #include #include #include @@ -388,16 +389,8 @@ static char *opt_important_plugin(const char *arg, struct lightningd *ld) static char *opt_set_hsm_password(struct lightningd *ld) { struct termios current_term, temp_term; - char *passwd = NULL, *passwd_confirmation = NULL; + char *passwd = NULL, *passwd_confirmation = NULL, *err; size_t passwd_size = 0; - u8 salt[16] = "c-lightning\0\0\0\0\0"; - ld->encrypted_hsm = true; - - ld->config.keypass = tal(NULL, struct secret); - /* Don't swap the encryption key ! */ - if (sodium_mlock(ld->config.keypass->data, - sizeof(ld->config.keypass->data)) != 0) - return "Could not lock hsm_secret encryption key memory."; /* Get the password from stdin, but don't echo it. */ if (tcgetattr(fileno(stdin), ¤t_term) != 0) @@ -426,21 +419,14 @@ static char *opt_set_hsm_password(struct lightningd *ld) return "Could not restore terminal options."; printf("\n"); - /* Derive the key from the password. */ - if (strlen(passwd) < crypto_pwhash_argon2id_PASSWD_MIN) - return "Password too short to be able to derive a key from it."; - if (strlen(passwd) > crypto_pwhash_argon2id_PASSWD_MAX) - return "Password too long to be able to derive a key from it."; - if (crypto_pwhash(ld->config.keypass->data, sizeof(ld->config.keypass->data), - passwd, strlen(passwd), salt, - /* INTERACTIVE needs 64 MiB of RAM, MODERATE needs 256, - * and SENSITIVE needs 1024. */ - crypto_pwhash_argon2id_OPSLIMIT_MODERATE, - crypto_pwhash_argon2id_MEMLIMIT_MODERATE, - crypto_pwhash_ALG_ARGON2ID13) != 0) - return "Could not derive a key from the password."; + ld->config.keypass = tal(NULL, struct secret); + err = hsm_secret_encryption_key(passwd, ld->config.keypass); + if (err) + return err; + ld->encrypted_hsm = true; free(passwd); free(passwd_confirmation); + return NULL; } diff --git a/lightningd/test/run-find_my_abspath.c b/lightningd/test/run-find_my_abspath.c index 142cbfb50..13a64342c 100644 --- a/lightningd/test/run-find_my_abspath.c +++ b/lightningd/test/run-find_my_abspath.c @@ -50,6 +50,9 @@ s64 db_get_intvar(struct db *db UNNEEDED, char *varname UNNEEDED, s64 defval UNN /* Generated stub for db_in_transaction */ bool db_in_transaction(struct db *db UNNEEDED) { fprintf(stderr, "db_in_transaction called!\n"); abort(); } +/* Generated stub for discard_key */ +void discard_key(struct secret *key TAKES UNNEEDED) +{ fprintf(stderr, "discard_key called!\n"); abort(); } /* Generated stub for ecdh_hsmd_setup */ void ecdh_hsmd_setup(int hsm_fd UNNEEDED, void (*failed)(enum status_failreason UNNEEDED, diff --git a/tools/Makefile b/tools/Makefile index d4c5a4fc1..b4dc87710 100644 --- a/tools/Makefile +++ b/tools/Makefile @@ -17,7 +17,7 @@ tools/headerversions: FORCE tools/headerversions.o $(CCAN_OBJS) tools/check-bolt: tools/check-bolt.o $(CCAN_OBJS) $(TOOLS_COMMON_OBJS) -tools/hsmtool: tools/hsmtool.o $(CCAN_OBJS) $(TOOLS_COMMON_OBJS) $(BITCOIN_OBJS) common/amount.o common/bech32.o common/bigsize.o common/configdir.o common/derive_basepoints.o common/descriptor_checksum.o common/node_id.o common/type_to_string.o common/version.o wire/fromwire.o wire/towire.o +tools/hsmtool: tools/hsmtool.o $(CCAN_OBJS) $(TOOLS_COMMON_OBJS) $(BITCOIN_OBJS) common/amount.o common/bech32.o common/bigsize.o common/configdir.o common/derive_basepoints.o common/descriptor_checksum.o common/hsm_encryption.o common/node_id.o common/type_to_string.o common/version.o wire/fromwire.o wire/towire.o tools/lightning-hsmtool: tools/hsmtool cp $< $@ diff --git a/tools/hsmtool.c b/tools/hsmtool.c index 66dfb6022..6bbab5a30 100644 --- a/tools/hsmtool.c +++ b/tools/hsmtool.c @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -91,7 +92,7 @@ static void get_encrypted_hsm_secret(struct secret *hsm_secret, { int fd; struct secret key; - u8 salt[16] = "c-lightning\0\0\0\0\0"; + char *err; crypto_secretstream_xchacha20poly1305_state crypto_state; u8 header[crypto_secretstream_xchacha20poly1305_HEADERBYTES]; /* The cipher size is static with xchacha20poly1305. */ @@ -106,14 +107,13 @@ static void get_encrypted_hsm_secret(struct secret *hsm_secret, if (!read_all(fd, cipher, sizeof(cipher))) errx(ERROR_HSM_FILE, "Could not read cipher body"); - if (crypto_pwhash(key.data, sizeof(key.data), passwd, strlen(passwd), salt, - crypto_pwhash_argon2id_OPSLIMIT_MODERATE, - crypto_pwhash_argon2id_MEMLIMIT_MODERATE, - crypto_pwhash_ALG_ARGON2ID13) != 0) - errx(ERROR_LIBSODIUM, "Could not derive a key from the password."); + err = hsm_secret_encryption_key(passwd, &key); + if (err) + errx(ERROR_LIBSODIUM, "%s", err); if (crypto_secretstream_xchacha20poly1305_init_pull(&crypto_state, header, - key.data) != 0) + key.data) != 0) errx(ERROR_LIBSODIUM, "Could not initialize the crypto state"); + discard_key(&key); if (crypto_secretstream_xchacha20poly1305_pull(&crypto_state, hsm_secret->data, NULL, 0, cipher, sizeof(cipher), NULL, 0) != 0) @@ -253,8 +253,7 @@ static int encrypt_hsm(const char *hsm_secret_path) { int fd; struct secret key, hsm_secret; - char *passwd, *passwd_confirmation; - u8 salt[16] = "c-lightning\0\0\0\0\0"; + char *passwd, *passwd_confirmation, *err; crypto_secretstream_xchacha20poly1305_state crypto_state; u8 header[crypto_secretstream_xchacha20poly1305_HEADERBYTES]; /* The cipher size is static with xchacha20poly1305. */ @@ -282,14 +281,13 @@ static int encrypt_hsm(const char *hsm_secret_path) /* Derive the encryption key from the password provided, and try to encrypt * the seed. */ - if (crypto_pwhash(key.data, sizeof(key.data), passwd, strlen(passwd), salt, - crypto_pwhash_argon2id_OPSLIMIT_MODERATE, - crypto_pwhash_argon2id_MEMLIMIT_MODERATE, - crypto_pwhash_ALG_ARGON2ID13) != 0) - errx(ERROR_LIBSODIUM, "Could not derive a key from the password."); + err = hsm_secret_encryption_key(passwd, &key); + if (err) + errx(ERROR_LIBSODIUM, "%s", err); if (crypto_secretstream_xchacha20poly1305_init_push(&crypto_state, header, key.data) != 0) errx(ERROR_LIBSODIUM, "Could not initialize the crypto state"); + discard_key(&key); if (crypto_secretstream_xchacha20poly1305_push(&crypto_state, cipher, NULL, hsm_secret.data, sizeof(hsm_secret.data),