In order to be able to safely append to existing log files, pflogd(8) attempts to validate/or move invalid/broken pflog pcap files out of the way on its own. I noticed that this is not compatible with unveil(2), as pflogd(8) would need to be able to rename(2) files in /var/log to /var/log/pflog.bad.*
Currently, if pflogd(8) is unable to move the log file out of the way, it suspends logging until it receives SIGHUP/SIGALRM indicating that the log file has been moved out of the way manually. I propose that this become the default and only behaviour. pflogd(8) already reports to syslog that logging has been suspended: pflogd[93742]: Invalid/incompatible log file, move it away pflogd[93742]: Logging suspended: open error And now when resumed: pflogd[93742]: Logging resumed This part can be dropped if found to be too noisy. It looks like in the past this was used when the log format had changed incompatibly, I/O errors might indicate you have bigger issues, so we probably shouldn't be writing to disk.. Comments? Ok? :-) -Bryan. Index: pflogd.8 =================================================================== RCS file: /cvs/src/sbin/pflogd/pflogd.8,v retrieving revision 1.49 diff -u -p -u -r1.49 pflogd.8 --- pflogd.8 30 May 2017 17:15:06 -0000 1.49 +++ pflogd.8 5 Aug 2018 12:18:34 -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 --- pflogd.c 9 Sep 2017 13:02:52 -0000 1.58 +++ pflogd.c 5 Aug 2018 12:18:34 -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 */ @@ -238,25 +236,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 +303,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 +618,7 @@ main(int argc, char **argv) bufpkt = 0; } - if (reset_dump(Xflag) < 0) { + if (reset_dump() < 0) { if (Xflag) return (1); @@ -666,10 +643,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 --- privsep.c 9 Sep 2017 13:02:52 -0000 1.30 +++ privsep.c 5 Aug 2018 12:18:34 -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; @@ -195,13 +193,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 +216,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 +294,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 */