> On Sat, Aug 04, 2018 at 10:40:11AM -0600, Bob Beck wrote:
> > On Fri, Aug 03, 2018 at 06:31:00AM +0200, Sebastien Marie wrote:
> > > On Thu, Aug 02, 2018 at 03:42:03PM +0200, Sebastien Marie wrote:
> > > > On Mon, Jul 30, 2018 at 07:55:35AM -0600, Bob Beck wrote:
> > > > > yeah the latter will be the way to go
> > > > >
> > > >
> >
> > > > new diff with direct lookup using an indirection table.
> > > >
> > >
> > > new (emergency) version with PLEDGE_CHOWN consideration for unveil(2).
> > >
> > > sorry for having missed it.
> > >
> >
> > All good because you gave me inspiration, after I ran your diff.
> >
> > I tied unveil to the pledge flags when I first did it because it was
> > convenient - I think this thig with chmod (and the awkwardness of
> > PLEDGE_STAT, etc. etc.) just shows that this was a decision of
> > convienience in the short term that is going to bite us in the long
> > term.
> >
> > The lookup table is clever, but is frankly, voodoo :) I don't like
> > trying to follow the logic of what maps to what and be concerned
> > about what flags are where just for the sake of this, and it
> > makes things ugly to read.
> >
> > I think I would rather add my own char to the namei structure,
> > and set it appropriately in the same places that pledge does. IMO
> > this makes looking at the source code for system calls much clearer
> > int the kernel - rather than trying to fathom in your head how a
> > combination of pledge flags will turn into unveil.
>
> it seems to me it is a bit duplicating annotations: annotating syscalls
> for pledge, and annotating syscalls for unveil, while pledge has just
> more granuality. but I can understand the problem with an additional
> level for translating pledge-flags to unveil-flags.
>
> in consequence, you will have to change some bits in man page, as "r"
> will not be anymore "corresponding to the pledge(2) promise rpath" :)
>
> > So this is a somewhat "minimal" diff tha puts the flags in
> > namei.h, and checks them as per your change, but rather
> > than using a lookup table just expressly sets them
> > for each system call appropriately.. it passes regress
> > as is.
> >
> > I think after doing this I can probably go in an get rid of
> > the awkward PLEDGE_STAT and simplify BYPASS considerably
> > as well, but I will do that separately.
> >
> > ok?
>
> some comments inlined.
>
> and one important for starting: you should consider (ni_unveil==0) as
> deny by default, instead of allow by default. eventually having an
> panic(9) or an assert(9) (but it should be too early for now in case we
> want it).
>
> it is important because else if we miss an explicit initialisation (the
> struct is zeroed, ni_unveil=0 by default), it will be an allowed
> operation by default and we will *not* see the problem. with deny or a
> panic(9) we will not miss it: someone will complain.
>
> for a panic(9), it could be done in namei(), eventually just after
> pledge_namei() call, to avoid checking the variable too often.
>
>
> another important thing I just realized: not all filesystem operations
> were pledeable, and some haven't ni_pledge because the function was
> unreachable while pledged (but pledge_namei() has a panic for such
> cases).
>
> Some examples that will need consideration for unveil(2):
> - mount(2)
> - unmount(2)
> - quotactl(2)
> - chroot(2)
> - getfh(2)
> - acct(2)
> - coredump()
> - loadfirmware() - I think ifconfig(1) could make the kernel loading a
> firmware for some network card
>
> so having ni_unveil separated from ni_pledge could be good to be able to
> manage these namei() calls in unveiled programs.
>
>
> > Index: kern/kern_unveil.c
> > ===================================================================
> > RCS file: /cvs/src/sys/kern/kern_unveil.c,v
> > retrieving revision 1.9
> > diff -u -p -u -p -r1.9 kern_unveil.c
> > --- kern/kern_unveil.c 30 Jul 2018 15:16:27 -0000 1.9
> > +++ kern/kern_unveil.c 4 Aug 2018 16:13:07 -0000
> > @@ -40,6 +40,11 @@
> > #define UNVEIL_MAX_VNODES 128
> > #define UNVEIL_MAX_NAMES 128
> >
> > +#define UNVEIL_READ 0x01
> > +#define UNVEIL_WRITE 0x02
> > +#define UNVEIL_CREATE 0x04
> > +#define UNVEIL_EXEC 0x08
> > +
>
> the flags are duplicated with namei.h. I think namei.h is the right
> place, and there could be removed from here.
>
> > static inline int
> > unvname_compare(const struct unvname *n1, const struct unvname *n2)
> > {
> > @@ -552,32 +558,32 @@ unveil_flagmatch(struct nameidata *ni, u
> > CLR(ni->ni_pledge, PLEDGE_STATLIE);
> > return 1;
> > }
> > - if (ni->ni_pledge & PLEDGE_RPATH) {
> > - if ((flags & PLEDGE_RPATH) == 0) {
> > + if (ni->ni_unveil & UNVEIL_READ) {
> > + if ((flags & UNVEIL_READ) == 0) {
> > #ifdef DEBUG_UNVEIL
> > printf("Pledge wants read but disallowed\n");
>
> some debug printf should be changed too, as you are decoupling unveil from
> pledge.
>
> > #endif
> > return 0;
> > }
> > }
> > - if (ni->ni_pledge & PLEDGE_WPATH) {
> > - if ((flags & PLEDGE_WPATH) == 0) {
> > + if (ni->ni_unveil & UNVEIL_WRITE) {
> > + if ((flags & UNVEIL_WRITE) == 0) {
> > #ifdef DEBUG_UNVEIL
> > printf("Pledge wants write but disallowed\n");
> > #endif
> > return 0;
> > }
> > }
> > - if (ni->ni_pledge & PLEDGE_EXEC) {
> > - if ((flags & PLEDGE_EXEC) == 0) {
> > + if (ni->ni_unveil & UNVEIL_EXEC) {
> > + if ((flags & UNVEIL_EXEC) == 0) {
> > #ifdef DEBUG_UNVEIL
> > printf("Pledge wants exec but disallowed\n");
> > #endif
> > return 0;
> > }
> > }
> > - if (ni->ni_pledge & PLEDGE_CPATH) {
> > - if ((flags & PLEDGE_CPATH) == 0) {
> > + if (ni->ni_unveil & UNVEIL_CREATE) {
> > + if ((flags & UNVEIL_CREATE) == 0) {
> > #ifdef DEBUG_UNVEIL
> > printf("Pledge wants cpath but disallowed\n");
> > #endif
> > Index: kern/vfs_syscalls.c
> > ===================================================================
> > RCS file: /cvs/src/sys/kern/vfs_syscalls.c,v
> > retrieving revision 1.300
> > diff -u -p -u -p -r1.300 vfs_syscalls.c
> > --- kern/vfs_syscalls.c 3 Aug 2018 02:36:11 -0000 1.300
> > +++ kern/vfs_syscalls.c 4 Aug 2018 15:55:25 -0000
> > @@ -1648,6 +1662,7 @@ dounlinkat(struct proc *p, int fd, const
> > NDINITAT(&nd, DELETE, LOCKPARENT | LOCKLEAF, UIO_USERSPACE,
> > fd, path, p);
> > nd.ni_pledge = PLEDGE_CPATH;
> > + nd.ni_unveil = UNVEIL_CREATE;
> > if ((error = namei(&nd)) != 0)
> > return (error);
> > vp = nd.ni_vp;
> > @@ -1795,6 +1810,7 @@ dofaccessat(struct proc *p, int fd, cons
> >
> > NDINITAT(&nd, LOOKUP, FOLLOW | LOCKLEAF, UIO_USERSPACE, fd, path, p);
> > nd.ni_pledge = PLEDGE_RPATH | PLEDGE_STAT;
> > + nd.ni_unveil = 0; /* XXX No flags == allow it */
>
> see my comment about ni_unveil != 0.
>
> as you still have check on (ni_pledge & PLEDGE_STAT), it should be still
> ok.
>
> > if ((error = namei(&nd)) != 0)
> > goto out;
> > vp = nd.ni_vp;
> > @@ -1865,6 +1881,7 @@ dofstatat(struct proc *p, int fd, const
> > follow = (flag & AT_SYMLINK_NOFOLLOW) ? NOFOLLOW : FOLLOW;
> > NDINITAT(&nd, LOOKUP, follow | LOCKLEAF, UIO_USERSPACE, fd, path, p);
> > nd.ni_pledge = PLEDGE_RPATH | PLEDGE_STAT;
> > + nd.ni_unveil = 0;
> > if ((error = namei(&nd)) != 0)
> > return (error);
> > error = vn_stat(nd.ni_vp, &sb, p);
> > @@ -1923,6 +1940,7 @@ sys_pathconf(struct proc *p, void *v, re
> > NDINIT(&nd, LOOKUP, FOLLOW | LOCKLEAF, UIO_USERSPACE,
> > SCARG(uap, path), p);
> > nd.ni_pledge = PLEDGE_RPATH;
> > + nd.ni_unveil = UNVEIL_READ;
> > if ((error = namei(&nd)) != 0)
> > return (error);
> > error = VOP_PATHCONF(nd.ni_vp, SCARG(uap, name), retval);
> > @@ -1972,6 +1990,7 @@ doreadlinkat(struct proc *p, int fd, con
> >
> > NDINITAT(&nd, LOOKUP, NOFOLLOW | LOCKLEAF, UIO_USERSPACE, fd, path, p);
> > nd.ni_pledge = PLEDGE_RPATH | PLEDGE_STAT;
> > + nd.ni_unveil = 0;
> > if ((error = namei(&nd)) != 0)
> > NDINITAT(&fromnd, DELETE, WANTPARENT | SAVESTART, UIO_USERSPACE,
> > fromfd, from, p);
> > fromnd.ni_pledge = PLEDGE_RPATH | PLEDGE_CPATH;
> > + fromnd.ni_unveil = UNVEIL_READ | UNVEIL_WRITE;
>
> no. here you need UNVEIL_READ | UNVEIL_CREATE: the node will be unlinked
> if the operation successed. and you used UNVEIL_CREATE for unlink(2).
>
Yes, you are correct here.. changed. my bad
> > if ((error = namei(&fromnd)) != 0)
> > return (error);
> > fvp = fromnd.ni_vp;
> > @@ -2945,6 +2973,7 @@ sys_revoke(struct proc *p, void *v, regi
> >
> > NDINIT(&nd, LOOKUP, FOLLOW, UIO_USERSPACE, SCARG(uap, path), p);
> > nd.ni_pledge = PLEDGE_RPATH | PLEDGE_TTY;
> > + nd.ni_unveil = UNVEIL_READ;
>
> I would put UNVEIL_READ|UNVEIL_WRITE : the invalidation is a kind of
> modification.
>
Yeah, I was on the fence on that one when I did it. You are reading
the tty device from the filesystem, but the thing you are invalidating
is actually an operation on the tty, not anything to do with the
filesystem itself - but this could go either way...... Theo? I want
your opinion here :)
