On Mon, Jul 30, 2018 at 12:00:59PM -0600, Theo de Raadt wrote:
> + for (i=0; flags[i].pledge != 0; i++)
> + if (ISSET(pledge_flags, flags[i].pledge)) {
> + SET(permissions, flags[i].unveil);
> + CLR(pledge_flags, flags[i].pledge);
> + }
>
> Rather than iterating, can this be done as a direct lookup?
>
> table[PLEDGE_RPATH] = ...
> table[PLEDGE_RPATH | PLEDGE_WPATH] = ..
>
> unveil = table[pledge & range_enforcing_mask];
direct lookup isn't really possible: we would have to map a 64bits long
array in order to be exhaustive (and catch missing bits).
so I tried several alternate things.
(a) the first one was to try to reduce the array size for iterating, and
collapsing bits.
without detailing, it seems the compiler to be smart enough to unroll
the loops and use constants instead of an indirection with an array
(tested clang on i386, but objdump seems to not show me all the asm, so
I am still a bit unsure). having the "missing bits" mecanism in
DIAGNOSTIC could help too to avoid developpment checks in mainline.
but it has the drawback to put too much hope in the fact the compiler
will do the right thing, specially on all platforms (and compiler
version) we support.
(b) another point could be to extend `struct nameidata' to compute
`ni_unveil' from `ni_pledge' at namei() time. this way, we still derive
unveil flags from pledge flags, but the evaluation is done once per
namei() call (instead of once per compoment).
one possible drawback (or restriction) is the conversion is done on the
*initial* value of ni_pledge. for now, only unveil(2) itself add some
flags to ni_pledge (for example PLEDGE_STATLIE), so it shouldn't be a
problem.
(c) last possibility would be to heavy use macros to deduce UNVEIL
semantic from PLEDGE flags.
it could be done this way:
+#ifdef _KERNEL
+
+/* unveil flags definition (for affectation only) */
+#define UNVEIL_READ PLEDGE_RPATH
+#define UNVEIL_WRITE PLEDGE_WPATH
+#define UNVEIL_CREATE PLEDGE_CPATH
+#define UNVEIL_EXEC PLEDGE_EXEC
+
+#define PLEDGE_UVR_MASK (PLEDGE_RPATH|PLEDGE_UNIX)
+#define PLEDGE_UVW_MASK
(PLEDGE_WPATH|PLEDGE_FATTR|PLEDGE_CHOWN|PLEDGE_TTY|PLEDGE_UNIX)
+#define PLEDGE_UVC_MASK (PLEDGE_CPATH|PLEDGE_DPATH)
+#define PLEDGE_UVX_MASK (PLEDGE_EXEC)
+#define PLEDGE_UVz_MASK (PLEDGE_STAT|PLEDGE_STATLIE)
+
+#define PLEDGE_UV_MASK
(PLEDGE_UVR_MASK|PLEDGE_UVW_MASK|PLEDGE_UVC_MASK|PLEDGE_UVX_MASK\
+ |PLEDGE_UVz_MASK)
+
+/* unveil macros (for manipulating flags) */
+#define unveilable_flags(f) (((f) & ~(PLEDGE_UV_MASK)) == 0)
+#define unveil_read_isset(f) ((f) & PLEDGE_UVR_MASK)
+#define unveil_write_isset(f) ((f) & PLEDGE_UVW_MASK)
+#define unveil_create_isset(f) ((f) & PLEDGE_UVC_MASK)
+#define unveil_exec_isset(f) ((f) & PLEDGE_UVX_MASK)
+
+#endif
and later, use only macros for doing checks:
// affectation using UNVEIL_*
case 'r':
*perms |= UNVEIL_READ;
break;
// checking if value is bounded in what unveil expects
if (! unveilable_flags(ni_pledge))
panic("missing bits in pledge->unveil conversion");
// getting unveil semantic from PLEDGE flags
if (unveil_read_isset(ni_pledge))
...
the main drawback I see with this method, is it could be somehow fragile
for developers. for example, if someone doesn't use unveil_write_isset()
macro but prefer to directly use UNVEIL_WRITE for checking "w", he will
be wrong. it seems to me it isn't a clean way to do the thing (even if
it could be efficient).
in resume: it seems to me that doing the conversion in
unveil_flagmatch() is wrong to me. placing it in namei() seems more
consistent (and less resource consuming). it also permits to have simple
interface with UNVEIL constants instead of using macros.
the diff belows implements it (but I also have full diff for others
possibilities).
in particular:
- use separate namespace for UNVEIL and PLEDGE flags
- UNVEIL flags are `int' values (instead of uint64_t)
- add `ni_unveil' member to `struct nameidata'
- complete `ni_unveil' from `ni_pledge' in namei()
--
Sebastien Marie
Index: sys/namei.h
===================================================================
RCS file: /cvs/src/sys/sys/namei.h,v
retrieving revision 1.35
diff -u -p -r1.35 namei.h
--- sys/namei.h 13 Jul 2018 09:25:23 -0000 1.35
+++ sys/namei.h 31 Jul 2018 08:49:59 -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 */
+ int ni_unveil; /* derived unveil flags from ni_pledge
*/
/*
* Results: returned from/manipulated by lookup
*/
Index: sys/proc.h
===================================================================
RCS file: /cvs/src/sys/sys/proc.h,v
retrieving revision 1.254
diff -u -p -r1.254 proc.h
--- sys/proc.h 28 Jul 2018 18:07:26 -0000 1.254
+++ sys/proc.h 31 Jul 2018 09:02:56 -0000
@@ -130,7 +130,7 @@ struct tusage {
struct unvname {
char *un_name;
size_t un_namesize;
- uint64_t un_flags;
+ int un_flags;
RBT_ENTRY(unvnmae) un_rbt;
};
@@ -424,7 +424,7 @@ struct unveil {
struct vnode *uv_vp;
struct unvname_rbt uv_names;
struct rwlock uv_lock;
- u_int64_t uv_flags;
+ int uv_flags;
};
struct uidinfo {
Index: kern/kern_unveil.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_unveil.c,v
retrieving revision 1.9
diff -u -p -r1.9 kern_unveil.c
--- kern/kern_unveil.c 30 Jul 2018 15:16:27 -0000 1.9
+++ kern/kern_unveil.c 31 Jul 2018 09:30:33 -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_EXEC 0x04
+#define UNVEIL_CREATE 0x08
+
static inline int
unvname_compare(const struct unvname *n1, const struct unvname *n2)
{
@@ -50,7 +55,7 @@ unvname_compare(const struct unvname *n1
}
struct unvname *
-unvname_new(const char *name, size_t size, uint64_t flags)
+unvname_new(const char *name, size_t size, int flags)
{
struct unvname *ret = malloc(sizeof(struct unvname), M_PROC, M_WAITOK);
ret->un_name = malloc(size, M_PROC, M_WAITOK);
@@ -118,7 +123,7 @@ unveil_delete_names(struct unveil *uv)
}
void
-unveil_add_name(struct unveil *uv, char *name, uint64_t flags)
+unveil_add_name(struct unveil *uv, char *name, int flags)
{
struct unvname *unvn;
@@ -310,7 +315,7 @@ unveil_lookup(struct vnode *vp, struct p
}
int
-unveil_parsepermissions(const char *permissions, uint64_t *perms)
+unveil_parsepermissions(const char *permissions, int *perms)
{
size_t i = 0;
char c;
@@ -319,16 +324,16 @@ unveil_parsepermissions(const char *perm
while ((c = permissions[i++]) != '\0') {
switch (c) {
case 'r':
- *perms |= PLEDGE_RPATH;
+ *perms |= UNVEIL_READ;
break;
case 'w':
- *perms |= PLEDGE_WPATH;
+ *perms |= UNVEIL_WRITE;
break;
case 'x':
- *perms |= PLEDGE_EXEC;
+ *perms |= UNVEIL_EXEC;
break;
case 'c':
- *perms |= PLEDGE_CPATH;
+ *perms |= UNVEIL_CREATE;
break;
default:
return -1;
@@ -338,7 +343,7 @@ unveil_parsepermissions(const char *perm
}
int
-unveil_setflags(uint64_t *flags, uint64_t nflags)
+unveil_setflags(int *flags, int nflags)
{
#if 0
if (((~(*flags)) & nflags) != 0) {
@@ -403,7 +408,7 @@ unveil_add(struct proc *p, struct nameid
struct unveil *uv;
int directory_add;
int ret = EINVAL;
- u_int64_t flags;
+ int flags;
KASSERT(ISSET(ndp->ni_cnd.cn_flags, HASBUF)); /* must have SAVENAME */
@@ -525,12 +530,43 @@ unveil_add(struct proc *p, struct nameid
return ret;
}
+void
+unveil_complete_nameidata(struct proc *p, struct nameidata *ni)
+{
+ static const struct {
+ int unveil;
+ uint64_t pledge;
+ } flags[] = {
+ { UNVEIL_READ, PLEDGE_RPATH|PLEDGE_UNIX },
+ { UNVEIL_WRITE,
PLEDGE_WPATH|PLEDGE_FATTR|PLEDGE_CHOWN|PLEDGE_TTY|PLEDGE_UNIX },
+ { UNVEIL_EXEC, PLEDGE_EXEC},
+ { UNVEIL_CREATE, PLEDGE_CPATH|PLEDGE_DPATH },
+ };
+ uint64_t ni_pledge = ni->ni_pledge;
+ int i;
+
+ if (p->p_p->ps_uvpaths == NULL)
+ return;
+
+ for (i=0; i < nitems(flags); i++)
+ if (ISSET(ni_pledge, flags[i].pledge))
+ SET(ni->ni_unveil, flags[i].unveil);
+
+#ifdef DIAGNOSTIC
+ for (i=0; i < nitems(flags); i++)
+ CLR(ni_pledge, flags[i].pledge);
+
+ if ((ni_pledge & ~(PLEDGE_UNVEIL|PLEDGE_STAT|PLEDGE_STATLIE)) != 0)
+ panic("missing flag in pledge->unveil conversion: %llu",
ni_pledge);
+#endif
+}
+
/*
* XXX this will probably change.
* XXX collapse down later once debug surely unneded
*/
int
-unveil_flagmatch(struct nameidata *ni, uint64_t flags)
+unveil_flagmatch(struct nameidata *ni, int flags)
{
if (flags == 0) {
if (ni->ni_pledge & PLEDGE_STAT) {
@@ -552,32 +588,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");
#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_lookup.c
===================================================================
RCS file: /cvs/src/sys/kern/vfs_lookup.c,v
retrieving revision 1.72
diff -u -p -r1.72 vfs_lookup.c
--- kern/vfs_lookup.c 30 Jul 2018 00:16:59 -0000 1.72
+++ kern/vfs_lookup.c 31 Jul 2018 09:31:47 -0000
@@ -59,6 +59,7 @@
void unveil_check_component(struct proc *p, struct nameidata *ni, struct vnode
*dp );
int unveil_check_final(struct proc *p, struct nameidata *ni);
+void unveil_complete_nameidata(struct proc *p, struct nameidata *ni);
void
ndinitat(struct nameidata *ndp, u_long op, u_long flags,
@@ -177,6 +178,9 @@ fail:
error = pledge_namei(p, ndp, cnp->cn_pnbuf);
if (error)
goto fail;
+
+ /* complete ni_unveil flags from ni_pledge */
+ unveil_complete_nameidata(p, ndp);
/*
* Check if starting from root directory or current directory.
