Re: [kbd] [PATCH] vlock: allow sudo user to unlock his session

On Sat, Aug 01, 2020 at 04:19:59PM +0300, Mikhail Novosyolov wrote:
> 
> https://github.com/legionus/kbd/pull/45
> 
> 
> If a non-root user ran sth like "sudo -i" and vlock'ed from inside it,
> then that user himself should be able to unlock his console.
> 
> [user@HP-Elite-7300 tmp]$ echo $LOGNAME
> user
> [user@HP-Elite-7300 tmp]$ sudo -i
> root@HP-Elite-7300:~# echo $LOGNAME
> root
> root@HP-Elite-7300:~# echo $SUDO_USER
> user
> root@HP-Elite-7300:~#
> 
> Tested on rosa2019.1 + kbd 2.2.0 + this patch:
> [root@rosa-2019 kbd]# su - user
> [user@rosa-2019 ~]$ sudo -i
> [sudo] password for user:
> [root@rosa-2019 ~]# vlock
> Данное устройство tty (console) не является виртуальной консолью.
> Блокировка console установлена user.
> Пароль:
> [root@rosa-2019 ~]#
> sudo root session was successfully unlocked with user's password.
> [root@rosa-2019 ~]# unset SUDO_USER
> [root@rosa-2019 ~]# vlock
> Данное устройство tty (console) не является виртуальной консолью.
> Блокировка console установлена root.
> Пароль:
> root password is requested without $SUDO_ENV.

I don't like the idea of implicitly changing the user through environment
variables. SUDO_USER can be exposed accidentally or leak into the
environment due to an error. In this case, you will lock the console
without being able to unlock.

Also, your patch will not allow you to block the console by another user
or by root.

> Another vlock implementation [1, 2] does not check that UIDs match,
> I do not see sense in this check, removing it to make what I want work.
> 
> [1] Another vlock implementation: https://github.com/WorMzy/vlock
> [2] My similar patch for it: https://github.com/mikhailnov/vlock/commit/ba38d5d563cdfaad3b2f260248b3434c235a7afd
> ---
>  src/vlock/username.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/src/vlock/username.c b/src/vlock/username.c
> index a26a148..4c6d295 100644
> --- a/src/vlock/username.c
> +++ b/src/vlock/username.c
> @@ -40,17 +40,18 @@ get_username(void)
>  {
>      const char *name;
>      struct passwd *pw = 0;
> +    char *logname = NULL;
>      uid_t uid         = getuid();
>  
> -    char *logname = getenv("LOGNAME");
> +    /* If a non-root runs a sudo session, ask for user's
> +     * password to unlock it, not root's password */
> +    logname = getenv("SUDO_USER");
> +    if (logname == NULL)
> +        logname = getenv("LOGNAME");
>  
> -    if (logname) {
> -        pw = getpwnam(logname);
> -        /* Ensure uid is same as current. */
> -        if (pw && pw->pw_uid != uid)
> -            pw = 0;
> -    }
> -    if (!pw)
> +    pw = getpwnam(logname);
> +
> +    if (!pw && uid)
>          pw = getpwuid(uid);
>  
>      if (!pw)
> -- 
> 
> Please CC me when replying, I am not subscribed to [email protected]
> The same patch was submited as a pull request on Github: https://github.com/legionus/kbd/pull/45
> 
> _______________________________________________
> kbd mailing list
> [email protected]
> https://lists.altlinux.org/mailman/listinfo/kbd

-- 
Rgrds, legion