From 1513a2d07e28c969d940a0fdc704d4ec30533bf3 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Sat, 2 Jan 2021 22:47:03 +0100 Subject: [PATCH] hsmtool: input encryption password from stdin This slightly breaks the API, but still accept the input: we just don't take it into account anymore. For `dumponchaindescriptors`, we have to still take the old place of the `network` parameter into account to not entirely break the API. Changelog-Added: hsmtool: password must now be entered on stdin. Password passed on the command line are discarded. Signed-off-by: Antoine Poinsot --- tests/test_wallet.py | 48 ++++++++---- tools/Makefile | 2 +- tools/hsmtool.c | 172 +++++++++++++++++++++++++++++++------------ 3 files changed, 159 insertions(+), 63 deletions(-) diff --git a/tests/test_wallet.py b/tests/test_wallet.py index af9e05d3c824..36ce7b63121f 100644 --- a/tests/test_wallet.py +++ b/tests/test_wallet.py @@ -6,7 +6,7 @@ from pyln.client import RpcError, Millisatoshi from utils import ( only_one, wait_for, sync_blockheight, EXPERIMENTAL_FEATURES, - VALGRIND, check_coin_moves + VALGRIND, check_coin_moves, TailableProc ) import os @@ -985,6 +985,14 @@ def test_hsm_secret_encryption(node_factory): assert id == l1.rpc.getinfo()["id"] +class HsmTool(TailableProc): + """Helper for testing the hsmtool as a subprocess""" + def __init__(self, *args): + TailableProc.__init__(self) + assert hasattr(self, "env") + self.cmd_line = ["tools/hsmtool", *args] + + @unittest.skipIf(VALGRIND, "It does not play well with prompt and key derivation.") def test_hsmtool_secret_decryption(node_factory): l1 = node_factory.get_node() @@ -1004,13 +1012,20 @@ def test_hsmtool_secret_decryption(node_factory): l1.stop() # We can't use a wrong password ! - cmd_line = ["tools/hsmtool", "decrypt", hsm_path, "A wrong pass"] - with pytest.raises(subprocess.CalledProcessError): - subprocess.check_call(cmd_line) + hsmtool = HsmTool("decrypt", hsm_path) + hsmtool.start(stdin=slave_fd, + stdout=subprocess.PIPE, stderr=subprocess.PIPE) + hsmtool.wait_for_log(r"Enter hsm_secret password:") + os.write(master_fd, "A wrong pass\n\n".encode("utf-8")) + hsmtool.proc.wait(5) + hsmtool.is_in_log(r"Wrong password") # Decrypt it with hsmtool - cmd_line[3] = password[:-1] - subprocess.check_call(cmd_line) + hsmtool.start(stdin=slave_fd, + stdout=subprocess.PIPE, stderr=subprocess.PIPE) + hsmtool.wait_for_log(r"Enter hsm_secret password:") + os.write(master_fd, password.encode("utf-8")) + assert hsmtool.proc.wait(5) == 0 # Then test we can now start it without password l1.daemon.opts.pop("encrypted-hsm") l1.daemon.start(stdin=slave_fd, wait_for_initialized=True) @@ -1018,11 +1033,14 @@ def test_hsmtool_secret_decryption(node_factory): l1.stop() # Test we can encrypt it offline - cmd_line[1] = "encrypt" - subprocess.check_call(cmd_line) + hsmtool = HsmTool("encrypt", hsm_path) + hsmtool.start(stdin=slave_fd, + stdout=subprocess.PIPE, stderr=subprocess.PIPE) + hsmtool.wait_for_log(r"Enter hsm_secret password:") + os.write(master_fd, password.encode("utf-8")) + assert hsmtool.proc.wait(5) == 0 # Now we need to pass the encrypted-hsm startup option l1.stop() - with pytest.raises(subprocess.CalledProcessError, match=r'returned non-zero exit status 1'): subprocess.check_call(l1.daemon.cmd_line) @@ -1034,12 +1052,17 @@ def test_hsmtool_secret_decryption(node_factory): l1.daemon.wait_for_log(r'The hsm_secret is encrypted') os.write(master_fd, password.encode("utf-8")) l1.daemon.wait_for_log("Server started with public key") + print(node_id, l1.rpc.getinfo()["id"]) assert node_id == l1.rpc.getinfo()["id"] l1.stop() # And finally test that we can also decrypt if encrypted with hsmtool - cmd_line[1] = "decrypt" - subprocess.check_call(cmd_line) + hsmtool = HsmTool("decrypt", hsm_path) + hsmtool.start(stdin=slave_fd, + stdout=subprocess.PIPE, stderr=subprocess.PIPE) + hsmtool.wait_for_log(r"Enter hsm_secret password:") + os.write(master_fd, password.encode("utf-8")) + assert hsmtool.proc.wait(5) == 0 l1.daemon.opts.pop("encrypted-hsm") l1.daemon.start(stdin=slave_fd, wait_for_initialized=True) assert node_id == l1.rpc.getinfo()["id"] @@ -1052,8 +1075,7 @@ def test_hsmtool_dump_descriptors(node_factory, bitcoind): # Get a tpub descriptor of lightningd's wallet hsm_path = os.path.join(l1.daemon.lightning_dir, TEST_NETWORK, "hsm_secret") - cmd_line = ["tools/hsmtool", "dumponchaindescriptors", hsm_path, "", - "testnet"] + cmd_line = ["tools/hsmtool", "dumponchaindescriptors", hsm_path, "testnet"] out = subprocess.check_output(cmd_line).decode("utf8").split("\n") descriptor = [l for l in out if l.startswith("wpkh(tpub")][0] diff --git a/tools/Makefile b/tools/Makefile index 125377f5d2c1..d4c5a4fc18e9 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/derive_basepoints.o common/descriptor_checksum.o common/node_id.o common/type_to_string.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/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 a95d544ca99c..cc82d17e39ef 100644 --- a/tools/hsmtool.c +++ b/tools/hsmtool.c @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include @@ -29,20 +30,20 @@ #define ERROR_LIBWALLY 4 #define ERROR_KEYDERIV 5 #define ERROR_LANG_NOT_SUPPORTED 6 +#define ERROR_TERM 7 static void show_usage(const char *progname) { printf("%s [arguments]\n", progname); printf("methods:\n"); - printf(" - decrypt \n"); - printf(" - encrypt \n"); + printf(" - decrypt \n"); + printf(" - encrypt \n"); printf(" - dumpcommitments " - " [hsm_secret password]\n"); + "\n"); printf(" - guesstoremote " - " [hsm_secret password]\n"); - printf(" - generatehsm \n"); - printf(" - dumponchaindescriptors [password] " - "[network]\n"); + "\n"); + printf(" - generatehsm \n"); + printf(" - dumponchaindescriptors [network]\n"); exit(0); } @@ -157,13 +158,57 @@ static void get_channel_seed(struct secret *channel_seed, struct node_id *peer_i info, strlen(info)); } -static int decrypt_hsm(const char *hsm_secret_path, const char *passwd) +/* We detect an encrypted hsm_secret as a hsm_secret which is larger than + * the plaintext seed. */ +static bool hsm_secret_is_encrypted(const char *hsm_secret_path) { - int fd; struct stat st; + if (stat(hsm_secret_path, &st) != 0) + errx(ERROR_HSM_FILE, "Could not stat hsm_secret"); + return st.st_size > 32; +} + +/* Read a pass from stdin, disabling echoing as done in lightning/options for the + * --encrypted-hsm startup option. + * Caller must free the returned string. */ +static char *read_stdin_pass(void) +{ + struct termios current_term, temp_term; + char *passwd = NULL; + size_t passwd_size = 0; + + if (tcgetattr(fileno(stdin), ¤t_term) != 0) + errx(ERROR_TERM, "Could not get current terminal options."); + temp_term = current_term; + temp_term.c_lflag &= ~ECHO; + if (tcsetattr(fileno(stdin), TCSAFLUSH, &temp_term) != 0) + errx(ERROR_TERM, "Could not disable pass echoing."); + /* If we don't flush we might end up being buffered and we might seem + * to hang while we wait for the password. */ + fflush(stdout); + if (getline(&passwd, &passwd_size, stdin) < 0) + errx(ERROR_TERM, "Could not read pass from stdin."); + if (passwd[strlen(passwd) - 1] == '\n') + passwd[strlen(passwd) - 1] = '\0'; + if (tcsetattr(fileno(stdin), TCSAFLUSH, ¤t_term) != 0) + errx(ERROR_TERM, "Could not restore terminal options."); + + return passwd; +} + +static int decrypt_hsm(const char *hsm_secret_path) +{ + int fd; struct secret hsm_secret; + char *passwd; const char *dir, *backup; + /* This checks the file existence, too. */ + if (!hsm_secret_is_encrypted(hsm_secret_path)) + errx(ERROR_USAGE, "hsm_secret is not encrypted"); + printf("Enter hsm_secret password:\n"); + passwd = read_stdin_pass(); + if (sodium_init() == -1) err(ERROR_LIBSODIUM, "Could not initialize libsodium. Not enough entropy ?"); @@ -171,11 +216,10 @@ static int decrypt_hsm(const char *hsm_secret_path, const char *passwd) dir = path_dirname(NULL, hsm_secret_path); backup = path_join(dir, dir, "hsm_secret.backup"); - if (stat(hsm_secret_path, &st) != 0) - err(ERROR_HSM_FILE, "Could not stat hsm_secret"); - if (st.st_size <= 32) - err(ERROR_HSM_FILE, "hsm_secret is not encrypted"); get_encrypted_hsm_secret(&hsm_secret, hsm_secret_path, passwd); + /* Once the encryption key derived, we don't need it anymore. */ + if (passwd) + free(passwd); /* Create a backup file, "just in case". */ rename(hsm_secret_path, backup); @@ -205,11 +249,11 @@ static int decrypt_hsm(const char *hsm_secret_path, const char *passwd) return 0; } -static int encrypt_hsm(const char *hsm_secret_path, const char *passwd) +static int encrypt_hsm(const char *hsm_secret_path) { int fd; - struct stat st; struct secret key, hsm_secret; + char *passwd; u8 salt[16] = "c-lightning\0\0\0\0\0"; crypto_secretstream_xchacha20poly1305_state crypto_state; u8 header[crypto_secretstream_xchacha20poly1305_HEADERBYTES]; @@ -217,6 +261,15 @@ static int encrypt_hsm(const char *hsm_secret_path, const char *passwd) u8 cipher[sizeof(struct secret) + crypto_secretstream_xchacha20poly1305_ABYTES]; const char *dir, *backup; + /* This checks the file existence, too. */ + if (hsm_secret_is_encrypted(hsm_secret_path)) + errx(ERROR_USAGE, "hsm_secret is already encrypted"); + + printf("Enter hsm_secret password:\n"); + /* TODO: make the user double check the password. */ + passwd = read_stdin_pass(); + get_hsm_secret(&hsm_secret, hsm_secret_path); + dir = path_dirname(NULL, hsm_secret_path); backup = path_join(dir, dir, "hsm_secret.backup"); @@ -224,12 +277,6 @@ static int encrypt_hsm(const char *hsm_secret_path, const char *passwd) err(ERROR_LIBSODIUM, "Could not initialize libsodium. Not enough entropy ?"); - if (stat(hsm_secret_path, &st) != 0) - err(ERROR_HSM_FILE, "Could not stat hsm_secret"); - if (st.st_size > 32) - err(ERROR_USAGE, "hsm_secret is already encrypted"); - get_hsm_secret(&hsm_secret, 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, @@ -246,6 +293,10 @@ static int encrypt_hsm(const char *hsm_secret_path, const char *passwd) NULL, 0, 0) != 0) err(ERROR_LIBSODIUM, "Could not encrypt the seed."); + /* Once the encryption key derived, we don't need it anymore. */ + if (passwd) + free(passwd); + /* Create a backup file, "just in case". */ rename(hsm_secret_path, backup); fd = open(hsm_secret_path, O_CREAT|O_EXCL|O_WRONLY, 0400); @@ -276,19 +327,25 @@ static int encrypt_hsm(const char *hsm_secret_path, const char *passwd) } static int dump_commitments_infos(struct node_id *node_id, u64 channel_id, - u64 depth, char *hsm_secret_path, char *passwd) + u64 depth, char *hsm_secret_path) { struct sha256 shaseed; struct secret hsm_secret, channel_seed, per_commitment_secret; struct pubkey per_commitment_point; + char *passwd; secp256k1_ctx = secp256k1_context_create(SECP256K1_CONTEXT_VERIFY | SECP256K1_CONTEXT_SIGN); - if (passwd) + /* This checks the file existence, too. */ + if (hsm_secret_is_encrypted(hsm_secret_path)) { + printf("Enter hsm_secret password:\n"); + passwd = read_stdin_pass(); get_encrypted_hsm_secret(&hsm_secret, hsm_secret_path, passwd); - else + free(passwd); + } else get_hsm_secret(&hsm_secret, hsm_secret_path); + get_channel_seed(&channel_seed, node_id, channel_id, &hsm_secret); derive_shaseed(&channel_seed, &shaseed); @@ -325,9 +382,10 @@ static int dump_commitments_infos(struct node_id *node_id, u64 channel_id, * :param passwd: The *optional* hsm_secret password */ static int guess_to_remote(const char *address, struct node_id *node_id, - u64 tries, char *hsm_secret_path, char *passwd) + u64 tries, char *hsm_secret_path) { struct secret hsm_secret, channel_seed, basepoint_secret; + char *passwd; struct pubkey basepoint; struct ripemd160 pubkeyhash; /* We only support P2WPKH, hence 20. */ @@ -346,9 +404,13 @@ static int guess_to_remote(const char *address, struct node_id *node_id, secp256k1_ctx = secp256k1_context_create(SECP256K1_CONTEXT_VERIFY | SECP256K1_CONTEXT_SIGN); - if (passwd) + /* This checks the file existence, too. */ + if (hsm_secret_is_encrypted(hsm_secret_path)) { + printf("Enter hsm_secret password:\n"); + passwd = read_stdin_pass(); get_encrypted_hsm_secret(&hsm_secret, hsm_secret_path, passwd); - else + free(passwd); + } else get_hsm_secret(&hsm_secret, hsm_secret_path); for (u64 dbid = 1; dbid < tries ; dbid++) { @@ -517,10 +579,11 @@ static int generate_hsm(const char *hsm_secret_path) return 0; } -static int dumponchaindescriptors(const char *hsm_secret_path, const char *passwd, +static int dumponchaindescriptors(const char *hsm_secret_path, const char *old_passwd UNUSED, const bool is_testnet) { struct secret hsm_secret; + char *passwd; u8 bip32_seed[BIP32_ENTROPY_LEN_256]; u32 salt = 0; u32 version = is_testnet ? @@ -529,9 +592,13 @@ static int dumponchaindescriptors(const char *hsm_secret_path, const char *passw char *enc_xpub, *descriptor; struct descriptor_checksum checksum; - if (passwd) + /* This checks the file existence, too. */ + if (hsm_secret_is_encrypted(hsm_secret_path)) { + printf("Enter hsm_secret password:\n"); + passwd = read_stdin_pass(); get_encrypted_hsm_secret(&hsm_secret, hsm_secret_path, passwd); - else + free(passwd); + } else get_hsm_secret(&hsm_secret, hsm_secret_path); /* We use m/0/0/k as the derivation tree for onchain funds. */ @@ -582,37 +649,37 @@ int main(int argc, char *argv[]) show_usage(argv[0]); if (streq(method, "decrypt")) { - if (argc < 4) + if (argc < 3) show_usage(argv[0]); - return decrypt_hsm(argv[2], argv[3]); + return decrypt_hsm(argv[2]); } if (streq(method, "encrypt")) { - if (argc < 4) + if (argc < 3) show_usage(argv[0]); - return encrypt_hsm(argv[2], argv[3]); + return encrypt_hsm(argv[2]); } if (streq(method, "dumpcommitments")) { - /* node_id channel_id depth hsm_secret ?password? */ + /* node_id channel_id depth hsm_secret */ if (argc < 6) show_usage(argv[0]); struct node_id node_id; if (!node_id_from_hexstr(argv[2], strlen(argv[2]), &node_id)) err(ERROR_USAGE, "Bad node id"); return dump_commitments_infos(&node_id, atol(argv[3]), atol(argv[4]), - argv[5], argc >= 7 ? argv[6] : NULL); + argv[5]); } if (streq(method, "guesstoremote")) { - /* address node_id depth hsm_secret ?password? */ + /* address node_id depth hsm_secret */ if (argc < 6) show_usage(argv[0]); struct node_id node_id; if (!node_id_from_hexstr(argv[3], strlen(argv[3]), &node_id)) errx(ERROR_USAGE, "Bad node id"); return guess_to_remote(argv[2], &node_id, atol(argv[4]), - argv[5], argc >= 7 ? argv[6] : NULL); + argv[5]); } if (streq(method, "generatehsm")) { @@ -631,23 +698,30 @@ int main(int argc, char *argv[]) } if (streq(method, "dumponchaindescriptors")) { + char *net = NULL; bool is_testnet; + if (argc < 3) show_usage(argv[0]); - if (argc > 4) { - is_testnet = streq(argv[4], "testnet"); - if (!is_testnet && !streq(argv[4], "bitcoin")) - errx(ERROR_USAGE, "Network '%s' not supported." - " Supported networks: bitcoin (default)," - " testnet", - argv[4]); - } else + if (argc > 3) + net = argv[3]; + /* Previously, we accepted hsm_secret passwords on the command line. + * This shifted the location of the network parameter. + * TODO: remove this 3 releases after v0.9.3 */ + if (deprecated_apis && argc > 4) + net = argv[4]; + + if (streq(net, "testnet")) + is_testnet = true; + else if (!streq(net, "bitcoin")) + errx(ERROR_USAGE, "Network '%s' not supported." + " Supported networks: bitcoin (default)," + " testnet", net); + else is_testnet = false; - return dumponchaindescriptors(argv[2], - argc > 3 && !streq(argv[3], "") ? argv[3] : NULL, - is_testnet); + return dumponchaindescriptors(argv[2], NULL, is_testnet); } show_usage(argv[0]);