Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

hsm_secret generation from a seed-phrase #4065

Merged
merged 1 commit into from
Nov 9, 2020

Conversation

positiveblue
Copy link

Add generatehsm method to hsmtool to derivate BIP32 seeds from a
mnemonic using the BIP39 standard.

The new method uses libwally for the BIP39 to BIP32 derivation. It also
fails if an hsm_secret file already exists, so we do not overwrite
someone else's wallet without noticing.

It allows the use of passphrases, the ECHO mode in the terminal is
disable for higher security.

@positiveblue
Copy link
Author

Related to the issue created during the Lightning HackSprint May 2020 #3717

@ZmnSCPxj @darosior I guess that you are the right people for reviewing this

Copy link
Collaborator

@darosior darosior left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, will review soon (:tm:) but strong concept ack :)

Copy link
Collaborator

@darosior darosior left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a quick pass as Travis does not pass yet, but wanted to give it a look!

tools/hsmtool.c Outdated
Comment on lines 376 to 377
char word[50];
uint n_words;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can you declare those at the top of the function?

tools/hsmtool.c Outdated
char word[50];
uint n_words;
for (n_words = 0; n_words < 24; n_words++) {
scanf("%s", word);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not compile (-Werror=unused-result)

tools/hsmtool.c Outdated
Comment on lines 378 to 438
for (n_words = 0; n_words < 24; n_words++) {
scanf("%s", word);
/* We need to concat the strings using ' ' so libwally can
* validate it */
if (n_words == 0) {
strcpy(mnemonic, word);
} else {
strcat(mnemonic, " ");
strcat(mnemonic, word);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not taking an entire line instead ? You could use getline as we do today for the password (but to be explicit i think we should keep the echo for the mnemonics):

if (getline(&passwd, &passwd_size, stdin) < 0)
return "Could not read password from stdin.";

tools/hsmtool.c Outdated
Comment on lines 416 to 417
/* Replace the last character which will always be '\n' */
passphrase[strlen(passphrase) - 1] = '\0';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not always:

#include <inttypes.h>
#include <stdio.h>
#include <string.h>

int main(int argc, char **argv) {
	char *test = NULL;
	size_t len;

	getline(&test, &len, stdin);
	printf("%s\n", test[strlen(test) - 1] == '\n' ? "true" : "false");
}
darosior@darosior:~$ gcc a.c
darosior@darosior:~$ ./a.out 
test_pass
true
darosior@darosior:~$ printf passphrase_from_elsewhere |./a.out 
false

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a really good catch because I did not think about that... Thanks @darosior!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, after some time playing with this I think that we have a problem here... I do not see a way to know if the last character is noise or was introduced by the user. The reason is that getline returns the number of characters that where read, including the \n if present but not putting it in the read bytes. Then, if we know that we readed 2 chars, we do not know if test[1] is noise that was there because the user pushed enter \n or if the user executed printf "xc" | ./myprogram and then test[1] is the "c"

I think that the best is going to be to assume that the user is using this interactively and always replace the last character by \0 as it was the enter and was not supposed to be part of the passphrase.

If not, we have that

#include <inttypes.h>
#include <stdio.h>
#include <string.h>

int main(int argc, char **argv) {
	char *test = NULL;
	size_t len;

	size_t characters = getline(&test, &len, stdin);
        printf("read chars: %ld\n", characters);
	printf("%s\n", test[strlen(test) - 1] == '\n' ? "true" : "false");
}

Then...

~/tmp ~$ ./a.out 
a
read chars: 2
true
~/tmp ~$ printf "ab" | ./a.out
read chars: 2
false

Copy link
Collaborator

@darosior darosior Oct 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not doing it as we do it for --encrypted-hsm ?

if (passwd[strlen(passwd) - 1] == '\n')
passwd[strlen(passwd) - 1] = '\0';

EDIT: as per your example:

#include <inttypes.h>
#include <stdio.h>
#include <string.h>

int main(int argc, char **argv) {
	char *test = NULL;
	size_t len;

	size_t characters = getline(&test, &len, stdin);
	if (test[strlen(test) - 1] == '\n')
		test[strlen(test) - 1] = '\0';
	printf("read chars: %ld\n", characters);
	printf("%s\n", test[strlen(test) - 1] == '\n' ? "true" : "false");
}
darosior@darosior:~$ ./a.out 
a
read chars: 2
false
darosior@darosior:~$ printf "ab" | ./a.out
read chars: 2
false

tools/hsmtool.c Outdated
@@ -33,6 +35,7 @@ static void show_usage(const char *progname)
"<path/to/hsm_secret> [hsm_secret password]\n");
printf(" - guesstoremote <P2WPKH address> <node id> <tries> "
"<path/to/hsm_secret> [hsm_secret password]\n");
printf(" - generatehsm <path/to/hsm_secret>\n");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
printf(" - generatehsm <path/to/hsm_secret>\n");
printf(" - generatehsm <path/to/new//hsm_secret>\n");

To imply it's not existing yet ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not used to that nomenclature but is it common for new files to be described as that?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely not, entirely baked by myself (though i'm almost sure to have read that somewhere as well) because it's clearer in my mind. :)
Was more an opinion than a request!

tools/hsmtool.c Outdated
if (close(fd) != 0)
errx(ERROR_USAGE, "Error closing hsm_secret file");

printf("New hsm_secret file created. Use hsmtool to encrypt the BIP32 seed if needed");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
printf("New hsm_secret file created. Use hsmtool to encrypt the BIP32 seed if needed");
printf("New hsm_secret file created. Use the `encrypt` command to encrypt the BIP32 seed if needed");

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe also echo the path that the hsm_secret file was created at?

New hsm_secret file created. -> New /path/to/hsm_secret created.

tools/hsmtool.c Outdated
Comment on lines 510 to 514
/* if hsm_secred already exists we abort the process
* we do not want to lose someone else's funds */
struct stat st;
if (stat(hsm_secret_path, &st) == 0)
errx(ERROR_USAGE, "hsm_secret file already exists");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice sanity check :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd echo the path that already exists here also e.g.

Suggested change
/* if hsm_secred already exists we abort the process
* we do not want to lose someone else's funds */
struct stat st;
if (stat(hsm_secret_path, &st) == 0)
errx(ERROR_USAGE, "hsm_secret file already exists");
/* if hsm_secred already exists we abort the process
* we do not want to lose someone else's funds */
struct stat st;
if (stat(hsm_secret_path, &st) == 0)
errx(ERROR_USAGE, "/path/to/hsm_secret already exists. Aborting.");

@@ -76,4 +80,4 @@ Note: the modules in the ccan/ directory have their own licenses, but
the rest of the code is covered by the BSD-style MIT license\.
Main web site: \fIhttps://github.com/ElementsProject/lightning\fR

This comment was marked as outdated.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I did not squash it because I saw so many files changed (and not added in the gitignore that it was weird...) I will do it at the end

tools/hsmtool.c Outdated
printf("Introduce the BIP39 24 word list:\n");
char word[50];
uint n_words;
for (n_words = 0; n_words < 24; n_words++) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BIP39 specifies that sets of 12,15,18,21, and 24 are valid mnemonic word list lengths.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we support all of them? How do you want the user to define how long their mnemonic is? another input asking for the number? two empty lines in a row?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well if you do 'readline" for an entire line which includes the complete passphrase, there's no problem with word counting? I'm not sure how robust libwally's validation method is; you might want to validate that that works as expected

tools/hsmtool.c Outdated
@@ -368,6 +371,91 @@ static int guess_to_remote(const char *address, struct node_id *node_id,
return 1;
}

static void read_mnemonic(char *mnemonic) {
printf("Introduce the BIP39 24 word list:\n");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
printf("Introduce the BIP39 24 word list:\n");
printf("Input the BIP39 word list:\n");

tools/hsmtool.c Outdated
if (n_words == 0) {
strcpy(mnemonic, word);
} else {
strcat(mnemonic, " ");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Japanese (and Chinese?) word lists use a different word separator character. It's probably safer to have the input be a single line, instead of word tokens.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will update it to support all the languages

tools/hsmtool.c Outdated

static void read_passphrase(char *passphrase) {
struct termios current_term, temp_term;
printf("Warning: remember that different passphrase yield different "
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
printf("Warning: remember that different passphrase yield different "
printf("Warning: remember that different passphrases yield different "

tools/hsmtool.c Outdated
{
char mnemonic[BIP39_WORDLIST_LEN];
read_mnemonic(mnemonic);
if (bip39_mnemonic_validate(NULL, mnemonic) != 0)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this correctly validate non-English word list sets? You might try the test cases from https://github.com/bip32JP/bip32JP.github.io/blob/master/test_JP_BIP39.json

tools/hsmtool.c Outdated
if (close(fd) != 0)
errx(ERROR_USAGE, "Error closing hsm_secret file");

printf("New hsm_secret file created. Use hsmtool to encrypt the BIP32 seed if needed");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe also echo the path that the hsm_secret file was created at?

New hsm_secret file created. -> New /path/to/hsm_secret created.

tools/hsmtool.c Outdated
@@ -413,5 +501,20 @@ int main(int argc, char *argv[])
argv[5], argc >= 7 ? argv[6] : NULL);
}

if (streq(method, "generatehsm")) {
if ( argc != 3)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if ( argc != 3)
if (argc != 3)

tools/hsmtool.c Outdated

char *hsm_secret_path = argv[2];

/* if hsm_secred already exists we abort the process
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/* if hsm_secred already exists we abort the process
/* if hsm_secret already exists we abort the process

tools/hsmtool.c Outdated
Comment on lines 510 to 514
/* if hsm_secred already exists we abort the process
* we do not want to lose someone else's funds */
struct stat st;
if (stat(hsm_secret_path, &st) == 0)
errx(ERROR_USAGE, "hsm_secret file already exists");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd echo the path that already exists here also e.g.

Suggested change
/* if hsm_secred already exists we abort the process
* we do not want to lose someone else's funds */
struct stat st;
if (stat(hsm_secret_path, &st) == 0)
errx(ERROR_USAGE, "hsm_secret file already exists");
/* if hsm_secred already exists we abort the process
* we do not want to lose someone else's funds */
struct stat st;
if (stat(hsm_secret_path, &st) == 0)
errx(ERROR_USAGE, "/path/to/hsm_secret already exists. Aborting.");

@niftynei niftynei added the clightning_twit Tag to nudge @niftynei to post to @clightning_twit label Oct 15, 2020
@positiveblue positiveblue force-pushed the plugin-bip39 branch 6 times, most recently from 91a727a to edab496 Compare October 29, 2020 01:35
@positiveblue
Copy link
Author

@niftynei I updated the PR. Now it reads the mnemonic all at once using getline() and use that directly.

If the list of words is not provided in the right (standard) format (word1 word2 word3...) it fails. Because we use libwally under the hood, we can validate and derive the seed without any problem, supporting out of the box all the mnemonic length 12, 18, 24...) and multiple languages.

I tried to compare the obtained results with the ones from other bip39 implementations and I got the same results (with and without passphrase) for english, spanish, italian, chinese... including non ASCII characters too.

However, there is one still failing. Japanese mnemonics do not pass the libwally validation. I wonder if it has something to do with the terminal byte encoding or something like that because with Chinese characters I did not have any problem... do you know if you have to do something special with JP characters? Even so, I can just show and error if Japanese is selected (or not show the option) and leave it as a TODO for the next PR.

Copy link
Collaborator

@darosior darosior left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One main request on my side: we should support passphrase == NULL instead of passphrase == '\0'.

tools/hsmtool.c Outdated
Comment on lines 375 to 383
printf("Select your language:\n");
printf(" 0) English (en)\n");
printf(" 1) Spanish (es)\n");
printf(" 2) French (fr)\n");
printf(" 3) Italian (it)\n");
printf(" 4) Japanese (jp)\n");
printf(" 5) Chinese simplified (zhs)\n");
printf(" 6) Chinese traditional (zht)\n");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conceptually, i feel that only ever supporting English as the international language for mnemonics is safer. Don't want to block you on this though.

tools/hsmtool.c Outdated
printf(" 6) Chinese traditional (zht)\n");
printf("Select [0-6]: ");

char *selection=NULL;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:
char *selection = NULL;

@@ -51,6 +51,10 @@ and is usually no greater than the number of channels that the node has
ever had\.
Specify \fIpassword\fR if the \fBhsm_secret\fR is encrypted\.


\fBgeneratehsm\fR \fIhsm_secret_path\fR
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: frombip39 / secretfrombip39 / generatefrombip39 ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation says that it generates the key following the BIP39 and also the text prompted to the user during the generation... if you prefer one of those names I am happy to change it but bip39 should be the standard by now (even when lnd nodes use aezeed). In the future, if we want to support multiple "standards", we can add a flag: generatehsm --alg={aezeed, bip39,..}

tools/hsmtool.c Outdated
errx(ERROR_USAGE, "Could not read line from stdin.");
int selected = atoi(selection);

char *lang = malloc(4);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This leaks

tools/hsmtool.c Outdated
Comment on lines 472 to 473
/* Replace the last character because we assume it was the newline '\n' */
(*passphrase)[characters-1] = '\0';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we assume it was ? Couldn't we just check it is ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also we should support passphrase == NULL, for the very common "no passphrase" case !

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, characters can in theory be 0

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we assume it was ? Couldn't we just check it is ?

well, the user needs to introduce a value an press enter (getline()). I can add a check and fail in case something weird is going on.

Also we should support passphrase == NULL, for the very common "no passphrase" case !

The empty word and NULL have the same effect in libwally. I doubt that this never changes but I can substitute it by NULL when the user does not add any character to their passphrase.

Also, characters can in theory be 0

How do we get the line then? ctrl-d or something? In that case I think it's ok to just segfault

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, the user needs to introduce a value an press enter (getline()). I can add a check and fail in case something weird is going on.

#4065 (comment) ... I know it's harder to get there with the two consecutive getline, but that's a tool and it's going to be used in scripts and stuff.
Moreover, you are overriding the user passphrase here! This definitely needs a sanity check (maybe a belt and suspenders, but at least the belt will do for now!).
If you are really confident about this, make it an assert:

assert((*passphrase)[characters-1] == '\n');
(*passphrase)[characters-1] = '\0';

But i think it's a bad idea and can be triggered. Don't know what's wrong with just doing:

if ((*passphrase)[characters-1] == '\n')
    (*passphrase)[characters-1] = '\0';

The empty word and NULL have the same effect in libwally. I doubt that this never changes but I can substitute it by NULL when the user does not add any character to their passphrase.

Oh, i was not aware of that :) I just check the doc comment stating "if no passphrase then set to NULL". So yeah better to rely on documented behaviour i guess!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check added

tools/hsmtool.c Outdated
Comment on lines 415 to 420
default:
errx(ERROR_USAGE, "Invalid language selection, select one from the list [0-6].");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This cannot be reached, atoi will give you 0 if throw it garbage

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use strtol or similar

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This cannot be reached, atoi will give you 0 if throw it garbage

This is true, and I did not know it! Maybe the safest thing to do then is to take the first byte from the user's input and subtract '0' instead of doing any str to number conversion.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm i'd say the safest thing here to go for strtol, which errors on --well-- error. From the manpage:

The program shown below demonstrates the use of strtol(). The first command-line argument specifies a string from which strtol() should parse a number. The second (optional) argument specifies the
base to be used for the conversion. (This argument is converted to numeric form using atoi(3), a function that performs no error checking and has a simpler interface than strtol().) Some examples of
the results produced by this program are the following:

       $ ./a.out 123
       strtol() returned 123
       $ ./a.out '    123'
       strtol() returned 123
       $ ./a.out 123abc
       strtol() returned 123
       Further characters after number: abc
       $ ./a.out 123abc 55
       strtol: Invalid argument
       $ ./a.out ''
       No digits were found
       $ ./a.out 4000000000
       strtol: Numerical result out of range

Comment on lines +484 to +487
char *passphrase = NULL;
read_passphrase(&passphrase);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If read_passphrase set passphrase, it's never freed

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this, is there any automatic tool that checks for this silly cases that I forgot? (so I can use it before future contributions and you do not have to lose your time on "simple" memory leaks)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just went looking for missing free, but you can test with valgrind or clang with leak sanitizer enabled (configure --enable-address-sanitizer) to at least make sure the frequent paths don't leak.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even better: write a test to trigger those codepaths :)

tools/hsmtool.c Outdated
errx(ERROR_USAGE, "Could not read line from stdin.");

int selection = selected[0] - '0';
if (characters != 2 || selection < 0 || selection > 6)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

scanf would be more appropriate here, rather than checking the length of the string and doing the char-to-int conversion.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it to getline() because scanf left the \n char and sequential getline()failed. What is the best way to deal with that?

tools/hsmtool.c Outdated
bip39_get_wordlist("it", words);
break;
case 4:
errx(ERROR_LANG_NOT_SUPPORTED, "Sorry, the language selected is not supported yet.");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's strange to be offering an option and then declining the selection.

@@ -368,6 +372,148 @@ static int guess_to_remote(const char *address, struct node_id *node_id,
return 1;
}

static void get_words(struct words **words) {
printf("Select your language:\n");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seem to be quite a number of repetitions in the code here, that might be better suited for a list of structs that contain the various pieces of information:

struct wordlist_lang {
	char *abbr;
	char *name;
};

struct wordlist_lang languages[] = {
	{"en", "English"},
	{"es", "Spanish"},
	{"fr", "French"},
	{"it", "Italian"},
	{"jp", "Japanese"},
	{"zhs", "Chinese Simplified"},
	{"zht", "Chinese Traditional"},
};

Then you can use ARRAY_SIZE to determine the size of the array at compile time, iterate through them when printing the menu, and indexing with the result after selection to find the abbreviation to use when fetching the wordlist from libwally instead of having to update the printf, index checks and switch-statement below.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

tools/hsmtool.c Outdated
char *line = NULL;
size_t line_size = 0;

printf("Introduce your BIP39 word list:\n");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd add separated by space, so people don't attempt to write one per line, which is what I'd have done :-)

@cdecker cdecker added this to the v0.9.2 milestone Oct 30, 2020
@cdecker
Copy link
Member

cdecker commented Oct 30, 2020

Excellent addition @positiveblue, small nits aside, I think this is a great feature, and I'm positive we'll merge it before the next release 👍

Copy link
Collaborator

@darosior darosior left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just two small things left, starting to look pretty good :-)

tools/hsmtool.c Outdated
struct termios current_term, temp_term;
printf("Warning: remember that different passphrase yield different "
"bitcoin wallets.\n");
printf("Leave it empty for using the value by default (echo is "
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, probably better to say "if left empty, no password is used".

tools/hsmtool.c Outdated
Comment on lines 470 to 475
// If the user did not introduce any password, we want to set passphrase
// to NULL not to '\0' for libwally
if (strlen(*passphrase) == 0) {
free(*passphrase);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't set it to NULL here

free(*passphrase);
*passphrase = NULL;

Copy link
Collaborator

@niftynei niftynei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few more nits, otherwise looks good.

It occurred to me while looking into this that the "normal" BIP39 to BIP32 pathway isn't true for c-lightning. Typically you'd pass the seed generated by BIP39's derivation directly to bip32_key_from_seed. However, in hsmd.c, we add an intermediate derivation (using an hkdf), passing the BIP39 seed into this function to get yet another seed, which we then pass to bip32_key_from_seed.

This isn't a blocker for this patch. However, it will prevent "seed portability". You can't use a mnemonic here and anticipate being able to "seamlessly" import your lightning funds elsewhere using the same mnemonic.

If we want 'full compliance' with the BIP39 to BIP32 pathway, we'll need to update the seed derivation to remove this secondary step. This would be a good opportunity to more fully embrace BIP32 pathways for different key paths -- I hear LND has an (undocumented) pathway scheme for key generation and usage that might be worth porting over to c-lightning ...

char *endptr;
long val = strtol(selected, &endptr, 10);
if (errno == ERANGE || (errno != 0 && val == 0) || endptr == selected || val < 0 || val > ARRAY_SIZE(languages))
errx(ERROR_USAGE, "Invalid language selection, select one from the list [0-6].");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be 0- ARRAY_SIZE? 😉

tools/hsmtool.c Outdated
if (characters < 0)
errx(ERROR_USAGE, "Could not read passphrase from stdin.");

// newline is not part of the valid passphrase
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style-nit: we use /* */ for comments

tools/hsmtool.c Outdated
// to NULL not to '\0' for libwally
if (strlen(*passphrase) == 0) {
free(*passphrase);
*passphrase = NULL;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: spacing here is off?

tools/hsmtool.c Outdated
errx(ERROR_USAGE, "Error writing secret to hsm_secret file");

if (fsync(fd) != 0)
errx(ERROR_USAGE, "Error fsycning hsm_secret file");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
errx(ERROR_USAGE, "Error fsycning hsm_secret file");
errx(ERROR_USAGE, "Error fsyncing hsm_secret file");

@darosior
Copy link
Collaborator

darosior commented Nov 6, 2020

@positiveblue feel free to ping me as soon as you force push, would like to ACK this before we cut the release :-)

This would be a good opportunity to more fully embrace BIP32 pathways for different key paths -- I hear LND has an (undocumented) pathway scheme for key generation and usage that might be worth porting over to c-lightning ...

Hehe .. #3687 :)

tools: Add `generatehsm` method to hsmtool to derivate BIP32 seeds from a
mnemonic using the BIP39 standard.

The new method uses libwally for the BIP39 to BIP32 derivation. It also
fails if an hsm_secret file already exists, so we do not overwrite
someone else's wallet without noticing.

It allows the use of passphrases, the ECHO mode in the terminal is
disable for higher security.

It currently supports "en", "es", "fr", "it", "jp", "zhs", "zht".

Changelog-Added: hsmtool: `hsm_secret` generation from a seed-phrase following BIP39.
@positiveblue
Copy link
Author

@darosior I see the problems that you mention in #3687

Because many projects are already using c-lightning we can not simple ask them to close all the channels and open them again. If we really want the project to support BIP39, Azeed (lnd) and other derivations we should support the old way as a legacy feature and introduce the new third-party compatible derivations as default.

For Azeed is easy, but for BIP39 is hard taking into account that we can not use the current populate_secretstuff function form hsmd.c.

Right now people can only recover their wallet by copy the hsm_secret file. This PR could be helpful for people creating their hsm_secret in a reproducible way for the future. However, at some point the project should just use the default bip39 derivation path and add a flag for all wallets (which would use the current hkdf_sha256 step)

What do you think about it?

@niftynei
Copy link
Collaborator

niftynei commented Nov 9, 2020

ACK 7cc0674

@niftynei niftynei merged commit fa1483a into ElementsProject:master Nov 9, 2020
@darosior
Copy link
Collaborator

post-merge ACK 7cc0674 . Thanks for having worked on this @positiveblue !

Right now people can only recover their wallet by copy the hsm_secret file.

This a not a good way to backup and/or recover the wallet, without the db you may (are going to?) be missing utxos. See #4171 for more discussions about that

@niftynei niftynei removed the clightning_twit Tag to nudge @niftynei to post to @clightning_twit label Nov 17, 2020
@darosior
Copy link
Collaborator

darosior commented Jan 2, 2021

Hmm so i'm pretty sure to have tested this with encrypt and decrypt, and even just that it yields an actual hsm_secret. But it so happens that it creates a 64-bytes hsm_secret !
I tested both master and this branch, and neither does it not seem to be libwally as it didn't change much: https://github.com/ElementsProject/libwally-core/blob/master/src/bip39.c ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants