On Thu, Aug 16, 2018 at 04:28:03PM -0400, Bryan Steele wrote:
> On Thu, Aug 16, 2018 at 04:20:54PM -0400, Bryan Steele wrote:
> > This adds unveil to pflogd(8)
> >
> > pflogd(8) is a special case, residing in /sbin, it's a static PIE. As
> > such, I thought it might be worth experimenting with execpromises here.
> > This allows re-exec after privdrop, and removes chroot(2) in favour of
> > only unveil(2) and pledge(2). I've left the code there for now just in
> > case portability is a concern.
>
> Please ignore this diff for now.
>
> > The privileged part of pflogd(8) is now disallowed from accessing most
> > of the filesystem, veiled except for read-only opening of /dev/bpf, and
> > rwc for the log file, typically /var/log/pflog. This process cannot yet
> > pledge(2), so special care is needed to make sure no library functions
> > called use files permitted by certain pledges.
> >
> > The unprivileged part already runs pledged "stdio recvfd"
> >
> > This includes the diff I sent previously, I'd like to commit that part
> > separately, any oks on that one?
> >
> > https://marc.info/?l=openbsd-tech&m=153347271628532&w=2
> >
> > Also, ok for this? ;-)
> >
> > -Bryan.
Sorry about that. New diff.
Index: pflogd.8
===================================================================
RCS file: /cvs/src/sbin/pflogd/pflogd.8,v
retrieving revision 1.49
diff -u -p -u -r1.49 pflogd.8
--- sbin/pflogd/pflogd.8 30 May 2017 17:15:06 -0000 1.49
+++ sbin/pflogd/pflogd.8 16 Aug 2018 20:36:11 -0000
@@ -86,9 +86,8 @@ temporarily uses the old snaplen to keep
tries to preserve the integrity of the log file against I/O errors.
Furthermore, integrity of an existing log file is verified before
appending.
-If there is an invalid log file or an I/O error, the log file is moved
-out of the way and a new one is created.
-If a new file cannot be created, logging is suspended until a
+If there is an invalid log file or an I/O error, logging is suspended
+until a
.Dv SIGHUP
or a
.Dv SIGALRM
Index: pflogd.c
===================================================================
RCS file: /cvs/src/sbin/pflogd/pflogd.c,v
retrieving revision 1.58
diff -u -p -u -r1.58 pflogd.c
--- sbin/pflogd/pflogd.c 9 Sep 2017 13:02:52 -0000 1.58
+++ sbin/pflogd/pflogd.c 16 Aug 2018 20:36:11 -0000
@@ -75,7 +75,7 @@ int flush_buffer(FILE *);
int if_exists(char *);
void logmsg(int, const char *, ...);
void purge_buffer(void);
-int reset_dump(int);
+int reset_dump(void);
int scan_dump(FILE *, off_t);
int set_snaplen(int);
void set_suspended(int);
@@ -84,8 +84,6 @@ void sig_close(int);
void sig_hup(int);
void usage(void);
-static int try_reset_dump(int);
-
/* buffer must always be greater than snaplen */
static int bufpkt = 0; /* number of packets in buffer */
static int buflen = 0; /* allocated size of buffer */
@@ -199,6 +197,11 @@ if_exists(char *ifname)
int
init_pcap(void)
{
+ if (unveil("/dev/bpf", "r") == -1) {
+ logmsg(LOG_ERR, "unveil: %s", strerror(errno));
+ return (-1);
+ }
+
hpcap = pcap_open_live(interface, snaplen, 1, PCAP_TO_MS, errbuf);
if (hpcap == NULL) {
logmsg(LOG_ERR, "Failed to initialize: %s", errbuf);
@@ -220,6 +223,11 @@ init_pcap(void)
return (-1);
}
+ if (unveil(NULL, NULL) == -1) {
+ logmsg(LOG_ERR, "unveil: %s", strerror(errno));
+ return (-1);
+ }
+
return (0);
}
@@ -238,25 +246,7 @@ set_snaplen(int snap)
}
int
-reset_dump(int nomove)
-{
- int ret;
-
- for (;;) {
- ret = try_reset_dump(nomove);
- if (ret <= 0)
- break;
- }
-
- return (ret);
-}
-
-/*
- * tries to (re)open log file, nomove flag is used with -x switch
- * returns 0: success, 1: retry (log moved), -1: error
- */
-int
-try_reset_dump(int nomove)
+reset_dump(void)
{
struct pcap_file_header hdr;
struct stat st;
@@ -323,12 +313,9 @@ try_reset_dump(int nomove)
}
} else if (scan_dump(fp, st.st_size)) {
fclose(fp);
- if (nomove || priv_move_log()) {
- logmsg(LOG_ERR,
- "Invalid/incompatible log file, move it away");
- return (-1);
- }
- return (1);
+ logmsg(LOG_ERR,
+ "Invalid/incompatible log file, move it away");
+ return (-1);
}
dpcap = fp;
@@ -641,7 +628,7 @@ main(int argc, char **argv)
bufpkt = 0;
}
- if (reset_dump(Xflag) < 0) {
+ if (reset_dump() < 0) {
if (Xflag)
return (1);
@@ -666,10 +653,14 @@ main(int argc, char **argv)
if (gotsig_close)
break;
if (gotsig_hup) {
- if (reset_dump(0)) {
+ int was_suspended = suspended;
+ if (reset_dump()) {
logmsg(LOG_ERR,
"Logging suspended: open error");
set_suspended(1);
+ } else {
+ if (was_suspended)
+ logmsg(LOG_NOTICE, "Logging resumed");
}
gotsig_hup = 0;
}
Index: privsep.c
===================================================================
RCS file: /cvs/src/sbin/pflogd/privsep.c,v
retrieving revision 1.30
diff -u -p -u -r1.30 privsep.c
--- sbin/pflogd/privsep.c 9 Sep 2017 13:02:52 -0000 1.30
+++ sbin/pflogd/privsep.c 16 Aug 2018 20:36:11 -0000
@@ -42,7 +42,6 @@
enum cmd_types {
PRIV_INIT_PCAP, /* init pcap fdpass bpf */
PRIV_SET_SNAPLEN, /* set the snaplength */
- PRIV_MOVE_LOG, /* move logfile away */
PRIV_OPEN_LOG, /* open logfile for appending */
};
@@ -54,7 +53,6 @@ static int may_read(int, void *, size_t
static void must_read(int, void *, size_t);
static void must_write(int, void *, size_t);
static int set_snaplen(int snap);
-static int move_log(const char *name);
extern char *filename;
extern char *interface;
@@ -133,6 +131,9 @@ priv_init(int Pflag, int argc, char *arg
setproctitle("[priv]");
+ if (unveil(filename, "rwc") == -1)
+ err(1, "unveil");
+
#if 0
/* This needs to do bpf ioctl */
BROKEN if (pledge("stdio rpath wpath cpath sendfd proc bpf", NULL) == -1)
@@ -195,13 +196,6 @@ BROKEN if (pledge("stdio rpath wpath cpa
filename, strerror(olderrno));
break;
- case PRIV_MOVE_LOG:
- logmsg(LOG_DEBUG,
- "[priv]: msg PRIV_MOVE_LOG received");
- ret = move_log(filename);
- must_write(socks[0], &ret, sizeof(int));
- break;
-
default:
logmsg(LOG_ERR, "[priv]: unknown command %d", cmd);
_exit(1);
@@ -225,48 +219,6 @@ set_snaplen(int snap)
return 0;
}
-static int
-move_log(const char *name)
-{
- char ren[PATH_MAX];
- int len;
-
- for (;;) {
- int fd;
-
- len = snprintf(ren, sizeof(ren), "%s.bad.XXXXXXXX", name);
- if (len >= sizeof(ren)) {
- logmsg(LOG_ERR, "[priv] new name too long");
- return (1);
- }
-
- /* lock destination */
- fd = mkstemp(ren);
- if (fd >= 0) {
- close(fd);
- break;
- }
- if (errno != EINTR) {
- logmsg(LOG_ERR, "[priv] failed to create new name: %s",
- strerror(errno));
- return (1);
- }
- }
-
- if (rename(name, ren)) {
- logmsg(LOG_ERR, "[priv] failed to rename %s to %s: %s",
- name, ren, strerror(errno));
- unlink(ren);
- return (1);
- }
-
- logmsg(LOG_NOTICE,
- "[priv]: log file %s moved to %s", name, ren);
-
- return (0);
-}
-
-
/* receive bpf fd from privileged process using fdpass and init pcap */
int
priv_init_pcap(int snaplen)
@@ -345,22 +297,6 @@ priv_open_log(void)
fd = receive_fd(priv_fd);
return (fd);
-}
-
-/* Move-away and reopen log-file */
-int
-priv_move_log(void)
-{
- int cmd, ret;
-
- if (priv_fd < 0)
- errx(1, "%s: called from privileged portion", __func__);
-
- cmd = PRIV_MOVE_LOG;
- must_write(priv_fd, &cmd, sizeof(int));
- must_read(priv_fd, &ret, sizeof(int));
-
- return (ret);
}
/* If priv parent gets a TERM or HUP, pass it through to child instead */
