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 */

Reply via email to