Re: IPv6 routing header type 0
* Theo de Raadt dera...@cvs.openbsd.org [2013-11-15 01:38]: My diff was on tech@ for one day during a hackathon before I commited it. NOT hidden / circulated privately. The reasons why I removed the check in the stack are: - Scanning headers in the forwarding path is against the spirit of IPv6. One day someone should find the people who pushed RH0 into IPv6 and punish them. ok henning :) - It is pf's job to add more security. It is. However, you will note that in IPv4 land we have sysctl net.inet.ip.sourceroute. It defaults to 0 (off). RH is like IPv4 source routing, except on steriods. Would any of us at this time recommend net.inet.ip.sourceroute=1, or to go further, remove the code disabling code from the kernel and assume that pf is doing the filtering? I doubt it. that analogy is actually a good one. net.inet.ip.sourceroute controls wether we OBEY src routes. as in, we don't by default, as we don't obey RH0 at all, without a button. we do, however, NOT remove src routing information from forwarded packets. - The scanning was done twice with pf enabled. This latter point is very valid. I am very happy with your new approach that does the extra scanning only if pf is disabled. no doubt that is an improvement. I only believe in this approach when the header is already cache-hot, and there is little performance. Untimately if many feel pf is always on, then there is no argument for resisting code for the pf is disabled case... heh :) -- Henning Brauer, h...@bsws.de, henn...@openbsd.org BS Web Services GmbH, http://bsws.de, Full-Service ISP Secure Hosting, Mail and DNS Services. Dedicated Servers, Root to Fully Managed Henning Brauer Consulting, http://henningbrauer.com/
Re: IPv6 routing header type 0
On Thu, Nov 14, 2013 at 05:38:14PM -0700, Theo de Raadt wrote:
Beautiful.
I seems there was enough discussion. The Security argument is more
important than the others. The new diff has no performance impact
when pf is turned on.
So I need OKs.
bluhm
Index: net/pf.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf.c,v
retrieving revision 1.857
diff -u -p -u -p -r1.857 pf.c
--- net/pf.c30 Oct 2013 11:35:10 - 1.857
+++ net/pf.c13 Nov 2013 23:14:32 -
@@ -6490,6 +6490,7 @@ pf_test(sa_family_t af, int fwdir, struc
}
}
pd.eh = eh;
+ pd.m-m_pkthdr.pf.flags |= PF_TAG_PROCESSED;
switch (pd.virtual_proto) {
Index: netinet6/ip6_input.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/ip6_input.c,v
retrieving revision 1.120
diff -u -p -u -p -r1.120 ip6_input.c
--- netinet6/ip6_input.c11 Nov 2013 09:15:35 - 1.120
+++ netinet6/ip6_input.c13 Nov 2013 23:38:22 -
@@ -122,6 +122,7 @@ struct ifqueue ip6intrq;
struct ip6stat ip6stat;
void ip6_init2(void *);
+int ip6_check_rh0hdr(struct mbuf *, int *);
int ip6_hopopts_input(u_int32_t *, u_int32_t *, struct mbuf **, int *);
struct mbuf *ip6_pullexthdr(struct mbuf *, size_t, int);
@@ -331,6 +332,20 @@ ip6_input(struct mbuf *m)
srcrt = !IN6_ARE_ADDR_EQUAL(odst, ip6-ip6_dst);
#endif
+ /*
+* Be more secure than RFC5095 and scan for type 0 routing headers.
+* If pf has already scanned the header chain, do not do it twice.
+*/
+ if (!(m-m_pkthdr.pf.flags PF_TAG_PROCESSED)
+ ip6_check_rh0hdr(m, off)) {
+ ip6stat.ip6s_badoptions++;
+ in6_ifstat_inc(m-m_pkthdr.rcvif, ifs6_in_discard);
+ in6_ifstat_inc(m-m_pkthdr.rcvif, ifs6_in_hdrerr);
+ icmp6_error(m, ICMP6_PARAM_PROB, ICMP6_PARAMPROB_HEADER, off);
+ /* m is already freed */
+ return;
+ }
+
if (IN6_IS_ADDR_LOOPBACK(ip6-ip6_src) ||
IN6_IS_ADDR_LOOPBACK(ip6-ip6_dst)) {
ours = 1;
@@ -698,6 +713,74 @@ ip6_input(struct mbuf *m)
return;
bad:
m_freem(m);
+}
+
+/* scan packet for RH0 routing header. Mostly stolen from pf.c:pf_test() */
+int
+ip6_check_rh0hdr(struct mbuf *m, int *offp)
+{
+ struct ip6_hdr *ip6 = mtod(m, struct ip6_hdr *);
+ struct ip6_rthdr rthdr;
+ struct ip6_ext opt6;
+ u_int8_t proto = ip6-ip6_nxt;
+ int done = 0, lim, off, rh_cnt = 0;
+
+ off = ((caddr_t)ip6 - m-m_data) + sizeof(struct ip6_hdr);
+ lim = min(m-m_pkthdr.len, ntohs(ip6-ip6_plen) + sizeof(*ip6));
+ do {
+ switch (proto) {
+ case IPPROTO_ROUTING:
+ *offp = off;
+ if (rh_cnt++) {
+ /* more then one rh header present */
+ return (1);
+ }
+
+ if (off + sizeof(rthdr) lim) {
+ /* packet to short to make sense */
+ return (1);
+ }
+
+ m_copydata(m, off, sizeof(rthdr), (caddr_t)rthdr);
+
+ if (rthdr.ip6r_type == IPV6_RTHDR_TYPE_0) {
+ *offp += offsetof(struct ip6_rthdr, ip6r_type);
+ return (1);
+ }
+
+ off += (rthdr.ip6r_len + 1) * 8;
+ proto = rthdr.ip6r_nxt;
+ break;
+ case IPPROTO_AH:
+ case IPPROTO_HOPOPTS:
+ case IPPROTO_DSTOPTS:
+ /* get next header and header length */
+ if (off + sizeof(opt6) lim) {
+ /*
+* Packet to short to make sense, we could
+* reject the packet but as a router we
+* should not do that so forward it.
+*/
+ return (0);
+ }
+
+ m_copydata(m, off, sizeof(opt6), (caddr_t)opt6);
+
+ if (proto == IPPROTO_AH)
+ off += (opt6.ip6e_len + 2) * 4;
+ else
+ off += (opt6.ip6e_len + 1) * 8;
+ proto = opt6.ip6e_nxt;
+ break;
+ case IPPROTO_FRAGMENT:
+ default:
+ /* end of header stack */
+ done = 1;
+ break;
+ }
+ } while (!done);
+
+ return (0);
}
/*
Index: sys/mbuf.h
===
Re: IPv6 routing header type 0
On 15 November 2013 15:08, Alexander Bluhm alexander.bl...@gmx.net wrote: On Thu, Nov 14, 2013 at 05:38:14PM -0700, Theo de Raadt wrote: Beautiful. I seems there was enough discussion. The Security argument is more important than the others. The new diff has no performance impact when pf is turned on. So I need OKs. bluhm OK mikeb
Re: IPv6 routing header type 0
On Thu, Nov 14, 2013 at 4:27 AM, Alexander Bluhm
alexander.bl...@gmx.net wrote:
On Fri, Oct 18, 2013 at 08:45:02PM +0200, Alexander Bluhm wrote:
Our IPv6 stack scans all extension headers for routing header type
0 and drops the packet if it finds one. RFC 5095 demands to handle
a routing header type 0 like an unrecognised routing type. This
is enough to protect the own machine.
To protect a network as a firewall, we have pf which does the same
full scan in pf_walk_header6(). As pf is enabled by default, nothing
changes for most users. If you turn off pf on your router, you
should not expect extra protection.
I would like to get rid of the double scanning and the old disabled
code in the IPv6 stack.
Theo and others don't like that change as it decreases security.
There are hosts out there that still process RH0 and there are
OpenBSD routers with pf disabled.
This diff brings back the header chain scanning. As an improvement
it only scans if pf has not done that before.
Note that ip6_check_rh0hdr() can be easily tricked by hiding the
routing header type 0 behind a fragment header. Only pf can protect
you correctly as it reassembles on the forwarding path. So I am
not sure wether it is worth adding it again.
Comments?
I guess it would help people who decide to disable pf for slight
performance benefit ?
bluhm
Index: net/pf.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf.c,v
retrieving revision 1.857
diff -u -p -u -p -r1.857 pf.c
--- net/pf.c30 Oct 2013 11:35:10 - 1.857
+++ net/pf.c13 Nov 2013 23:14:32 -
@@ -6490,6 +6490,7 @@ pf_test(sa_family_t af, int fwdir, struc
}
}
pd.eh = eh;
+ pd.m-m_pkthdr.pf.flags |= PF_TAG_PROCESSED;
switch (pd.virtual_proto) {
Index: netinet6/ip6_input.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/ip6_input.c,v
retrieving revision 1.120
diff -u -p -u -p -r1.120 ip6_input.c
--- netinet6/ip6_input.c11 Nov 2013 09:15:35 - 1.120
+++ netinet6/ip6_input.c13 Nov 2013 23:38:22 -
@@ -122,6 +122,7 @@ struct ifqueue ip6intrq;
struct ip6stat ip6stat;
void ip6_init2(void *);
+int ip6_check_rh0hdr(struct mbuf *, int *);
int ip6_hopopts_input(u_int32_t *, u_int32_t *, struct mbuf **, int *);
struct mbuf *ip6_pullexthdr(struct mbuf *, size_t, int);
@@ -331,6 +332,20 @@ ip6_input(struct mbuf *m)
srcrt = !IN6_ARE_ADDR_EQUAL(odst, ip6-ip6_dst);
#endif
+ /*
+* Be more secure than RFC5095 and scan for type 0 routing headers.
+* If pf has already scanned the header chain, do not do it twice.
+*/
+ if (!(m-m_pkthdr.pf.flags PF_TAG_PROCESSED)
+ ip6_check_rh0hdr(m, off)) {
+ ip6stat.ip6s_badoptions++;
+ in6_ifstat_inc(m-m_pkthdr.rcvif, ifs6_in_discard);
+ in6_ifstat_inc(m-m_pkthdr.rcvif, ifs6_in_hdrerr);
+ icmp6_error(m, ICMP6_PARAM_PROB, ICMP6_PARAMPROB_HEADER, off);
+ /* m is already freed */
+ return;
+ }
+
if (IN6_IS_ADDR_LOOPBACK(ip6-ip6_src) ||
IN6_IS_ADDR_LOOPBACK(ip6-ip6_dst)) {
ours = 1;
@@ -698,6 +713,74 @@ ip6_input(struct mbuf *m)
return;
bad:
m_freem(m);
+}
+
+/* scan packet for RH0 routing header. Mostly stolen from pf.c:pf_test() */
+int
+ip6_check_rh0hdr(struct mbuf *m, int *offp)
+{
+ struct ip6_hdr *ip6 = mtod(m, struct ip6_hdr *);
+ struct ip6_rthdr rthdr;
+ struct ip6_ext opt6;
+ u_int8_t proto = ip6-ip6_nxt;
+ int done = 0, lim, off, rh_cnt = 0;
+
+ off = ((caddr_t)ip6 - m-m_data) + sizeof(struct ip6_hdr);
+ lim = min(m-m_pkthdr.len, ntohs(ip6-ip6_plen) + sizeof(*ip6));
+ do {
+ switch (proto) {
+ case IPPROTO_ROUTING:
+ *offp = off;
+ if (rh_cnt++) {
+ /* more then one rh header present */
+ return (1);
+ }
+
+ if (off + sizeof(rthdr) lim) {
+ /* packet to short to make sense */
+ return (1);
+ }
+
+ m_copydata(m, off, sizeof(rthdr), (caddr_t)rthdr);
+
+ if (rthdr.ip6r_type == IPV6_RTHDR_TYPE_0) {
+ *offp += offsetof(struct ip6_rthdr,
ip6r_type);
+ return (1);
+ }
+
+ off += (rthdr.ip6r_len + 1) * 8;
+ proto = rthdr.ip6r_nxt;
+ break;
+ case IPPROTO_AH:
+ case IPPROTO_HOPOPTS:
+
Re: IPv6 routing header type 0
* Alexander Bluhm alexander.bl...@gmx.net [2013-11-14 01:29]: Theo and others don't like that change as it decreases security. There are hosts out there that still process RH0 and there are OpenBSD routers with pf disabled. This diff brings back the header chain scanning. As an improvement it only scans if pf has not done that before. Note that ip6_check_rh0hdr() can be easily tricked by hiding the routing header type 0 behind a fragment header. Only pf can protect you correctly as it reassembles on the forwarding path. So I am not sure wether it is worth adding it again. to be quite honest I don't see the point. the protection in teh stack is either very incomplete and easy enough to trick - you point it out yourself, fragment - or very expensive. especially given that pf is enabled by default: make sure the stack doesn't process RH0 itself, and otherwise leave it to pf. aka the status quo. -- Henning Brauer, h...@bsws.de, henn...@openbsd.org BS Web Services GmbH, http://bsws.de, Full-Service ISP Secure Hosting, Mail and DNS Services. Dedicated Servers, Root to Fully Managed Henning Brauer Consulting, http://henningbrauer.com/
Re: IPv6 routing header type 0
I guess it would help people who decide to disable pf for slight performance benefit ? Quite obviously people are doing this on a variety of machines, such as bgp routers.
Re: IPv6 routing header type 0
* Alexander Bluhm alexander.bl...@gmx.net [2013-11-14 01:29]: Theo and others don't like that change as it decreases security. There are hosts out there that still process RH0 and there are OpenBSD routers with pf disabled. This diff brings back the header chain scanning. As an improvement it only scans if pf has not done that before. Note that ip6_check_rh0hdr() can be easily tricked by hiding the routing header type 0 behind a fragment header. Only pf can protect you correctly as it reassembles on the forwarding path. So I am not sure wether it is worth adding it again. to be quite honest I don't see the point. the protection in teh stack is either very incomplete and easy enough to trick - you point it out yourself, fragment - or very expensive. especially given that pf is enabled by default: make sure the stack doesn't process RH0 itself, and otherwise leave it to pf. aka the status quo. You're wrong about the status quo. It *was* being filtered, until a month ago.
Re: IPv6 routing header type 0
* Theo de Raadt dera...@cvs.openbsd.org [2013-11-14 16:35]: * Alexander Bluhm alexander.bl...@gmx.net [2013-11-14 01:29]: Theo and others don't like that change as it decreases security. There are hosts out there that still process RH0 and there are OpenBSD routers with pf disabled. This diff brings back the header chain scanning. As an improvement it only scans if pf has not done that before. Note that ip6_check_rh0hdr() can be easily tricked by hiding the routing header type 0 behind a fragment header. Only pf can protect you correctly as it reassembles on the forwarding path. So I am not sure wether it is worth adding it again. to be quite honest I don't see the point. the protection in teh stack is either very incomplete and easy enough to trick - you point it out yourself, fragment - or very expensive. especially given that pf is enabled by default: make sure the stack doesn't process RH0 itself, and otherwise leave it to pf. aka the status quo. You're wrong about the status quo. it is the status quo *right now* It *was* being filtered, until a month ago. no news here - we had discussed that change at the time. pretty sure you were in the loop even. I stand by my point either way. the stack check for forwarded packets is either very incomplete or expensive. the aproach stack protects the local machine (in this case: don't obey RH0), pf handles forwarded packets matches what we do generally. -- Henning Brauer, h...@bsws.de, henn...@openbsd.org BS Web Services GmbH, http://bsws.de, Full-Service ISP Secure Hosting, Mail and DNS Services. Dedicated Servers, Root to Fully Managed Henning Brauer Consulting, http://henningbrauer.com/
Re: IPv6 routing header type 0
it is the status quo *right now* Look, you can't call something the status quo when a commit was made 1 month ago, to a REAL status quo that existed for 10 years when itojun made the change... and immediately after this recent commit we started arguying about the change. Go find out what status quo means. It *was* being filtered, until a month ago. no news here - we had discussed that change at the time. pretty sure you were in the loop even. No, I was not in the loop before it was commited, and many others were not either. Apparently only you and mikeb were, and you did NOT push for more people to know about the change before it was commited. I stand by my point either way. the stack check for forwarded packets is either very incomplete or expensive. the aproach stack protects the local machine (in this case: don't obey RH0), pf handles forwarded packets matches what we do generally. And if pf is disabled, you have an astoundly disruptive attack on your hands. Then the itojun check helps, not for all cases, but for the most common form of the attack which is the most distruptive. On the other hand If pf is enabled, then the existing code is entirely free since it doesn't get run (I am talking about the new version of the bluhm change to bring the behaviour back ONLY if pf is disabled). The non-pf RH0 filtering case is worthwhile.
Re: IPv6 routing header type 0
* Theo de Raadt dera...@cvs.openbsd.org [2013-11-14 18:47]: it is the status quo *right now* Look, you can't call something the status quo when a commit was made 1 month ago, to a REAL status quo that existed for 10 years when itojun made the change... and immediately after this recent commit we started arguying about the change. Go find out what status quo means. let's not get into this, leads us nowhere. It *was* being filtered, until a month ago. no news here - we had discussed that change at the time. pretty sure you were in the loop even. No, I was not in the loop before it was commited, and many others were not either. Apparently only you and mikeb were, and you did NOT push for more people to know about the change before it was commited. mikeb? bluhm worked on comitted that otoh. i'm still pretty damn sure you were Cc'd; won't dig for old mail just to prove it; don't see the point, doesn't change anything now anyway. The non-pf RH0 filtering case is worthwhile. and here we disagree. -- Henning Brauer, h...@bsws.de, henn...@openbsd.org BS Web Services GmbH, http://bsws.de, Full-Service ISP Secure Hosting, Mail and DNS Services. Dedicated Servers, Root to Fully Managed Henning Brauer Consulting, http://henningbrauer.com/
Re: IPv6 routing header type 0
* Theo de Raadt dera...@cvs.openbsd.org [2013-11-14 18:47]: it is the status quo *right now* Look, you can't call something the status quo when a commit was made 1 month ago, to a REAL status quo that existed for 10 years when itojun made the change... and immediately after this recent commit we started arguying about the change. Go find out what status quo means. let's not get into this, leads us nowhere. I believe Alexander should either take us back to the status quo, or move us to the new world where we have a solution for the non-pf case as well. You are arguing for a case with NO PROTECTION against RH0. Ridiculous. mikeb? bluhm worked on comitted that otoh. i'm still pretty damn sure you were Cc'd; won't dig for old mail just to prove it; don't see the point, doesn't change anything now anyway. It was not shown to enough people. PERIOD. With your name as an OK on the commit, you share the responsibility of having made sure that everyone saw it, before that commit. The fact that a discussion is happening now AFTERWARDS, indicates that you failed to identify this as contentious, and involve everyone. You know better. The non-pf RH0 filtering case is worthwhile. and here we disagree. Do you run any routers with pf disabled? If so, please identify one, for a demonstration.
Re: IPv6 routing header type 0
On 14 November 2013 18:52, Henning Brauer lists-openbsdt...@bsws.de wrote: * Theo de Raadt dera...@cvs.openbsd.org [2013-11-14 18:47]: it is the status quo *right now* Look, you can't call something the status quo when a commit was made 1 month ago, to a REAL status quo that existed for 10 years when itojun made the change... and immediately after this recent commit we started arguying about the change. Go find out what status quo means. let's not get into this, leads us nowhere. It *was* being filtered, until a month ago. no news here - we had discussed that change at the time. pretty sure you were in the loop even. No, I was not in the loop before it was commited, and many others were not either. Apparently only you and mikeb were, and you did NOT push for more people to know about the change before it was commited. mikeb? bluhm worked on comitted that otoh. i'm still pretty damn sure you were Cc'd; won't dig for old mail just to prove it; don't see the point, doesn't change anything now anyway. we have discussed that with bluhm in berlin and initially i had the same opinion: leave the check in the stack, but he has convinced me that it's rather pf's job to do it. i'm not against bringing it back and his diff looks fine to me, esp. since it avoids double check that was there before. The non-pf RH0 filtering case is worthwhile. and here we disagree. -- Henning Brauer, h...@bsws.de, henn...@openbsd.org BS Web Services GmbH, http://bsws.de, Full-Service ISP Secure Hosting, Mail and DNS Services. Dedicated Servers, Root to Fully Managed Henning Brauer Consulting, http://henningbrauer.com/
Re: IPv6 routing header type 0
Mike, we have discussed that with bluhm in berlin and initially i had the same opinion: leave the check in the stack, but he has convinced me that it's rather pf's job to do it. I agree. If pf is enabled, it can do the job and there is no need for a second scan. i'm not against bringing it back and his diff looks fine to me, esp. since it avoids double check that was there before. His new diff resumes the scan/removal when pf is disabled. It at least tries to do *something* against at least some variations of a blistering attack. that is why I support it. Basically, if he commits his new version, he has retained the filtering for both cases, but sped up the pf-enabled case. The non-pf RH0 filtering case is worthwhile. and here we disagree. Henning, you are way off the map.
Re: IPv6 routing header type 0
On Thu, Nov 14, 2013 at 10:04 PM, Mike Belopuhov m...@belopuhov.com wrote: On 14 November 2013 18:52, Henning Brauer lists-openbsdt...@bsws.de wrote: * Theo de Raadt dera...@cvs.openbsd.org [2013-11-14 18:47]: it is the status quo *right now* Look, you can't call something the status quo when a commit was made 1 month ago, to a REAL status quo that existed for 10 years when itojun made the change... and immediately after this recent commit we started arguying about the change. Go find out what status quo means. let's not get into this, leads us nowhere. It *was* being filtered, until a month ago. no news here - we had discussed that change at the time. pretty sure you were in the loop even. No, I was not in the loop before it was commited, and many others were not either. Apparently only you and mikeb were, and you did NOT push for more people to know about the change before it was commited. mikeb? bluhm worked on comitted that otoh. i'm still pretty damn sure you were Cc'd; won't dig for old mail just to prove it; don't see the point, doesn't change anything now anyway. we have discussed that with bluhm in berlin and initially i had the same opinion: leave the check in the stack, but he has convinced me that it's rather pf's job to do it. i'm not against bringing it back and his diff looks fine to me, esp. since it avoids double check that was there before. Personally, I wasn't aware that variations of RH0 attacks are possible, and that PF is the only way to mitigate the attacks completely, until alexender blumh mentioned. To me, that is an important piece of information for end-users running v6 networks. Do you guys think that it might be worth mentioning this in pfctl man page ? Index: src/sbin/pfctl/pfctl.8 === RCS file: /cvs/src/sbin/pfctl/pfctl.8,v retrieving revision 1.163 diff -u -p -r1.163 pfctl.8 --- src/sbin/pfctl/pfctl.8 21 Jul 2013 17:22:49 - 1.163 +++ src/sbin/pfctl/pfctl.8 14 Nov 2013 20:03:46 - @@ -175,6 +175,9 @@ Overrides the definition of in the ruleset. .It Fl d Disable the packet filter. + +This disables advanced protection against Routing Type 0 attacks as the network +stack only has a basic protection solution to Routing Type 0 vulnerability. .It Fl e Enable the packet filter. .It Fl F Ar modifier The non-pf RH0 filtering case is worthwhile. and here we disagree. -- Henning Brauer, h...@bsws.de, henn...@openbsd.org BS Web Services GmbH, http://bsws.de, Full-Service ISP Secure Hosting, Mail and DNS Services. Dedicated Servers, Root to Fully Managed Henning Brauer Consulting, http://henningbrauer.com/ -- This message is strictly personal and the opinions expressed do not represent those of my employers, either past or present.
Re: IPv6 routing header type 0
we have discussed that with bluhm in berlin and initially i had the same opinion: leave the check in the stack, but he has convinced me that it's rather pf's job to do it. i'm not against bringing it back and his diff looks fine to me, esp. since it avoids double check that was there before. Personally, I wasn't aware that variations of RH0 attacks are possible, and that PF is the only way to mitigate the attacks completely, until alexender blumh mentioned. pf was NOT the only way to mitigate the attacks. Until a month ago, there was automatic filtering, which worked against a subset of the attack scenarios, including the most common and most damaging cases. At least, that is my recollection from the time when the ORIGINAL change went in during the itojun era. The idea was that if the filtering is known to be super cheap, do it. Our source tree has many similar filters, for example, libc/rpc has code to handle FTP bounce attacks. When handling something damaging was possible at a specific layer, historically we have done such filtering, even if something else might be doing it. To me, that is an important piece of information for end-users running v6 networks. By that token, everything is an important piece of information for everyone running everything? Do you guys think that it might be worth mentioning this in pfctl man page ? No way. Mentioning low-level effects in high-level places is an unsuitable documentation practice. Documentation is supposed to be terse. If you mention that specific issue, you will soon end up wanting to mention 40 other similar issues, and soon you are not documenting -d anymore, and the pfctl manual page is an unreadable book. Lacking a terse starting point, soon we have users who don't know where to start. The result is far greater knowledge gaps that what you just pointed out.
Re: IPv6 routing header type 0
* Theo de Raadt dera...@cvs.openbsd.org [2013-11-14 19:00]: * Theo de Raadt dera...@cvs.openbsd.org [2013-11-14 18:47]: it is the status quo *right now* Look, you can't call something the status quo when a commit was made 1 month ago, to a REAL status quo that existed for 10 years when itojun made the change... and immediately after this recent commit we started arguying about the change. Go find out what status quo means. let's not get into this, leads us nowhere. I believe Alexander should either take us back to the status quo, or move us to the new world where we have a solution for the non-pf case as well. You are arguing for a case with NO PROTECTION against RH0. Ridiculous. no protection for hosts BEHIND an OpenBSD box forwarding v6 traffic and not running pf, correct. the box itself is protected, by, well, itself. just like such an openbsd box doesn't protect the boxes behind it from pretty much anything else. that's what pf is for. The non-pf RH0 filtering case is worthwhile. and here we disagree. Do you run any routers with pf disabled? If so, please identify one, for a demonstration. yes, I do. utterly pointless, since a) no v6 there at all and b) several pf pairs behind it and nothing else - as in, everything else is behind those pf boxes. -- Henning Brauer, h...@bsws.de, henn...@openbsd.org BS Web Services GmbH, http://bsws.de, Full-Service ISP Secure Hosting, Mail and DNS Services. Dedicated Servers, Root to Fully Managed Henning Brauer Consulting, http://henningbrauer.com/
Re: IPv6 routing header type 0
On Thu, Nov 14, 2013 at 11:00:37AM -0700, Theo de Raadt wrote: It was not shown to enough people. PERIOD. My diff was on tech@ for one day during a hackathon before I commited it. Not enough people discussed it back then. Fine. Let's discuss it now. The reasons why I removed the check in the stack are: - Scanning headers in the forwarding path is against the spirit of IPv6. - pf deals much better with fragments and headers now. - When the check was added, there was no RFC. Now I am following RFC5095. - It is pf's job to add more security. - The scanning was done twice with pf enabled. Now I am tempted to put it back because: - Theo says there a lot of OpenBSD boxes without pf attached to the internet. - Fernando Gont says there are plenty of legacy implementations supporting RH0. - Fernando Gont says it is not the most secure approach to remove the check. - I have removed the double scan when pf is enabled. bluhm
Re: IPv6 routing header type 0
My diff was on tech@ for one day during a hackathon before I commited it. Not enough people discussed it back then. Fine. Let's discuss it now. The reasons why I removed the check in the stack are: - Scanning headers in the forwarding path is against the spirit of IPv6. One day someone should find the people who pushed RH0 into IPv6 and punish them. - pf deals much better with fragments and headers now. True. - When the check was added, there was no RFC. Now I am following RFC5095. I don't think RFC5095 goes far enough. Itojun also wanted a more penalizing solution. The idea behind the RH0 scanning is that it would dampen the pressure of RH0 attacks. If there are fewer participants, it is less feasible to use it to attack small pipes here or there. - It is pf's job to add more security. It is. However, you will note that in IPv4 land we have sysctl net.inet.ip.sourceroute. It defaults to 0 (off). RH is like IPv4 source routing, except on steriods. Would any of us at this time recommend net.inet.ip.sourceroute=1, or to go further, remove the code disabling code from the kernel and assume that pf is doing the filtering? I doubt it. And yet, the IPv4 sourcerouting RFCs are just as toothless... - The scanning was done twice with pf enabled. This latter point is very valid. I am very happy with your new approach that does the extra scanning only if pf is disabled. Now I am tempted to put it back because: - Theo says there a lot of OpenBSD boxes without pf attached to the internet. There are scenarios where pf is run disabled, or pf is not watching the up-stream/down-stream traffic. When pf is run, it only protects the local machine. - Fernando Gont says there are plenty of legacy implementations supporting RH0. - Fernando Gont says it is not the most secure approach to remove the check. If Fernando sees them, then the case is solid. All you need is a few hosts honouring RH0 on the opposite of a thin pipe, and you can cause blistering damage. The idea behind the RH0 scanning was to dampen the effects of a dead option. I believe this idea will be reasonable until RH0 is extinct. I only believe in this approach when the header is already cache-hot, and there is little performance. Untimately if many feel pf is always on, then there is no argument for resisting code for the pf is disabled case... - I have removed the double scan when pf is enabled. Beautiful.
Re: IPv6 routing header type 0
On Fri, Oct 18, 2013 at 08:45:02PM +0200, Alexander Bluhm wrote:
Our IPv6 stack scans all extension headers for routing header type
0 and drops the packet if it finds one. RFC 5095 demands to handle
a routing header type 0 like an unrecognised routing type. This
is enough to protect the own machine.
To protect a network as a firewall, we have pf which does the same
full scan in pf_walk_header6(). As pf is enabled by default, nothing
changes for most users. If you turn off pf on your router, you
should not expect extra protection.
I would like to get rid of the double scanning and the old disabled
code in the IPv6 stack.
Theo and others don't like that change as it decreases security.
There are hosts out there that still process RH0 and there are
OpenBSD routers with pf disabled.
This diff brings back the header chain scanning. As an improvement
it only scans if pf has not done that before.
Note that ip6_check_rh0hdr() can be easily tricked by hiding the
routing header type 0 behind a fragment header. Only pf can protect
you correctly as it reassembles on the forwarding path. So I am
not sure wether it is worth adding it again.
Comments?
bluhm
Index: net/pf.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf.c,v
retrieving revision 1.857
diff -u -p -u -p -r1.857 pf.c
--- net/pf.c30 Oct 2013 11:35:10 - 1.857
+++ net/pf.c13 Nov 2013 23:14:32 -
@@ -6490,6 +6490,7 @@ pf_test(sa_family_t af, int fwdir, struc
}
}
pd.eh = eh;
+ pd.m-m_pkthdr.pf.flags |= PF_TAG_PROCESSED;
switch (pd.virtual_proto) {
Index: netinet6/ip6_input.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/ip6_input.c,v
retrieving revision 1.120
diff -u -p -u -p -r1.120 ip6_input.c
--- netinet6/ip6_input.c11 Nov 2013 09:15:35 - 1.120
+++ netinet6/ip6_input.c13 Nov 2013 23:38:22 -
@@ -122,6 +122,7 @@ struct ifqueue ip6intrq;
struct ip6stat ip6stat;
void ip6_init2(void *);
+int ip6_check_rh0hdr(struct mbuf *, int *);
int ip6_hopopts_input(u_int32_t *, u_int32_t *, struct mbuf **, int *);
struct mbuf *ip6_pullexthdr(struct mbuf *, size_t, int);
@@ -331,6 +332,20 @@ ip6_input(struct mbuf *m)
srcrt = !IN6_ARE_ADDR_EQUAL(odst, ip6-ip6_dst);
#endif
+ /*
+* Be more secure than RFC5095 and scan for type 0 routing headers.
+* If pf has already scanned the header chain, do not do it twice.
+*/
+ if (!(m-m_pkthdr.pf.flags PF_TAG_PROCESSED)
+ ip6_check_rh0hdr(m, off)) {
+ ip6stat.ip6s_badoptions++;
+ in6_ifstat_inc(m-m_pkthdr.rcvif, ifs6_in_discard);
+ in6_ifstat_inc(m-m_pkthdr.rcvif, ifs6_in_hdrerr);
+ icmp6_error(m, ICMP6_PARAM_PROB, ICMP6_PARAMPROB_HEADER, off);
+ /* m is already freed */
+ return;
+ }
+
if (IN6_IS_ADDR_LOOPBACK(ip6-ip6_src) ||
IN6_IS_ADDR_LOOPBACK(ip6-ip6_dst)) {
ours = 1;
@@ -698,6 +713,74 @@ ip6_input(struct mbuf *m)
return;
bad:
m_freem(m);
+}
+
+/* scan packet for RH0 routing header. Mostly stolen from pf.c:pf_test() */
+int
+ip6_check_rh0hdr(struct mbuf *m, int *offp)
+{
+ struct ip6_hdr *ip6 = mtod(m, struct ip6_hdr *);
+ struct ip6_rthdr rthdr;
+ struct ip6_ext opt6;
+ u_int8_t proto = ip6-ip6_nxt;
+ int done = 0, lim, off, rh_cnt = 0;
+
+ off = ((caddr_t)ip6 - m-m_data) + sizeof(struct ip6_hdr);
+ lim = min(m-m_pkthdr.len, ntohs(ip6-ip6_plen) + sizeof(*ip6));
+ do {
+ switch (proto) {
+ case IPPROTO_ROUTING:
+ *offp = off;
+ if (rh_cnt++) {
+ /* more then one rh header present */
+ return (1);
+ }
+
+ if (off + sizeof(rthdr) lim) {
+ /* packet to short to make sense */
+ return (1);
+ }
+
+ m_copydata(m, off, sizeof(rthdr), (caddr_t)rthdr);
+
+ if (rthdr.ip6r_type == IPV6_RTHDR_TYPE_0) {
+ *offp += offsetof(struct ip6_rthdr, ip6r_type);
+ return (1);
+ }
+
+ off += (rthdr.ip6r_len + 1) * 8;
+ proto = rthdr.ip6r_nxt;
+ break;
+ case IPPROTO_AH:
+ case IPPROTO_HOPOPTS:
+ case IPPROTO_DSTOPTS:
+ /* get next header and header length */
+ if (off + sizeof(opt6) lim) {
+ /*
+* Packet to short to make sense, we could
+
IPv6 routing header type 0
Hi,
Our IPv6 stack scans all extension headers for routing header type
0 and drops the packet if it finds one. RFC 5095 demands to handle
a routing header type 0 like an unrecognised routing type. This
is enough to protect the own machine.
To protect a network as a firewall, we have pf which does the same
full scan in pf_walk_header6(). As pf is enabled by default, nothing
changes for most users. If you turn off pf on your router, you
should not expect extra protection.
I would like to get rid of the double scanning and the old disabled
code in the IPv6 stack.
ok?
bluhm
Index: netinet6/ip6_input.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/ip6_input.c,v
retrieving revision 1.115
diff -u -p -u -p -r1.115 ip6_input.c
--- netinet6/ip6_input.c17 Oct 2013 16:27:46 - 1.115
+++ netinet6/ip6_input.c18 Oct 2013 17:21:52 -
@@ -122,7 +122,6 @@ struct ifqueue ip6intrq;
struct ip6stat ip6stat;
void ip6_init2(void *);
-int ip6_check_rh0hdr(struct mbuf *, int *);
int ip6_hopopts_input(u_int32_t *, u_int32_t *, struct mbuf **, int *);
struct mbuf *ip6_pullexthdr(struct mbuf *, size_t, int);
@@ -318,15 +317,6 @@ ip6_input(struct mbuf *m)
}
#endif
- if (ip6_check_rh0hdr(m, off)) {
- ip6stat.ip6s_badoptions++;
- in6_ifstat_inc(m-m_pkthdr.rcvif, ifs6_in_discard);
- in6_ifstat_inc(m-m_pkthdr.rcvif, ifs6_in_hdrerr);
- icmp6_error(m, ICMP6_PARAM_PROB, ICMP6_PARAMPROB_HEADER, off);
- /* m is already freed */
- return;
- }
-
#if NPF 0
/*
* Packet filter
@@ -705,74 +695,6 @@ ip6_input(struct mbuf *m)
return;
bad:
m_freem(m);
-}
-
-/* scan packet for RH0 routing header. Mostly stolen from pf.c:pf_test() */
-int
-ip6_check_rh0hdr(struct mbuf *m, int *offp)
-{
- struct ip6_hdr *ip6 = mtod(m, struct ip6_hdr *);
- struct ip6_rthdr rthdr;
- struct ip6_ext opt6;
- u_int8_t proto = ip6-ip6_nxt;
- int done = 0, lim, off, rh_cnt = 0;
-
- off = ((caddr_t)ip6 - m-m_data) + sizeof(struct ip6_hdr);
- lim = min(m-m_pkthdr.len, ntohs(ip6-ip6_plen) + sizeof(*ip6));
- do {
- switch (proto) {
- case IPPROTO_ROUTING:
- *offp = off;
- if (rh_cnt++) {
- /* more then one rh header present */
- return (1);
- }
-
- if (off + sizeof(rthdr) lim) {
- /* packet to short to make sense */
- return (1);
- }
-
- m_copydata(m, off, sizeof(rthdr), (caddr_t)rthdr);
-
- if (rthdr.ip6r_type == IPV6_RTHDR_TYPE_0) {
- *offp += offsetof(struct ip6_rthdr, ip6r_type);
- return (1);
- }
-
- off += (rthdr.ip6r_len + 1) * 8;
- proto = rthdr.ip6r_nxt;
- break;
- case IPPROTO_AH:
- case IPPROTO_HOPOPTS:
- case IPPROTO_DSTOPTS:
- /* get next header and header length */
- if (off + sizeof(opt6) lim) {
- /*
-* Packet to short to make sense, we could
-* reject the packet but as a router we
-* should not do that so forward it.
-*/
- return (0);
- }
-
- m_copydata(m, off, sizeof(opt6), (caddr_t)opt6);
-
- if (proto == IPPROTO_AH)
- off += (opt6.ip6e_len + 2) * 4;
- else
- off += (opt6.ip6e_len + 1) * 8;
- proto = opt6.ip6e_nxt;
- break;
- case IPPROTO_FRAGMENT:
- default:
- /* end of header stack */
- done = 1;
- break;
- }
- } while (!done);
-
- return (0);
}
/*
Index: netinet6/route6.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/route6.c,v
retrieving revision 1.17
diff -u -p -u -p -r1.17 route6.c
--- netinet6/route6.c 11 Jun 2008 19:00:50 - 1.17
+++ netinet6/route6.c 18 Oct 2013 17:59:45 -
@@ -44,10 +44,6 @@
#include netinet/icmp6.h
-#if 0
-static int ip6_rthdr0(struct mbuf *, struct ip6_hdr *, struct ip6_rthdr0 *);
-#endif
-
/*
* proto is unused
*/
@@ -68,43 +64,12 @@ route6_input(struct mbuf **mp, int *offp
}
switch
