yeah the latter will be the way to go On Mon, Jul 30, 2018 at 06:02 Sebastien Marie <[email protected]> wrote:
> Hi, > > I think unveil_flagmatch() isn't complete and/or has not the right > semantic. > > A bit of internals for starting (I will speak about ni_pledge, people > that know what it is and how it works with pledge/unveil could go to > "what is the problem" part). > > unveil(2) works with the syscall annotation introduced for pledge(2). > > When kernel needs to resolv a name to vnode, it used namei() function. > For that it initializes a special structure used as namei() argument: > `struct nameidata`. This struct has a field `ni_pledge` used to let vfs > subsystem know what kind of syscalls called it. This way, underline > subsystem doesn't have to know all syscalls, and could work on them by > "group". > > For example, when you call open(2), depending the flags argument used, > ni_pledge will contain PLEDGE_RPATH, PLEDGE_WPATH, or PLEDGE_CPATH. The > subsystem has a quick and accurate view of the intented usage of the > vnode. > > pledge(2) uses it to check that you have the promise of intent to use, > and unveil(2) uses it too in a similar way, in particular in > unveil_flagmatch() to check if the process has the accurate permission > for this particular vnode. > > > > Now, what is the problem with unveil_flagmatch() ? > > If I exclude the PLEDGE_STAT stuff from the equation (I am not still > convinced it is the right way to do it - but it isn't the question for > now), unveil_flagmatch() has a default policy to allow the operation, > and only check for some flags in ni_pledge to deny it. > > For simple syscall like open(2) there is no problem. The possible > value of ni_pledge are composed with only PLEDGE_RPATH, PLEDGE_WPATH, or > PLEDGE_CPATH. > > But for some others syscalls, it isn't the case. > > For example, chmod(2) will set ni_pledge to "PLEDGE_FATTR | > PLEDGE_RPATH". For pledge(2), it means you must have "fattr" (capability > to change attribute on the node) and "rpath" (capability to read the > node) promise to use such syscall. > > As unveil(2) doesn't have the "fattr" concept, and unveil_flagmatch() > will check only for PLEDGE_RPATH, PLEDGE_WPATH, PLEDGE_CPATH and > PLEDGE_EXEC, we end with unsound policy: you could use chmod(2) with just > "r" on unveiled part of the filesystem. > > Some others flags could occurs in ni_pledge: > - PLEDGE_CHOWN: chown(2) family > - PLEDGE_DPATH: mknod(2), mkfifo(2) > - PLEDGE_FATTR: utimes(2) family, chmod(2) family, truncate(2), chflags(2) > - PLEDGE_TTY: revoke(2) > - PLEDGE_UNIX: bind(2), connect(2) > > unveil_flagmatch() has a special case for "" flag: it means deny. > but as soon as you have "r", "w", "x" or "c", you have an allow policy > by default. Check will be done only for a part of ni_pledge. > > > I see basically two possibilities: > - extending unveil(2) permissions to match pledge(2) flags - but I don't > like it, unveil(2) should be kept simple. > > - having a separate namespace for unveil and pledge flags (to avoid > confusion), and translating all pledge flags to unveil flags. > > PLEDGE_RPATH => UNVEIL_READ > PLEDGE_WPATH => UNVEIL_WRITE > PLEDGE_CPATH => UNVEIL_CREATE > PLEDGE_EXEC => UNVEIL_EXEC > PLEDGE_CHOWN => UNVEIL_WRITE > PLEDGE_DPATH => UNVEIL_CREATE > PLEDGE_FATTR => UNVEIL_WRITE > PLEDGE_TTY => UNVEIL_WRITE > PLEDGE_UNIX => UNVEIL_READ|UNVEIL_WRITE (requiring both) > any others => panic(9) > > This way, we could be really exhaustive in unveil_flagmatch() without > having to bother for future PLEDGE flag addition (as we will panic(9) if > some developer doesn't add it where intented). > > Thanks. > -- > Sebastien Marie >
