On Fri, Jan 07, 2022 at 10:21:04AM +0100, Claudio Jeker wrote:
> 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.
Ah no, that was some other flag for something else :)
Anyway I decided to make UNVEIL_USERSET a real flag and set it whenever a
node has flags set by the user. This way it is possible to check if a node
was added implicit or explict.
Now to not alter the result of EACCES vs ENOENT one also needs to check if
any of the other flags was set or not. Because of this I added UNVEIL_MASK
and made sure that all return cases where that matters make this
distinction.
This diff seems to fix both of your test cases and also passes regress.
--
:wq Claudio
Index: kern/kern_unveil.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_unveil.c,v
retrieving revision 1.51
diff -u -p -r1.51 kern_unveil.c
--- kern/kern_unveil.c 9 Sep 2021 13:02:36 -0000 1.51
+++ kern/kern_unveil.c 8 Jan 2022 11:26:01 -0000
@@ -345,7 +345,7 @@ unveil_parsepermissions(const char *perm
size_t i = 0;
char c;
- *perms = 0;
+ *perms = UNVEIL_USERSET;
while ((c = permissions[i++]) != '\0') {
switch (c) {
case 'r':
@@ -708,11 +708,13 @@ unveil_check_final(struct proc *p, struc
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);
#endif
+ if (uv != NULL)
+ ni->ni_unveil_match = uv;
goto done;
}
if (!unveil_flagmatch(ni, uv->uv_flags)) {
@@ -722,7 +724,7 @@ unveil_check_final(struct proc *p, struc
pr->ps_comm, pr->ps_pid, ni->ni_vp);
#endif
pr->ps_acflag |= AUNVEIL;
- if (uv->uv_flags & UNVEIL_USERSET)
+ if (uv->uv_flags & UNVEIL_MASK)
return EACCES;
else
return ENOENT;
@@ -764,12 +766,15 @@ unveil_check_final(struct proc *p, struc
#endif
/*
* If dir has user set restrictions fail with
- * EACCES. Otherwise, use any covering match
- * that we found above this dir.
+ * EACCES or ENOENT. Otherwise, use any covering
+ * match that we found above this dir.
*/
if (uv->uv_flags & UNVEIL_USERSET) {
pr->ps_acflag |= AUNVEIL;
- return EACCES;
+ if (uv->uv_flags & UNVEIL_MASK)
+ return EACCES;
+ else
+ return ENOENT;
}
/* start backtrack from this node */
ni->ni_unveil_match = uv;
@@ -820,7 +825,10 @@ done:
pr->ps_comm, pr->ps_pid, uv->uv_vp);
#endif
pr->ps_acflag |= AUNVEIL;
- return EACCES;
+ if (uv->uv_flags & UNVEIL_MASK)
+ return EACCES;
+ else
+ return ENOENT;
}
#ifdef DEBUG_UNVEIL
printf("unveil: %s(%d) check cover for vnode %p, uv_cover
%zd\n",
Index: sys/namei.h
===================================================================
RCS file: /cvs/src/sys/sys/namei.h,v
retrieving revision 1.48
diff -u -p -r1.48 namei.h
--- sys/namei.h 2 Sep 2021 12:35:23 -0000 1.48
+++ sys/namei.h 8 Jan 2022 11:32:39 -0000
@@ -268,6 +268,7 @@ struct nchstats {
#define UNVEIL_WRITE 0x02
#define UNVEIL_CREATE 0x04
#define UNVEIL_EXEC 0x08
-#define UNVEIL_USERSET 0x0F
+#define UNVEIL_USERSET 0x10
+#define UNVEIL_MASK 0x0F
#endif /* !_SYS_NAMEI_H_ */
