commit e9f2f7e63daa9c3d9a0502a707dfed959aa4f6e3
parent b04abb4f48b81b5ee3b62ce6b86ae5f658898f7e
Author: Stephen Gregoratto <dev@sgregoratto.me>
Date: Wed, 17 Jun 2020 13:05:30 +1000
Overhaul the bpf filter setup stage, unveil early
The original code was sketchy and didn't do proper error checking!
I've rewritten most of it to rely on strlcat/cpy instead.
This kills the get_filter_from_string() function in the process.
Also, I realised that none of the functions were opening any new files,
so we can do the final call to unveil early.
Diffstat:
| M | ngrep.c | | | 184 | ++++++++++++++++++++++++++++++++++++++++++------------------------------------- |
| M | ngrep.h | | | 13 | +++++-------- |
2 files changed, 102 insertions(+), 95 deletions(-)
diff --git a/ngrep.c b/ngrep.c
@@ -5,10 +5,12 @@
* Please refer to the LICENSE file for more information.
*
*/
-#include <sys/types.h>
-#include <sys/socket.h>
#include <sys/ioctl.h>
+#include <sys/limits.h>
+#include <sys/socket.h>
+#include <sys/stat.h>
#include <sys/time.h>
+#include <sys/types.h>
#include <net/if.h>
#include <net/ethertypes.h>
@@ -246,6 +248,11 @@ main(int argc, char **argv)
}
}
+ if (unveil(NULL, NULL) == -1) {
+ warn("unveil");
+ clean_exit(2);
+ }
+
if (show_hex && dump_func != &dump_formatted) {
warnx("-x is incompatible with -W");
usage();
@@ -257,31 +264,21 @@ main(int argc, char **argv)
if (setup_pcap_source())
clean_exit(2);
- if (unveil(NULL, NULL) == -1) {
- warn("unveil");
+ /*
+ * XXX: Originally, the filter would be setup again without the vlan
+ * hack if the first one failed. Not sure if we need to do this.
+ */
+ if (setup_bpf_filter(argv) == -1) {
+ warnx("cannot compile filter: %s", pcap_geterr(pd));
clean_exit(2);
}
- /* Setup BPF filter */
- if (setup_bpf_filter(argv)) {
- include_vlan = 0;
- if (filter) {
- free(filter);
- filter = NULL;
- }
- if (setup_bpf_filter(argv)) {
- warnx("pcap: %s", pcap_geterr(pd));
- clean_exit(2);
- }
- }
-
drop_privs();
- if (filter) {
- if (quiet < 2)
- printf("filter: %s\n", filter);
- free(filter);
- }
+ if (quiet < 2)
+ printf("filter: %s\n", filter);
+ free(filter);
+ filter = NULL;
/* Setup matcher */
if (match_data) {
@@ -389,7 +386,6 @@ setup_pcap_source(void)
pcap_datalink(pd));
return 1;
}
-
return 0;
}
@@ -397,20 +393,17 @@ int
setup_bpf_filter(char **argv)
{
if (filter_file) {
- char buf[1024] = {0};
- FILE *f = fopen(filter_file, "r");
-
- if (!f || !fgets(buf, sizeof(buf) - 1, f)) {
- warn("cannot read filter from %s", filter_file);
- usage();
- }
- fclose(f);
-
- filter = get_filter_from_string(buf);
-
- if (pcap_compile(pd, &pcapfilter, filter, 0, mask.s_addr))
- return 1;
+ filter = get_filter_from_file();
+ if (pcap_compile(pd, &pcapfilter, filter, 0, mask.s_addr) == -1)
+ return -1;
} else if (argv[optind]) {
+ /*
+ * XXX: Find a smarter way of doing this that doesn't eat memory.
+ *
+ * If the filter doesn't compile the first time,
+ * we assume that there is no match expression and include the
+ * previous argument in the filter.
+ */
filter = get_filter_from_argv(&argv[optind]);
if (pcap_compile(pd, &pcapfilter, filter, 0, mask.s_addr)) {
@@ -418,19 +411,22 @@ setup_bpf_filter(char **argv)
filter = get_filter_from_argv(&argv[optind - 1]);
if (pcap_compile(pd, &pcapfilter, filter, 0, mask.s_addr))
- return 1;
+ return -1;
match_data = NULL;
}
} else {
- filter = include_vlan ? strdup(BPF_TEMPLATE_IP_VLAN) : strdup(BPF_TEMPLATE_IP);
+ filter = include_vlan ?
+ strdup(BPF_TEMPLATE_IP_VLAN) : strdup(BPF_TEMPLATE_IP);
+ if (filter == NULL)
+ err(2, "strdup");
if (pcap_compile(pd, &pcapfilter, filter, 0, mask.s_addr))
- return 1;
+ return -1;
}
if (pcap_setfilter(pd, &pcapfilter))
- return 1;
+ return -1;
return 0;
}
@@ -958,76 +954,90 @@ dump_formatted(unsigned char *data, uint32_t len, uint16_t mindex,
}
char *
-get_filter_from_string(char *str)
+get_filter_from_file(void)
{
+ char *fstr;
+ size_t size, prefix;
const char *template;
- char *mine, *s;
- uint32_t len;
-
- template = include_vlan ?
- BPF_TEMPLATE_USERSPEC_IP_VLAN :
- BPF_TEMPLATE_USERSPEC_IP;
-
- if (!str || !*str)
- return NULL;
+ int fd;
+ struct stat st;
+ ssize_t flen;
- len = (uint32_t) strlen(str);
+ /* filter = prefix + " and ( " + file + " )" */
+ template = include_vlan ? BPF_TEMPLATE_IP_VLAN : BPF_TEMPLATE_IP;
+ prefix = strlen(template) + 5;
- for (s = str; *s; s++)
- if (*s == '\r' || *s == '\n')
- *s = ' ';
-
- if (!(mine = (char *) malloc(len + strlen(template) + 1)))
- return NULL;
-
- memset(mine, 0, len + strlen(template) + 1);
+ if ((fd = open(filter_file, O_RDONLY)) == -1) {
+ warn("cannot open %s", filter_file);
+ clean_exit(2);
+ }
+ if (fstat(fd, &st) == -1) {
+ warn("cannot stat %s", filter_file);
+ clean_exit(2);
+ }
+ if (st.st_size >= SSIZE_MAX) {
+ warn("file too long: %s", filter_file);
+ clean_exit(2);
+ }
- sprintf(mine, template, str);
+ size = prefix + 8 + st.st_size + 2;
+ if (size >= SIZE_MAX)
+ warnx("fstr too long: %s", filter_file);
+ if ((fstr = malloc(size)) == NULL)
+ err(1, "malloc");
+
+ (void)strlcpy(fstr, template, size);
+ (void)strlcat(fstr, " and ( ", size);
+ flen = read(fd, fstr + prefix, st.st_size);
+ if (flen == -1) {
+ warn("read %s", filter_file);
+ clean_exit(2);
+ } else if (flen != st.st_size) {
+ warn("short read of %s: expected %lld, got %zu", filter_file, st.st_size, flen);
+ clean_exit(2);
+ }
+ (void)strlcat(fstr, " )", size);
- return mine;
+ return fstr;
}
char *
get_filter_from_argv(char **argv)
{
+ char *fstr;
+ size_t size;
+ char **arg = argv;
const char *template;
- char **arg = argv, *theirs, *mine;
- char *from, *to;
- uint32_t len = 0;
- template = include_vlan ?
- BPF_TEMPLATE_USERSPEC_IP_VLAN :
- BPF_TEMPLATE_USERSPEC_IP;
+ /* filter = prefix + " and ( " + args + " )" */
+ template = include_vlan ? BPF_TEMPLATE_IP_VLAN : BPF_TEMPLATE_IP;
+ size = strlen(template) + 8 + 2;
- if (!*arg)
+ if (arg == NULL)
return NULL;
- while (*arg)
- len += (uint32_t) strlen(*arg++) + 1;
+ for (; *arg != NULL; arg++)
+ size += strlen(*arg) + 1;
- if (!(theirs = (char *) malloc(len + 1)) ||
- !(mine = (char *) malloc(len + strlen(template) + 1)))
- return NULL;
-
- memset(theirs, 0, len + 1);
- memset(mine, 0, len + strlen(template) + 1);
-
- arg = argv;
- to = theirs;
-
- while ((from = *arg++)) {
- while ((*to++ = *from++));
- *(to - 1) = ' ';
+ if (size >= SIZE_MAX) {
+ warnx("filter too long");
+ clean_exit(2);
}
- sprintf(mine, template, theirs);
+ if ((fstr = malloc(size)) == NULL)
+ err(2, "malloc");
- free(theirs);
+ (void)strlcpy(fstr, template, size);
+ (void)strlcat(fstr, " and ( ", size);
+ for (arg = argv; *arg != NULL; arg++) {
+ (void)strlcat(fstr, *arg, size);
+ (void)strlcat(fstr, " ", size);
+ }
+ (void)strlcat(fstr, ")", size);
- return mine;
+ return fstr;
}
-
uint8_t
strishex(char *str)
{
diff --git a/ngrep.h b/ngrep.h
@@ -48,14 +48,11 @@
* targeting IP traffic, and compensating for the variable offset in the packet
* decoder.
*/
+#define BPF_FILTER_IP_TYPE "(ip || ip6)"
+#define BPF_TEMPLATE_IP BPF_FILTER_IP_TYPE
+#define BPF_TEMPLATE_IP_VLAN "(" BPF_FILTER_IP_TYPE " || (vlan && " BPF_FILTER_IP_TYPE "))"
-#define BPF_FILTER_IP_TYPE "(ip || ip6)"
-#define BPF_TEMPLATE_IP BPF_FILTER_IP_TYPE
-#define BPF_TEMPLATE_IP_VLAN "(" BPF_FILTER_IP_TYPE " || (vlan && " BPF_FILTER_IP_TYPE "))"
-#define BPF_TEMPLATE_USERSPEC_IP "( %s) and " BPF_TEMPLATE_IP
-#define BPF_TEMPLATE_USERSPEC_IP_VLAN "( %s) and " BPF_TEMPLATE_IP_VLAN
-
-#define WORD_REGEX "((^%s\\W)|(\\W%s$)|(\\W%s\\W))"
+#define WORD_REGEX "((^%s\\W)|(\\W%s$)|(\\W%s\\W))"
/*
* Single-char packet "ident" flags.
@@ -97,7 +94,7 @@ void print_time_absolute(struct pcap_pkthdr *);
void print_time_diff(struct pcap_pkthdr *);
void print_time_offset(struct pcap_pkthdr *);
-char *get_filter_from_string(char *);
+char *get_filter_from_file(void);
char *get_filter_from_argv(char **);
uint8_t strishex(char *);