Re: openssl's *strlcy
On Fri, Apr 18, 2014 at 09:41:47PM -0400, Jacob L. Leifman wrote: I'm guessing that openssl was incorporated into OpenBSD base without prior sufficient audit by the OBSD devs because it was presumed to have better auditing / quality control upstream given its security critical nature and function. (A number of devs have commented in the past about the [lack of] code style, but I get the impression no-one expected the degree of *sloppiness* now being uncovered.) So here's a question, are there any other chunks of code that have been imported en-mass from an upstream source that could/should use an audit? especially, something that some of us non-developers might be able to assist with? No, you're mistaken. We've known for a while it was on the very dirty side, but there are obvious human reasons because of which people were reluctant to dive in. Note that it's on a par with a lot of opensource code out there, unfortunately. At some point, you have to make choices, as the amount of time and manpower we have is limited.
Re: openssl's *strlcy
I'm guessing that openssl was incorporated into OpenBSD base without prior sufficient audit by the OBSD devs because it was presumed to have better auditing / quality control upstream given its security critical nature and function. Everyone has to take shortcuts. After what you've seen here, can you name anyone who didn't take the same shortcut? Meanwhile, is any other operating system out there auditing the OpenSSH they get from here? (A number of devs have commented in the past about the [lack of] code style, but I get the impression no-one expected the degree of *sloppiness* now being uncovered.) It is very surprising. So here's a question, are there any other chunks of code that have been imported en-mass from an upstream source that could/should use an audit? especially, something that some of us non-developers might be able to assist with? ^^ Sorry, no, because it is a development process.
Re: openssl's *strlcy
Seems it is ok to use strlcat/strlcpy that way in some cases:
$ cat src/usr.sbin/smtpd/*.c | egrep -c ' strlc(at|py)\('
249
Hi Claus @ Sendmail [come on, your employeer matters when you point
at code like this, you know better]
smtpd is a new project. The 2-3 developers working on it should do
better, indeed. I hope they fix them all in 48 hours.
All of those calls should do something with the range check result, or
if truncation is determined to be the desired safe condition, be
annotated with (void) to indicate an audit has occured. That is best
practice.
On the other hand, the 2-decade OpenSSL group has a massive commercial
userbase, and this problem was allowed to persist. Commit history shows
it has been getting worse, not better.
Look at the OpenSSL list I posted again. Some of those are using
sizeof(src).
Shall you and I make a bet about when OpenSSL has all these calls
fixed to check for overflow and truncation?
IPv6 DoS sysctl man page additions
Hi All, I'm taking a short break from playing with pf statistics. There were 4 sysctls added from KAME, but the man pages weren't updated accordingly. (Adapted from the NetBSD man page changes) Feedback welcomed. Index: lib/libc/gen/sysctl.3 === RCS file: /cvs/src/lib/libc/gen/sysctl.3,v retrieving revision 1.228 diff -u -p -u -p -r1.228 sysctl.3 --- lib/libc/gen/sysctl.3 21 Jan 2014 03:15:45 - 1.228 +++ lib/libc/gen/sysctl.3 19 Apr 2014 10:58:30 - @@ -1676,11 +1676,15 @@ The currently defined protocols and name .It ip6 Ta hdrnestlimit Ta integer Ta yes .It ip6 Ta hlim Ta integer Ta yes .It ip6 Ta log_interval Ta integer Ta yes +.It ip6 Ta maxdynroutes Ta integer Ta yes .It ip6 Ta maxfragpackets Ta integer Ta yes .It ip6 Ta maxfrags Ta integer Ta yes +.It ip6 Ta maxifprefixes Ta integer Ta yes +.It ip6 Ta maxifdefrouters Ta integer Ta yes .It ip6 Ta mforwarding Ta integer Ta yes .It ip6 Ta multicast_mtudisc Ta integer Ta yes .It ip6 Ta multipath Ta integer Ta yes +.It ip6 Ta neighborgcthresh Ta integer Ta yes .It ip6 Ta redirect Ta integer Ta yes .It ip6 Ta rr_prune Ta integer Ta yes .It ip6 Ta use_deprecated Ta integer Ta yes @@ -1834,6 +1838,11 @@ IPv6 packet forwarding engine. The value indicates the number of seconds of interval which must elapse between log output. .Pp +.It Li ip6.maxdynroutes +Maximum number of routes created by redirect. +Set it to negative to disable. +The default value is 4096. +.Pp .It Li ip6.maxfragpackets The maximum number of fragmented packets the node will accept. 0 means that the node will not accept any fragmented packets. @@ -1846,6 +1855,17 @@ The maximum number of fragments the node \-1 means that the node will accept as many fragments as it receives. The flag is provided basically for avoiding possible DoS attacks. .Pp +.It Li ip6.maxifprefixes +Maximum number of prefixes created by route advertisements per interface. +Set it to negative to disable. +The default value is 16. +.Pp +.It Li ip6.maxifdefrouters 16 +Maximum number of default routers created by route advertisements per +interface. +Set it to negative to disable. +The default value is 16. +.Pp .It Li ip6.mforwarding If set to 1, then multicast forwarding is enabled for the host. The default is 0. @@ -1861,6 +1881,11 @@ If set to 0, the ICMPv6 Too Big message This variable enables multipath routing for IPv6 addresses. If set to 0, only the first route selected will be used for a given destination regardless of how many routes exist in the routing table. +.Pp +.It Li ip6.neighborgcthresh +Maximum number of entries in neighbor cache. +Set to negative to disable. +The default value is 2048. .Pp .It Li ip6.redirect Returns 1 when ICMPv6 redirects may be sent by the node. Index: sbin/sysctl/sysctl.8 === RCS file: /cvs/src/sbin/sysctl/sysctl.8,v retrieving revision 1.173 diff -u -p -u -p -r1.173 sysctl.8 --- sbin/sysctl/sysctl.828 Oct 2013 21:02:35 - 1.173 +++ sbin/sysctl/sysctl.819 Apr 2014 10:58:30 - @@ -301,10 +301,14 @@ and a few require a kernel compiled with .It net.inet6.ip6.use_deprecated Ta integer Ta yes .It net.inet6.ip6.rr_prune Ta integer Ta yes .It net.inet6.ip6.v6only Ta integer Ta no +.It net.inet6.ip6.maxdynroutes Ta integer Ta yes .It net.inet6.ip6.maxfrags Ta integer Ta yes +.It net.inet6.ip6.maxifprefixes Ta integer Ta yes +.It net.inet6.ip6.maxifdefrouters Ta integer Ta yes .It net.inet6.ip6.mforwarding Ta integer Ta yes .It net.inet6.ip6.multipath Ta integer Ta yes .It net.inet6.ip6.multicast_mtudisc Ta integer Ta yes +.It net.inet6.ip6.neighborgcthresh Ta integer Ta yes .It net.inet6.icmp6.rediraccept Ta integer Ta yes .It net.inet6.icmp6.redirtimeout Ta integer Ta yes .It net.inet6.icmp6.nd6_prune Ta integer Ta yes
Re: IPv6 DoS sysctl man page additions
On Sat, Apr 19, 2014 at 04:04:30AM -0700, Loganaden Velvindron wrote: Hi All, I'm taking a short break from playing with pf statistics. There were 4 sysctls added from KAME, but the man pages weren't updated accordingly. (Adapted from the NetBSD man page changes) Feedback welcomed. Removed trailing spaces and use set to instead of set it to based on feedback from sthen@ Index: lib/libc/gen/sysctl.3 === RCS file: /cvs/src/lib/libc/gen/sysctl.3,v retrieving revision 1.228 diff -u -p -u -p -r1.228 sysctl.3 --- lib/libc/gen/sysctl.3 21 Jan 2014 03:15:45 - 1.228 +++ lib/libc/gen/sysctl.3 19 Apr 2014 11:17:13 - @@ -1676,11 +1676,15 @@ The currently defined protocols and name .It ip6 Ta hdrnestlimit Ta integer Ta yes .It ip6 Ta hlim Ta integer Ta yes .It ip6 Ta log_interval Ta integer Ta yes +.It ip6 Ta maxdynroutes Ta integer Ta yes .It ip6 Ta maxfragpackets Ta integer Ta yes .It ip6 Ta maxfrags Ta integer Ta yes +.It ip6 Ta maxifprefixes Ta integer Ta yes +.It ip6 Ta maxifdefrouters Ta integer Ta yes .It ip6 Ta mforwarding Ta integer Ta yes .It ip6 Ta multicast_mtudisc Ta integer Ta yes .It ip6 Ta multipath Ta integer Ta yes +.It ip6 Ta neighborgcthresh Ta integer Ta yes .It ip6 Ta redirect Ta integer Ta yes .It ip6 Ta rr_prune Ta integer Ta yes .It ip6 Ta use_deprecated Ta integer Ta yes @@ -1834,6 +1838,11 @@ IPv6 packet forwarding engine. The value indicates the number of seconds of interval which must elapse between log output. .Pp +.It Li ip6.maxdynroutes +Maximum number of routes created by redirect. +Set to negative to disable. +The default value is 4096. +.Pp .It Li ip6.maxfragpackets The maximum number of fragmented packets the node will accept. 0 means that the node will not accept any fragmented packets. @@ -1846,6 +1855,17 @@ The maximum number of fragments the node \-1 means that the node will accept as many fragments as it receives. The flag is provided basically for avoiding possible DoS attacks. .Pp +.It Li ip6.maxifprefixes +Maximum number of prefixes created by route advertisements per interface. +Set to negative to disable. +The default value is 16. +.Pp +.It Li ip6.maxifdefrouters 16 +Maximum number of default routers created by route advertisements per +interface. +Set to negative to disable. +The default value is 16. +.Pp .It Li ip6.mforwarding If set to 1, then multicast forwarding is enabled for the host. The default is 0. @@ -1861,6 +1881,11 @@ If set to 0, the ICMPv6 Too Big message This variable enables multipath routing for IPv6 addresses. If set to 0, only the first route selected will be used for a given destination regardless of how many routes exist in the routing table. +.Pp +.It Li ip6.neighborgcthresh +Maximum number of entries in neighbor cache. +Set to negative to disable. +The default value is 2048. .Pp .It Li ip6.redirect Returns 1 when ICMPv6 redirects may be sent by the node. Index: sbin/sysctl/sysctl.8 === RCS file: /cvs/src/sbin/sysctl/sysctl.8,v retrieving revision 1.173 diff -u -p -u -p -r1.173 sysctl.8 --- sbin/sysctl/sysctl.828 Oct 2013 21:02:35 - 1.173 +++ sbin/sysctl/sysctl.819 Apr 2014 11:17:15 - @@ -301,10 +301,14 @@ and a few require a kernel compiled with .It net.inet6.ip6.use_deprecated Ta integer Ta yes .It net.inet6.ip6.rr_prune Ta integer Ta yes .It net.inet6.ip6.v6only Ta integer Ta no +.It net.inet6.ip6.maxdynroutes Ta integer Ta yes .It net.inet6.ip6.maxfrags Ta integer Ta yes +.It net.inet6.ip6.maxifprefixes Ta integer Ta yes +.It net.inet6.ip6.maxifdefrouters Ta integer Ta yes .It net.inet6.ip6.mforwarding Ta integer Ta yes .It net.inet6.ip6.multipath Ta integer Ta yes .It net.inet6.ip6.multicast_mtudisc Ta integer Ta yes +.It net.inet6.ip6.neighborgcthresh Ta integer Ta yes .It net.inet6.icmp6.rediraccept Ta integer Ta yes .It net.inet6.icmp6.redirtimeout Ta integer Ta yes .It net.inet6.icmp6.nd6_prune Ta integer Ta yes
openssl fallout, dealing with it
Since the tree wants to move fast, here's a sketch of how we deal with ports breakage. - someone makes sure everyone is aware there's breakage. ports and src should communicate so we assert what got removed intentionally, what got removed by accident. - if the removal is not accidental, fix critical ports asap. By critical, I mean stuff such as, oh say, ruby, python, openldap, kdelibs, since that tends to take out a large chunk of the ports tree, thus effectively hiding further breakage. - iterate until most of the turds are gone.
Re: fix for ifa RB tree corruption
On 18/04/14(Fri) 18:12, Claudio Jeker wrote: Bad stuff happens when the ifa lookup tree gets corrupted. In my case local traffic was suddenly no longer local and was forwarded to lo0 ad infinitum. Which lookup exactly? This was caused by the usage of rdomains and destroing pseudo interfaces. The sadl address was still in rdomain 0, was therefor not found in the tree and so you end up with a bad node and a use after free. I'm really interested in this case, because I think that there's no benefit of storing the link-layer addresses in the RB-tree. I believe we could even remove them from the tree, so I'd better fix the code relying this behavior. The following diff solves this by removing and readding the sadl to the RB tree when switching rdomain. If you're doing that because of a call to ifa_ifwithaddr() I'd suggest to modify the calling function instead. I'd guess it's ifa_ifwithroute(), isn't it? Since link-layer addresses contain the index of their related interface, I don't see the point of doing a lookup. OK? -- :wq Claudio Index: if.c === RCS file: /cvs/src/sys/net/if.c,v retrieving revision 1.283 diff -u -p -r1.283 if.c --- if.c 10 Apr 2014 13:47:21 - 1.283 +++ if.c 18 Apr 2014 16:03:11 - @@ -1515,6 +1580,8 @@ ifioctl(struct socket *so, u_long cmd, c #ifdef INET in_ifdetach(ifp); #endif + /* remove sadl from ifa RB tree */ + ifa_del(ifp, ifp-if_lladdr); splx(s); } @@ -1525,6 +1592,9 @@ ifioctl(struct socket *so, u_long cmd, c /* Add interface to the specified rdomain */ ifp-if_rdomain = ifr-ifr_rdomainid; + + /* re-add sadl to the ifa RB tree in new rdomain */ + ifa_add(ifp, ifp-if_lladdr); break; case SIOCAIFGROUP:
Re: openssl's *strlcpy
On Sat, Apr 19, 2014, Theo de Raadt wrote: Hi Claus @ Sendmail [come on, your employeer matters when you point It does? That must be something american or english -- it doesn't matter for me: I'm not talking for my (ex-)employer but only as an individual. In my country of origin I've never encountered that kind of association... And I was reluctant to mail this as I was afraid someone would leave the tech level (despite the name of the list). I don't see the point in that, so I won't reply to this anymore. you are so shocked.
Re: fix for ifa RB tree corruption
On Sat, Apr 19, 2014 at 03:09:40PM +0200, Martin Pieuchot wrote: On 18/04/14(Fri) 18:12, Claudio Jeker wrote: Bad stuff happens when the ifa lookup tree gets corrupted. In my case local traffic was suddenly no longer local and was forwarded to lo0 ad infinitum. Which lookup exactly? in_ouraddr() in ip_input() started to freak out because the RB tree was internally corrupt and in the worst case you would acutually panic while checking the tree. This was caused by the usage of rdomains and destroing pseudo interfaces. The sadl address was still in rdomain 0, was therefor not found in the tree and so you end up with a bad node and a use after free. I'm really interested in this case, because I think that there's no benefit of storing the link-layer addresses in the RB-tree. I believe we could even remove them from the tree, so I'd better fix the code relying this behavior. Yes, I'm not sure why we keep the sadl in the RB lookup tree. I think nothing is using that (but every time I thought something did not make sense in the network stack I was proven wrong so I'm more careful now). The following diff solves this by removing and readding the sadl to the RB tree when switching rdomain. If you're doing that because of a call to ifa_ifwithaddr() I'd suggest to modify the calling function instead. I'd guess it's ifa_ifwithroute(), isn't it? This will not help to fix a corrupted tree. One could argue that we remove the if_add / if_del from if_alloc_sadl / if_free_sadl but then something else my break. The RB tree uses the ifp-if_rdomain in the lookup function and if that changes all elements in that tree need to be reinserted. The SIFRDOMAIN code removes all the IP and IPv6 addresses but not the sadl one and that address is now corrupting the tree. Since link-layer addresses contain the index of their related interface, I don't see the point of doing a lookup. I would like to get this in since it is currently badly broken. e.g. it is easily triggered by running the route regress test. Then we can discuss if we can remove the sadl from the RB tree. OK? -- :wq Claudio Index: if.c === RCS file: /cvs/src/sys/net/if.c,v retrieving revision 1.283 diff -u -p -r1.283 if.c --- if.c10 Apr 2014 13:47:21 - 1.283 +++ if.c18 Apr 2014 16:03:11 - @@ -1515,6 +1580,8 @@ ifioctl(struct socket *so, u_long cmd, c #ifdef INET in_ifdetach(ifp); #endif + /* remove sadl from ifa RB tree */ + ifa_del(ifp, ifp-if_lladdr); splx(s); } @@ -1525,6 +1592,9 @@ ifioctl(struct socket *so, u_long cmd, c /* Add interface to the specified rdomain */ ifp-if_rdomain = ifr-ifr_rdomainid; + + /* re-add sadl to the ifa RB tree in new rdomain */ + ifa_add(ifp, ifp-if_lladdr); break; case SIOCAIFGROUP: -- :wq Claudio
Re: fix for ifa RB tree corruption
On 19/04/14(Sat) 16:10, Claudio Jeker wrote: On Sat, Apr 19, 2014 at 03:09:40PM +0200, Martin Pieuchot wrote: On 18/04/14(Fri) 18:12, Claudio Jeker wrote: Bad stuff happens when the ifa lookup tree gets corrupted. In my case local traffic was suddenly no longer local and was forwarded to lo0 ad infinitum. Which lookup exactly? in_ouraddr() in ip_input() started to freak out because the RB tree was internally corrupt and in the worst case you would acutually panic while checking the tree. Whoa! This was caused by the usage of rdomains and destroing pseudo interfaces. The sadl address was still in rdomain 0, was therefor not found in the tree and so you end up with a bad node and a use after free. I'm really interested in this case, because I think that there's no benefit of storing the link-layer addresses in the RB-tree. I believe we could even remove them from the tree, so I'd better fix the code relying this behavior. Yes, I'm not sure why we keep the sadl in the RB lookup tree. I think nothing is using that (but every time I thought something did not make sense in the network stack I was proven wrong so I'm more careful now). Agreed. The following diff solves this by removing and readding the sadl to the RB tree when switching rdomain. If you're doing that because of a call to ifa_ifwithaddr() I'd suggest to modify the calling function instead. I'd guess it's ifa_ifwithroute(), isn't it? This will not help to fix a corrupted tree. One could argue that we remove the if_add / if_del from if_alloc_sadl / if_free_sadl but then something else my break. The RB tree uses the ifp-if_rdomain in the lookup function and if that changes all elements in that tree need to be reinserted. The SIFRDOMAIN code removes all the IP and IPv6 addresses but not the sadl one and that address is now corrupting the tree. I have a diff that does that actually, but indeed it needs more testing. Since link-layer addresses contain the index of their related interface, I don't see the point of doing a lookup. I would like to get this in since it is currently badly broken. e.g. it is easily triggered by running the route regress test. Then we can discuss if we can remove the sadl from the RB tree. OK? ok mpi@, but could you please add an explicit comment explaining that we need to do that *because* of the RB-tree. So that when it dies, we don't leave them around. -- :wq Claudio Index: if.c === RCS file: /cvs/src/sys/net/if.c,v retrieving revision 1.283 diff -u -p -r1.283 if.c --- if.c 10 Apr 2014 13:47:21 - 1.283 +++ if.c 18 Apr 2014 16:03:11 - @@ -1515,6 +1580,8 @@ ifioctl(struct socket *so, u_long cmd, c #ifdef INET in_ifdetach(ifp); #endif + /* remove sadl from ifa RB tree */ + ifa_del(ifp, ifp-if_lladdr); splx(s); } @@ -1525,6 +1592,9 @@ ifioctl(struct socket *so, u_long cmd, c /* Add interface to the specified rdomain */ ifp-if_rdomain = ifr-ifr_rdomainid; + + /* re-add sadl to the ifa RB tree in new rdomain */ + ifa_add(ifp, ifp-if_lladdr); break; case SIOCAIFGROUP: -- :wq Claudio
IPv6 mtudisctimeout sysctl man page fix
Hi All, The code was added for MTU discovery timeout in IPv6, but the man page misses the description. Feedback welcomed. Index: sbin/sysctl/sysctl.8 === RCS file: /cvs/src/sbin/sysctl/sysctl.8,v retrieving revision 1.174 diff -u -p -u -p -r1.174 sysctl.8 --- sbin/sysctl/sysctl.819 Apr 2014 12:42:50 - 1.174 +++ sbin/sysctl/sysctl.819 Apr 2014 14:46:11 - @@ -319,6 +319,7 @@ and a few require a kernel compiled with .It net.inet6.icmp6.nodeinfo Ta integer Ta yes .It net.inet6.icmp6.errppslimit Ta integer Ta yes .It net.inet6.icmp6.nd6_maxnudhint Ta integer Ta yes +.It net.inet6.icmp6.mtudisctimeout Ta integer Ta yes .It net.inet6.icmp6.mtudisc_hiwat Ta integer Ta yes .It net.inet6.icmp6.mtudisc_lowat Ta integer Ta yes .It net.inet6.icmp6.nd6_debug Ta integer Ta yes Index: lib/libc/gen/sysctl.3 === RCS file: /cvs/src/lib/libc/gen/sysctl.3,v retrieving revision 1.229 diff -u -p -u -p -r1.229 sysctl.3 --- lib/libc/gen/sysctl.3 19 Apr 2014 12:42:50 - 1.229 +++ lib/libc/gen/sysctl.3 19 Apr 2014 14:46:12 - @@ -1656,6 +1656,7 @@ The currently defined protocols and name .Bl -column Protocol name multicast_mtudisc integer yes -offset indent .It Sy Protocol name Ta Sy Variable name Ta Sy Type Ta Sy Changeable .It icmp6 Ta errppslimit Ta integer Ta yes +.It icmp6 Ta mtudisctimeout Ta integer Ta yes .It icmp6 Ta mtudisc_hiwat Ta integer Ta yes .It icmp6 Ta mtudisc_lowat Ta integer Ta yes .It icmp6 Ta nd6_debug Ta integer Ta yes @@ -1700,6 +1701,10 @@ per second. ICMPv6 error messages exceeding this value are subject to rate limitation and will not go out from the node. A negative value will disable the rate limitation. +.Pp +.It Li icmp6.mtudisctimeout +Returns the number of seconds in which a route added by the Path MTU +Discovery engine will time out. .Pp .It Li icmp6.mtudisc_hiwat .It Li icmp6.mtudisc_lowat
Re: IPv6 mtudisctimeout sysctl man page fix
On Sat, Apr 19, 2014 at 07:51:34AM -0700, Loganaden Velvindron wrote: Hi All, The code was added for MTU discovery timeout in IPv6, but the man page misses the description. Feedback welcomed. s/icmp6/ip6 from henning@ and sthen@, and change from Return the number of seconds to Number of Seconds for both IPv4 and IPv6. Index: sbin/sysctl/sysctl.8 === RCS file: /cvs/src/sbin/sysctl/sysctl.8,v retrieving revision 1.174 diff -u -p -u -p -r1.174 sysctl.8 --- sbin/sysctl/sysctl.819 Apr 2014 12:42:50 - 1.174 +++ sbin/sysctl/sysctl.819 Apr 2014 15:14:39 - @@ -306,6 +306,7 @@ and a few require a kernel compiled with .It net.inet6.ip6.maxifprefixes Ta integer Ta yes .It net.inet6.ip6.maxifdefrouters Ta integer Ta yes .It net.inet6.ip6.mforwarding Ta integer Ta yes +.It net.inet6.ip6.mtudisctimeout Ta integer Ta yes .It net.inet6.ip6.multipath Ta integer Ta yes .It net.inet6.ip6.multicast_mtudisc Ta integer Ta yes .It net.inet6.ip6.neighborgcthresh Ta integer Ta yes Index: lib/libc/gen/sysctl.3 === RCS file: /cvs/src/lib/libc/gen/sysctl.3,v retrieving revision 1.229 diff -u -p -u -p -r1.229 sysctl.3 --- lib/libc/gen/sysctl.3 19 Apr 2014 12:42:50 - 1.229 +++ lib/libc/gen/sysctl.3 19 Apr 2014 15:14:42 - @@ -1476,7 +1476,7 @@ The default is 0. .It Li ip.mtudisc Returns 1 if Path MTU Discovery is enabled. .It Li ip.mtudisctimeout -Returns the number of seconds in which a route added by the Path MTU +Number of seconds in which a route added by the Path MTU Discovery engine will time out. When the route times out, the Path MTU Discovery engine will attempt to probe a larger path MTU. @@ -1682,6 +1682,7 @@ The currently defined protocols and name .It ip6 Ta maxifprefixes Ta integer Ta yes .It ip6 Ta maxifdefrouters Ta integer Ta yes .It ip6 Ta mforwarding Ta integer Ta yes +.It ip6 Ta mtudisctimeout Ta integer Ta yes .It ip6 Ta multicast_mtudisc Ta integer Ta yes .It ip6 Ta multipath Ta integer Ta yes .It ip6 Ta neighborgcthresh Ta integer Ta yes @@ -1881,6 +1882,10 @@ If set to 0, the ICMPv6 Too Big message This variable enables multipath routing for IPv6 addresses. If set to 0, only the first route selected will be used for a given destination regardless of how many routes exist in the routing table. +.Pp +.It Li ip6.mtudisctimeout +Number of seconds in which a route added by the Path MTU +Discovery engine will time out. .Pp .It Li ip6.neighborgcthresh Maximum number of entries in neighbor cache.
Re: IPv6 mtudisctimeout sysctl man page fix
On Sat, Apr 19, 2014 at 08:19:23AM -0700, Loganaden Velvindron wrote: On Sat, Apr 19, 2014 at 07:51:34AM -0700, Loganaden Velvindron wrote: Hi All, The code was added for MTU discovery timeout in IPv6, but the man page misses the description. Feedback welcomed. s/icmp6/ip6 from henning@ and sthen@, and change from Return the number of seconds to Number of Seconds for both IPv4 and IPv6. Added a bit more description for IPv6. Index: sbin/sysctl/sysctl.8 === RCS file: /cvs/src/sbin/sysctl/sysctl.8,v retrieving revision 1.174 diff -u -p -u -p -r1.174 sysctl.8 --- sbin/sysctl/sysctl.819 Apr 2014 12:42:50 - 1.174 +++ sbin/sysctl/sysctl.819 Apr 2014 16:04:37 - @@ -306,6 +306,7 @@ and a few require a kernel compiled with .It net.inet6.ip6.maxifprefixes Ta integer Ta yes .It net.inet6.ip6.maxifdefrouters Ta integer Ta yes .It net.inet6.ip6.mforwarding Ta integer Ta yes +.It net.inet6.ip6.mtudisctimeout Ta integer Ta yes .It net.inet6.ip6.multipath Ta integer Ta yes .It net.inet6.ip6.multicast_mtudisc Ta integer Ta yes .It net.inet6.ip6.neighborgcthresh Ta integer Ta yes Index: lib/libc/gen/sysctl.3 === RCS file: /cvs/src/lib/libc/gen/sysctl.3,v retrieving revision 1.229 diff -u -p -u -p -r1.229 sysctl.3 --- lib/libc/gen/sysctl.3 19 Apr 2014 12:42:50 - 1.229 +++ lib/libc/gen/sysctl.3 19 Apr 2014 16:04:38 - @@ -1476,7 +1476,7 @@ The default is 0. .It Li ip.mtudisc Returns 1 if Path MTU Discovery is enabled. .It Li ip.mtudisctimeout -Returns the number of seconds in which a route added by the Path MTU +Number of seconds in which a route added by the Path MTU Discovery engine will time out. When the route times out, the Path MTU Discovery engine will attempt to probe a larger path MTU. @@ -1682,6 +1682,7 @@ The currently defined protocols and name .It ip6 Ta maxifprefixes Ta integer Ta yes .It ip6 Ta maxifdefrouters Ta integer Ta yes .It ip6 Ta mforwarding Ta integer Ta yes +.It ip6 Ta mtudisctimeout Ta integer Ta yes .It ip6 Ta multicast_mtudisc Ta integer Ta yes .It ip6 Ta multipath Ta integer Ta yes .It ip6 Ta neighborgcthresh Ta integer Ta yes @@ -1881,6 +1882,12 @@ If set to 0, the ICMPv6 Too Big message This variable enables multipath routing for IPv6 addresses. If set to 0, only the first route selected will be used for a given destination regardless of how many routes exist in the routing table. +.Pp +.It Li ip6.mtudisctimeout +Number of seconds in which a route added by the Path MTU +Discovery engine will time out. +When the route times out, the Path MTU Discovery engine will attempt +to probe a larger path MTU. .Pp .It Li ip6.neighborgcthresh Maximum number of entries in neighbor cache.
Re: stop advertising disabling pmtud and window size increasing
On Sat, Apr 19, 2014 at 06:21:40PM +0200, Henning Brauer wrote: very rarely if ever needed any more. we should not trick people into thinking they are impoving sth doing so, it's rather the opposite these days. Yes please. We should remove the buttons people should not touch unless they know what they are doing. In which case they will also find the button themself. Index: etc/sysctl.conf === RCS file: /cvs/src/etc/sysctl.conf,v retrieving revision 1.54 diff -u -p -r1.54 sysctl.conf --- etc/sysctl.conf 20 Sep 2012 12:51:43 - 1.54 +++ etc/sysctl.conf 19 Apr 2014 16:18:38 - @@ -15,8 +15,6 @@ #net.inet6.ip6.accept_rtadv=1# 1=Permit IPv6 autoconf (forwarding must be 0) #net.inet.tcp.always_keepalive=1 # 1=Keepalives for all connections (e.g. hotel/airport NAT) #net.inet.tcp.keepidle=100 # 100=send TCP keepalives every 50 seconds -#net.inet.tcp.rfc1323=0 # 0=Disable TCP RFC1323 extensions (for if tcp is slow) -#net.inet.tcp.rfc3390=0 # 0=Disable RFC3390 for TCP window increasing #net.inet.esp.enable=0 # 0=Disable the ESP IPsec protocol #net.inet.ah.enable=0# 0=Disable the AH IPsec protocol #net.inet.esp.udpencap=0 # 0=Disable ESP-in-UDP encapsulation -- 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/ -- :wq Claudio
Re: help needed from someone with an sk(4)
On 2014-04-19, Henning Brauer lists-openbsdt...@bsws.de wrote: we're in the same boat here - it's ust that I don't care too much either way (both of us) doesn't really help in taking a decision :/ Well, in that case I suggest that we remove this hack from all drivers that have it. A network driver shouldn't have to examine higher protocol layers. And IPv4 is obsolete anyway. I'll look at gem; that one I can test. my main concern is getting rid of the in_cksum_phdr (yes yes, tehre is a SECOND function doing essentially the same, but one thing after the other) and especially in_cksum_addword APIs. Nobody shall use this horror again. FWIW, I added in6_cksum_phdr() with the IPv6 checksumming so it would parallel the IPv4 stuff. -- Christian naddy Weisgerber na...@mips.inka.de
Re: openssl's *strlcy
On Fri, Apr 18, 2014 at 05:19:15PM -0700, Claus Assmann wrote:
Seems it is ok to use strlcat/strlcpy that way in some cases:
$ cat src/usr.sbin/smtpd/*.c | egrep -c ' strlc(at|py)\('
249
We tend to be very strict with our checks in smtpd and we did not check
in various places because the copies occur between buffers of same size
or because length is checked and handled earlier or later.
However, your mail prompted me into having an audit, and so I spent the
whole day looking at every single call to make sure we didn't miss some
truncation check somewhere.
I started by removing all void casts to make sure I did not assume some
were ok just because they were previously marked as such.
I then proceeded to casting void the easy and obvious ones, for example
those that copy constant strings to larger buffers, the ones that can't
overflow because of former checks or identical buffer size copies, ...
This was the most common case.
In some cases, it was a bit less obvious as the buffer was coming from
another process through imsg, I made sure the assumptions were correct
and void casted those too when I was 100% sure they were safe and that
the code path was trivial to follow.
In a couple cases I did add checks even though it was safe because the
code path was a bit tricky and it seemed like the check was not taking
place at the appropriate place (best example is rev 1.15 of table.c).
In places where it's safe because two buffers use different defines of
same value, I also added checks just in case we ever change one of the
values without paying attention to the consequences.
Finally, there were some instances where we did miss a check that would
later lead to a failure with an inappropriate error message (ie: if the
interface name exceeded the size of a buffer, we would fail in an ioctl
call a bit later rather than right away).
Anyways, all calls are now checked and you can review the commits which
were done today, I made it clear in every commit log was fixed.
NOW IS TIME FOR WINE AND CHEEZE.
--
Gilles Chehade
https://www.poolp.org @poolpOrg
Remove RX offload hack from gem(4), hme(4), hme(4/sparc)
This removes a RX offload hack similar to the one just deleted from
sk(4). These chips can only add 16-bit words starting from some
offset, and so the driver gives them the likely start of the TCP/UDP
payload and then tries to compensate and... ugh.
Affected drivers:
* gem(4)
* hme(4)
* another hme(4) driver for sparc
I've successfully tested gem(4) on a Blade 100. I don't have any
hme(4) hardware.
ok?
Index: dev/ic/gem.c
===
RCS file: /cvs/src/sys/dev/ic/gem.c,v
retrieving revision 1.102
diff -u -p -r1.102 gem.c
--- dev/ic/gem.c14 Mar 2014 11:04:24 - 1.102
+++ dev/ic/gem.c19 Apr 2014 19:00:01 -
@@ -120,7 +120,6 @@ int gem_eint(struct gem_softc *, u_int)
intgem_rint(struct gem_softc *);
intgem_tint(struct gem_softc *, u_int32_t);
intgem_pint(struct gem_softc *);
-void gem_rxcksum(struct mbuf *, u_int64_t);
#ifdef GEM_DEBUG
#defineDPRINTF(sc, x) if ((sc)-sc_arpcom.ac_if.if_flags IFF_DEBUG)
\
@@ -811,9 +810,6 @@ gem_init(struct ifnet *ifp)
/* Encode Receive Descriptor ring size: four possible values */
v = gem_ringsize(GEM_NRXDESC /*XXX*/);
- /* RX TCP/UDP checksum offset */
- v |= ((ETHER_HDR_LEN + sizeof(struct ip))
- GEM_RX_CONFIG_CXM_START_SHFT);
/* Enable DMA */
bus_space_write_4(t, h, GEM_RX_CONFIG,
v|(GEM_THRSH_1024GEM_RX_CONFIG_FIFO_THRS_SHIFT)|
@@ -948,95 +944,6 @@ gem_init_regs(struct gem_softc *sc)
}
/*
- * RX TCP/UDP checksum
- */
-void
-gem_rxcksum(struct mbuf *m, u_int64_t rxstat)
-{
- struct ether_header *eh;
- struct ip *ip;
- struct udphdr *uh;
- int32_t hlen, len, pktlen;
- u_int16_t cksum, *opts;
- u_int32_t temp32;
- union pseudoh {
- struct hdr {
- u_int16_t len;
- u_int8_t ttl;
- u_int8_t proto;
- u_int32_t src;
- u_int32_t dst;
- } h;
- u_int16_t w[6];
- } ph;
-
- pktlen = m-m_pkthdr.len;
- if (pktlen sizeof(struct ether_header))
- return;
- eh = mtod(m, struct ether_header *);
- if (eh-ether_type != htons(ETHERTYPE_IP))
- return;
- ip = (struct ip *)(eh + 1);
- if (ip-ip_v != IPVERSION)
- return;
-
- hlen = ip-ip_hl 2;
- pktlen -= sizeof(struct ether_header);
- if (hlen sizeof(struct ip))
- return;
- if (ntohs(ip-ip_len) hlen)
- return;
- if (ntohs(ip-ip_len) != pktlen)
- return;
- if (ip-ip_off htons(IP_MF | IP_OFFMASK))
- return; /* can't handle fragmented packet */
-
- switch (ip-ip_p) {
- case IPPROTO_TCP:
- if (pktlen (hlen + sizeof(struct tcphdr)))
- return;
- break;
- case IPPROTO_UDP:
- if (pktlen (hlen + sizeof(struct udphdr)))
- return;
- uh = (struct udphdr *)((caddr_t)ip + hlen);
- if (uh-uh_sum == 0)
- return; /* no checksum */
- break;
- default:
- return;
- }
-
- cksum = htons(~(rxstat GEM_RD_CHECKSUM));
- /* cksum fixup for IP options */
- len = hlen - sizeof(struct ip);
- if (len 0) {
- opts = (u_int16_t *)(ip + 1);
- for (; len 0; len -= sizeof(u_int16_t), opts++) {
- temp32 = cksum - *opts;
- temp32 = (temp32 16) + (temp32 65535);
- cksum = temp32 65535;
- }
- }
-
- ph.h.len = htons(ntohs(ip-ip_len) - hlen);
- ph.h.ttl = 0;
- ph.h.proto = ip-ip_p;
- ph.h.src = ip-ip_src.s_addr;
- ph.h.dst = ip-ip_dst.s_addr;
- temp32 = cksum;
- opts = ph.w[0];
- temp32 += opts[0] + opts[1] + opts[2] + opts[3] + opts[4] + opts[5];
- temp32 = (temp32 16) + (temp32 65535);
- temp32 += (temp32 16);
- cksum = ~temp32;
- if (cksum == 0) {
- m-m_pkthdr.csum_flags |=
- M_TCP_CSUM_IN_OK | M_UDP_CSUM_IN_OK;
- }
-}
-
-/*
* Receive interrupt.
*/
int
@@ -1100,8 +1007,6 @@ gem_rint(struct gem_softc *sc)
ifp-if_ipackets++;
m-m_pkthdr.rcvif = ifp;
m-m_pkthdr.len = m-m_len = len;
-
- gem_rxcksum(m, rxstat);
#if NBPFILTER 0
if (ifp-if_bpf)
Index: dev/ic/hme.c
===
RCS file: /cvs/src/sys/dev/ic/hme.c,v
retrieving revision 1.63
diff -u -p -r1.63 hme.c
--- dev/ic/hme.c7 Aug 2013 01:06:29 - 1.63
+++ dev/ic/hme.c19 Apr 2014 19:12:29 -
@@ -105,8 +105,6 @@ void
[patch] courier-imap-4.13 imapd patch replacing malloc, strcat and strcpy with asprintf
Hi,
I'm using OpenBSD 5.5. courier-imap-4.13 is in the ports tree and it's
quite a mess. I started looking at it today with the hope of just
replacing some of the malloc,strcat strcpy calls with asprintf, but it
became clear before long that there's lots more issues with this code.
Regardless, here's a patch which fixes some of the malloc,strcat
strcpy spaghetti in imapd.c
I figure you guys are fairly busy with OpenSSL right now so if it's OK
with you I'll get working on the rest of this code.
I spoke with the courier-imap team and their response was less than
satisfactory - claiming asprintf code won't compile on non-linux
platforms.
--- imapd_orig.c Sat Apr 19 19:38:18 2014
+++ imapd.c Sat Apr 19 20:09:48 2014
@@ -343,9 +343,8 @@
if (q)
{
- r=malloc(strlen(q)+sizeof(/.));
- if (!r) write_error_exit(0);
- strcat(strcpy(r, q), /.);
+ if(asprintf(r, %s%s, q, /.) == -1)
+ write_error_exit(0);
if (access(r, 0) == 0)
{
free(r);
@@ -1373,11 +1372,9 @@
static char *alloc_filename(const char *mbox, const char *name)
{
-char *p=malloc(strlen(mbox)+strlen(name)+sizeof(/cur/));
-
- if (!p) write_error_exit(0);
-
- strcat(strcat(strcpy(p, mbox), /cur/), name);
+ char *p;
+ if(asprintf(p, %s%s%s, mbox, /cur/, name) == -1)
+ write_error_exit(0);
return (p);
}
@@ -1812,14 +1809,12 @@
{
#if EXPLICITDIRSYNC
- char *p=malloc(strlen(folder)+sizeof(/new));
+ char *p;
int fd;
-
- if (!p)
+
+ if(asprintf(p, %s%s, folder, /new) == -1)
write_error_exit(0);
- p=strcat(strcpy(p, folder), /new);
-
fd=open(p, O_RDONLY);
if (fd = 0)
@@ -1828,7 +1823,8 @@
close(fd);
}
- p=strcat(strcpy(p, folder), /cur);
+ if(asprintf(p, %s%s, folder, /cur) == -1)
+ write_error_exit(0);
fd=open(p, O_RDONLY);
@@ -2212,12 +2208,10 @@
{
struct imapscaninfo minfo;
- char *p=malloc(strlen(new_path)+sizeof(/ IMAPDB));
+ char *p;
+ if(asprintf(p, %s%s, new_path, / IMAPDB) == -1)
+write_error_exit(0);
- if (!p)
- write_error_exit(0);
-
- strcat(strcpy(p, new_path), / IMAPDB);
unlink(p);
free(p);
imapscan_init(minfo);
@@ -2448,14 +2442,12 @@
}
return 0;
}
- q=malloc(strlen(p)+sizeof(/new));
- if (!q)
+ if(asprintf(q, %s%s, p, /new) == -1)
{
free(p);
maildir_aclt_list_destroy(aclt_list);
return -1;
}
- strcat(strcpy(q, p), /new);
free(p);
if (maildir_aclt_list_add(aclt_list,
@@ -4216,11 +4208,10 @@
if (p (p=maildir_shareddir(., p+1)) != NULL)
{
int rc;
- char *q=malloc(strlen(p)+sizeof(/shared));
-
- if (!q) write_error_exit(0);
-
- strcat(strcpy(q, p), /shared);
+ char *q;
+
+ if(asprintf(q, %s%s, p, /shared) == -1)
+ write_error_exit(0);
free(p);
rc=append(tag, tok-tokenbuf, q);
free(q);
@@ -6041,10 +6032,9 @@
isshared=0;
if (is_sharedsubdir(cqinfo.destmailbox))
{
- char *p=malloc(strlen(cqinfo.destmailbox)+sizeof(/shared));
-
- if (!p) write_error_exit(0);
- strcat(strcpy(p, cqinfo.destmailbox), /shared);
+ char *p;
+ if(asprintf(p, %s%s, cqinfo.destmailbox, /shared) == -1)
+ write_error_exit(0);
free(mailbox);
mailbox=cqinfo.destmailbox=p;
--- imapd_orig.c Sat Apr 19 19:38:18 2014
+++ imapd.c Sat Apr 19 20:09:48 2014
@@ -343,9 +343,8 @@
if (q)
{
- r=malloc(strlen(q)+sizeof(/.));
- if (!r) write_error_exit(0);
- strcat(strcpy(r, q), /.);
+ if(asprintf(r, %s%s, q, /.) == -1)
+write_error_exit(0);
if (access(r, 0) == 0)
{
free(r);
@@ -1373,11 +1372,9 @@
static char *alloc_filename(const char *mbox, const char *name)
{
-char *p=malloc(strlen(mbox)+strlen(name)+sizeof(/cur/));
-
- if (!p) write_error_exit(0);
-
- strcat(strcat(strcpy(p, mbox), /cur/), name);
+ char *p;
+ if(asprintf(p, %s%s%s, mbox, /cur/, name) == -1)
+ write_error_exit(0);
return (p);
}
@@ -1812,14 +1809,12 @@
{
#if EXPLICITDIRSYNC
- char *p=malloc(strlen(folder)+sizeof(/new));
+ char *p;
int fd;
-
- if (!p)
+
+ if(asprintf(p, %s%s, folder, /new) == -1)
write_error_exit(0);
- p=strcat(strcpy(p, folder), /new);
-
fd=open(p, O_RDONLY);
if (fd = 0)
@@ -1828,7 +1823,8 @@
close(fd);
}
- p=strcat(strcpy(p, folder), /cur);
+ if(asprintf(p, %s%s, folder, /cur) == -1)
+ write_error_exit(0);
fd=open(p, O_RDONLY);
@@ -2212,12 +2208,10 @@
{
struct imapscaninfo minfo;
- char *p=malloc(strlen(new_path)+sizeof(/ IMAPDB));
+ char *p;
+ if(asprintf(p, %s%s, new_path, / IMAPDB) == -1)
+write_error_exit(0);
- if (!p)
- write_error_exit(0);
-
- strcat(strcpy(p, new_path), / IMAPDB);
unlink(p);
free(p);
imapscan_init(minfo);
@@ -2448,14 +2442,12 @@
}
return 0;
}
- q=malloc(strlen(p)+sizeof(/new));
- if (!q)
+ if(asprintf(q, %s%s, p, /new) == -1)
{
free(p);
maildir_aclt_list_destroy(aclt_list);
return -1;
}
- strcat(strcpy(q, p), /new);
free(p);
if (maildir_aclt_list_add(aclt_list,
@@ -4216,11 +4208,10 @@
if (p (p=maildir_shareddir(., p+1)) != NULL)
{
int rc;
-char *q=malloc(strlen(p)+sizeof(/shared));
-
-if (!q) write_error_exit(0);
-
-strcat(strcpy(q, p), /shared);
+char
