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

[Alexey Gladkov <gladkov.alexey@gmail.com>]

On Sun, Aug 09, 2020 at 11:50:07PM +0300, Mikhail Novosyolov wrote:
> 
> 
> 9 августа 2020 г. 19:08:47 GMT+03:00, Alexey Gladkov <gladkov.alexey@gmail.com> пишет:
> >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.
> 
> I also don't like it, but don't see much difference with setting
> LOGNAME=vasya before running vlock and then being unable to unlock the
> console without root due to fallback to uid=0...

Now the LOGNAME is essentially not used. The vlock calls getpwnam and if
the pw_uid does not match with current uid, vlock calls getpwuid.
Checking the uid protects against incorrect LOGNAME.

Your patch removes uid check and forces vlock to always use environment
variables. Now an incorrect LOGNAME cannot change the behavior of vlock,
but with your patch it will.

I think your patch is completely wrong.

I rather think that the LOGNAME check is unnecessary here. Dmitry can
probably remember why this check is here.

> > 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.
> 
> What do you mean?

If I want to block the console with a root password, then I can do:

$ sudo vlock

> >
> >> 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
> >kbd@lists.altlinux.org
> >> The same patch was submited as a pull request on Github:
> >https://github.com/legionus/kbd/pull/45
> >> 
> >> _______________________________________________
> >> kbd mailing list
> >> kbd@lists.altlinux.org
> >> https://lists.altlinux.org/mailman/listinfo/kbd
> 
> -- 
> Простите за краткость, создано в K-9 Mail.
> 

-- 
Rgrds, legion