Re: pf state tracking and tos/dscp
Adam Gensler openbsd at kristenandadam.net writes: all udp 1.1.1.1:4500 - 172.28.1.3:4500 MULTIPLE:MULTIPLE age 00:15:50, expires in 00:00:57, 394:196 pkts, 52356:39176 bytes, rule 37 put pass all tos higher in your ruleset and see if it matches.
Re: pf state tracking and tos/dscp
Adam Gensler openbsd at kristenandadam.net writes: local_nets = { 172.28.1.0/24, 172.28.10.0/24, 172.28.11.0/24 } work871 = 172.28.1.3 pass in quick inet proto udp from $work871 tos 0xB8 tag VOIP-RTP pass in quick inet proto udp from $work871 tos 0x60 tag VOIP-SIG pass in quick inet proto { tcp, udp } from $local_nets Another possible thing I see, is a tunnel originating side. Since tos rules you have are unidirectional (in terms of match), they will create state if only first packet comes from $work871. However, first packet coming from other side will match another rule and create state, so all subsequent tunnel's packets will not hit tos rules.
Re: pf state tracking and tos/dscp
The TOS class isn't (and can't be) used to match packets to the state. Once you have created state from a packet with one TOS class, other packets with the same src/dest ip/port match this state even if the class is different. (It has to be this way - say you are natting - you wouldn't want a different source port if some packets happen to use different TOS). PF states include two queue names - one for normal traffic, one for high prioroty traffic: empty TCP ACKs and packets with TOS 'lowdelay' (0x10). So perhaps you would get somewhere by using 'match in from $work871 tos 0xB8 set tos lowdelay' to re-mark the incoming packets as 'lowdelay' and using e.g. 'set queue (vpn, vpn-fast)'. Otherwise, if you don't need NAT, disabling state tracking for these packets might be an option. Adam Gensler open...@kristenandadam.net wrote: Hi all, I've been playing with pf for a number of months now and I've come across a situation that I'm having trouble finding a solution for. Specifically I'm working with the following topology: Internet --- OpenBSD box --- Cisco router --- other devices The Cisco router (a small 800 series router) is sitting behind an OpenBSD box (version 5.1). This Cisco router has an IPsec tunnel connected to another router out on the Internet. So, all the OpenBSD box sees is a bunch of encrypted frames (udp port 4500). That's all fine and dandy, that works well, no problems there. However, some of the devices behind the Cisco router are setting the tos/dscp bits on their packets. I would like to be able to prioritize those packets in pf for altq handling. So, I've created the following rules: local_nets = { 172.28.1.0/24, 172.28.10.0/24, 172.28.11.0/24 } work871 = 172.28.1.3 pass in quick inet proto udp from $work871 tos 0xB8 tag VOIP-RTP pass in quick inet proto udp from $work871 tos 0x60 tag VOIP-SIG pass in quick inet proto { tcp, udp } from $local_nets The idea here being that ingress traffic from 172.28.1.3, with the various tos values will be tagged with a specific tag. On the egress side I match on that tag and then apply it to a queue. That isn't working though. It seems that PF creates a state entry first for the overall ipsec tunnel using the third rule. After that state gets established all subsequent packets do not evaluate the rules, even if those packets have different tos values. This leaves me to believe that pf isn't creating a state entry tuple that contains the tos value. I've confirmed the TOS bits are being carried through to the IP header of the IPSEC packets. Here's a tcpdump of the incoming packets from my LAN interface (vr0): tcpdump: listening on vr0, link-type EN10MB (Ethernet), capture size 65535 bytes 21:37:39.464357 IP (tos 0xc0, ttl 255, id 848, offset 0, flags [none], proto UDP (17), length 144) 172.28.1.3.4500 1.1.1.1.4500: UDP-encap: ESP(spi=0x32284280,seq=0x12e), length 116 21:37:39.483350 IP (tos 0xc0, ttl 255, id 1707, offset 0, flags [none], proto UDP (17), length 29) 172.28.1.3.4500 64.102.7.50.4500: isakmp-nat-keep-alive 21:37:42.365389 IP (tos 0x0, ttl 255, id 849, offset 0, flags [none], proto UDP (17), length 152) 172.28.1.3.4500 1.1.1.1.4500: UDP-encap: ESP(spi=0x32284280,seq=0x12f), length 124 21:37:45.465724 IP (tos 0xc0, ttl 255, id 850, offset 0, flags [none], proto UDP (17), length 144) 172.28.1.3.4500 1.1.1.1.4500: UDP-encap: ESP(spi=0x32284280,seq=0x130), length 116 21:37:47.370081 IP (tos 0x0, ttl 255, id 851, offset 0, flags [none], proto UDP (17), length 152) 172.28.1.3.4500 1.1.1.1.4500: UDP-encap: ESP(spi=0x32284280,seq=0x131), length 124 21:37:49.256302 IP (tos 0x60, ttl 255, id 852, offset 0, flags [none], proto UDP (17), length 120) 172.28.1.3.4500 1.1.1.1.4500: UDP-encap: ESP(spi=0x32284280,seq=0x132), length 92 From this it's clear that the last packet has tos 0x60 set. However, if I look at this particular rule, it doesn't have any matches or state entries: pass in quick inet proto udp from 172.28.1.3 to any tos 0x60 keep state tag VOIP-SIG [ Evaluations: 34Packets: 0 Bytes: 0 States: 0 ] [ Inserted: uid 0 pid 13666 State Creations: 0 ] Instead, there's a state entry logged for this traffic under the third rule: all udp 1.1.1.1:4500 - 172.28.1.3:4500 MULTIPLE:MULTIPLE age 00:15:50, expires in 00:00:57, 394:196 pkts, 52356:39176 bytes, rule 37 Based on the above, it seems like the state entries don't include the tos information making it impossible to properly classify traffic that is encrypted with ipsec. The only way to differentiate various traffic streams contained within the tunnel is via tos/dscp. This is fairly common practice in the Enterprise routing space. I'd love to be able to do the same thing here. Am I missing something obvious? I've tested OpenBSD 5.1 and (for what its worth) FreeBSD 9.0 and also pfsense 2.1beta1, I see the same behavior on all of them. Thanks in advance, Adam
Re: [PATCH] add filter by host functionality to syslogd
On 05/22/2013 06:39 PM, Ted Unangst wrote: On Wed, May 22, 2013 at 12:06, Gregory Edigarov wrote: works for me, with only one limitation: now only for resolvable hosts, i.e one cannot have +192.168.2.1 * /some/file Looking at the diff, I think it's not resolvable hosts, but whatever hostname the sending machine decides to tell you? no, it is really a resolvable hosts. works correctly with name in /etc/hosts. My first thought is that we shouldn't rely on that, and syslogd should refuse requests entirely from servers it doesn't like. My second thought is that's what pf is for and spoofing syslog entries is already pretty easy, so this is fine, but it needs to be documented as such. -- With best regards, Gregory Edigarov
Re: [PATCH] add filter by host functionality to syslogd
no, it is really a resolvable hosts. It is unsafe.
Re: route(8) use -inet6 automatically for addresses containing :
On 2013/05/23 15:03, Sebastian Benoit wrote: Stuart Henderson(st...@openbsd.org) on 2013.05.22 21:18:05 +0100: On 2013/05/22 20:47, Stuart Henderson wrote: does anyone see a downside to this? i see none other than making the maze of options of route(8) a bit more bizarre. if the address family is not explicitly specified, assume v6 if it looks like it may be an ipv6 address. allows e.g. route get 2001:200:dff:fff1:216:3eff:feb1:44d7 without needing to specify -inet6. oops, as pointed out by jca@, I missed aflen (or rather, saw it and for some unknown reason thought it didn't matter, I blame my tooth ;) Index: route.c === RCS file: /cvs/src/sbin/route/route.c,v retrieving revision 1.161 diff -u -p -r1.161 route.c --- route.c 21 Mar 2013 04:43:17 - 1.161 +++ route.c 22 May 2013 20:15:53 - @@ -803,8 +803,13 @@ getaddr(int which, char *s, struct hoste int afamily, bits; if (af == 0) { - af = AF_INET; - aflen = sizeof(struct sockaddr_in); + if (strchr(s, ':') != NULL) { + af = AF_INET6; + aflen = sizeof(struct sockaddr_in6); + } else { + af = AF_INET; + aflen = sizeof(struct sockaddr_in); + } } afamily = af; /* local copy of af so we can change it */ ok maybe a note in the manpage? Possible manpage wording.. Index: route.8 === RCS file: /cvs/src/sbin/route/route.8,v retrieving revision 1.70 diff -u -p -r1.70 route.8 --- route.8 13 Jul 2012 10:15:53 - 1.70 +++ route.8 23 May 2013 13:49:50 - @@ -310,6 +310,9 @@ Actual data, in hexadecimal format .El .Pp +In the absence of modifiers, an address is assumed to be IPv4, +unless containing a : character when it is treated as IPv6. +.Pp The optional modifier .Fl link specifies that all subsequent addresses are specified as link-level addresses,
Re: [PATCH] add filter by host functionality to syslogd
On Thu, May 23, 2013 at 12:57, Gregory Edigarov wrote: On 05/22/2013 06:39 PM, Ted Unangst wrote: On Wed, May 22, 2013 at 12:06, Gregory Edigarov wrote: works for me, with only one limitation: now only for resolvable hosts, i.e one cannot have +192.168.2.1 * /some/file Looking at the diff, I think it's not resolvable hosts, but whatever hostname the sending machine decides to tell you? no, it is really a resolvable hosts. works correctly with name in /etc/hosts. I don't understand how that can be, since your patch isn't doing any DNS resolution. if (f-f_program) if (strcmp(prog, f-f_program) != 0) continue; + if (f-f_host) + if (strcmp(from,f-f_host) != 0) + continue; That's just a string compare. The remote host can send any string it wants.
Re: [PATCH] add filter by host functionality to syslogd
On 05/23/2013 07:20 PM, Ted Unangst wrote: On Thu, May 23, 2013 at 12:57, Gregory Edigarov wrote: On 05/22/2013 06:39 PM, Ted Unangst wrote: On Wed, May 22, 2013 at 12:06, Gregory Edigarov wrote: works for me, with only one limitation: now only for resolvable hosts, i.e one cannot have +192.168.2.1 * /some/file Looking at the diff, I think it's not resolvable hosts, but whatever hostname the sending machine decides to tell you? no, it is really a resolvable hosts. works correctly with name in /etc/hosts. I don't understand how that can be, since your patch isn't doing any DNS resolution. if (f-f_program) if (strcmp(prog, f-f_program) != 0) continue; + if (f-f_host) + if (strcmp(from,f-f_host) != 0) + continue; That's just a string compare. The remote host can send any string it wants. yes, it doesn't do any host resolution itself and there is no need in this, because syslogd already does this. (the resolution happens in main cycle, namely cvthname() function call). the changes just compare the hostname given in configs and the hostname reported by cvthname, then act accordinly. -- With best regards, Gregory Edigarov
Re: [PATCH] add filter by host functionality to syslogd
On Thu, May 23, 2013 at 19:55, Gregory Edigarov wrote: That's just a string compare. The remote host can send any string it wants. yes, it doesn't do any host resolution itself and there is no need in this, because syslogd already does this. (the resolution happens in main cycle, namely cvthname() function call). the changes just compare the hostname given in configs and the hostname reported by cvthname, then act accordinly. Oh, my mistake. I thought host, like program, came via the network. Thanks for explaining that. I should have looked at the whole program again.
mandoc strlcat
I was looking at mandoc and noticed it has too many strlcats (a common affliction affecting quite a few programs.) It's faster and simpler to use snprintf. The code in roff.c was doing something twisty with the length argument to strlcpy. Doing fancy length tricks kind of defeats the purpose of having a simple to use function like strlcpy. I believe the intention was to only copy part of the string, so I have retained that feature. Index: html.c === RCS file: /cvs/src/usr.bin/mandoc/html.c,v retrieving revision 1.30 diff -u -p -r1.30 html.c --- html.c 28 May 2012 13:00:51 - 1.30 +++ html.c 23 May 2013 20:52:43 - @@ -618,10 +618,7 @@ void bufcat_style(struct html *h, const char *key, const char *val) { - bufcat(h, key); - bufcat(h, :); - bufcat(h, val); - bufcat(h, ;); + bufcat_fmt(h, %s:%s;, key, val); } void Index: man_html.c === RCS file: /cvs/src/usr.bin/mandoc/man_html.c,v retrieving revision 1.48 diff -u -p -r1.48 man_html.c --- man_html.c 17 Nov 2012 00:25:20 - 1.48 +++ man_html.c 23 May 2013 20:50:52 - @@ -301,9 +301,10 @@ man_root_pre(MAN_ARGS) struct tag *t, *tt; char b[BUFSIZ], title[BUFSIZ]; - b[0] = 0; if (man-vol) - (void)strlcat(b, man-vol, BUFSIZ); + strlcpy(b, man-vol, sizeof(b)); + else + b[0] = '\0'; assert(man-title); assert(man-msec); Index: mandocdb.c === RCS file: /cvs/src/usr.bin/mandoc/mandocdb.c,v retrieving revision 1.42 diff -u -p -r1.42 mandocdb.c --- mandocdb.c 24 May 2012 23:33:23 - 1.42 +++ mandocdb.c 23 May 2013 20:23:31 - @@ -286,7 +286,6 @@ mandocdb(int argc, char *argv[]) int ch, i, flags; DB *hash; /* temporary keyword hashtable */ BTREEINFOinfo; /* btree configuration */ - size_t sz1, sz2; struct buf buf, /* keyword buffer */ dbuf; /* description buffer */ struct of *of; /* list of files for processing */ @@ -397,19 +396,12 @@ mandocdb(int argc, char *argv[]) perror(dir); exit((int)MANDOCLEVEL_BADARG); } - if (strlcat(pbuf, /, PATH_MAX) = PATH_MAX) { - fprintf(stderr, %s: path too long\n, pbuf); - exit((int)MANDOCLEVEL_BADARG); - } - - strlcat(mdb.dbn, pbuf, MAXPATHLEN); - sz1 = strlcat(mdb.dbn, MANDOC_DB, MAXPATHLEN); - - strlcat(mdb.idxn, pbuf, MAXPATHLEN); - sz2 = strlcat(mdb.idxn, MANDOC_IDX, MAXPATHLEN); - if (sz1 = MAXPATHLEN || sz2 = MAXPATHLEN) { - fprintf(stderr, %s: path too long\n, mdb.idxn); + if ((size_t)snprintf(mdb.dbn, sizeof(mdb.dbn), %s/%s, pbuf, + MANDOC_DB) = sizeof(mdb.dbn) || + (size_t)snprintf(mdb.idxn, sizeof(mdb.idxn), %s/%s, pbuf, + MANDOC_IDX) = sizeof(mdb.dbn)) { + fprintf(stderr, %s: path too long\n, mdb.dbn); exit((int)MANDOCLEVEL_BADARG); } @@ -486,8 +478,8 @@ mandocdb(int argc, char *argv[]) flags = O_CREAT | O_EXCL | O_RDWR; while (NULL == mdb.db) { - strlcpy(mdb.dbn, MANDOC_DB, MAXPATHLEN); - strlcat(mdb.dbn, .XX, MAXPATHLEN); + snprintf(mdb.dbn, sizeof(mdb.dbn), %s.XX, + MANDOC_DB); if (NULL == mktemp(mdb.dbn)) { perror(mdb.dbn); exit((int)MANDOCLEVEL_SYSERR); @@ -500,8 +492,8 @@ mandocdb(int argc, char *argv[]) } } while (NULL == mdb.idx) { - strlcpy(mdb.idxn, MANDOC_IDX, MAXPATHLEN); - strlcat(mdb.idxn, .XX, MAXPATHLEN); + snprintf(mdb.idxn, sizeof(mdb.idxn), %s.XX, + MANDOC_IDX); if (NULL == mktemp(mdb.idxn)) { perror(mdb.idxn); unlink(mdb.dbn); @@ -1809,12 +1801,8 @@ ofile_dirbuild(const char *dir, const ch continue; } - buf[0] = '\0'; - strlcat(buf, dir, MAXPATHLEN); - strlcat(buf, /, MAXPATHLEN); - sz = strlcat(buf, fn, MAXPATHLEN); - - if (MAXPATHLEN = sz) { + if ((size_t)snprintf(buf,
simplify strlcat in tcpdump
just pfctl_osfp.c. harder to replace strlcat when it's in a loop, but some of the straight line calls can be done as snprintf followed by one strlcat. worth it? Index: pfctl_osfp.c === RCS file: /cvs/src/usr.sbin/tcpdump/pfctl_osfp.c,v retrieving revision 1.7 diff -u -p -r1.7 pfctl_osfp.c --- pfctl_osfp.c18 Oct 2010 15:55:28 - 1.7 +++ pfctl_osfp.c23 May 2013 21:28:31 - @@ -1001,33 +1001,28 @@ const char * print_ioctl(struct pf_osfp_ioctl *fp) { static char buf[1024]; - char tmp[32]; + char tmp[1024]; int i, opt; + char *c; - *buf = '\0'; + c = ; + tmp[0] = '\0'; if (fp-fp_flags PF_OSFP_WSIZE_DC) - strlcat(buf, *, sizeof(buf)); + c = *; else if (fp-fp_flags PF_OSFP_WSIZE_MSS) - strlcat(buf, S, sizeof(buf)); + c = S; else if (fp-fp_flags PF_OSFP_WSIZE_MTU) - strlcat(buf, T, sizeof(buf)); + c = T; else { if (fp-fp_flags PF_OSFP_WSIZE_MOD) - strlcat(buf, %, sizeof(buf)); + c = %; snprintf(tmp, sizeof(tmp), %d, fp-fp_wsize); - strlcat(buf, tmp, sizeof(buf)); } - strlcat(buf, :, sizeof(buf)); + snprintf(buf, sizeof(buf), %s%s, c, tmp); - snprintf(tmp, sizeof(tmp), %d, fp-fp_ttl); + snprintf(tmp, sizeof(tmp), :%d:%d:, fp-fp_ttl, + (fp-fp_flags PF_OSFP_DF) ? 1 : 0); strlcat(buf, tmp, sizeof(buf)); - strlcat(buf, :, sizeof(buf)); - - if (fp-fp_flags PF_OSFP_DF) - strlcat(buf, 1, sizeof(buf)); - else - strlcat(buf, 0, sizeof(buf)); - strlcat(buf, :, sizeof(buf)); if (fp-fp_flags PF_OSFP_PSIZE_DC) strlcat(buf, *, sizeof(buf)); @@ -1083,16 +1078,10 @@ print_ioctl(struct pf_osfp_ioctl *fp) if (i != 0) strlcat(buf, ,, sizeof(buf)); } - strlcat(buf, :, sizeof(buf)); - - strlcat(buf, fp-fp_os.fp_class_nm, sizeof(buf)); - strlcat(buf, :, sizeof(buf)); - strlcat(buf, fp-fp_os.fp_version_nm, sizeof(buf)); - strlcat(buf, :, sizeof(buf)); - strlcat(buf, fp-fp_os.fp_subtype_nm, sizeof(buf)); - strlcat(buf, :, sizeof(buf)); - snprintf(tmp, sizeof(tmp), TcpOpts %d 0x%llx, fp-fp_optcnt, + snprintf(tmp, sizeof(tmp), :%s:%s:%s:TcpOpts %d 0x%llx, + fp-fp_os.fp_class_nm, fp-fp_os.fp_version_nm, + fp-fp_os.fp_subtype_nm, fp-fp_optcnt, (long long int)fp-fp_tcpopts); strlcat(buf, tmp, sizeof(buf));
Re: mandoc strlcat
On Thu, May 23, 2013 at 05:05:45PM -0400, Ted Unangst wrote: I was looking at mandoc and noticed it has too many strlcats (a common affliction affecting quite a few programs.) It's faster and simpler to use snprintf. In glibc snprintf has a memory allocation failure mode. I'm curious: is OpenBSD committed to avoiding extensions (locale features, etc) which might trigger allocation failure?
Re: mandoc strlcat
On Thu, May 23, 2013 at 05:05:45PM -0400, Ted Unangst wrote: I was looking at mandoc and noticed it has too many strlcats (a common affliction affecting quite a few programs.) It's faster and simpler to use snprintf. In glibc snprintf has a memory allocation failure mode. In OpenBSD, snprintf is designed to be thread and signal-handler safe, as long as you don't use certain dangerous features. I'm afraid I can't find documentation which defines which are dangerous or not, but remember auditing our tree to improve the situation. I'm curious: is OpenBSD committed to avoiding extensions (locale features, etc) which might trigger allocation failure? I don't know if we are commited to such a restriction. We could add such things, but then put them in the dangerous catagory, to not be used in unsafe situations... Hmm, where are our docs for that...
Re: mandoc strlcat
On Thu, May 23, 2013 at 21:38, Theo de Raadt wrote: In glibc snprintf has a memory allocation failure mode. I'm curious: is OpenBSD committed to avoiding extensions (locale features, etc) which might trigger allocation failure? Yes. I mean, what in the world is snprintf doing allocating some locale crap to implement a behavior that strlcat clearly doesn't need to allocate memory for? I don't know if we are commited to such a restriction. We could add such things, but then put them in the dangerous catagory, to not be used in unsafe situations... Hmm, where are our docs for that... It's in man signal. The only thing you can't use is floating point, because dtoa is crazy, but I think it'd even be possible to pass the buffer in from vfprintf and make that signal safe too. Just nobody cares.
Re: mandoc strlcat
It's in man signal. The only thing you can't use is floating point, because dtoa is crazy, The *5 table, yes. I tried to improve the situation there. I nearly lost my mind.