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
> <https://cgit.freedesktop.org/libbsd/tree/src/readpassphrase.c>
> 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 <dmudrich@gmx.de>
Fixes: 155c9421b935 ("libmisc: agetpass(), erase_pass(): Add functions for getting passwords safely")
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>

diff --git a/libmisc/agetpass.c b/libmisc/agetpass.c
index 52941e51e827a6aab71e08ce7740c27e13f0cb5d..8b45b15dba69200491f31f27eb87880131d3111b 100644 (file)
--- 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);
 }