Discussion:
[PATCH 0/3] Password agent fixes
(too old to reply)
David Härdeman
2014-02-12 22:55:06 UTC
Permalink
The following series contains patch 1 - 3 of the previous patchset that
I've posted. Lennart wanted a single "Id=" instead of the two "Purpose="
and "Target=" strings in the .ask files, so I've changed it accordingly.

The two following patches are just small fixes.

I've left out the binary protocol patch for now.

---

David Härdeman (3):
Add more password agent information
Fix keysize handling in cryptsetup (bits vs. bytes)
Fix askpass buffer overflow


src/ask-password/ask-password.c | 14 +++++++++++---
src/cryptsetup/cryptsetup.c | 19 +++++++++++++++----
src/shared/ask-password-api.c | 14 ++++++++++++--
src/shared/ask-password-api.h | 6 ++++--
4 files changed, 42 insertions(+), 11 deletions(-)
--
David Härdeman
David Härdeman
2014-02-12 22:55:11 UTC
Permalink
Add an (optional) "Id" key in the password agent .ask files. The Id is
supposed to be a simple string in "<subsystem>:<target>" form which
is used to provide more information on what the requested passphrase
is to be used for (which e.g. allows an agent to only react to cryptsetup
requests).
---
src/ask-password/ask-password.c | 14 +++++++++++---
src/cryptsetup/cryptsetup.c | 11 +++++++++--
src/shared/ask-password-api.c | 9 +++++++--
src/shared/ask-password-api.h | 6 ++++--
4 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/src/ask-password/ask-password.c b/src/ask-password/ask-password.c
index ea0c623..f939eab 100644
--- a/src/ask-password/ask-password.c
+++ b/src/ask-password/ask-password.c
@@ -43,6 +43,7 @@
#include "def.h"

static const char *arg_icon = NULL;
+static const char *arg_id = NULL;
static const char *arg_message = NULL;
static bool arg_use_tty = true;
static usec_t arg_timeout = DEFAULT_TIMEOUT_USEC;
@@ -58,7 +59,8 @@ static int help(void) {
" --timeout=SEC Timeout in sec\n"
" --no-tty Ask question via agent even on TTY\n"
" --accept-cached Accept cached passwords\n"
- " --multiple List multiple passwords if available\n",
+ " --multiple List multiple passwords if available\n"
+ " --id=ID Query identifier (e.g. cryptsetup:/dev/sda5)\n",
program_invocation_short_name);

return 0;
@@ -71,7 +73,8 @@ static int parse_argv(int argc, char *argv[]) {
ARG_TIMEOUT,
ARG_NO_TTY,
ARG_ACCEPT_CACHED,
- ARG_MULTIPLE
+ ARG_MULTIPLE,
+ ARG_ID
};

static const struct option options[] = {
@@ -81,6 +84,7 @@ static int parse_argv(int argc, char *argv[]) {
{ "no-tty", no_argument, NULL, ARG_NO_TTY },
{ "accept-cached", no_argument, NULL, ARG_ACCEPT_CACHED },
{ "multiple", no_argument, NULL, ARG_MULTIPLE },
+ { "id", required_argument, NULL, ARG_ID },
{}
};

@@ -119,6 +123,10 @@ static int parse_argv(int argc, char *argv[]) {
arg_multiple = true;
break;

+ case ARG_ID:
+ arg_id = optarg;
+ break;
+
case '?':
return -EINVAL;

@@ -162,7 +170,7 @@ int main(int argc, char *argv[]) {
} else {
char **l;

- if ((r = ask_password_agent(arg_message, arg_icon, timeout, arg_accept_cached, &l)) >= 0) {
+ if ((r = ask_password_agent(arg_message, arg_icon, arg_id, timeout, arg_accept_cached, &l)) >= 0) {
char **p;

STRV_FOREACH(p, l) {
diff --git a/src/cryptsetup/cryptsetup.c b/src/cryptsetup/cryptsetup.c
index 033c0cd..f72cf9f 100644
--- a/src/cryptsetup/cryptsetup.c
+++ b/src/cryptsetup/cryptsetup.c
@@ -260,6 +260,7 @@ static int get_password(const char *name, usec_t until, bool accept_cached, char
int r;
char **p;
_cleanup_free_ char *text = NULL;
+ _cleanup_free_ char *id = NULL;

assert(name);
assert(passwords);
@@ -267,7 +268,10 @@ static int get_password(const char *name, usec_t until, bool accept_cached, char
if (asprintf(&text, "Please enter passphrase for disk %s!", name) < 0)
return log_oom();

- r = ask_password_auto(text, "drive-harddisk", until, accept_cached, passwords);
+ if (asprintf(&id, "cryptsetup:%s", name) < 0)
+ return log_oom();
+
+ r = ask_password_auto(text, "drive-harddisk", id, until, accept_cached, passwords);
if (r < 0) {
log_error("Failed to query password: %s", strerror(-r));
return r;
@@ -281,7 +285,10 @@ static int get_password(const char *name, usec_t until, bool accept_cached, char
if (asprintf(&text, "Please enter passphrase for disk %s! (verification)", name) < 0)
return log_oom();

- r = ask_password_auto(text, "drive-harddisk", until, false, &passwords2);
+ if (asprintf(&id, "cryptsetup-verification:%s", name) < 0)
+ return log_oom();
+
+ r = ask_password_auto(text, "drive-harddisk", id, until, false, &passwords2);
if (r < 0) {
log_error("Failed to query verification password: %s", strerror(-r));
return r;
diff --git a/src/shared/ask-password-api.c b/src/shared/ask-password-api.c
index a328f14..1c18274 100644
--- a/src/shared/ask-password-api.c
+++ b/src/shared/ask-password-api.c
@@ -298,6 +298,7 @@ fail:
int ask_password_agent(
const char *message,
const char *icon,
+ const char *id,
usec_t until,
bool accept_cached,
char ***_passphrases) {
@@ -370,6 +371,9 @@ int ask_password_agent(
if (icon)
fprintf(f, "Icon=%s\n", icon);

+ if (id)
+ fprintf(f, "Id=%s\n", id);
+
fflush(f);

if (ferror(f)) {
@@ -548,7 +552,8 @@ finish:
return r;
}

-int ask_password_auto(const char *message, const char *icon, usec_t until, bool accept_cached, char ***_passphrases) {
+int ask_password_auto(const char *message, const char *icon, const char *id,
+ usec_t until, bool accept_cached, char ***_passphrases) {
assert(message);
assert(_passphrases);

@@ -569,5 +574,5 @@ int ask_password_auto(const char *message, const char *icon, usec_t until, bool
return r;

} else
- return ask_password_agent(message, icon, until, accept_cached, _passphrases);
+ return ask_password_agent(message, icon, id, until, accept_cached, _passphrases);
}
diff --git a/src/shared/ask-password-api.h b/src/shared/ask-password-api.h
index 288a0f4..3839a2d 100644
--- a/src/shared/ask-password-api.h
+++ b/src/shared/ask-password-api.h
@@ -25,6 +25,8 @@

int ask_password_tty(const char *message, usec_t until, const char *flag_file, char **_passphrase);

-int ask_password_agent(const char *message, const char *icon, usec_t until, bool accept_cached, char ***_passphrases);
+int ask_password_agent(const char *message, const char *icon, const char *id,
+ usec_t until, bool accept_cached, char ***_passphrases);

-int ask_password_auto(const char *message, const char *icon, usec_t until, bool accept_cached, char ***_passphrases);
+int ask_password_auto(const char *message, const char *icon, const char *id,
+ usec_t until, bool accept_cached, char ***_passphrases);
Lennart Poettering
2014-03-25 00:03:48 UTC
Permalink
On Wed, 12.02.14 23:55, David Härdeman (***@hardeman.nu) wrote:

Sorry for the late review!
Post by David Härdeman
Add an (optional) "Id" key in the password agent .ask files. The Id is
supposed to be a simple string in "<subsystem>:<target>" form which
is used to provide more information on what the requested passphrase
is to be used for (which e.g. allows an agent to only react to cryptsetup
requests).
I wonder if this is related to the "keyhandler" stuff Benjamin Sans has
asked for.

http://lists.freedesktop.org/archives/systemd-devel/2014-March/017869.html

Benjamin, can you comment?
Post by David Härdeman
- r = ask_password_auto(text, "drive-harddisk", until, accept_cached, passwords);
+ if (asprintf(&id, "cryptsetup:%s", name) < 0)
+ return log_oom();
+
+ r = ask_password_auto(text, "drive-harddisk", id, until,
accept_cached, passwords);
Hmm, no tabs please...

Also, please use strappend() for cases like this, where we just want to
concatenate two strings.

That said, I wodner, if we should escape the second part of the string,
just to be sure. Using cescape() here would suffice?
Post by David Härdeman
if (r < 0) {
log_error("Failed to query password: %s", strerror(-r));
return r;
@@ -281,7 +285,10 @@ static int get_password(const char *name, usec_t until, bool accept_cached, char
if (asprintf(&text, "Please enter passphrase for disk %s! (verification)", name) < 0)
return log_oom();
- r = ask_password_auto(text, "drive-harddisk", until, false, &passwords2);
+ if (asprintf(&id, "cryptsetup-verification:%s", name) < 0)
+ return log_oom();
+
Similar here.

Otherwise this looks good to go, but I'd like to see a comment by
Benjamin, to see if this would work for him, too!

Lennart
--
Lennart Poettering, Red Hat
David Härdeman
2014-03-25 00:46:21 UTC
Permalink
Post by Lennart Poettering
Post by David Härdeman
Add an (optional) "Id" key in the password agent .ask files. The Id is
supposed to be a simple string in "<subsystem>:<target>" form which
is used to provide more information on what the requested passphrase
is to be used for (which e.g. allows an agent to only react to cryptsetup
requests).
I wonder if this is related to the "keyhandler" stuff Benjamin Sans has
asked for.
I think Benjamin and I have basically both come up with the same
solution (though I haven't changed the option from "keyscript=" to
"keyhandler=" since that would break backwards compatibility...which is
partly the point of the whole exercise)...

Bejamin's approach does not seem to solve the binary key part of the
puzzle either...(passing binary keys from the keyscript, as opposed to
passphrases).

My proposed approach is provided in more detail in the corresponding
Debian bug report, see:
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=618862#44

So yes, I think they're related...as in they are independent
implementations of the same thing :)
Post by Lennart Poettering
http://lists.freedesktop.org/archives/systemd-devel/2014-March/017869.html
Benjamin, can you comment?
Post by David Härdeman
- r = ask_password_auto(text, "drive-harddisk", until, accept_cached, passwords);
+ if (asprintf(&id, "cryptsetup:%s", name) < 0)
+ return log_oom();
+
+ r = ask_password_auto(text, "drive-harddisk", id, until,
accept_cached, passwords);
Hmm, no tabs please...
Also, please use strappend() for cases like this, where we just want to
concatenate two strings.
That said, I wodner, if we should escape the second part of the string,
just to be sure. Using cescape() here would suffice?
And risk breaking backward compatibility? (I was too lazy to check
exactly what cescape() handles right now)...

The "name" (second part of the string) comes from /etc/crypttab, which
is writeable by root only on any sane system? (otherwise it'd be dead
easy to insert a keyscript in between which does some keylogging).
Post by Lennart Poettering
Post by David Härdeman
if (r < 0) {
log_error("Failed to query password: %s", strerror(-r));
return r;
@@ -281,7 +285,10 @@ static int get_password(const char *name, usec_t until, bool accept_cached, char
if (asprintf(&text, "Please enter passphrase for disk %s! (verification)", name) < 0)
return log_oom();
- r = ask_password_auto(text, "drive-harddisk", until, false, &passwords2);
+ if (asprintf(&id, "cryptsetup-verification:%s", name) < 0)
+ return log_oom();
+
Similar here.
Otherwise this looks good to go, but I'd like to see a comment by
Benjamin, to see if this would work for him, too!
Lennart
--
Lennart Poettering, Red Hat
--
David Härdeman
Lennart Poettering
2014-03-25 01:06:30 UTC
Permalink
Post by David Härdeman
So yes, I think they're related...as in they are independent
implementations of the same thing :)
Would be great on settle on a common implementation... ;-)
Post by David Härdeman
Post by Lennart Poettering
Also, please use strappend() for cases like this, where we just want to
concatenate two strings.
That said, I wodner, if we should escape the second part of the string,
just to be sure. Using cescape() here would suffice?
And risk breaking backward compatibility? (I was too lazy to check
exactly what cescape() handles right now)...
Well, I mostly care for what is written in the drop-in snippet. The idea
is that the receiving side would unescape the thing again.

There's cescape() and cunespace() already defined, which should make
this easy.
Post by David Härdeman
The "name" (second part of the string) comes from /etc/crypttab, which
is writeable by root only on any sane system? (otherwise it'd be dead
easy to insert a keyscript in between which does some keylogging).
Well, it's fine if crypttab doesn't do escaping, but when we write it to
the snippet and when we read it I think we should do the escaping, since
the whole logic might end up being used in other cases where a password
is queried, too. The id used here, is after all foreign to the password
api stuff, it just passes it along, hence it should maybe just escape
the whole thing, and pass it through, and that would be it.

Lennart
--
Lennart Poettering, Red Hat
Benjamin SANS
2014-03-25 08:50:10 UTC
Permalink
Hi David
Post by David Härdeman
I think Benjamin and I have basically both come up with the same
solution (though I haven't changed the option from "keyscript=" to
"keyhandler=" since that would break backwards compatibility...which is
partly the point of the whole exercise)...
I agree here, the keyscript option would be much better.
Post by David Härdeman
Bejamin's approach does not seem to solve the binary key part of the
puzzle either...(passing binary keys from the keyscript, as opposed to
passphrases).
Actually it does, but I'm not very proud of the fix...
Here is an explanation:

- When using a keyscript, the agent creates a temporary file like so:

char temp[] = "/run/systemd/ask-password/tmp.XXXXXX";
int fd = mkostemp(temp, O_WRONLY|O_CLOEXEC);

- It then forks, redirect the standard output of the child to this temporary
file, and execv the keyscript.

- Finally, it returns via the socket the path of this temporary file.

But all of this is based on the assumption that /run is a tmpfs and that it is
safe enough to temporary store the key.
Post by David Härdeman
My proposed approach is provided in more detail in the corresponding
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=618862#44
So yes, I think they're related...as in they are independent
implementations of the same thing :)
Post by Lennart Poettering
http://lists.freedesktop.org/archives/systemd-devel/2014-March/017869.html
Benjamin, can you comment?
A more detailed comment is available here:
http://lists.freedesktop.org/archives/systemd-devel/2014-March/017955.html
--
Benjamin SANS
David Härdeman
2014-03-25 10:01:22 UTC
Permalink
Post by Benjamin SANS
Post by David Härdeman
Bejamin's approach does not seem to solve the binary key part of the
puzzle either...(passing binary keys from the keyscript, as opposed to
passphrases).
Actually it does, but I'm not very proud of the fix...
char temp[] = "/run/systemd/ask-password/tmp.XXXXXX";
int fd = mkostemp(temp, O_WRONLY|O_CLOEXEC);
- It then forks, redirect the standard output of the child to this temporary
file, and execv the keyscript.
- Finally, it returns via the socket the path of this temporary file.
But all of this is based on the assumption that /run is a tmpfs and that it is
safe enough to temporary store the key.
Ah, mea culpa. I missed that you proposed changes to systemd's own
agent as well. Myopia, since my approach was to use an additional (new)
agent which only handles the keyscript= case, I just assumed you did as
well :)

BTW, it should be noted that since the agent API allows for concurrent
agents, it's still possible to use e.g. both a keyscript and keyboard
input as a backup with my approach...the systemd agent is smart enough
to remove the TTY passphrase prompt once an answer has been provided via
the other agent.
--
David Härdeman
David Härdeman
2014-03-25 01:13:51 UTC
Permalink
Post by Lennart Poettering
Post by David Härdeman
- r = ask_password_auto(text, "drive-harddisk", until, accept_cached, passwords);
+ if (asprintf(&id, "cryptsetup:%s", name) < 0)
+ return log_oom();
+
+ r = ask_password_auto(text, "drive-harddisk", id, until,
accept_cached, passwords);
Hmm, no tabs please...
I've told vim to respect your indentation :)
Post by Lennart Poettering
Also, please use strappend() for cases like this, where we just want to
concatenate two strings.
Hmmm, the asprinf use there matches the style of the code of the rest of
the function....for example, with the patch applied that part reads:

if (asprintf(&text, "Please enter passphrase for disk %s!", name) < 0)
return log_oom();

if (asprintf(&id, "cryptsetup:%s", name) < 0)
return log_oom();

Changing the second asprintf to use strappend and cescape wouldn't
really make it more readable, would it?
Post by Lennart Poettering
Post by David Härdeman
if (r < 0) {
log_error("Failed to query password: %s", strerror(-r));
return r;
@@ -281,7 +285,10 @@ static int get_password(const char *name, usec_t until, bool accept_cached, char
if (asprintf(&text, "Please enter passphrase for disk %s! (verification)", name) < 0)
return log_oom();
- r = ask_password_auto(text, "drive-harddisk", until, false, &passwords2);
+ if (asprintf(&id, "cryptsetup-verification:%s", name) < 0)
+ return log_oom();
+
Similar here.
Ibid
--
David Härdeman
Lennart Poettering
2014-03-25 02:48:09 UTC
Permalink
Post by David Härdeman
Post by Lennart Poettering
Also, please use strappend() for cases like this, where we just want to
concatenate two strings.
Hmmm, the asprinf use there matches the style of the code of the rest of
Yeah, the password query stuff is relatively old code, and strappend()
is a more recent addition...

We try to avoid asprintf() where possible, on the simple grounds that is
is measurebly slow. In this case that doesn't matter much though, since
it's nothing we execute repeatedly.

Anyway, it's nitpicking, doesn't really matter and I'd merge it
anyway...

Oh, one thing though: if the concatenated strings are known to be
limited in size, then strappenda() is a better option actually than
either asprintf() or strappend(). It allocates on the stack, and thus
doesn't fail, and requires no checking nor freeing. Doesn't work inside
of loops though.
Post by David Härdeman
if (asprintf(&text, "Please enter passphrase for disk %s!", name) < 0)
return log_oom();
if (asprintf(&id, "cryptsetup:%s", name) < 0)
return log_oom();
Changing the second asprintf to use strappend and cescape wouldn't
really make it more readable, would it?
If the id concat/escape stuff is needed multiple times, it might be a
good idea to turn it into a function of its own...

Lennart
--
Lennart Poettering, Red Hat
David Härdeman
2014-02-12 22:55:16 UTC
Permalink
The command line key-size is in bits but the libcryptsetup API expects bytes.
---
src/cryptsetup/cryptsetup.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/cryptsetup/cryptsetup.c b/src/cryptsetup/cryptsetup.c
index f72cf9f..e7e8066 100644
--- a/src/cryptsetup/cryptsetup.c
+++ b/src/cryptsetup/cryptsetup.c
@@ -414,7 +414,7 @@ static int attach_luks_or_plain(struct crypt_device *cd,
/* for CRYPT_PLAIN limit reads
* from keyfile to key length, and
* ignore keyfile-size */
- opt_keyfile_size = opt_key_size / 8;
+ opt_keyfile_size = opt_key_size;

/* In contrast to what the name
* crypt_setup() might suggest this
@@ -577,7 +577,11 @@ int main(int argc, char *argv[]) {
else
until = 0;

- opt_key_size = (opt_key_size > 0 ? opt_key_size : 256);
+ if (opt_key_size % 8) {
+ log_warning("Key size invalid (not a multiple of 8).");
+ goto finish;
+ }
+ opt_key_size = (opt_key_size > 0 ? opt_key_size : 256) / 8;

if (key_file) {
struct stat st;
Lennart Poettering
2014-03-25 00:10:37 UTC
Permalink
Post by David Härdeman
The command line key-size is in bits but the libcryptsetup API expects bytes.
This doesn't apply anymore :-(

Could you rebase please?
Post by David Härdeman
---
src/cryptsetup/cryptsetup.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/cryptsetup/cryptsetup.c b/src/cryptsetup/cryptsetup.c
index f72cf9f..e7e8066 100644
--- a/src/cryptsetup/cryptsetup.c
+++ b/src/cryptsetup/cryptsetup.c
@@ -414,7 +414,7 @@ static int attach_luks_or_plain(struct crypt_device *cd,
/* for CRYPT_PLAIN limit reads
* from keyfile to key length, and
* ignore keyfile-size */
- opt_keyfile_size = opt_key_size / 8;
+ opt_keyfile_size = opt_key_size;
/* In contrast to what the name
* crypt_setup() might suggest this
@@ -577,7 +577,11 @@ int main(int argc, char *argv[]) {
else
until = 0;
- opt_key_size = (opt_key_size > 0 ? opt_key_size : 256);
+ if (opt_key_size % 8) {
+ log_warning("Key size invalid (not a multiple of 8).");
+ goto finish;
+ }
+ opt_key_size = (opt_key_size > 0 ? opt_key_size : 256) / 8;
I'd really prefer if we devide here by 8 as early as we parse it, so
that we never get confused by this. i.e. move this to parse_one_option()?

Lennart
--
Lennart Poettering, Red Hat
David Härdeman
2014-02-12 22:55:21 UTC
Permalink
Fix askpass overflow in reading a passphrase from a tty.
Doesn't seem security sensitive, but add a check for correctness.
---
src/shared/ask-password-api.c | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/src/shared/ask-password-api.c b/src/shared/ask-password-api.c
index 1c18274..045cff2 100644
--- a/src/shared/ask-password-api.c
+++ b/src/shared/ask-password-api.c
@@ -213,6 +213,11 @@ int ask_password_tty(
loop_write(ttyfd, "*", 1, false);

dirty = true;
+
+ if (p >= (sizeof(passphrase) - 1)) {
+ loop_write(ttyfd, "\n", 1, false);
+ break;
+ }
}
}
Lennart Poettering
2014-03-25 00:29:10 UTC
Permalink
Post by David Härdeman
Fix askpass overflow in reading a passphrase from a tty.
Doesn't seem security sensitive, but add a check for correctness.
Ouch, embarassing!
Post by David Härdeman
---
src/shared/ask-password-api.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/src/shared/ask-password-api.c b/src/shared/ask-password-api.c
index 1c18274..045cff2 100644
--- a/src/shared/ask-password-api.c
+++ b/src/shared/ask-password-api.c
@@ -213,6 +213,11 @@ int ask_password_tty(
loop_write(ttyfd, "*", 1, false);
dirty = true;
+
+ if (p >= (sizeof(passphrase) - 1)) {
+ loop_write(ttyfd, "\n", 1, false);
+ break;
+ }
Hmm, we should probably write an \a out and then ignore the char, i
figure... I added such a change now.

Thanks!

Lennart
--
Lennart Poettering, Red Hat
Continue reading on narkive:
Loading...