From 5c5dc756419e1d3cad63c66c4bb803eadf1074fd Mon Sep 17 00:00:00 2001 From: Alejandro Colomar Date: Sun, 19 Feb 2023 19:26:56 +0100 Subject: [PATCH] libmisc: agetpass(): Fix bug detecting truncation On 2/19/23 18:09, David Mudrich wrote: > I am working on a RAM based Linux OS from source, and try to use > latest versions of all software. I found shadow needs libbsd's > readpassphrase(3) as superior alternative to getpass(3). While > considering if I a) include libbsd, or include libbsd's code of > readpassphrase(3) into shadow, found, that libbsd's readpassphrase(3) > never returns \n or \r > > line 122, while agetpass() uses a check for \n in agetpass.c line 108. > I assume it always fails. Indeed, it always failed. I made a mistake when writing agetpass(), assuming that readpassphrase(3) would keep newlines. > > I propose a check of len == PASS_MAX - 1, with false positive error for > exactly PASS_MAX - 1 long passwords. Instead, I added an extra byte to the allocation to allow a maximum password length of PASS_MAX (which is the maximum for getpass(3), which we're replacing. While doing that, I notice that my previous implementation also had another bug (minor): The maximum password length was PASS_MAX - 1 instead of PASS_MAX. That's also fixed in this commit. Reported-by: David Mudrich Fixes: 155c9421b935 ("libmisc: agetpass(), erase_pass(): Add functions for getting passwords safely") Cc: Iker Pedrosa Signed-off-by: Alejandro Colomar --- libmisc/agetpass.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/libmisc/agetpass.c b/libmisc/agetpass.c index 52941e51..8b45b15d 100644 --- a/libmisc/agetpass.c +++ b/libmisc/agetpass.c @@ -19,7 +19,7 @@ #if !defined(PASS_MAX) -#define PASS_MAX BUFSIZ +#define PASS_MAX BUFSIZ - 1 #endif @@ -93,29 +93,31 @@ agetpass(const char *prompt) char *pass; size_t len; - pass = malloc(PASS_MAX); + /* + * Since we want to support passwords upto PASS_MAX, we need + * PASS_MAX bytes for the password itself, and one more byte for + * the terminating '\0'. We also want to detect truncation, and + * readpassphrase(3) doesn't detect it, so we need some trick. + * Let's add one more byte, and if the password uses it, it + * means the introduced password was longer than PASS_MAX. + */ + pass = malloc(PASS_MAX + 2); if (pass == NULL) return NULL; - if (readpassphrase(prompt, pass, PASS_MAX, RPP_REQUIRE_TTY) == NULL) + if (readpassphrase(prompt, pass, PASS_MAX + 2, RPP_REQUIRE_TTY) == NULL) goto fail; len = strlen(pass); - - if (len == 0) - return pass; - - if (pass[len - 1] != '\n') { + if (len == PASS_MAX + 1) { errno = ENOBUFS; goto fail; } - pass[len - 1] = '\0'; - return pass; fail: - freezero(pass, PASS_MAX); + freezero(pass, PASS_MAX + 2); return NULL; } @@ -123,5 +125,5 @@ fail: void erase_pass(char *pass) { - freezero(pass, PASS_MAX); + freezero(pass, PASS_MAX + 2); }