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)
> return (error);
> vp = nd.ni_vp;
> @@ -2035,6 +2054,7 @@ dochflagsat(struct proc *p, int fd, cons
> follow = (atflags & AT_SYMLINK_NOFOLLOW) ? NOFOLLOW : FOLLOW;
> NDINITAT(&nd, LOOKUP, follow, UIO_USERSPACE, fd, path, p);
> nd.ni_pledge = PLEDGE_FATTR | PLEDGE_RPATH;
> + nd.ni_unveil = UNVEIL_WRITE;
I am unsure if it should be UNVEIL_READ | UNVEIL_WRITE. but I think you
still need UNVEIL_READ on the parent in order to being here. so it
should be ok (I will try to verify it).
> if ((error = namei(&nd)) != 0)
> return (error);
> return (dovchflags(p, nd.ni_vp, flags));
> @@ -2138,6 +2158,7 @@ dofchmodat(struct proc *p, int fd, const
> follow = (flag & AT_SYMLINK_NOFOLLOW) ? NOFOLLOW : FOLLOW;
> NDINITAT(&nd, LOOKUP, follow, UIO_USERSPACE, fd, path, p);
> nd.ni_pledge = PLEDGE_FATTR | PLEDGE_RPATH;
> + nd.ni_unveil = UNVEIL_WRITE;
> if ((error = namei(&nd)) != 0)
> return (error);
> vp = nd.ni_vp;
> @@ -2237,6 +2258,7 @@ dofchownat(struct proc *p, int fd, const
> follow = (flag & AT_SYMLINK_NOFOLLOW) ? NOFOLLOW : FOLLOW;
> NDINITAT(&nd, LOOKUP, follow, UIO_USERSPACE, fd, path, p);
> nd.ni_pledge = PLEDGE_CHOWN | PLEDGE_RPATH;
> + nd.ni_unveil = UNVEIL_WRITE;
> if ((error = namei(&nd)) != 0)
> return (error);
> vp = nd.ni_vp;
> @@ -2289,6 +2311,7 @@ sys_lchown(struct proc *p, void *v, regi
>
> NDINIT(&nd, LOOKUP, NOFOLLOW, UIO_USERSPACE, SCARG(uap, path), p);
> nd.ni_pledge = PLEDGE_CHOWN | PLEDGE_RPATH;
> + nd.ni_unveil = UNVEIL_WRITE;
> if ((error = namei(&nd)) != 0)
> return (error);
> vp = nd.ni_vp;
> @@ -2441,6 +2464,7 @@ doutimensat(struct proc *p, int fd, cons
> follow = (flag & AT_SYMLINK_NOFOLLOW) ? NOFOLLOW : FOLLOW;
> NDINITAT(&nd, LOOKUP, follow, UIO_USERSPACE, fd, path, p);
> nd.ni_pledge = PLEDGE_FATTR | PLEDGE_RPATH;
> + nd.ni_unveil = UNVEIL_WRITE;
> if ((error = namei(&nd)) != 0)
> return (error);
> vp = nd.ni_vp;
> @@ -2588,6 +2612,7 @@ sys_truncate(struct proc *p, void *v, re
>
> NDINIT(&nd, LOOKUP, FOLLOW, UIO_USERSPACE, SCARG(uap, path), p);
> nd.ni_pledge = PLEDGE_FATTR | PLEDGE_RPATH;
> + nd.ni_unveil = UNVEIL_WRITE;
> if ((error = namei(&nd)) != 0)
> return (error);
> vp = nd.ni_vp;
> @@ -2713,6 +2738,7 @@ dorenameat(struct proc *p, int fromfd, c
> 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).
> 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.
> if ((error = namei(&nd)) != 0)
> return (error);
> vp = nd.ni_vp;
> Index: sys/namei.h
> ===================================================================
> RCS file: /cvs/src/sys/sys/namei.h,v
> retrieving revision 1.35
> diff -u -p -u -p -r1.35 namei.h
> --- sys/namei.h 13 Jul 2018 09:25:23 -0000 1.35
> +++ sys/namei.h 4 Aug 2018 15:53:12 -0000
> @@ -59,6 +59,7 @@ struct nameidata {
> struct vnode *ni_startdir; /* starting directory */
> struct vnode *ni_rootdir; /* logical root directory */
> uint64_t ni_pledge; /* expected pledge for namei */
> + u_char ni_unveil; /* required unveil flags for namei */
> /*
> * Results: returned from/manipulated by lookup
> */
> @@ -250,4 +251,11 @@ struct nchstats {
> { "ncs_dothits", CTLTYPE_QUAD }, \
> { "nch_dotdothits", CTLTYPE_QUAD }, \
> }
> +
> +/* Unveil flags for namei */
> +#define UNVEIL_READ 0x01
> +#define UNVEIL_WRITE 0x02
> +#define UNVEIL_CREATE 0x04
> +#define UNVEIL_EXEC 0x08
> +
> #endif /* !_SYS_NAMEI_H_ */
--
Sebastien Marie
