On Fri, Jan 07, 2022 at 09:56:47AM +0100, Sebastien Marie wrote:
> On Tue, Dec 21, 2021 at 01:57:01PM +0100, Claudio Jeker wrote:
> > On Wed, Dec 01, 2021 at 04:43:13PM +0100, Claudio Jeker wrote:
> > > On Wed, Dec 01, 2021 at 02:14:40PM +0100, Sebastien Marie wrote:
> > > > Hi,
> > > >
> > > > I have a program with unexpected unveil violation.
> > > >
> > > > I put the whole / read-only, and next few programs executable (the
> > > > purpose is to restrict the executable files to only a small set).
> > > >
> > > > The directory containing the executable is not visible anymore.
> > > >
> > > > $ cat test.c
> > > > #include <sys/stat.h>
> > > >
> > > > #include <err.h>
> > > > #include <stdio.h>
> > > > #include <stdlib.h>
> > > > #include <unistd.h>
> > > >
> > > > int
> > > > main(int argc, char *argv[])
> > > > {
> > > > struct stat sb;
> > > >
> > > > if (unveil("/", "r") == -1)
> > > > err(EXIT_FAILURE, "unveil: /");
> > > > if (unveil("/usr/bin/id", "rx") == -1)
> > > > err(EXIT_FAILURE, "unveil: /usr/bin/id");
> > > >
> > > > if (unveil(NULL, NULL) == -1)
> > > > err(EXIT_FAILURE, "unveil");
> > > >
> > > > if (stat("/usr/bin", &sb) == -1)
> > > > err(EXIT_FAILURE, "stat: /usr/bin");
> > > >
> > > > return EXIT_SUCCESS;
> > > > }
> > > > $ cc -Wall test.c
> > > > $ ./a.out
> > > > a.out: stat: /usr/bin: No such file or directory
> > > >
> > > > If I explicity add `unveil("/usr/bin", "r")`, it is working as expected.
> > > >
> > > > The order of unveil("/") and unveil("/usr/bin/id") doesn't change the
> > > > problem. unveil(NULL, NULL) isn't required for reproducing.
> > > >
> > >
> > > The problem is that /usr/bin is a unveil node to cover the terminal
> > > /usr/bin/id file. Now this node has an no access permissions set and
> > > returns an access violation. Now the could should actually fallback to the
> > > case where covering unveils are looked at.
> > >
> > > The following diff seems to fix your case. Regress still passes as well.
> >
> > This has been sitting around for a long time. What should we do with this?
> >
> > The fix regards directories that have no uv_flags set as no match since they
> > only hold individual file unveil information. The code then passes down to
> > the end where a backtrack up the tree happens looking for the first node
> > that has some flags set.
>
> I finally be able to test it, and I think it is permitting too much.
>
> > diff --git sys/kern/kern_unveil.c sys/kern/kern_unveil.c
> > index 801c210c113..76fc1b3d4db 100644
> > --- sys/kern/kern_unveil.c
> > +++ sys/kern/kern_unveil.c
> > @@ -708,7 +708,7 @@ unveil_check_final(struct proc *p, struct nameidata *ni)
> > if (ni->ni_vp != NULL && ni->ni_vp->v_type == VDIR) {
> > /* We are matching a directory terminal component */
> > uv = unveil_lookup(ni->ni_vp, pr, NULL);
> > - if (uv == NULL) {
> > + if (uv == NULL || (uv->uv_flags & UNVEIL_USERSET) == 0) {
> > #ifdef DEBUG_UNVEIL
> > printf("unveil: %s(%d) no match for vnode %p\n",
> > pr->ps_comm, pr->ps_pid, ni->ni_vp);
> >
>
> Using `(uv->uv_flags & UNVEIL_USERSET) == 0` doesn't differenciate between:
> - the directory has no unveil setting and should use parent value
> - the directory has unveil setting to "" (and shouldn't use parent value)
>
> It means with:
>
> if (unveil("/", "r") == -1)
> err(EXIT_FAILURE, "unveil: /");
> if (unveil("/usr", "") == -1)
> err(EXIT_FAILURE, "unveil: /usr");
>
> any file under /usr is still visible (the unveil value used is the one
> from "/").
>
> There are several possiblities:
> - add a new flag for "unconfigured directory with uvn_name"
I think that is the best way of solving this.
IIRC there was a flag similar to that once but I removed that in a
cleanup. Guess I need to bring that back.
> - change a bit the way adding a name on directory with unveil(2) is done:
> unveil("/", "r")
> => add (perm "r") on vnode "/"
> unveil("/usr/bin/id", "rx")
> => add (perm "r") on vnode "/usr/bin" [value inherited from "/"]
> => add (name "id", perm "rx") on vnode "/usr/bin"
>
> it makes the order of unveil(2) calls important.
I don't think this is an option. unveil(2) tries very hard to make the
order not matter. So lets not add code that breaks this/
> - restrict a bit the unveil(2) API and required explicit unveil
> permission on the directory holding uvn_name (it will make the
> use-case an error).
Again I think this is against the current use cases. Unveil just some
binaries that need to be executed is a common idiom and making this harder
is not a good idea.
> Thanks (and sorry for the delay).
No worries, I will work on a new diff :)
--
:wq Claudio
