From 8fc8de382ab3f90bc8eb4807a5ee841a6bb5f647 Mon Sep 17 00:00:00 2001 From: Samanta Navarro Date: Fri, 28 Apr 2023 11:55:14 +0000 Subject: [PATCH] login_prompt: Do not parse environment variables Parsing optional environment variables after a login name is a feature which is neither documented nor available in util-linux or busybox login which are other wide spread login utilities used in Linux distributions as reference. Removing this feature resolves two issues: - A memory leak exists if variables without an equal sign are used, because set_env creates copies on its own. This could lead to OOM situations in privileged part of login or may lead to heap spraying. - Environment variables are not reset between login attempts. This could lead to additional environment variables set for a user who never intended to do so. Proof of Concept on a system with shadow login without PAM and util-linux agetty: 1. Provoke an invalid login, e.g. user `noone` and password `invalid`. This starts shadow login and subsequent inputs are passed through the function login_prompt. 2. Provoke an invalid login with environment variables, e.g. user `noone HISTFILE=/tmp/owo` and password `invalid`. 3. Log in correctly with user `root`. Now you can see with `echo $HISTFILE` that `/tmp/owo` has been set for the root user. This requires a malicious failed login attempt and a successful login within the configured login timeout (default 60 seconds). Signed-off-by: Samanta Navarro --- libmisc/loginprompt.c | 41 ++--------------------------------------- 1 file changed, 2 insertions(+), 39 deletions(-) diff --git a/libmisc/loginprompt.c b/libmisc/loginprompt.c index db1b7313..f1efa3f6 100644 --- a/libmisc/loginprompt.c +++ b/libmisc/loginprompt.c @@ -14,7 +14,6 @@ #include #include #include -#include #include "alloc.h" #include "prototypes.h" @@ -95,51 +94,15 @@ void login_prompt (const char *prompt, char *name, int namesize) /* * Skip leading whitespace. This makes " username" work right. - * Then copy the rest (up to the end or the first "non-graphic" - * character into the username. + * Then copy the rest (up to the end) into the username. */ for (cp = buf; *cp == ' ' || *cp == '\t'; cp++); - for (i = 0; i < namesize - 1 && isgraph (*cp); name[i++] = *cp++); - while (isgraph (*cp)) { - cp++; - } - - if ('\0' != *cp) { - cp++; - } + for (i = 0; i < namesize - 1 && *cp != '\0'; name[i++] = *cp++); name[i] = '\0'; - /* - * This is a disaster, at best. The user may have entered extra - * environmental variables at the prompt. There are several ways - * to do this, and I just take the easy way out. - */ - - if ('\0' != *cp) { /* process new variables */ - char *nvar; - int count = 1; - int envc; - - for (envc = 0; envc < MAX_ENV; envc++) { - nvar = strtok ((0 != envc) ? NULL : cp, " \t,"); - if (NULL == nvar) { - break; - } - if (strchr (nvar, '=') != NULL) { - envp[envc] = nvar; - } else { - size_t len = strlen (nvar) + 32; - envp[envc] = XMALLOCARRAY (len, char); - (void) snprintf (envp[envc], len, - "L%d=%s", count++, nvar); - } - } - set_env (envc, envp); - } - /* * Set the SIGQUIT handler back to its original value */