On Sun, Aug 05, 2018 at 12:01:09PM -0600, Bob Beck wrote:
> So this gets rid of unveil's PLEDGE_STAT.
>
> Instead we use UNVEIL_INSPECT which is set by the stat and access opeerations
> that are needed for realpath() type traversals that effectively call
> stat/access
> for each component of a pathname before doing a final operation on the end.
>
> The intended semantic of UNVEIL_INSPECT (which is only used in the kernel)
> is to allow inspection of vnodes that are traversed on the way to an
> unveil'ed component - just like what PLEDGE_STAT did.
>
> This also removes the use of PLEDGE_STATLIE in unveil - theo and I had
> discussed that this was probably fine in lubljana, but I never did it
> then. I'll remove STATLIE later if we decide that's the way we
> are going.
PLEDGE_STATLIE is a leftover from previous pledge-whitelisted-paths
implementation. currently, it is unused in pledge(2).
> Passes regress - realpath still works, etc. etc.
> ok?
globally fine with me. but some comments inlined
> Index: kern/kern_unveil.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/kern_unveil.c,v
> retrieving revision 1.11
> diff -u -p -u -p -r1.11 kern_unveil.c
> --- kern/kern_unveil.c 5 Aug 2018 14:23:57 -0000 1.11
> +++ kern/kern_unveil.c 5 Aug 2018 17:45:52 -0000
> @@ -394,10 +401,15 @@ unveil_add_traversed_vnodes(struct proc
> if (unveil_lookup(vp, p) == NULL) {
> vref(vp);
> vp->v_uvcount++;
> - unveil_add_vnode(p->p_p, vp);
> + uv = unveil_add_vnode(p->p_p, vp);
> + if (uv != NULL)
> + uv->uv_flags = UNVEIL_INSPECT;
> + else
> + ret = E2BIG;
currently, unveil_add_vnode() couldn't return NULL (memory is already
allocated, and the fact that free cell is available was already checked).
Additionally, it will be complex to deal with a potential E2BIG without
side-effects: at this point we already committed several things (final
vnode is already added, name in vnode registered, ...)
> }
> }
> }
> + return ret;
> }
>
> int
> @@ -524,7 +536,7 @@ unveil_add(struct proc *p, struct nameid
>
> done:
> if (ret == 0)
> - unveil_add_traversed_vnodes(p, ndp);
> + ret = unveil_add_traversed_vnodes(p, ndp);
> unveil_free_traversed_vnodes(ndp);
> pool_put(&namei_pool, ndp->ni_cnd.cn_pnbuf);
> return ret;
we couldn't return an error and have changes committed. if you want it,
we need to unroll all the previous changes.
> @@ -607,7 +609,7 @@ unveil_check_component(struct proc *p, s
> (uv = unveil_lookup(dp, p)) != NULL) {
> /* if directory flags match, it's a match */
> if (unveil_flagmatch(ni, uv->uv_flags)) {
> - if (uv->uv_flags) {
> + if (uv->uv_flags & UNVEIL_RWXC) {
> ni->ni_unveil_match = uv;
I spent some time to understand the meaning: you don't want to
considere UNVEIL_INJECT for setting ni_unveil_match.
naming is really a complex thing :)
using UNVEIL_USERSET, for the bits that could be set by the user ? (it
is a similar name used for pledge).
> #ifdef DEBUG_UNVEIL
> printf("unveil: %s(%d): component
> directory match"
> Index: sys/namei.h
> ===================================================================
> RCS file: /cvs/src/sys/sys/namei.h,v
> retrieving revision 1.36
> diff -u -p -u -p -r1.36 namei.h
> --- sys/namei.h 5 Aug 2018 14:23:57 -0000 1.36
> +++ sys/namei.h 5 Aug 2018 17:45:52 -0000
> @@ -257,5 +257,7 @@ struct nchstats {
> #define UNVEIL_WRITE 0x02
> #define UNVEIL_CREATE 0x04
> #define UNVEIL_EXEC 0x08
> +#define UNVEIL_RWXC 0x0F
> +#define UNVEIL_INSPECT 0x80
>
> #endif /* !_SYS_NAMEI_H_ */
please remove UNVEIL_* flags from kern/kern_unveil.c or keep them in
sync.
> Index: sys/pledge.h
> ===================================================================
> RCS file: /cvs/src/sys/sys/pledge.h,v
> retrieving revision 1.37
> diff -u -p -u -p -r1.37 pledge.h
> --- sys/pledge.h 13 Jul 2018 09:25:23 -0000 1.37
> +++ sys/pledge.h 5 Aug 2018 17:45:52 -0000
> @@ -68,9 +68,8 @@
> * to track program behaviours which have been observed.
> */
> #define PLEDGE_USERSET 0x0fffffffffffffffULL
> -#define PLEDGE_STAT 0x2000000000000000ULL /* XXX this is a stat */
> -#define PLEDGE_STATLIE 0x4000000000000000ULL
> -#define PLEDGE_YPACTIVE 0x8000000000000000ULL /* YP use detected and
> allowed */
> +#define PLEDGE_STATLIE 0x2000000000000000ULL
> +#define PLEDGE_YPACTIVE 0x4000000000000000ULL /* YP use detected and
> allowed */
>
> #ifdef PLEDGENAMES
> static struct {
please don't change PLEDGE_YPACTIVE neither PLEDGE_STATLIE bits. they
starts wih high bit set and go to lower.
--
Sebastien Marie
