From: spiros thanasoulas <dsp@2f30.org>
Subject: Re: lpd control missing chdir after chroot
To: Florian Obser <florian@narrans.de>
Cc: tech@openbsd.org
Date: Fri, 22 Mar 2024 11:48:06 -0600

On Fri, Mar 22, 2024 at 06:27:04PM +0100, Florian Obser wrote:
> Amazing that anything works, ever...
> 
> I got an OK from tb but I'm not going to commit this, I think we can just fix the whole function in one go.
> 
> Don't have time for it now though.
ouch i can't believe i hadn't spotted this bug which was
right in front of my eyes :\ !

here is an updated diff which also chroots to /var/empty because
the way it was set up id would chroot to /root which was the home
of the daemon user.

Index: control.c
===================================================================
RCS file: /cvs/src/usr.sbin/lpd/control.c,v
retrieving revision 1.2
diff -u -p -r1.2 control.c
--- control.c   28 Dec 2022 21:30:17 -0000      1.2
+++ control.c   22 Mar 2024 17:44:35 -0000
@@ -25,6 +25,7 @@
 #include <errno.h>
 #include <event.h>
 #include <imsg.h>
+#include <paths.h>
 #include <pwd.h>
 #include <signal.h>
 #include <stdlib.h>
@@ -71,13 +72,15 @@ control(int debug, int verbose)
        if ((pw = getpwnam(LPD_USER)) == NULL)
                fatalx("unknown user " LPD_USER);

+       if (chroot(_PATH_VAREMPTY) == -1)
+               fatal("%s: chroot", __func__);
+       if (chdir("/") == -1)
+               fatal("%s: chdir", __func__);
+
        if (setgroups(1, &pw->pw_gid) ||
            setresgid(pw->pw_gid, pw->pw_gid, pw->pw_gid) ||
            setresuid(pw->pw_uid, pw->pw_uid, pw->pw_uid))
                fatal("cannot drop privileges");
-
-       if (chroot(pw->pw_dir) == 1)
-               fatal("%s: chroot", __func__);

        if (pledge("stdio unix recvfd sendfd", NULL) == -1)
                fatal("%s: pledge", __func__);


> 
> On 22 March 2024 18:22:45 CET, Theo Buehler <tb@theobuehler.org> wrote:
> >On Fri, Mar 22, 2024 at 06:18:18PM +0100, Alexander Bluhm wrote:
> >> On Fri, Mar 22, 2024 at 05:58:16PM +0100, Florian Obser wrote:
> >> > this seems reasonable.
> >> > OK florian or I can commit it if someone wants to OK it...
> >> 
> >> It calls chroot() after setresuid().  How can that work?
> >> frontend() does it in correct order.
> >
> >It "works" because the return value check is wrong: == 1 vs == -1.
> >(I missed that before seeing your mail).
> >
> >> 
> >> > On 2024-03-22 10:40 -06, spiros thanasoulas <dsp@2f30.org> wrote:
> >> > > Hey list! 
> >> > > I had sent this about a month ago and didn't get any
> >> > > feedback, so giving it a second shot. 
> >> > > lpd control is missing chdiring after chroot. any comments?
> >> > > Thanks :)
> >> > >
> >> > > On Tue, Feb 13, 2024 at 09:52:47AM -0700, spiros thanasoulas wrote:
> >> > >> This adds it in the same fashion as it appears in frontend.c
> >> > >> 
> >> > >
> >> > > Index: control.c
> >> > > ===================================================================
> >> > > RCS file: /cvs/src/usr.sbin/lpd/control.c,v
> >> > > retrieving revision 1.2
> >> > > diff -u -p -u -p -r1.2 control.c
> >> > > --- control.c   28 Dec 2022 21:30:17 -0000      1.2
> >> > > +++ control.c   13 Feb 2024 16:47:43 -0000
> >> > > @@ -78,6 +78,8 @@ control(int debug, int verbose)
> >> > >
> >> > >         if (chroot(pw->pw_dir) == 1)
> >> > >                 fatal("%s: chroot", __func__);
> >> > > +        if (chdir("/") == -1)
> >> > > +                fatal("%s: chdir", __func__);
> >> > >
> >> > >         if (pledge("stdio unix recvfd sendfd", NULL) == -1)
> >> > >                 fatal("%s: pledge", __func__);
> >> > >
> >> > 
> >> > -- 
> >> > In my defence, I have been left unsupervised.
> >> 
> >
> 
> -- 
> Sent from a mobile device. Please excuse poor formatting.
>