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"
- 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.
- 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).
Thanks (and sorry for the delay).
--
Sebastien Marie
