From d135419e3276c71824bfa97a60826cd9cd367797 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 21 Jan 2019 20:01:38 +0100 Subject: [PATCH 1/7] cryptsetup: use free_and_replace() where appropriate --- src/cryptsetup/cryptsetup.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/cryptsetup/cryptsetup.c b/src/cryptsetup/cryptsetup.c index daf26aad70..49a7307809 100644 --- a/src/cryptsetup/cryptsetup.c +++ b/src/cryptsetup/cryptsetup.c @@ -408,8 +408,7 @@ static int get_password(const char *vol, const char *src, usec_t until, bool acc return log_oom(); strncpy(c, *p, arg_key_size); - free(*p); - *p = c; + free_and_replace(*p, c); } *ret = TAKE_PTR(passwords); From 0ffff81abdc7db82dc7fd72cd066074d6b3a476c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 21 Jan 2019 20:02:33 +0100 Subject: [PATCH 2/7] cryptsetup: modernize some log message invocations --- src/cryptsetup/cryptsetup.c | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/src/cryptsetup/cryptsetup.c b/src/cryptsetup/cryptsetup.c index 49a7307809..7072a89325 100644 --- a/src/cryptsetup/cryptsetup.c +++ b/src/cryptsetup/cryptsetup.c @@ -485,10 +485,8 @@ static int attach_luks_or_plain(struct crypt_device *cd, if (!arg_type || STR_IN_SET(arg_type, ANY_LUKS, CRYPT_LUKS1)) { r = crypt_load(cd, CRYPT_LUKS, NULL); - if (r < 0) { - log_error("crypt_load() failed on device %s.\n", crypt_get_device_name(cd)); - return r; - } + if (r < 0) + return log_error_errno(r, "crypt_load() failed on device %s: %m", crypt_get_device_name(cd)); if (data_device) r = crypt_set_data_device(cd, data_device); @@ -623,10 +621,8 @@ static int run(int argc, char *argv[]) { /* Arguments: systemd-cryptsetup attach VOLUME SOURCE-DEVICE [PASSWORD] [OPTIONS] */ - if (argc < 4) { - log_error("attach requires at least two arguments."); - return -EINVAL; - } + if (argc < 4) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "attach requires at least two arguments."); if (argc >= 5 && argv[4][0] && @@ -718,10 +714,8 @@ static int run(int argc, char *argv[]) { log_warning("Invalid passphrase."); } - if (arg_tries != 0 && tries >= arg_tries) { - log_error("Too many attempts; giving up."); - return -EPERM; - } + if (arg_tries != 0 && tries >= arg_tries) + return log_error_errno(SYNTHETIC_ERRNO(EPERM), "Too many attempts to activate; giving up."); } else if (streq(argv[1], "detach")) { @@ -739,10 +733,8 @@ static int run(int argc, char *argv[]) { if (r < 0) return log_error_errno(r, "Failed to deactivate: %m"); - } else { - log_error("Unknown verb %s.", argv[1]); - return -EINVAL; - } + } else + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Unknown verb %s.", argv[1]); return 0; } From b7a0fead10959b03a1fa642a5ae7aca3a6a3dee9 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 21 Jan 2019 20:13:11 +0100 Subject: [PATCH 3/7] cryptsetup: add some commenting about EAGAIN generation --- src/cryptsetup/cryptsetup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cryptsetup/cryptsetup.c b/src/cryptsetup/cryptsetup.c index 7072a89325..bc4ff5f5a7 100644 --- a/src/cryptsetup/cryptsetup.c +++ b/src/cryptsetup/cryptsetup.c @@ -450,7 +450,7 @@ static int attach_tcrypt( r = read_one_line_file(key_file, &passphrase); if (r < 0) { log_error_errno(r, "Failed to read password file '%s': %m", key_file); - return -EAGAIN; + return -EAGAIN; /* log with the actual error, but return EAGAIN */ } params.passphrase = passphrase; From aed68083c045175dd7b5852012e553401f6b51c8 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 21 Jan 2019 20:14:42 +0100 Subject: [PATCH 4/7] cryptsetup: don't line-break so aggressively --- src/cryptsetup/cryptsetup.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/src/cryptsetup/cryptsetup.c b/src/cryptsetup/cryptsetup.c index bc4ff5f5a7..7264401200 100644 --- a/src/cryptsetup/cryptsetup.c +++ b/src/cryptsetup/cryptsetup.c @@ -527,23 +527,16 @@ static int attach_luks_or_plain(struct crypt_device *cd, cipher_mode = "cbc-essiv:sha256"; } - /* for CRYPT_PLAIN limit reads - * from keyfile to key length, and - * ignore keyfile-size */ + /* for CRYPT_PLAIN limit reads from keyfile to key length, and ignore keyfile-size */ arg_keyfile_size = arg_key_size; - /* In contrast to what the name - * crypt_setup() might suggest this - * doesn't actually format anything, - * it just configures encryption - * parameters when used for plain - * mode. */ + /* In contrast to what the name crypt_setup() might suggest this doesn't actually format + * anything, it just configures encryption parameters when used for plain mode. */ r = crypt_format(cd, CRYPT_PLAIN, cipher, cipher_mode, NULL, NULL, arg_keyfile_size, ¶ms); /* hash == NULL implies the user passed "plain" */ pass_volume_key = (params.hash == NULL); } - if (r < 0) return log_error_errno(r, "Loading of cryptographic parameters failed: %m"); From 44ce4255147ab308c1f13580147c693204c322e8 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 21 Jan 2019 20:19:57 +0100 Subject: [PATCH 5/7] cryptsetup: downgrade a log message we ignore --- src/cryptsetup/cryptsetup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cryptsetup/cryptsetup.c b/src/cryptsetup/cryptsetup.c index 7264401200..c8e90cf60d 100644 --- a/src/cryptsetup/cryptsetup.c +++ b/src/cryptsetup/cryptsetup.c @@ -623,7 +623,7 @@ static int run(int argc, char *argv[]) { !streq(argv[4], "none")) { if (!path_is_absolute(argv[4])) - log_error("Password file path '%s' is not absolute. Ignoring.", argv[4]); + log_warning("Password file path '%s' is not absolute. Ignoring.", argv[4]); else key_file = argv[4]; } From 906962f312e3f17b1e76cc6b9f4f344d76df1d7f Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 21 Jan 2019 20:20:35 +0100 Subject: [PATCH 6/7] cryptsetup: add comment what EAGAIN means when activating --- src/cryptsetup/cryptsetup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cryptsetup/cryptsetup.c b/src/cryptsetup/cryptsetup.c index c8e90cf60d..56d3e34895 100644 --- a/src/cryptsetup/cryptsetup.c +++ b/src/cryptsetup/cryptsetup.c @@ -697,7 +697,7 @@ static int run(int argc, char *argv[]) { flags); if (r >= 0) break; - if (r == -EAGAIN) { + if (r == -EAGAIN) { /* Passphrase not correct? Let's try again! */ key_file = NULL; continue; } From 6f177c7dc092eb68762b4533d41b14244adb2a73 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 21 Jan 2019 20:19:11 +0100 Subject: [PATCH 7/7] cryptsetup: rework how we log about activation failures First of all let's always log where the errors happen, and not in an upper stackframe, in all cases. Previously we'd do this somethis one way and sometimes another, which resulted in sometimes duplicate logging and sometimes none. When we cannot activate something due to bad password the kernel gives us EPERM. Let's uniformly return this EAGAIN, so tha the next password is tried. (previously this was done in most cases but not in all) When we get EPERM let's also explicitly indicate that this probably means the password is simply wrong. Fixes: #11498 --- src/cryptsetup/cryptsetup.c | 46 ++++++++++++++++++++++--------------- 1 file changed, 28 insertions(+), 18 deletions(-) diff --git a/src/cryptsetup/cryptsetup.c b/src/cryptsetup/cryptsetup.c index 56d3e34895..9cb52ddf26 100644 --- a/src/cryptsetup/cryptsetup.c +++ b/src/cryptsetup/cryptsetup.c @@ -460,14 +460,19 @@ static int attach_tcrypt( r = crypt_load(cd, CRYPT_TCRYPT, ¶ms); if (r < 0) { - if (key_file && r == -EPERM) - return log_error_errno(SYNTHETIC_ERRNO(EAGAIN), - "Failed to activate using password file '%s'.", - key_file); - return r; + if (key_file && r == -EPERM) { + log_error_errno(r, "Failed to activate using password file '%s'. (Key data not correct?)", key_file); + return -EAGAIN; /* log the actual error, but return EAGAIN */ + } + + return log_error_errno(r, "Failed to load tcrypt superblock on device %s: %m", crypt_get_device_name(cd)); } - return crypt_activate_by_volume_key(cd, name, NULL, 0, flags); + r = crypt_activate_by_volume_key(cd, name, NULL, 0, flags); + if (r < 0) + return log_error_errno(r, "Failed to activate tcrypt device %s: %m", crypt_get_device_name(cd)); + + return 0; } static int attach_luks_or_plain(struct crypt_device *cd, @@ -486,7 +491,7 @@ static int attach_luks_or_plain(struct crypt_device *cd, if (!arg_type || STR_IN_SET(arg_type, ANY_LUKS, CRYPT_LUKS1)) { r = crypt_load(cd, CRYPT_LUKS, NULL); if (r < 0) - return log_error_errno(r, "crypt_load() failed on device %s: %m", crypt_get_device_name(cd)); + return log_error_errno(r, "Failed to load LUKS superblock on device %s: %m", crypt_get_device_name(cd)); if (data_device) r = crypt_set_data_device(cd, data_device); @@ -548,22 +553,30 @@ static int attach_luks_or_plain(struct crypt_device *cd, if (key_file) { r = crypt_activate_by_keyfile_offset(cd, name, arg_key_slot, key_file, arg_keyfile_size, arg_keyfile_offset, flags); - if (r < 0) { - log_error_errno(r, "Failed to activate with key file '%s': %m", key_file); - return -EAGAIN; + if (r == -EPERM) { + log_error_errno(r, "Failed to activate with key file '%s'. (Key data incorrect?)", key_file); + return -EAGAIN; /* Log actual error, but return EAGAIN */ } + if (r < 0) + return log_error_errno(r, "Failed to activate with key file '%s': %m", key_file); } else { char **p; + r = -EINVAL; STRV_FOREACH(p, passwords) { if (pass_volume_key) r = crypt_activate_by_volume_key(cd, name, *p, arg_key_size, flags); else r = crypt_activate_by_passphrase(cd, name, arg_key_slot, *p, strlen(*p), flags); - if (r >= 0) break; } + if (r == -EPERM) { + log_error_errno(r, "Failed to activate with specified passphrase. (Passphrase incorrect?)"); + return -EAGAIN; /* log actual error, but return EAGAIN */ + } + if (r < 0) + return log_error_errno(r, "Failed to activate with specified passphrase: %m"); } return r; @@ -697,14 +710,11 @@ static int run(int argc, char *argv[]) { flags); if (r >= 0) break; - if (r == -EAGAIN) { /* Passphrase not correct? Let's try again! */ - key_file = NULL; - continue; - } - if (r != -EPERM) - return log_error_errno(r, "Failed to activate: %m"); + if (r != -EAGAIN) + return r; - log_warning("Invalid passphrase."); + /* Passphrase not correct? Let's try again! */ + key_file = NULL; } if (arg_tries != 0 && tries >= arg_tries)