Re: unveil in process accounting and lastcomm
On Thu, Jul 25, 2019 at 10:06:52AM -0400, Bryan Steele wrote:
> On Thu, Jul 18, 2019 at 05:44:21PM -0400, Bryan Steele wrote:
> > On Thu, Jul 18, 2019 at 11:46:46AM -0400, Bryan Steele wrote:
> > > 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.
> >
> > Here's a potential diff for both pflogd and spamlogd, I've tested it
> > works with pflogd. I only compile tested spamlogd. But it should avoid
> > the unveil accounting errors.
> >
> > This duplicates a lot of code into both, but I don't feel like trying
> > to extent the libpcap API for this.
> >
> > comments? ok?
>
> With bluhm@'s unveil accounting diff, is anyone ok wit this this
> approach to fixing the issue? These are the only pcap_open_live(3)
> consumers in base, but I don't feel comfortable trying to extend
> the libpcap API, and pretty much everything is already poking at
> libpcap internals due to bad API design for privsep.
>
> -Bryan.
Here's a new diff that renames the local function to avoid conflicting
with future attempts at improving the libpcap API. Requested by deraadt@
-Bryan.
Index: pflogd.c
===
RCS file: /cvs/src/sbin/pflogd/pflogd.c,v
retrieving revision 1.59
diff -u -p -u -r1.59 pflogd.c
--- sbin/pflogd/pflogd.c26 Aug 2018 18:24:46 - 1.59
+++ sbin/pflogd/pflogd.c25 Jul 2019 14:23:14 -
@@ -73,6 +73,7 @@ void dump_packet(u_char *, const struct
void dump_packet_nobuf(u_char *, const struct pcap_pkthdr *, const u_char *);
int flush_buffer(FILE *);
int if_exists(char *);
+pcap_t *pflog_read_live(const char *, int, int, int, char *);
void logmsg(int, const char *, ...);
void purge_buffer(void);
int reset_dump(void);
@@ -194,10 +195,95 @@ if_exists(char *ifname)
return (if_nametoindex(ifname) != 0);
}
+pcap_t *
+pflog_read_live(const char *source, int slen, int promisc, int to_ms,
+char *ebuf)
+{
+ int fd;
+ struct bpf_version bv;
+ struct ifreqifr;
+ u_int v, dlt = DLT_PFLOG;
+ pcap_t *p;
+
+ if (source == NULL || slen <= 0)
+ return (NULL);
+
+ p = pcap_create(source, ebuf);
+ if (p == NULL)
+ return (NULL);
+
+ /* Open bpf(4) read only */
+ if ((fd = open("/dev/bpf", O_RDONLY)) == -1)
+ return (NULL);
+
+ if (ioctl(fd, BIOCVERSION, ) == -1) {
+ snprintf(ebuf, PCAP_ERRBUF_SIZE, "BIOCVERSION: %s",
+ pcap_strerror(errno));
+ goto bad;
+ }
+
+ if (bv.bv_major != BPF_MAJOR_VERSION ||
+ bv.bv_minor < BPF_MINOR_VERSION) {
+ snprintf(ebuf, PCAP_ERRBUF_SIZE,
+ "kernel bpf filter out of date");
+ goto bad;
+ }
+
+ strlcpy(ifr.ifr_name, source, sizeof(ifr.ifr_name));
+ if (ioctl(fd, BIOCSETIF, ) == -1) {
+ snprintf(ebuf, PCAP_ERRBUF_SIZE, "BIOCSETIF: %s",
+ pcap_strerror(errno));
+ goto bad;
+ }
+
+ if (dlt != (u_int) -1 && ioctl(fd, BIOCSDLT, )) {
+ snprintf(ebuf, PCAP_ERRBUF_SIZE, "BIOCSDLT: %s",
+ pcap_strerror(errno));
+ goto bad;
+ }
+
+ p->fd = fd;
+ p->snapshot = slen;
+ p->linktype = dlt;
+
+ /* set timeout */
+ if (to_ms != 0) {
+ struct timeval to;
+ to.tv_sec = to_ms / 1000;
+ to.tv_usec = (to_ms * 1000) % 100;
+ if (ioctl(p->fd, BIOCSRTIMEOUT, ) == -1) {
+ snprintf(ebuf, PCAP_ERRBUF_SIZE, "BIOCSRTIMEOUT: %s",
+ pcap_strerror(errno));
+ }
+ }
+ if (promisc)
+ /* this is allowed to fail */
+ ioctl(fd, BIOCPROMISC, NULL);
+
+ if (ioctl(fd, BIOCGBLEN, ) == -1) {
+
Re: unveil in process accounting and lastcomm
On Thu, Jul 18, 2019 at 05:44:21PM -0400, Bryan Steele wrote:
> On Thu, Jul 18, 2019 at 11:46:46AM -0400, Bryan Steele wrote:
> > 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.
>
> Here's a potential diff for both pflogd and spamlogd, I've tested it
> works with pflogd. I only compile tested spamlogd. But it should avoid
> the unveil accounting errors.
>
> This duplicates a lot of code into both, but I don't feel like trying
> to extent the libpcap API for this.
>
> comments? ok?
With bluhm@'s unveil accounting diff, is anyone ok wit this this
approach to fixing the issue? These are the only pcap_open_live(3)
consumers in base, but I don't feel comfortable trying to extend
the libpcap API, and pretty much everything is already poking at
libpcap internals due to bad API design for privsep.
I'd appreciate if someone could test spamlogd(8) still works.
-Bryan.
Index: pflogd.c
===
RCS file: /cvs/src/sbin/pflogd/pflogd.c,v
retrieving revision 1.59
diff -u -p -r1.59 pflogd.c
--- sbin/pflogd/pflogd.c26 Aug 2018 18:24:46 - 1.59
+++ sbin/pflogd/pflogd.c25 Jul 2019 14:00:02 -
@@ -73,6 +73,7 @@ void dump_packet(u_char *, const struct
void dump_packet_nobuf(u_char *, const struct pcap_pkthdr *, const u_char *);
int flush_buffer(FILE *);
int if_exists(char *);
+pcap_t *pcap_live(const char *, int, int, int, char *);
void logmsg(int, const char *, ...);
void purge_buffer(void);
int reset_dump(void);
@@ -194,10 +195,95 @@ if_exists(char *ifname)
return (if_nametoindex(ifname) != 0);
}
+pcap_t *
+pcap_live(const char *source, int slen, int promisc, int to_ms,
+char *ebuf)
+{
+ int fd;
+ struct bpf_version bv;
+ struct ifreqifr;
+ u_int v, dlt = DLT_PFLOG;
+ pcap_t *p;
+
+ if (source == NULL || slen <= 0)
+ return (NULL);
+
+ p = pcap_create(source, ebuf);
+ if (p == NULL)
+ return (NULL);
+
+ /* Open bpf(4) read only */
+ if ((fd = open("/dev/bpf", O_RDONLY)) == -1)
+ return (NULL);
+
+ if (ioctl(fd, BIOCVERSION, ) == -1) {
+ snprintf(ebuf, PCAP_ERRBUF_SIZE, "BIOCVERSION: %s",
+ pcap_strerror(errno));
+ goto bad;
+ }
+
+ if (bv.bv_major != BPF_MAJOR_VERSION ||
+ bv.bv_minor < BPF_MINOR_VERSION) {
+ snprintf(ebuf, PCAP_ERRBUF_SIZE,
+ "kernel bpf filter out of date");
+ goto bad;
+ }
+
+ strlcpy(ifr.ifr_name, source, sizeof(ifr.ifr_name));
+ if (ioctl(fd, BIOCSETIF, ) == -1) {
+ snprintf(ebuf, PCAP_ERRBUF_SIZE, "BIOCSETIF: %s",
+ pcap_strerror(errno));
+ goto bad;
+ }
+
+ if (dlt != (u_int) -1 && ioctl(fd, BIOCSDLT, )) {
+ snprintf(ebuf, PCAP_ERRBUF_SIZE, "BIOCSDLT: %s",
+ pcap_strerror(errno));
+ goto bad;
+ }
+
+ p->fd = fd;
+ p->snapshot = slen;
+ p->linktype = dlt;
+
+ /* set timeout */
+ if (to_ms != 0) {
+ struct timeval to;
+ to.tv_sec = to_ms / 1000;
+ to.tv_usec = (to_ms * 1000) % 100;
+ if (ioctl(p->fd, BIOCSRTIMEOUT, ) == -1) {
+ snprintf(ebuf, PCAP_ERRBUF_SIZE, "BIOCSRTIMEOUT: %s",
+ pcap_strerror(errno));
+ }
+ }
+ if (promisc)
+ /* this is allowed to fail */
+ ioctl(fd, BIOCPROMISC, NULL);
+
+ if (ioctl(fd, BIOCGBLEN, ) == -1) {
+ snprintf(ebuf, PCAP_ERRBUF_SIZE, "BIOCGBLEN: %s",
+ pcap_strerror(errno));
+ goto bad;
+ }
+ p->bufsize = v;
+ p->buffer = malloc(p->bufsize);
+ if (p->buffer == NULL) {
+
Re: unveil in process accounting and lastcomm
Todd C. Miller wrote: > On Thu, 25 Jul 2019 12:00:48 +0200, Alexander Bluhm wrote: > > > Do we want unveil violators in the daily mail? We can turn it off > > if we get too many false positives. > > I think so. If it becomes annoying we can turn it off by default. I'm not sure how any of these could be false positives. They will all show that the software is trying to access something it shouldn't.
Re: unveil in process accounting and lastcomm
On Thu, 25 Jul 2019 12:00:48 +0200, Alexander Bluhm wrote: > Do we want unveil violators in the daily mail? We can turn it off > if we get too many false positives. I think so. If it becomes annoying we can turn it off by default. - todd
Re: unveil in process accounting and lastcomm
On Thu, Jul 25, 2019 at 12:00:48PM +0200, Alexander Bluhm wrote: > Do we want unveil violators in the daily mail? We can turn it off > if we get too many false positives. Janne Johansson recommend to mention lastcomm(1) in unveil(2) man page. Diff for daily, lastcomm(1), unveil(2). Kernel has been commited already. bluhm Index: etc/daily === RCS file: /data/mirror/openbsd/cvs/src/etc/daily,v retrieving revision 1.91 diff -u -p -r1.91 daily --- etc/daily 6 Feb 2018 19:57:37 - 1.91 +++ etc/daily 25 Jul 2019 09:56:20 - @@ -74,7 +74,7 @@ if [ -f /var/account/acct ]; then mv -f /var/account/acct.0 /var/account/acct.1 cp -f /var/account/acct /var/account/acct.0 sa -sq - lastcomm -f /var/account/acct.0 | grep -e ' -[A-Z]*[PT]' + lastcomm -f /var/account/acct.0 | grep -e ' -[A-Z]*[PTU]' fi # If ROOTBACKUP is set to 1 in the environment, and Index: usr.bin/lastcomm/lastcomm.1 === RCS file: /data/mirror/openbsd/cvs/src/usr.bin/lastcomm/lastcomm.1,v retrieving revision 1.19 diff -u -p -r1.19 lastcomm.1 --- usr.bin/lastcomm/lastcomm.1 27 Feb 2018 07:58:29 - 1.19 +++ usr.bin/lastcomm/lastcomm.1 25 Jul 2019 09:42:15 - @@ -115,10 +115,13 @@ indicates the command was terminated wit .Sq P indicates the command was terminated due to a .Xr pledge 2 -violation, and +violation, .Sq T indicates the command did a memory access violation detected by a -processor trap. +processor trap, and +.Sq U +indicates the command tried a file access that was prevented by +.Xr unveil 2 . .Sh FILES .Bl -tag -width /var/account/acct -compact .It Pa /var/account/acct Index: usr.bin/lastcomm/lastcomm.c === RCS file: /data/mirror/openbsd/cvs/src/usr.bin/lastcomm/lastcomm.c,v retrieving revision 1.27 diff -u -p -r1.27 lastcomm.c --- usr.bin/lastcomm/lastcomm.c 27 Feb 2018 07:58:29 - 1.27 +++ usr.bin/lastcomm/lastcomm.c 25 Jul 2019 09:41:34 - @@ -174,6 +174,7 @@ flagbits(int f) BIT(AXSIG, 'X'); BIT(APLEDGE, 'P'); BIT(ATRAP, 'T'); + BIT(AUNVEIL, 'U'); *p = '\0'; return (flags); } Index: lib/libc/sys/unveil.2 === RCS file: /data/mirror/openbsd/cvs/src/lib/libc/sys/unveil.2,v retrieving revision 1.17 diff -u -p -r1.17 unveil.2 --- lib/libc/sys/unveil.2 24 Mar 2019 19:55:31 - 1.17 +++ lib/libc/sys/unveil.2 25 Jul 2019 11:12:15 - @@ -132,6 +132,12 @@ use can be tricky because programs misbe unexpectedly disappear. In many cases it is easier to unveil the directories in which an application makes use of files. +After a process has terminated, +.Xr lastcomm 1 +will mark it with the +.Sq U +flag if file access was prevented by +.Nm unveil . .Sh RETURN VALUES .Rv -std .Sh ERRORS
Re: unveil in process accounting and lastcomm
On Thu, Jul 18, 2019 at 04:13:10PM +0200, Alexander Bluhm wrote: > $ lastcomm | grep -e '-[A-Z]U' > pflogd -FU root__ 0.00 secs Thu Jul 18 14:19 (2:33:22.00) Oops, I have forgotten to show the userland part of my diff. Do we want unveil violators in the daily mail? We can turn it off if we get too many false positives. ok? bluhm Index: etc/daily === RCS file: /data/mirror/openbsd/cvs/src/etc/daily,v retrieving revision 1.91 diff -u -p -r1.91 daily --- etc/daily 6 Feb 2018 19:57:37 - 1.91 +++ etc/daily 25 Jul 2019 09:56:20 - @@ -74,7 +74,7 @@ if [ -f /var/account/acct ]; then mv -f /var/account/acct.0 /var/account/acct.1 cp -f /var/account/acct /var/account/acct.0 sa -sq - lastcomm -f /var/account/acct.0 | grep -e ' -[A-Z]*[PT]' + lastcomm -f /var/account/acct.0 | grep -e ' -[A-Z]*[PTU]' fi # If ROOTBACKUP is set to 1 in the environment, and Index: usr.bin/lastcomm/lastcomm.1 === RCS file: /data/mirror/openbsd/cvs/src/usr.bin/lastcomm/lastcomm.1,v retrieving revision 1.19 diff -u -p -r1.19 lastcomm.1 --- usr.bin/lastcomm/lastcomm.1 27 Feb 2018 07:58:29 - 1.19 +++ usr.bin/lastcomm/lastcomm.1 25 Jul 2019 09:42:15 - @@ -115,10 +115,13 @@ indicates the command was terminated wit .Sq P indicates the command was terminated due to a .Xr pledge 2 -violation, and +violation, .Sq T indicates the command did a memory access violation detected by a -processor trap. +processor trap, and +.Sq U +indicates the command tried a file access that was prevented by +.Xr unveil 2 . .Sh FILES .Bl -tag -width /var/account/acct -compact .It Pa /var/account/acct Index: usr.bin/lastcomm/lastcomm.c === RCS file: /data/mirror/openbsd/cvs/src/usr.bin/lastcomm/lastcomm.c,v retrieving revision 1.27 diff -u -p -r1.27 lastcomm.c --- usr.bin/lastcomm/lastcomm.c 27 Feb 2018 07:58:29 - 1.27 +++ usr.bin/lastcomm/lastcomm.c 25 Jul 2019 09:41:34 - @@ -174,6 +174,7 @@ flagbits(int f) BIT(AXSIG, 'X'); BIT(APLEDGE, 'P'); BIT(ATRAP, 'T'); + BIT(AUNVEIL, 'U'); *p = '\0'; return (flags); }
Re: unveil in process accounting and lastcomm
I have worried about secret unveil failures, and I am happy with
this approach.
Alexander Bluhm wrote:
> 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.
>
> Could I put that in? Process accounting is cheap and does not hurt.
>
> I have added it localy to my daily mail like pledge. Then I will
> notice how many bugs or false positives we have.
>
> bluhm
>
> > $ 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.
> >
> > 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.
> >
> > 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 - 1.27
> > +++ kern/kern_unveil.c 18 Jul 2019 12:01:24 -
> > @@ -18,6 +18,7 @@
> >
> > #include
> >
> > +#include
> > #include
> > #include
> > #include
> > @@ -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 - 1.7
> > +++ sys/acct.h 18 Jul 2019 11:37:27 -
> > @@ -63,6 +63,7 @@ struct acct {
> > #defineAXSIG 0x10/* killed by a signal */
> > #defineAPLEDGE 0x20/* killed due to pledge violation */
> > #defineATRAP 0x40/* memory access violation */
> > +#defineAUNVEIL 0x80/* unveil access violation */
> > u_int8_t ac_flag; /* accounting flags */
> > };
>
Re: unveil in process accounting and lastcomm
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.
Could I put that in? Process accounting is cheap and does not hurt.
I have added it localy to my daily mail like pledge. Then I will
notice how many bugs or false positives we have.
bluhm
> $ 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.
>
> 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.
>
> 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.c14 Jul 2019 03:26:02 - 1.27
> +++ kern/kern_unveil.c18 Jul 2019 12:01:24 -
> @@ -18,6 +18,7 @@
>
> #include
>
> +#include
> #include
> #include
> #include
> @@ -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.h8 Jun 2017 17:14:02 - 1.7
> +++ sys/acct.h18 Jul 2019 11:37:27 -
> @@ -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 */
> };
Re: unveil in process accounting and lastcomm
On Thu, Jul 18, 2019 at 11:46:46AM -0400, Bryan Steele wrote:
> 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.
Here's a potential diff for both pflogd and spamlogd, I've tested it
works with pflogd. I only compile tested spamlogd. But it should avoid
the unveil accounting errors.
This duplicates a lot of code into both, but I don't feel like trying
to extent the libpcap API for this.
comments? ok?
Index: pflogd.c
===
RCS file: /cvs/src/sbin/pflogd/pflogd.c,v
retrieving revision 1.59
diff -u -p -u -r1.59 pflogd.c
--- sbin/pflogd/pflogd.c26 Aug 2018 18:24:46 - 1.59
+++ sbin/pflogd/pflogd.c18 Jul 2019 21:30:39 -
@@ -73,6 +73,7 @@ void dump_packet(u_char *, const struct
void dump_packet_nobuf(u_char *, const struct pcap_pkthdr *, const u_char *);
int flush_buffer(FILE *);
int if_exists(char *);
+pcap_t *pcap_live(const char *, int, int, int, char *);
void logmsg(int, const char *, ...);
void purge_buffer(void);
int reset_dump(void);
@@ -194,10 +195,97 @@ if_exists(char *ifname)
return (if_nametoindex(ifname) != 0);
}
+pcap_t *
+pcap_live(const char *source, int slen, int promisc, int to_ms,
+char *ebuf)
+{
+ int fd;
+ struct bpf_version bv;
+ struct ifreqifr;
+ u_int v, dlt = DLT_PFLOG;
+ pcap_t *p;
+
+ if (source == NULL || slen <= 0)
+ return (NULL);
+
+ p = pcap_create(source, ebuf);
+ if (p == NULL)
+ return (NULL);
+
+ /* Open bpf(4) read only */
+ if ((fd = open("/dev/bpf", O_RDONLY)) == -1)
+ return (NULL);
+
+ if (ioctl(fd, BIOCVERSION, ) == -1) {
+ snprintf(ebuf, PCAP_ERRBUF_SIZE, "BIOCVERSION: %s",
+ pcap_strerror(errno));
+ goto bad;
+ }
+
+ if (bv.bv_major != BPF_MAJOR_VERSION ||
+ bv.bv_minor < BPF_MINOR_VERSION) {
+ snprintf(ebuf, PCAP_ERRBUF_SIZE,
+ "kernel bpf filter out of date");
+ goto bad;
+ }
+
+ strlcpy(ifr.ifr_name, source, sizeof(ifr.ifr_name));
+ if (ioctl(fd, BIOCSETIF, ) == -1) {
+ snprintf(ebuf, PCAP_ERRBUF_SIZE, "BIOCSETIF: %s",
+ pcap_strerror(errno));
+ goto bad;
+ }
+
+ if (dlt != (u_int) -1 && ioctl(fd, BIOCSDLT, )) {
+ snprintf(ebuf, PCAP_ERRBUF_SIZE, "BIOCSDLT: %s",
+ pcap_strerror(errno));
+ goto bad;
+ }
+
+ p->fd = fd;
+ p->snapshot = slen;
+ p->linktype = dlt;
+
+ /* set timeout */
+ if (to_ms != 0) {
+ struct timeval to;
+ to.tv_sec = to_ms / 1000;
+ to.tv_usec = (to_ms * 1000) % 100;
+ if (ioctl(p->fd, BIOCSRTIMEOUT, ) < 0) {
+ snprintf(ebuf, PCAP_ERRBUF_SIZE, "BIOCSRTIMEOUT: %s",
+ pcap_strerror(errno));
+ }
+ }
+ if (promisc)
+ /* this is allowed to fail */
+ ioctl(fd, BIOCPROMISC, NULL);
+
+ if (ioctl(fd, BIOCGBLEN, ) < 0) {
+ snprintf(ebuf, PCAP_ERRBUF_SIZE, "BIOCGBLEN: %s",
+ pcap_strerror(errno));
+ goto bad;
+ }
+ p->bufsize = v;
+ p->buffer = malloc(p->bufsize);
+ if (p->buffer == NULL) {
+ snprintf(ebuf, PCAP_ERRBUF_SIZE, "malloc: %s",
+ pcap_strerror(errno));
+ goto bad;
+ }
+ p->activated = 1;
+ return (p);
+
+bad:
+ if (fd >= 0)
+ close(fd);
+ free(p);
+ return (NULL);
+}
+
int
init_pcap(void)
{
- hpcap = pcap_open_live(interface, snaplen, 1, PCAP_TO_MS, errbuf);
+ hpcap = pcap_live(interface, snaplen, 1, PCAP_TO_MS, errbuf);
if (hpcap == NULL) {
logmsg(LOG_ERR, "Failed to initialize:
Re: unveil in process accounting and lastcomm
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.c14 Jul 2019 03:26:02 - 1.27
> +++ kern/kern_unveil.c18 Jul 2019 12:01:24 -
> @@ -18,6 +18,7 @@
>
> #include
>
> +#include
> #include
> #include
> #include
> @@ -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.h8 Jun 2017 17:14:02 - 1.7
> +++ sys/acct.h18 Jul 2019 11:37:27 -
> @@ -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 */
> };
>
>
unveil in process accounting and lastcomm
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.
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.
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 - 1.27
+++ kern/kern_unveil.c 18 Jul 2019 12:01:24 -
@@ -18,6 +18,7 @@
#include
+#include
#include
#include
#include
@@ -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 - 1.7
+++ sys/acct.h 18 Jul 2019 11:37:27 -
@@ -63,6 +63,7 @@ struct acct {
#defineAXSIG 0x10/* killed by a signal */
#defineAPLEDGE 0x20/* killed due to pledge violation */
#defineATRAP 0x40/* memory access violation */
+#defineAUNVEIL 0x80/* unveil access violation */
u_int8_t ac_flag; /* accounting flags */
};
