On Thu, Jul 18, 2019 at 04:13:10PM +0200, Alexander Bluhm wrote:
> Hi,
> 
> Can we track unveil(2) violators in process accounting lastcomm(1)?
> This makes it easier to find them.
> 
> $ lastcomm | grep -e '-[A-Z]U'
> pflogd     -FU     root    __         0.00 secs Thu Jul 18 14:19 (2:33:22.00)
> 
> Seems that pflogd(8) has to be investigated.

Interesting.

This appears to be a false positive, libpcap will first attempt to open
/dev/bpf as O_RDWR and then falls back to O_RDONLY on EACCES. I'm fairly
confident that pflogd(8) does not need write access to bpf. If anything,
unveil(2) appears to have found an old bug here, as before pflogd was
always opening the device with both read/write permissions.

tcpdump avoids this by internalizing more parts of libpcap, and also
opening /dev/bpf O_RDONLY itself.

spamlogd appears to be the only other user of pcap_open_live() in base,
unfortunately it calls unveil after, so /dev/bpf is opened O_RDWR. I
don't use spamlogd, though.

> Also we keep record about programs that may be exploited and do
> something illegal.  We have the same mechanism for pledge(2).
> 
> Not sure if we want it for both EACCES and ENOENT cases.  If it
> creates false positives, we can change that later to EACCES only.

With all that said, I do feel like false positives are incredibly
likely for unveil. For example, I suspect in ports you'll find
software that checks for Linux-isms (/proc, /sys) and and then
fallback. Maybe just EACCES? I don't know.

> ok?
> 
> bluhm
> 
> Index: kern/kern_unveil.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/kern/kern_unveil.c,v
> retrieving revision 1.27
> diff -u -p -r1.27 kern_unveil.c
> --- kern/kern_unveil.c        14 Jul 2019 03:26:02 -0000      1.27
> +++ kern/kern_unveil.c        18 Jul 2019 12:01:24 -0000
> @@ -18,6 +18,7 @@
> 
>  #include <sys/param.h>
> 
> +#include <sys/acct.h>
>  #include <sys/mount.h>
>  #include <sys/filedesc.h>
>  #include <sys/proc.h>
> @@ -823,6 +824,7 @@ unveil_check_final(struct proc *p, struc
>                           " vnode %p\n",
>                           p->p_p->ps_comm, p->p_p->ps_pid, ni->ni_vp);
>  #endif
> +                     p->p_p->ps_acflag |= AUNVEIL;
>                       if (uv->uv_flags & UNVEIL_USERSET)
>                               return EACCES;
>                       else
> @@ -865,10 +867,11 @@ unveil_check_final(struct proc *p, struc
>                        * EACCESS. Otherwise, use any covering match
>                        * that we found above this dir.
>                        */
> -                     if (uv->uv_flags & UNVEIL_USERSET)
> +                     if (uv->uv_flags & UNVEIL_USERSET) {
> +                             p->p_p->ps_acflag |= AUNVEIL;
>                               return EACCES;
> -                     else
> -                             goto done;
> +                     }
> +                     goto done;
>               }
>               /* directory flags match, update match */
>               if (uv->uv_flags & UNVEIL_USERSET)
> @@ -881,6 +884,7 @@ unveil_check_final(struct proc *p, struc
>               printf("unveil: %s(%d) flag mismatch for terminal '%s'\n",
>                   p->p_p->ps_comm, p->p_p->ps_pid, tname->un_name);
>  #endif
> +             p->p_p->ps_acflag |= AUNVEIL;
>               return EACCES;
>       }
>       /* name and flags match in this dir. update match*/
> @@ -903,8 +907,10 @@ done:
>                   p->p_p->ps_comm, p->p_p->ps_pid, ni->ni_cnd.cn_nameptr,
>                   ni->ni_unveil_match->uv_vp);
>  #endif
> +             p->p_p->ps_acflag |= AUNVEIL;
>               return EACCES;
>       }
> +     p->p_p->ps_acflag |= AUNVEIL;
>       return ENOENT;
>  }
> 
> Index: sys/acct.h
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/sys/acct.h,v
> retrieving revision 1.7
> diff -u -p -r1.7 acct.h
> --- sys/acct.h        8 Jun 2017 17:14:02 -0000       1.7
> +++ sys/acct.h        18 Jul 2019 11:37:27 -0000
> @@ -63,6 +63,7 @@ struct acct {
>  #define      AXSIG   0x10            /* killed by a signal */
>  #define      APLEDGE 0x20            /* killed due to pledge violation */
>  #define      ATRAP   0x40            /* memory access violation */
> +#define      AUNVEIL 0x80            /* unveil access violation */
>       u_int8_t  ac_flag;      /* accounting flags */
>  };
> 
> 

Reply via email to