tcpdump: revisiting some old diffs, hoist opening of pf.os.

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

tcpdump: revisiting some old diffs, hoist opening of pf.os.

Bryan Steele
I'm revisiting some old tcpdump diffs, now that mestre@ has added proper
unveil(2) support! :-)

Refresher: https://marc.info/?l=openbsd-tech&m=150535073209723&w=2

This hoists opening pf.os(5) fingerprints '-o' from the 'RUN' state to
the 'FILTER' state, this will allow for a reduced pledge(2) at runtime
in the (currently root) monitor process.

This still works as well as it already has. :-)

    ( ... ) [tcp sum ok] (src OS: OpenBSD 6.1) 3311509932:3311509932(0) win 16384 <mss 1460,nop,nop,sackOK,nop,wscale 6,nop,nop,timestamp 3905153931 0> (DF) (ttl 64, id 41239, len 64)

The only potential difference is that if /etc/pf.os is replaced at
runtime, tcpdump won't reopen it.

I don't think that's a problem..

ok?

-Bryan.

Index: pfctl_osfp.c
===================================================================
RCS file: /cvs/src/usr.sbin/tcpdump/pfctl_osfp.c,v
retrieving revision 1.13
diff -u -p -u -r1.13 pfctl_osfp.c
--- usr.sbin/tcpdump/pfctl_osfp.c 28 May 2017 10:06:12 -0000 1.13
+++ usr.sbin/tcpdump/pfctl_osfp.c 7 Nov 2018 23:52:48 -0000
@@ -81,17 +81,14 @@ void print_name_list(int, struct name
 void sort_name_list(int, struct name_list *);
 struct name_entry *lookup_name_list(struct name_list *, const char *);
 
-/* XXX arbitrary */
-#define MAX_FP_LINE 1024
-
 /* Load fingerprints from a file */
 int
 pfctl_file_fingerprints(int dev, int opts, const char *fp_filename)
 {
- u_char buf[MAX_FP_LINE];
+ FILE *in;
  u_char *line;
  size_t len;
- int i, lineno = 0;
+ int i, fd, lineno = 0;
  int window, w_mod, ttl, df, psize, p_mod, mss, mss_mod, wscale,
     wscale_mod, optcnt, ts0;
  pf_tcpopts_t packed_tcpopts;
@@ -99,15 +96,22 @@ pfctl_file_fingerprints(int dev, int opt
  struct pf_osfp_ioctl fp;
 
  pfctl_flush_my_fingerprints(&classes);
+
+ fd = priv_open_pfosfp();
+ if (fd < 0)
+ return (1);
+
+ if ((in = fdopen(fd, "r")) == NULL) {
+ warn("%s", fp_filename);
+ return (1);
+ }
+
  class = version = subtype = desc = tcpopts = NULL;
 
  if ((opts & PF_OPT_NOACTION) == 0)
  pfctl_clear_fingerprints(dev, opts);
 
- priv_getlines(FTAB_PFOSFP);
- while ((len = priv_getline(buf, sizeof(buf))) > 0) {
- buf[len -1] = '\n';
- line = buf;
+ while ((line = fgetln(in, &len)) != NULL) {
  lineno++;
  free(class);
  free(version);
Index: privsep.c
===================================================================
RCS file: /cvs/src/usr.sbin/tcpdump/privsep.c,v
retrieving revision 1.49
diff -u -p -u -r1.49 privsep.c
--- usr.sbin/tcpdump/privsep.c 28 Sep 2018 06:48:59 -0000 1.49
+++ usr.sbin/tcpdump/privsep.c 7 Nov 2018 23:52:48 -0000
@@ -73,7 +73,8 @@ static const int allowed_max[] = {
  /* INIT */ ALLOW(PRIV_OPEN_BPF) | ALLOW(PRIV_OPEN_DUMP) |
  ALLOW(PRIV_SETFILTER),
  /* BPF */ ALLOW(PRIV_SETFILTER),
- /* FILTER */ ALLOW(PRIV_OPEN_OUTPUT) | ALLOW(PRIV_GETSERVENTRIES) |
+ /* FILTER */ ALLOW(PRIV_OPEN_PFOSFP) | ALLOW(PRIV_OPEN_OUTPUT) |
+ ALLOW(PRIV_GETSERVENTRIES) |
  ALLOW(PRIV_GETPROTOENTRIES) |
  ALLOW(PRIV_ETHER_NTOHOST) | ALLOW(PRIV_INIT_DONE),
  /* RUN */ ALLOW(PRIV_GETHOSTBYADDR) | ALLOW(PRIV_ETHER_NTOHOST) |
@@ -114,6 +115,7 @@ extern void set_slave_signals(void);
 
 static void impl_open_bpf(int, int *);
 static void impl_open_dump(int, const char *);
+static void impl_open_pfosfp(int);
 static void impl_open_output(int, const char *);
 static void impl_setfilter(int, char *, int *);
 static void impl_init_done(int, int *);
@@ -277,6 +279,8 @@ priv_exec(int argc, char *argv[])
  allowed_ext[STATE_RUN] |= ALLOW(PRIV_GETRPCBYNUMBER);
  allowed_ext[STATE_FILTER] |= ALLOW(PRIV_GETPROTOENTRIES);
  }
+ if (oflag)
+ allowed_ext[STATE_FILTER] |= ALLOW(PRIV_OPEN_PFOSFP);
 
  if (infile)
  cmdbuf = read_infile(infile);
@@ -297,6 +301,10 @@ priv_exec(int argc, char *argv[])
  test_state(cmd, STATE_BPF);
  impl_open_dump(sock, RFileName);
  break;
+ case PRIV_OPEN_PFOSFP:
+ test_state(cmd, STATE_FILTER);
+ impl_open_pfosfp(sock);
+ break;
  case PRIV_OPEN_OUTPUT:
  test_state(cmd, STATE_RUN);
  impl_open_output(sock, WFileName);
@@ -309,10 +317,6 @@ priv_exec(int argc, char *argv[])
  test_state(cmd, STATE_RUN);
  impl_init_done(sock, &bpfd);
 
- if (oflag) {
- if (unveil("/etc/pf.os", "r") == -1)
- err(1, "unveil");
- }
  if (unveil("/etc/ethers", "r") == -1)
  err(1, "unveil");
  if (unveil("/etc/rpc", "r") == -1)
@@ -416,6 +420,24 @@ impl_open_dump(int fd, const char *RFile
 }
 
 static void
+impl_open_pfosfp(int fd)
+{
+ int file, err = 0;
+
+ logmsg(LOG_DEBUG, "[priv]: msg PRIV_OPEN_PFOSFP received");
+
+ file = open(PF_OSFP_FILE, O_RDONLY, 0);
+ err = errno;
+ if (file < 0)
+ logmsg(LOG_DEBUG, "[priv]: failed to open %s: %s",
+    PF_OSFP_FILE, strerror(errno));
+ send_fd(fd, file);
+ must_write(fd, &err, sizeof(int));
+ if (file >= 0)
+ close(file);
+}
+
+static void
 impl_open_output(int fd, const char *WFileName)
 {
  int file, err;
@@ -818,6 +840,22 @@ priv_getline(char *line, size_t line_len
 
  /* read the line */
  return (read_string(priv_fd, line, line_len, __func__));
+}
+
+int
+priv_open_pfosfp(void)
+{
+ int fd, err = 0;
+ write_command(priv_fd, PRIV_OPEN_PFOSFP);
+
+ fd = receive_fd(priv_fd);
+ must_read(priv_fd, &err, sizeof(int));
+ if (fd < 0) {
+ warnc(err, "%s", PF_OSFP_FILE);
+ return (-1);
+ }
+
+ return (fd);
 }
 
 /* Read all data or return 1 for error. */
Index: privsep.h
===================================================================
RCS file: /cvs/src/usr.sbin/tcpdump/privsep.h,v
retrieving revision 1.10
diff -u -p -u -r1.10 privsep.h
--- usr.sbin/tcpdump/privsep.h 8 Sep 2017 19:10:57 -0000 1.10
+++ usr.sbin/tcpdump/privsep.h 7 Nov 2018 23:52:48 -0000
@@ -27,6 +27,7 @@
 enum cmd_types {
  PRIV_OPEN_BPF, /* open a bpf descriptor */
  PRIV_OPEN_DUMP, /* open dump file for reading */
+ PRIV_OPEN_PFOSFP, /* open pf.os(5) fingerprint db for reading */
  PRIV_OPEN_OUTPUT, /* open output file */
  PRIV_SETFILTER, /* set a bpf read filter */
  PRIV_GETHOSTBYADDR, /* resolve numeric address into hostname */
@@ -81,6 +82,9 @@ void priv_getlines(size_t);
 /* Retrieve a single line from a file, should be called repeatedly after
    calling priv_getlines() until it returns zero */
 size_t priv_getline(char *, size_t);
+
+/* Retrieve pf.os(5) fingerprints file descriptor */
+int priv_open_pfosfp();
 
 /* Return the pcap statistics upon completion */
 int priv_pcap_stats(struct pcap_stat *);
Index: tcpdump.c
===================================================================
RCS file: /cvs/src/usr.sbin/tcpdump/tcpdump.c,v
retrieving revision 1.87
diff -u -p -u -r1.87 tcpdump.c
--- usr.sbin/tcpdump/tcpdump.c 6 Jul 2018 07:13:21 -0000 1.87
+++ usr.sbin/tcpdump/tcpdump.c 7 Nov 2018 23:52:48 -0000
@@ -471,6 +471,8 @@ main(int argc, char **argv)
  bpf_dump(fcode, dflag);
  exit(0);
  }
+ if (oflag)
+ oflag = init_pfosfp();
  init_addrtoname(localnet, netmask);
 
  if (WFileName) {
@@ -500,8 +502,6 @@ main(int argc, char **argv)
  (void)fflush(stderr);
  }
 
- if (oflag)
- oflag = init_pfosfp();
  if (tflag > 0)
  thiszone = gmt2local(0);
 

Reply | Threaded
Open this post in threaded view
|

Re: tcpdump: revisiting some old diffs, cleanup unused functions

Bryan Steele
On Wed, Nov 07, 2018 at 07:06:09PM -0500, Bryan Steele wrote:
> I'm revisiting some old tcpdump diffs, now that mestre@ has added proper
> unveil(2) support! :-)
>
> Refresher: https://marc.info/?l=openbsd-tech&m=150535073209723&w=2
>
> This hoists opening pf.os(5) fingerprints '-o' from the 'RUN' state to
> the 'FILTER' state, this will allow for a reduced pledge(2) at runtime
> in the (currently root) monitor process.

This was a bit of copy & paste, sorry. This moves the opening of pf.os
earlier and avoids the unveil later on. Of course, reducing the runtime
pledge(2) promises will come later! :-)

>
> This still works as well as it already has. :-)
>
>     ( ... ) [tcp sum ok] (src OS: OpenBSD 6.1) 3311509932:3311509932(0) win 16384 <mss 1460,nop,nop,sackOK,nop,wscale 6,nop,nop,timestamp 3905153931 0> (DF) (ttl 64, id 41239, len 64)
>
> The only potential difference is that if /etc/pf.os is replaced at
> runtime, tcpdump won't reopen it.
>
> I don't think that's a problem..
>
> ok?
>
> -Bryan.

Remove the now unused internal privsep "getline" code, which passed
lines over a socket, replaced with explicit fdpassing of /etc/pf.os.

This depends on the previous diff..

ok?

-Bryan.

Index: privsep.c
===================================================================
RCS file: /cvs/src/usr.sbin/tcpdump/privsep.c,v
retrieving revision 1.49
diff -u -p -u -r1.49 privsep.c
--- privsep.c 28 Sep 2018 06:48:59 -0000 1.49
+++ privsep.c 8 Nov 2018 00:19:47 -0000
@@ -77,8 +77,8 @@ static const int allowed_max[] = {
  ALLOW(PRIV_GETPROTOENTRIES) |
  ALLOW(PRIV_ETHER_NTOHOST) | ALLOW(PRIV_INIT_DONE),
  /* RUN */ ALLOW(PRIV_GETHOSTBYADDR) | ALLOW(PRIV_ETHER_NTOHOST) |
- ALLOW(PRIV_GETRPCBYNUMBER) | ALLOW(PRIV_GETLINES) |
- ALLOW(PRIV_LOCALTIME) | ALLOW(PRIV_PCAP_STATS),
+ ALLOW(PRIV_GETRPCBYNUMBER) | ALLOW(PRIV_LOCALTIME) |
+ ALLOW(PRIV_PCAP_STATS),
  /* EXIT */ 0
 };
 
@@ -90,21 +90,10 @@ static int allowed_ext[] = {
  /* INIT */ ALLOW(PRIV_SETFILTER),
  /* BPF */ ALLOW(PRIV_SETFILTER),
  /* FILTER */ ALLOW(PRIV_GETSERVENTRIES),
- /* RUN */ ALLOW(PRIV_GETLINES) | ALLOW(PRIV_LOCALTIME) |
- ALLOW(PRIV_PCAP_STATS),
+ /* RUN */ ALLOW(PRIV_LOCALTIME) | ALLOW(PRIV_PCAP_STATS),
  /* EXIT */ 0
 };
 
-struct ftab {
- char *name;
- int max;
- int count;
-};
-
-static struct ftab file_table[] = {{PF_OSFP_FILE, 1, 0}};
-
-#define NUM_FILETAB (sizeof(file_table) / sizeof(struct ftab))
-
 int debug_level = LOG_INFO;
 int priv_fd = -1;
 volatile pid_t child_pid = -1;
@@ -123,7 +112,6 @@ static void impl_getrpcbynumber(int);
 static void impl_getserventries(int);
 static void impl_getprotoentries(int);
 static void impl_localtime(int fd);
-static void impl_getlines(int);
 static void impl_pcap_stats(int, int *);
 
 static void test_state(int, int);
@@ -345,10 +333,6 @@ priv_exec(int argc, char *argv[])
  test_state(cmd, STATE_RUN);
  impl_localtime(sock);
  break;
- case PRIV_GETLINES:
- test_state(cmd, STATE_RUN);
- impl_getlines(sock);
- break;
  case PRIV_PCAP_STATS:
  test_state(cmd, STATE_RUN);
  impl_pcap_stats(sock, &bpfd);
@@ -577,55 +561,6 @@ impl_localtime(int fd)
 }
 
 static void
-impl_getlines(int fd)
-{
- FILE *fp;
- char *buf, *lbuf, *file;
- size_t len, fid;
-
- logmsg(LOG_DEBUG, "[priv]: msg PRIV_GETLINES received");
-
- must_read(fd, &fid, sizeof(size_t));
- if (fid >= NUM_FILETAB)
- errx(1, "invalid file id");
-
- file = file_table[fid].name;
-
- if (file == NULL)
- errx(1, "invalid file referenced");
-
- if (file_table[fid].count >= file_table[fid].max)
- errx(1, "maximum open count exceeded for %s", file);
-
- file_table[fid].count++;
-
- if ((fp = fopen(file, "r")) == NULL) {
- write_zero(fd);
- return;
- }
-
- lbuf = NULL;
- while ((buf = fgetln(fp, &len))) {
- if (buf[len - 1] == '\n')
- buf[len - 1] = '\0';
- else {
- if ((lbuf = malloc(len + 1)) == NULL)
- err(1, NULL);
- memcpy(lbuf, buf, len);
- lbuf[len] = '\0';
- buf = lbuf;
- }
-
- write_string(fd, buf);
-
- free(lbuf);
- lbuf = NULL;
- }
- write_zero(fd);
- fclose(fp);
-}
-
-static void
 impl_pcap_stats(int fd, int *bpfd)
 {
  struct pcap_stat stats;
@@ -786,17 +721,6 @@ priv_localtime(const time_t *t)
  return &lt;
 }
 
-/* start getting lines from a file */
-void
-priv_getlines(size_t sz)
-{
- if (priv_fd < 0)
- errx(1, "%s called from privileged portion", __func__);
-
- write_command(priv_fd, PRIV_GETLINES);
- must_write(priv_fd, &sz, sizeof(size_t));
-}
-
 int
 priv_pcap_stats(struct pcap_stat *ps)
 {
@@ -806,18 +730,6 @@ priv_pcap_stats(struct pcap_stat *ps)
  write_command(priv_fd, PRIV_PCAP_STATS);
  must_read(priv_fd, ps, sizeof(*ps));
  return (0);
-}
-
-/* retrieve a line from a file, should be called repeatedly after calling
-   priv_getlines(), until it returns zero. */
-size_t
-priv_getline(char *line, size_t line_len)
-{
- if (priv_fd < 0)
- errx(1, "%s called from privileged portion", __func__);
-
- /* read the line */
- return (read_string(priv_fd, line, line_len, __func__));
 }
 
 /* Read all data or return 1 for error. */
Index: privsep.h
===================================================================
RCS file: /cvs/src/usr.sbin/tcpdump/privsep.h,v
retrieving revision 1.10
diff -u -p -u -r1.10 privsep.h
--- privsep.h 8 Sep 2017 19:10:57 -0000 1.10
+++ privsep.h 8 Nov 2018 00:19:47 -0000
@@ -21,9 +21,6 @@
 
 #define TCPDUMP_MAGIC 0xa1b2c3d4
 
-/* file ids used by priv_getlines */
-#define FTAB_PFOSFP 0
-
 enum cmd_types {
  PRIV_OPEN_BPF, /* open a bpf descriptor */
  PRIV_OPEN_DUMP, /* open dump file for reading */
@@ -35,7 +32,6 @@ enum cmd_types {
  PRIV_GETSERVENTRIES, /* get the service entries table */
  PRIV_GETPROTOENTRIES, /* get the ip protocol entries table */
  PRIV_LOCALTIME, /* return localtime */
- PRIV_GETLINES, /* get lines from a file */
  PRIV_INIT_DONE, /* signal that the initialization is done */
  PRIV_PCAP_STATS /* get pcap_stats() results */
 };
@@ -74,13 +70,6 @@ void priv_getprotoentries(void);
 /* Retrieve a single protocol entry, should be called repeatedly after
    calling priv_getprotoentries() until it returns zero */
 size_t priv_getprotoentry(char *, size_t, int *);
-
-/* Start getting lines from a file */
-void priv_getlines(size_t);
-
-/* Retrieve a single line from a file, should be called repeatedly after
-   calling priv_getlines() until it returns zero */
-size_t priv_getline(char *, size_t);
 
 /* Return the pcap statistics upon completion */
 int priv_pcap_stats(struct pcap_stat *);

Reply | Threaded
Open this post in threaded view
|

Re: tcpdump: revisiting some old diffs, cleanup unused functions

Ricardo Mestre-2
Still works as advertised, ok mestre@

On 19:32 Wed 07 Nov     , Bryan Steele wrote:

> On Wed, Nov 07, 2018 at 07:06:09PM -0500, Bryan Steele wrote:
> > I'm revisiting some old tcpdump diffs, now that mestre@ has added proper
> > unveil(2) support! :-)
> >
> > Refresher: https://marc.info/?l=openbsd-tech&m=150535073209723&w=2
> >
> > This hoists opening pf.os(5) fingerprints '-o' from the 'RUN' state to
> > the 'FILTER' state, this will allow for a reduced pledge(2) at runtime
> > in the (currently root) monitor process.
>
> This was a bit of copy & paste, sorry. This moves the opening of pf.os
> earlier and avoids the unveil later on. Of course, reducing the runtime
> pledge(2) promises will come later! :-)
>
> >
> > This still works as well as it already has. :-)
> >
> >     ( ... ) [tcp sum ok] (src OS: OpenBSD 6.1) 3311509932:3311509932(0) win 16384 <mss 1460,nop,nop,sackOK,nop,wscale 6,nop,nop,timestamp 3905153931 0> (DF) (ttl 64, id 41239, len 64)
> >
> > The only potential difference is that if /etc/pf.os is replaced at
> > runtime, tcpdump won't reopen it.
> >
> > I don't think that's a problem..
> >
> > ok?
> >
> > -Bryan.
>
> Remove the now unused internal privsep "getline" code, which passed
> lines over a socket, replaced with explicit fdpassing of /etc/pf.os.
>
> This depends on the previous diff..
>
> ok?
>
> -Bryan.
>
> Index: privsep.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/tcpdump/privsep.c,v
> retrieving revision 1.49
> diff -u -p -u -r1.49 privsep.c
> --- privsep.c 28 Sep 2018 06:48:59 -0000 1.49
> +++ privsep.c 8 Nov 2018 00:19:47 -0000
> @@ -77,8 +77,8 @@ static const int allowed_max[] = {
>   ALLOW(PRIV_GETPROTOENTRIES) |
>   ALLOW(PRIV_ETHER_NTOHOST) | ALLOW(PRIV_INIT_DONE),
>   /* RUN */ ALLOW(PRIV_GETHOSTBYADDR) | ALLOW(PRIV_ETHER_NTOHOST) |
> - ALLOW(PRIV_GETRPCBYNUMBER) | ALLOW(PRIV_GETLINES) |
> - ALLOW(PRIV_LOCALTIME) | ALLOW(PRIV_PCAP_STATS),
> + ALLOW(PRIV_GETRPCBYNUMBER) | ALLOW(PRIV_LOCALTIME) |
> + ALLOW(PRIV_PCAP_STATS),
>   /* EXIT */ 0
>  };
>  
> @@ -90,21 +90,10 @@ static int allowed_ext[] = {
>   /* INIT */ ALLOW(PRIV_SETFILTER),
>   /* BPF */ ALLOW(PRIV_SETFILTER),
>   /* FILTER */ ALLOW(PRIV_GETSERVENTRIES),
> - /* RUN */ ALLOW(PRIV_GETLINES) | ALLOW(PRIV_LOCALTIME) |
> - ALLOW(PRIV_PCAP_STATS),
> + /* RUN */ ALLOW(PRIV_LOCALTIME) | ALLOW(PRIV_PCAP_STATS),
>   /* EXIT */ 0
>  };
>  
> -struct ftab {
> - char *name;
> - int max;
> - int count;
> -};
> -
> -static struct ftab file_table[] = {{PF_OSFP_FILE, 1, 0}};
> -
> -#define NUM_FILETAB (sizeof(file_table) / sizeof(struct ftab))
> -
>  int debug_level = LOG_INFO;
>  int priv_fd = -1;
>  volatile pid_t child_pid = -1;
> @@ -123,7 +112,6 @@ static void impl_getrpcbynumber(int);
>  static void impl_getserventries(int);
>  static void impl_getprotoentries(int);
>  static void impl_localtime(int fd);
> -static void impl_getlines(int);
>  static void impl_pcap_stats(int, int *);
>  
>  static void test_state(int, int);
> @@ -345,10 +333,6 @@ priv_exec(int argc, char *argv[])
>   test_state(cmd, STATE_RUN);
>   impl_localtime(sock);
>   break;
> - case PRIV_GETLINES:
> - test_state(cmd, STATE_RUN);
> - impl_getlines(sock);
> - break;
>   case PRIV_PCAP_STATS:
>   test_state(cmd, STATE_RUN);
>   impl_pcap_stats(sock, &bpfd);
> @@ -577,55 +561,6 @@ impl_localtime(int fd)
>  }
>  
>  static void
> -impl_getlines(int fd)
> -{
> - FILE *fp;
> - char *buf, *lbuf, *file;
> - size_t len, fid;
> -
> - logmsg(LOG_DEBUG, "[priv]: msg PRIV_GETLINES received");
> -
> - must_read(fd, &fid, sizeof(size_t));
> - if (fid >= NUM_FILETAB)
> - errx(1, "invalid file id");
> -
> - file = file_table[fid].name;
> -
> - if (file == NULL)
> - errx(1, "invalid file referenced");
> -
> - if (file_table[fid].count >= file_table[fid].max)
> - errx(1, "maximum open count exceeded for %s", file);
> -
> - file_table[fid].count++;
> -
> - if ((fp = fopen(file, "r")) == NULL) {
> - write_zero(fd);
> - return;
> - }
> -
> - lbuf = NULL;
> - while ((buf = fgetln(fp, &len))) {
> - if (buf[len - 1] == '\n')
> - buf[len - 1] = '\0';
> - else {
> - if ((lbuf = malloc(len + 1)) == NULL)
> - err(1, NULL);
> - memcpy(lbuf, buf, len);
> - lbuf[len] = '\0';
> - buf = lbuf;
> - }
> -
> - write_string(fd, buf);
> -
> - free(lbuf);
> - lbuf = NULL;
> - }
> - write_zero(fd);
> - fclose(fp);
> -}
> -
> -static void
>  impl_pcap_stats(int fd, int *bpfd)
>  {
>   struct pcap_stat stats;
> @@ -786,17 +721,6 @@ priv_localtime(const time_t *t)
>   return &lt;
>  }
>  
> -/* start getting lines from a file */
> -void
> -priv_getlines(size_t sz)
> -{
> - if (priv_fd < 0)
> - errx(1, "%s called from privileged portion", __func__);
> -
> - write_command(priv_fd, PRIV_GETLINES);
> - must_write(priv_fd, &sz, sizeof(size_t));
> -}
> -
>  int
>  priv_pcap_stats(struct pcap_stat *ps)
>  {
> @@ -806,18 +730,6 @@ priv_pcap_stats(struct pcap_stat *ps)
>   write_command(priv_fd, PRIV_PCAP_STATS);
>   must_read(priv_fd, ps, sizeof(*ps));
>   return (0);
> -}
> -
> -/* retrieve a line from a file, should be called repeatedly after calling
> -   priv_getlines(), until it returns zero. */
> -size_t
> -priv_getline(char *line, size_t line_len)
> -{
> - if (priv_fd < 0)
> - errx(1, "%s called from privileged portion", __func__);
> -
> - /* read the line */
> - return (read_string(priv_fd, line, line_len, __func__));
>  }
>  
>  /* Read all data or return 1 for error. */
> Index: privsep.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/tcpdump/privsep.h,v
> retrieving revision 1.10
> diff -u -p -u -r1.10 privsep.h
> --- privsep.h 8 Sep 2017 19:10:57 -0000 1.10
> +++ privsep.h 8 Nov 2018 00:19:47 -0000
> @@ -21,9 +21,6 @@
>  
>  #define TCPDUMP_MAGIC 0xa1b2c3d4
>  
> -/* file ids used by priv_getlines */
> -#define FTAB_PFOSFP 0
> -
>  enum cmd_types {
>   PRIV_OPEN_BPF, /* open a bpf descriptor */
>   PRIV_OPEN_DUMP, /* open dump file for reading */
> @@ -35,7 +32,6 @@ enum cmd_types {
>   PRIV_GETSERVENTRIES, /* get the service entries table */
>   PRIV_GETPROTOENTRIES, /* get the ip protocol entries table */
>   PRIV_LOCALTIME, /* return localtime */
> - PRIV_GETLINES, /* get lines from a file */
>   PRIV_INIT_DONE, /* signal that the initialization is done */
>   PRIV_PCAP_STATS /* get pcap_stats() results */
>  };
> @@ -74,13 +70,6 @@ void priv_getprotoentries(void);
>  /* Retrieve a single protocol entry, should be called repeatedly after
>     calling priv_getprotoentries() until it returns zero */
>  size_t priv_getprotoentry(char *, size_t, int *);
> -
> -/* Start getting lines from a file */
> -void priv_getlines(size_t);
> -
> -/* Retrieve a single line from a file, should be called repeatedly after
> -   calling priv_getlines() until it returns zero */
> -size_t priv_getline(char *, size_t);
>  
>  /* Return the pcap statistics upon completion */
>  int priv_pcap_stats(struct pcap_stat *);
>

Reply | Threaded
Open this post in threaded view
|

Re: tcpdump: revisiting some old diffs, cleanup unused functions

Klemens Nanni-2
In reply to this post by Bryan Steele
OK

Reply | Threaded
Open this post in threaded view
|

Re: tcpdump: revisiting some old diffs, remove unused pledges

Bryan Steele
In reply to this post by Bryan Steele
On Wed, Nov 07, 2018 at 07:32:25PM -0500, Bryan Steele wrote:

> On Wed, Nov 07, 2018 at 07:06:09PM -0500, Bryan Steele wrote:
> > I'm revisiting some old tcpdump diffs, now that mestre@ has added proper
> > unveil(2) support! :-)
> >
> > Refresher: https://marc.info/?l=openbsd-tech&m=150535073209723&w=2
> >
> > This hoists opening pf.os(5) fingerprints '-o' from the 'RUN' state to
> > the 'FILTER' state, this will allow for a reduced pledge(2) at runtime
> > in the (currently root) monitor process.
>
> This was a bit of copy & paste, sorry. This moves the opening of pf.os
> earlier and avoids the unveil later on. Of course, reducing the runtime
> pledge(2) promises will come later! :-)
>
> >
> > This still works as well as it already has. :-)
> >
> >     ( ... ) [tcp sum ok] (src OS: OpenBSD 6.1) 3311509932:3311509932(0) win 16384 <mss 1460,nop,nop,sackOK,nop,wscale 6,nop,nop,timestamp 3905153931 0> (DF) (ttl 64, id 41239, len 64)
> >
> > The only potential difference is that if /etc/pf.os is replaced at
> > runtime, tcpdump won't reopen it.
> >
> > I don't think that's a problem..
> >
> > ok?
> >
> > -Bryan.
>

The first two diffs are in now. Thanks!

The "recvfd" promise doesn't appear to be used by the privileged
monitor process, which currently only handles resolving domain names,
and displaying BIOCGSTATS on ^C. It never sends or receives any file
descriptors while in the 'RUN' state. We can drop it. :-)

Unfortunately the "inet" promise appears to be necessary for some yp(8)
environments, as documented in ether_ntohost(3). I'm not familiar enough
with YP to know if this special /etc/ethers '+' line is typical, or if
DNS should be used instead? In that case it would be covered by the
existing "dns" promise and we could drop "inet" here.

I'd like to try this, and hopefully there's a better solution for YP &
pledge(2) later?

comments or ok?

-Bryan.

Index: privsep.c
===================================================================
RCS file: /cvs/src/usr.sbin/tcpdump/privsep.c,v
retrieving revision 1.50
diff -u -p -u -r1.50 privsep.c
--- usr.sbin/tcpdump/privsep.c 8 Nov 2018 14:06:09 -0000 1.50
+++ usr.sbin/tcpdump/privsep.c 8 Nov 2018 18:43:06 -0000
@@ -309,7 +309,7 @@ priv_exec(int argc, char *argv[])
  err(1, "unveil");
  if (unveil("/etc/rpc", "r") == -1)
  err(1, "unveil");
- if (pledge("stdio rpath inet dns recvfd bpf", NULL) == -1)
+ if (pledge("stdio rpath dns bpf", NULL) == -1)
  err(1, "pledge");
 
  break;