[PATCH 3/3] route(8): fail on nonexistent interfaces
Previously, a nonexistent interface would be converted to the invalid interface index 0. This is now rejected by the kernel, but route(8) can give a better error message by failing fast. --- sbin/route/route.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git sbin/route/route.c sbin/route/route.c index f1ec5bd38d8..13ab171cfd4 100644 --- sbin/route/route.c +++ sbin/route/route.c @@ -899,7 +899,8 @@ getaddr(int which, int af, char *s, struct hostent **hpp) } case AF_LINK: - su->sdl.sdl_index = if_nametoindex(s); + if ((su->sdl.sdl_index = if_nametoindex(s)) == 0) + errx(1, "no such interface %s", s); memset(>sdl.sdl_data, 0, sizeof(su->sdl.sdl_data)); return (1); case AF_MPLS: -- 2.26.2
[PATCH 2/3] Check for user-specified interfaces in RTA_GATEWAY
Both RTA_IFP and RTA_GATEWAY can use a sockaddr_dl struct. If either
specifies an explicit interface index, it must be honored. If both do,
and they are not equal, that is an error. It is also an error if RTA_IFP
specifies an index of 0.
Furthermore, if RTA_GATEWAY specifies a sockaddr_dl, it must not be used
where an AF_INET or AF_INET6 address is required.
---
sys/net/rtsock.c | 27 +--
1 file changed, 21 insertions(+), 6 deletions(-)
diff --git sys/net/rtsock.c sys/net/rtsock.c
index a156674fa6a..0ddab28272a 100644
--- sys/net/rtsock.c
+++ sys/net/rtsock.c
@@ -1221,6 +1221,7 @@ int
rtm_getifa(struct rt_addrinfo *info, unsigned int rtid)
{
struct ifnet*ifp = NULL;
+ uint16_tif_index = 0;
/*
* The "returned" `ifa' is guaranteed to be alive only if
@@ -1229,16 +1230,29 @@ rtm_getifa(struct rt_addrinfo *info, unsigned int rtid)
NET_ASSERT_LOCKED();
/*
-* ifp may be specified by sockaddr_dl; if so, it must be honored.
+* ifp and/or gateway may be specified by sockaddr_dl; if so, it must be
+* honored.
*/
if (info->rti_info[RTAX_IFP] != NULL) {
- struct sockaddr_dl *sdl;
+ if_index = satosdl(info->rti_info[RTAX_IFP])->sdl_index;
+ if (if_index == 0)
+ return (EINVAL);
+ }
+
+ if (info->rti_info[RTAX_GATEWAY] != NULL &&
+ info->rti_info[RTAX_GATEWAY]->sa_family == AF_LINK) {
+ uint16_t gateway_if;
- sdl = satosdl(info->rti_info[RTAX_IFP]);
- if ((ifp = if_get(sdl->sdl_index)) == NULL)
- return (ENXIO);
+ gateway_if = satosdl(info->rti_info[RTAX_GATEWAY])->sdl_index;
+ if (if_index == 0)
+ if_index = gateway_if;
+ else if (gateway_if != if_index)
+ return (EINVAL);
}
+ if (if_index != 0 && (ifp = if_get(if_index)) == NULL)
+ return (ENXIO);
+
#ifdef IPSEC
/*
* If the destination is a PF_KEY address, we'll look
@@ -1269,7 +1283,8 @@ rtm_getifa(struct rt_addrinfo *info, unsigned int rtid)
struct sockaddr *sa;
if ((sa = info->rti_info[RTAX_IFA]) == NULL)
- if ((sa = info->rti_info[RTAX_GATEWAY]) == NULL)
+ if ((sa = info->rti_info[RTAX_GATEWAY]) == NULL ||
+ sa->sa_family == AF_LINK)
sa = info->rti_info[RTAX_DST];
if (sa != NULL && ifp != NULL)
--
2.26.2
[PATCH 1/3] rtm_getifa: Never ignore RTA_IFP
If an RTM_ADD command on a routing socket includes an RTA_IFA sockaddr,
and that sockaddr is an exact match for one of the interfaces in the
relevant routing domain, any RTA_IFP sockaddr in the same message is
ignored. If there are multiple interfaces with the same IP address,
this can cause packets to be sent out the wrong interface. Fix this
by skipping the exact-match check on RTA_IFA if an RTA_IFP sockaddr
was sent.
The RTA_IFP sockaddr was also being ignored if there was no interface
with the requested index. Return ENXIO to userspace instead.
The bug can easily be seen by running the following commands as root
on a machine where vether0 and vether1 do not exist, and on which
the subnet 192.0.2.0/24 (reserved for documentation) is not in use:
vether1 -ifp vether1 -inet -ifa "$dummy_ip"
On a system with an unpatched kernel, the final command will show
that the route to 192.0.2.6 is via vether0. If this patch has been
applied, the route to 192.0.2.6 will be (correctly) via vether1.
---
sys/net/rtsock.c | 34 --
usr.sbin/arp/arp.c | 4 +---
2 files changed, 25 insertions(+), 13 deletions(-)
diff --git sys/net/rtsock.c sys/net/rtsock.c
index fa84ddc25e5..a156674fa6a 100644
--- sys/net/rtsock.c
+++ sys/net/rtsock.c
@@ -75,6 +75,7 @@
#include
#include
+#include
#include
#include
@@ -1228,29 +1229,40 @@ rtm_getifa(struct rt_addrinfo *info, unsigned int rtid)
NET_ASSERT_LOCKED();
/*
-* ifp may be specified by sockaddr_dl when protocol address
-* is ambiguous
+* ifp may be specified by sockaddr_dl; if so, it must be honored.
*/
if (info->rti_info[RTAX_IFP] != NULL) {
struct sockaddr_dl *sdl;
sdl = satosdl(info->rti_info[RTAX_IFP]);
- ifp = if_get(sdl->sdl_index);
+ if ((ifp = if_get(sdl->sdl_index)) == NULL)
+ return (ENXIO);
}
#ifdef IPSEC
/*
* If the destination is a PF_KEY address, we'll look
-* for the existence of a encap interface number or address
-* in the options list of the gateway. By default, we'll return
-* enc0.
+* for the existence of a encap interface number as the output
+* interface.
*/
if (info->rti_info[RTAX_DST] &&
- info->rti_info[RTAX_DST]->sa_family == PF_KEY)
- info->rti_ifa = enc_getifa(rtid, 0);
+ info->rti_info[RTAX_DST]->sa_family == PF_KEY) {
+ if (ifp != NULL) {
+ struct enc_softc *sc;
+ if (ifp->if_type != IFT_ENC) {
+ if_put(ifp);
+ return (EINVAL);
+ }
+ sc = ifp->if_softc;
+ info->rti_ifa = >sc_ifa;
+ } else
+ info->rti_ifa = enc_getifa(rtid, 0);
+ }
#endif
- if (info->rti_ifa == NULL && info->rti_info[RTAX_IFA] != NULL)
+ /* ifp overrides even an exact match on RTAX_IFA */
+ if (info->rti_ifa == NULL && ifp == NULL &&
+ info->rti_info[RTAX_IFA] != NULL)
info->rti_ifa = ifa_ifwithaddr(info->rti_info[RTAX_IFA], rtid);
if (info->rti_ifa == NULL) {
@@ -1273,6 +1285,9 @@ rtm_getifa(struct rt_addrinfo *info, unsigned int rtid)
sa, sa, rtid);
}
+ KASSERT(info->rti_ifa == NULL || ifp == NULL ||
+ ifp == info->rti_ifa->ifa_ifp);
+
if_put(ifp);
if (info->rti_ifa == NULL)
@@ -1435,7 +1450,6 @@ rtm_xaddrs(caddr_t cp, caddr_t cplim, struct rt_addrinfo
*rtinfo)
/*
* XXX Should be sizeof(struct sockaddr_dl), but
* route(8) has a bug and provides less memory.
-* arp(8) has another bug and uses sizeof pointer.
*/
size = 4;
break;
diff --git usr.sbin/arp/arp.c usr.sbin/arp/arp.c
index b09eedd7ec9..09d5b5034b8 100644
--- usr.sbin/arp/arp.c
+++ usr.sbin/arp/arp.c
@@ -259,7 +259,6 @@ parse_host(const char *host, struct in_addr *in)
struct sockaddr_in so_mask = { 8, 0, 0, { 0x } };
struct sockaddr_inarp blank_sin = { sizeof(blank_sin), AF_INET }, sin_m;
struct sockaddr_dl blank_sdl = { sizeof(blank_sdl), AF_LINK }, sdl_m;
-struct sockaddr_dl ifp_m = { sizeof(ifp_m), AF_LINK };
time_t expire_time;
intflags, export_only, doing_proxy, found_entry;
struct {
@@ -646,7 +645,7 @@ rtmsg(int cmd)
}
/* FALLTHROUGH */
case RTM_GET:
- rtm->rtm_addrs |= (RTA_DST | RTA_IFP);
+ rtm->rtm_addrs |= RTA_DST;
}
#define NEXTADDR(w, s) \
@@ -658,7 +657,6 @@ rtmsg(int cmd)
Re: acme-client: improve account creation error message
On 2020-09-15 04:25, Florian Obser wrote:
> On Mon, Sep 14, 2020 at 04:26:20PM -0500, Rafael Possamai wrote:
>>> please dont drop the all buffer , or keep it with -vv ?
>>> example : warnx("%s: bad JSON object:%s", p->newaccount, c->buf.buf);
>>>
>>> i don't want to ktrace it to see why the new certbot version is not working
>>
>> Yeah, I think it's good to have the choice to inspect the vomit, maybe you
>> stumble upon some useful nuggets for troubleshooting an obscure issue.
>>
>> I believe the JSON output is valid, just not pretty. Maybe we could add
>> syntax highlighting? (just kidding)
>>
>
> I don't understand what you people are going on about, I didn't touch
> the -v output. You can still roll around in json.
>
> It's not about if it's valid json or not, it's untrusted input that we
> spit on your console when used with -v.
Simple fix: ensure that all control characters are represented as
Unicode escape sequences. I believe that the JSON spec requires
this anyway.
Re: rtm_getifa: Never ignore RTA_IFP
On 2020-09-13 12:40, Claudio Jeker wrote: > On Sun, Sep 13, 2020 at 11:28:11AM -0400, Demi M. Obenour wrote: >> Resending without quoted-printable encoding, in case that helps. >> --- >> If an RTM_ADD command on a routing socket includes an RTA_IFA sockaddr, >> and that sockaddr is an exact match for one of the interfaces in the >> relevant routing domain, any RTA_IFP sockaddr in the same message is >> ignored. If there are multiple interfaces with the same IP address, >> this can cause packets to be sent out the wrong interface. Fix this >> by skipping the exact-match check on RTA_IFA if an RTA_IFP sockaddr >> was sent. >> >> The RTA_IFP sockaddr was also being ignored if there was no interface >> with the requested index. Return ENXIO to userspace instead. >> >> The bug can easily be seen by running the following commands as root >> on a machine where vether0 and vether1 do not exist, and on which >> the subnet 192.0.2.0/24 (reserved for documentation) is not in use: >> >> # ifconfig vether0 destroy 2>/dev/null >> # ifconfig vether1 destroy 2>/dev/null >> # dummy_mac=fe:ff:ff:ff:ff:ff dummy_ip=192.0.2.5 >> # ifconfig vether0 create lladdr "$dummy_mac" >> # ifconfig vether1 create lladdr "$dummy_mac" >> # ifconfig vether0 inet "$dummy_ip" prefixlen 32 >> # route -n delete "$dummy_ip/32" "$dummy_ip" >> # ifconfig vether1 inet "$dummy_ip" prefixlen 32 >> # route -n delete "$dummy_ip/32" "$dummy_ip" >> # route -n add -inet 192.0.2.6 -static -iface -llinfo -link \ >> vether1 -ifp vether1 -inet -ifa "$dummy_ip" >> # route -n show -inet >> >> On a system with an unpatched kernel, the final command will show >> that the route to 192.0.2.6 is via vether0. If this patch has been >> applied, the route to 192.0.2.6 will be (correctly) via vether1. >> > > Can you please explain why you think this fix is needed. Looking at the > code above my first reaction is don't do that. The configuration with the > same IP on multiple interface you do is something I would generally not > recommend. > It seems like you try to do a poor mans link aggregation and should > instead use trunk(4) or aggr(4). Thanks for your response. Indeed, having the same IP address on multiple interfaces is not (contrary to what I thought at first) common practice, so I will explain why I want to support it. The short answer is that when OpenBSD is used as a layer 3 IPv4 router with a significant number of ports, it is far simpler to assign a single IP to the OpenBSD machine, rather than having to assign a separate subnet for each connection. Since there is technically one network per interface, using the same IP address for all of the ports saves a significant number of IP addresses. With one IP address per machine, each machine only needs to be assigned a single IP address, no matter how many machines it is connected to. Furthermore, this address does not depend on what machines the router is connected to, which makes automatic IP address assignment much simpler. The long answer involves a discussion of QubesOS network topology, and I have omitted it in the interest of brevity. However, I would be more than happy to include it upon request. On Linux, forcing a route to use a particular interface and source address is trivial: ``` # ip route replace to "$subnet" dev "$iface" src "$src" ``` However, I was not able to find an OpenBSD equivalent for that command. The closest I was able to find was ``` route -n add -iface -inet -static "$subnet" -link "$iface" \ -ifp "$iface" -ifa "$src" ``` but it doesn’t work if there are multiple interfaces that have $src as their address. It is also worth noting that I was actually not using route(8) directly, but rather using a custom C program and routing sockets; this allowed me to specify the destination MAC address at the same time. As it happens, `route -n add -iface -inet -static "$subnet" -link "$iface"` works. However, it would not be sufficient if there were multiple IP addresses assigned to $iface and I needed to select which one to use. It also doesn’t let me specify a MAC address, although I might be able to do so in C. Finally, when I pass `-ifp "$iface"` to route(8), I expect that the route will go through $iface. If the kernel cannot ensure this, it should return an error to userspace, rather than silently ignoring what I told it to do. Thanks for your time and attention. Sincerely, Demi
rtm_getifa: Never ignore RTA_IFP
Resending without quoted-printable encoding, in case that helps.
---
If an RTM_ADD command on a routing socket includes an RTA_IFA sockaddr,
and that sockaddr is an exact match for one of the interfaces in the
relevant routing domain, any RTA_IFP sockaddr in the same message is
ignored. If there are multiple interfaces with the same IP address,
this can cause packets to be sent out the wrong interface. Fix this
by skipping the exact-match check on RTA_IFA if an RTA_IFP sockaddr
was sent.
The RTA_IFP sockaddr was also being ignored if there was no interface
with the requested index. Return ENXIO to userspace instead.
The bug can easily be seen by running the following commands as root
on a machine where vether0 and vether1 do not exist, and on which
the subnet 192.0.2.0/24 (reserved for documentation) is not in use:
# ifconfig vether0 destroy 2>/dev/null
# ifconfig vether1 destroy 2>/dev/null
# dummy_mac=fe:ff:ff:ff:ff:ff dummy_ip=192.0.2.5
# ifconfig vether0 create lladdr "$dummy_mac"
# ifconfig vether1 create lladdr "$dummy_mac"
# ifconfig vether0 inet "$dummy_ip" prefixlen 32
# route -n delete "$dummy_ip/32" "$dummy_ip"
# ifconfig vether1 inet "$dummy_ip" prefixlen 32
# route -n delete "$dummy_ip/32" "$dummy_ip"
# route -n add -inet 192.0.2.6 -static -iface -llinfo -link \
vether1 -ifp vether1 -inet -ifa "$dummy_ip"
# route -n show -inet
On a system with an unpatched kernel, the final command will show
that the route to 192.0.2.6 is via vether0. If this patch has been
applied, the route to 192.0.2.6 will be (correctly) via vether1.
---
sys/net/rtsock.c | 26 --
1 file changed, 20 insertions(+), 6 deletions(-)
diff --git sys/net/rtsock.c sys/net/rtsock.c
index fa84ddc25e5..dc8446bd78f 100644
--- sys/net/rtsock.c
+++ sys/net/rtsock.c
@@ -1235,7 +1235,8 @@ rtm_getifa(struct rt_addrinfo *info, unsigned int rtid)
struct sockaddr_dl *sdl;
sdl = satosdl(info->rti_info[RTAX_IFP]);
- ifp = if_get(sdl->sdl_index);
+ if ((ifp = if_get(sdl->sdl_index)) == NULL)
+ return (ENXIO);
}
#ifdef IPSEC
@@ -1246,11 +1247,19 @@ rtm_getifa(struct rt_addrinfo *info, unsigned int rtid)
* enc0.
*/
if (info->rti_info[RTAX_DST] &&
- info->rti_info[RTAX_DST]->sa_family == PF_KEY)
+ info->rti_info[RTAX_DST]->sa_family == PF_KEY) {
info->rti_ifa = enc_getifa(rtid, 0);
+
+ if (info->rti_ifa != NULL && ifp != NULL &&
+ ifp != info->rti_ifa->ifa_ifp) {
+ if_put(ifp);
+ return (EADDRNOTAVAIL);
+ }
+ }
#endif
- if (info->rti_ifa == NULL && info->rti_info[RTAX_IFA] != NULL)
+ if (info->rti_ifa == NULL && ifp == NULL &&
+ info->rti_info[RTAX_IFA] != NULL)
info->rti_ifa = ifa_ifwithaddr(info->rti_info[RTAX_IFA], rtid);
if (info->rti_ifa == NULL) {
@@ -1273,10 +1282,15 @@ rtm_getifa(struct rt_addrinfo *info, unsigned int rtid)
sa, sa, rtid);
}
- if_put(ifp);
-
- if (info->rti_ifa == NULL)
+ if (info->rti_ifa == NULL) {
+ if_put(ifp);
return (ENETUNREACH);
+ }
+
+ if (ifp != NULL && ifp != info->rti_ifa->ifa_ifp)
+ panic("rtm_getifa: returned route to wrong interface");
+
+ if_put(ifp);
return (0);
}
rtm_getifa: Never ignore RTA_IFP
If an RTM_ADD command on a routing socket includes an RTA_IFA sockaddr,
and that sockaddr is an exact match for one of the interfaces in the
relevant routing domain, any RTA_IFP sockaddr in the same message is
ignored. If there are multiple interfaces with the same IP address,
this can cause packets to be sent out the wrong interface. Fix this
by skipping the exact-match check on RTA_IFA if an RTA_IFP sockaddr
was sent.
The RTA_IFP sockaddr was also being ignored if there was no interface
with the requested index. Return ENXIO to userspace instead.
The bug can easily be seen by running the following commands as root
on a machine where vether0 and vether1 do not exist, and on which
the subnet 192.0.2.0/24 (reserved for documentation) is not in use:
# ifconfig vether0 destroy 2>/dev/null
# ifconfig vether1 destroy 2>/dev/null
# dummy_mac=fe:ff:ff:ff:ff:ff dummy_ip=192.0.2.5
# ifconfig vether0 create lladdr "$dummy_mac"
# ifconfig vether1 create lladdr "$dummy_mac"
# ifconfig vether0 inet "$dummy_ip" prefixlen 32
# route -n delete "$dummy_ip/32" "$dummy_ip"
# ifconfig vether1 inet "$dummy_ip" prefixlen 32
# route -n delete "$dummy_ip/32" "$dummy_ip"
# route -n add -inet 192.0.2.6 -static -iface -llinfo -link vether1 -ifp
vether1 -inet -ifa "$dummy_ip"
# route -n show -inet
On a system with an unpatched kernel, the final command will show
that the route to 192.0.2.6 is via vether0. If this patch has been
applied, the route to 192.0.2.6 will be (correctly) via vether1.
---
sys/net/rtsock.c | 26 --
1 file changed, 20 insertions(+), 6 deletions(-)
diff --git sys/net/rtsock.c sys/net/rtsock.c
index fa84ddc25e5..dc8446bd78f 100644
--- sys/net/rtsock.c
+++ sys/net/rtsock.c
@@ -1235,7 +1235,8 @@ rtm_getifa(struct rt_addrinfo *info, unsigned int rtid)
struct sockaddr_dl *sdl;
sdl = satosdl(info->rti_info[RTAX_IFP]);
- ifp = if_get(sdl->sdl_index);
+ if ((ifp = if_get(sdl->sdl_index)) == NULL)
+ return (ENXIO);
}
#ifdef IPSEC
@@ -1246,11 +1247,19 @@ rtm_getifa(struct rt_addrinfo *info, unsigned int rtid)
* enc0.
*/
if (info->rti_info[RTAX_DST] &&
- info->rti_info[RTAX_DST]->sa_family == PF_KEY)
+ info->rti_info[RTAX_DST]->sa_family == PF_KEY) {
info->rti_ifa = enc_getifa(rtid, 0);
+
+ if (info->rti_ifa != NULL && ifp != NULL &&
+ ifp != info->rti_ifa->ifa_ifp) {
+ if_put(ifp);
+ return (EADDRNOTAVAIL);
+ }
+ }
#endif
- if (info->rti_ifa == NULL && info->rti_info[RTAX_IFA] != NULL)
+ if (info->rti_ifa == NULL && ifp == NULL &&
+ info->rti_info[RTAX_IFA] != NULL)
info->rti_ifa = ifa_ifwithaddr(info->rti_info[RTAX_IFA], rtid);
if (info->rti_ifa == NULL) {
@@ -1273,10 +1282,15 @@ rtm_getifa(struct rt_addrinfo *info, unsigned int rtid)
sa, sa, rtid);
}
- if_put(ifp);
-
- if (info->rti_ifa == NULL)
+ if (info->rti_ifa == NULL) {
+ if_put(ifp);
return (ENETUNREACH);
+ }
+
+ if (ifp != NULL && ifp != info->rti_ifa->ifa_ifp)
+ panic("rtm_getifa: returned route to wrong interface");
+
+ if_put(ifp);
return (0);
}
signature.asc
Description: OpenPGP digital signature
Re: Proxy-arp behavior change between 5.9 and 6.0
On 2020-07-29 21:47, Andrew Hewus Fresh wrote: > I am helping some folks with some OpenBSD stuff, including at some > point being more proactive about updating, but they currently have an > OpenBSD 5.9 machine that has been routing traffic happily for a while. > Unfortunately, they can't currently update because the proxy arp > configuration that they are using on 5.9 doesn't work on anything newer. > > > Here is the excellent description from someone there, and I was able to > reproduce the issue on my laptop, so I don't believe a dmesg will be > particularly helpful, but here's one for my laptop that's not terribly > old. > > https://dmesgd.nycbug.org/index.cgi?do=view=5279 > > As I haven't ever actually used proxy arp on OpenBSD, so don't actually > know whether this was a glitch that was fixed or added. > > I do see that 60.html says the routing table is now based on ART, so > that seems a possible culprit. > > --- > > > We've found that a proxy-arp configuration we've relied on (which works > perfectly on OpenBSD 5.9 and earlier) no longer works on OpenBSD 6.0 thru 6.7. > > On OpenBSD 5.9, we can add an "arp -s" entry for a public IP on our > public-facing gateway router's external interface, followed by a host-route > for that same public IP (pointing to an internal subnet/interface). This > allows us to route public IPs to clients through our internal private subnets, > so a client can have a public IP assigned on their router, despite actually > being several hops deep inside our private network. > > With this proxy-arp configuration, from both the client router's perspective > and from an external perspective, it looks as if the client router is in the > public IP subnet. > > Unfortunately this method doesn't work in OpenBSD 6.0 and later. > > > Here's how it looks when you configure this on OpenBSD 5.9 > (working as expected with no errors): > > root@openbsd-59-pub-gw-rtr~ # arp -s 1.2.3.4 > [PUB-GW_em0_EXT-IF_MAC-ADDR] pub > root@openbsd-59-pub-gw-rtr~ # route add 1.2.3.4/32 10.0.2.47 > add host 1.2.3.4/32: gateway 10.0.2.47 > root@openbsd-59-pub-gw-rtr~ # netstat -rn -finet|grep 1.2.3.4 > 1.2.3.4 10.0.2.47 UGHS 0 0 - 8 em1 > 1.2.3.4/32 [PUB-GW_em0_EXT-IF_MAC-ADDR] ULS2 0 0 - > 8 em0 > # ^ Success: Routing table shows both the host-route and the "arp > -s" entry. > > > That same setup fails on OpenBSD 6.0 and later > (tested on OpenBSD 6.0, 6.2, 6.6, and 6.7). > > > If you add an arp -s entry before adding a host route for the same IP, the > "route add" fails with "File exists" error: > > root@openbsd-67-pub-gw-rtr~ # arp -s 1.2.3.4 > [PUB-GW_em0_EXT-IF_MAC-ADDR] pub > root@openbsd-67-pub-gw-rtr~ # route add 1.2.3.4/32 10.0.2.47 > !-> add host 1.2.3.4/32: gateway 10.0.2.47: File exists > root@openbsd-67-pub-gw-rtr~ # netstat -rn -finet|grep 1.2.3.4 > 1.2.3.4/32 [PUB-GW_em0_EXT-IF_MAC-ADDR] ULS2 0 0 - > 8 em0 > # ^ Failure: Routing table only shows the "arp -s" entry (failed to > add host-route) > > > If you add the route before the arp -s entry, "arp -s" fails with error > "set: proxy entry exists for non 802 device": > > root@openbsd-67-pub-gw-rtr~ # route add 1.2.3.4/32 10.0.2.47 > add host 1.2.3.4/32: gateway 10.0.2.47 > root@openbsd-67-pub-gw-rtr~ # arp -s 1.2.3.4 > [PUB-GW_em0_EXT-IF_MAC-ADDR] pub > !-> set: proxy entry exists for non 802 device > root@openbsd-67-pub-gw-rtr~ # netstat -rn -finet|grep 1.2.3.4 > 1.2.3.4 10.0.2.47 UGHS 0 0 - 8 em1 > # ^ Failure: Routing table only shows the host-route (failed to add > arp -s entry) > > > For most of our clients who request a dedicated public IP, we're able to > assign their router a private IP, which we then bi-NAT to a dedicated public > IP on our gateway router. Unfortunately the bi-NAT method doesn't work for > certain routers' built-in VPNs, so we're looking for a way to re-create the > functionality we had with proxy-arp on OpenBSD v5.9. I imagine there's some > workaround or alternate method to achieve the same thing on OpenBSD 6.x, but I > haven't found it yet. You can avoid this problem by first setting the ARP entry, and then adding the host route with a lower (better) priority. On my system (which runs OpenBSD-CURRENT), ARP entries have priority 8, so your host route should have priority below 8. Of course, you need to do this before interfaces come up, as otherwise packets might be routed to the wrong destination. If you do this, you won’t be able to use arp(8) to delete the proxy ARP entry. This is due to a limitation in arp(8), which is fixed by the following patch. This patch has been tested on my -CURRENT system and also allows creating an entry even if a route entry (with a priority other than 8) already exists. I would appreciate
[PATCH v3] Tighter pledges for ftp(1)
This prevents non-interactive invocations of ftp(1) from spawning
external commands in case they are compromised. This is a significant
security benefit for sysupgrade(8).
In the future, output files (specified with -o) can be opened before
pledge(2) is called, which will improve security further.
Sincerely,
Demi M. Obenour
Index: usr.bin/ftp/main.c
===
RCS file: /cvs/src/usr.bin/ftp/main.c,v
retrieving revision 1.131
diff -u -p -u -p -r1.131 main.c
--- usr.bin/ftp/main.c 11 Feb 2020 18:41:39 - 1.131
+++ usr.bin/ftp/main.c 3 May 2020 18:18:31 -
@@ -483,27 +483,33 @@ main(volatile int argc, char *argv[])
(void)signal(SIGWINCH, setttywidth);
if (argc > 0) {
+ char **arg;
+ int needs_exec = 0;
if (isurl(argv[0])) {
- if (pipeout) {
#ifndef SMALL
- if (pledge("stdio rpath dns tty inet proc exec
fattr",
- NULL) == -1)
- err(1, "pledge");
-#else
- if (pledge("stdio rpath dns tty inet fattr",
- NULL) == -1)
- err(1, "pledge");
+ for (arg = argv; *arg; ++arg)
+ needs_exec |= is_interactive_url(*arg);
#endif
+ if (pipeout) {
+ if (needs_exec) {
+ if (pledge("stdio rpath dns tty inet
proc exec fattr",
+ NULL) == -1)
+ err(1, "pledge");
+ } else {
+ if (pledge("stdio rpath dns tty inet
fattr",
+ NULL) == -1)
+ err(1, "pledge");
+ }
} else {
-#ifndef SMALL
- if (pledge("stdio rpath wpath cpath dns tty
inet proc exec fattr",
- NULL) == -1)
- err(1, "pledge");
-#else
- if (pledge("stdio rpath wpath cpath dns tty
inet fattr",
- NULL) == -1)
- err(1, "pledge");
-#endif
+ if (needs_exec) {
+ if (pledge("stdio rpath wpath cpath dns
tty inet proc exec fattr",
+ NULL) == -1)
+ err(1, "pledge");
+ } else {
+ if (pledge("stdio rpath wpath cpath dns
tty inet fattr",
+ NULL) == -1)
+ err(1, "pledge");
+ }
}
rval = auto_fetch(argc, argv, outfile);
Index: usr.bin/ftp/fetch.c
===
RCS file: /cvs/src/usr.bin/ftp/fetch.c,v
retrieving revision 1.194
diff -u -p -u -p -r1.194 fetch.c
--- usr.bin/ftp/fetch.c 22 Feb 2020 01:00:07 - 1.194
+++ usr.bin/ftp/fetch.c 3 May 2020 18:18:32 -
@@ -1601,6 +1601,32 @@ hextochar(const char *str)
return ret;
}
+#ifndef SMALL
+int
+is_interactive_url(const char *p)
+{
+ size_t urllen;
+ const char *path;
+ if (strncasecmp(p, HTTP_URL, sizeof(HTTP_URL) - 1) == 0 ||
+#ifndef NOSSL
+ strncasecmp(p, HTTPS_URL, sizeof(HTTPS_URL) - 1) == 0 ||
+#endif /* !NOSSL */
+ strncasecmp(p, FILE_URL, sizeof(FILE_URL) - 1) == 0)
+ return (0);
+ if ((urllen = strlen(p)) == 0)
+ errx(1, "empty URL not allowed");
+ if (p[urllen - 1] == '/')
+ return (1);
+ if (strncasecmp(p, FTP_URL, sizeof(FTP_URL) - 1) != 0)
+ return (0);
+ p += sizeof(FTP_URL) - 1;
+ urllen -= sizeof(FTP_URL) - 1;
+ if (urllen == 0)
+ errx(1, "empty URL not allowed");
+ return strchr(p, '/') == NULL;
+}
+#endif
+
int
isurl(const char *p)
{
Index: usr.bin/ftp/extern.h
===
RCS file: /cvs/src/usr.bin/ftp/extern.h,v
retrieving revision 1.51
diff -u -p -u -p -r1.51 extern.h
--- usr.bin/ftp/extern.h16 May 2019 12:44:17 - 1.51
+++ usr.bin/ftp/extern.h3 May 2020 18:18:32 -
@@ -88,6 +88,7 @@ char *hookup(char *, char *);
intinitconn(void);
void intr(void);
intisurl(const char *);
+intis_interactive_url(const char *);
intftp_login(const char *, char *, char *);
void lostpeer(void);
void makeargv(void);
Re: Tighter pledges for ftp(1)
On 2020-05-03 12:18, Theo de Raadt wrote: > Thanks Stuart. > > > The lesson is clear. No pledge/unveil work unless you test *ALL PROGRAM > BEHAVIOURS*. Doing less than the full testing is ... uhm, egotistical. Sorry about that. Hopefully my next version will fix it. > And it is completely normal that as the pledges and unveils harden, > the amount of test cases to discover exceeds expectation. Indeed. Sincerely, Demi signature.asc Description: OpenPGP digital signature
Re: Tighter pledges for ftp(1)
On 2020-05-03 12:13, Stuart Henderson wrote: > On 2020/05/02 20:19, Demi M. Obenour wrote: >> The following patch tightens the pledges for ftp(1). >> >> This guarantees that ftp(1) cannot spawn child processes when operating >> in batch mode, which is a significant security win. > > It breaks interactive mode (!ls, more somefile, get somefile "|rot13"), > something is wrong with how you decide that exec is needed. > > Also it complicates the code for the SMALL version used on the ramdisk > (and maybe makes the pledge weaker too, the code is no longer easy to > follow so I didn't work out for sure) The ramdisk version should be fine. The variable `needs_exec` is initialized to 0, and it is never assigned to in SMALL mode, so the stronger pledges are used. Sincerely, Demi signature.asc Description: OpenPGP digital signature
Tighter pledges for ftp(1)
The following patch tightens the pledges for ftp(1).
This guarantees that ftp(1) cannot spawn child processes when operating
in batch mode, which is a significant security win.
Index: usr.bin/ftp/main.c
===
RCS file: /cvs/src/usr.bin/ftp/main.c,v
retrieving revision 1.131
diff -u -p -u -p -r1.131 main.c
--- usr.bin/ftp/main.c 11 Feb 2020 18:41:39 - 1.131
+++ usr.bin/ftp/main.c 3 May 2020 00:15:28 -
@@ -483,27 +483,42 @@ main(volatile int argc, char *argv[])
(void)signal(SIGWINCH, setttywidth);
if (argc > 0) {
+ char **arg;
+ int needs_exec = 0;
if (isurl(argv[0])) {
- if (pipeout) {
#ifndef SMALL
- if (pledge("stdio rpath dns tty inet proc exec
fattr",
- NULL) == -1)
- err(1, "pledge");
-#else
- if (pledge("stdio rpath dns tty inet fattr",
- NULL) == -1)
- err(1, "pledge");
+ for (arg = argv; *arg; ++arg) {
+ size_t arglen;
+ const char *const url = *arg;
+ if ((arglen = strlen(url)) == 0)
+ errx(1, "empty URL not allowed");
+ else if (strncasecmp("http:/", url, 6) &&
+ strncasecmp("https:/", url, 7) &&
+ strncasecmp("file:", url, 5) &&
+ url[arglen] == '/')
+ needs_exec = 1;
+ }
#endif
+ if (pipeout) {
+ if (needs_exec) {
+ if (pledge("stdio rpath dns tty inet
proc exec fattr",
+ NULL) == -1)
+ err(1, "pledge");
+ } else {
+ if (pledge("stdio rpath dns tty inet
fattr",
+ NULL) == -1)
+ err(1, "pledge");
+ }
} else {
-#ifndef SMALL
- if (pledge("stdio rpath wpath cpath dns tty
inet proc exec fattr",
- NULL) == -1)
- err(1, "pledge");
-#else
- if (pledge("stdio rpath wpath cpath dns tty
inet fattr",
- NULL) == -1)
- err(1, "pledge");
-#endif
+ if (needs_exec) {
+ if (pledge("stdio rpath wpath cpath dns
tty inet proc exec fattr",
+ NULL) == -1)
+ err(1, "pledge");
+ } else {
+ if (pledge("stdio rpath wpath cpath dns
tty inet fattr",
+ NULL) == -1)
+ err(1, "pledge");
+ }
}
rval = auto_fetch(argc, argv, outfile);
Re: Tighter pledges for ftp(1)
On 2020-05-02 17:04, Hiltjo Posthuma wrote:
> On Sat, May 02, 2020 at 04:48:38PM -0400, Qubes privileged user wrote:
>> The following patch tightens the pledges for ftp(1).
>>
>> This provides some additional guarantees, including that ftp(1) cannot
>> spawn child processes. This is a significant security win for
>> sysupgrade(8).
>>
>> I hope I did not mess up the diff - this is my first time submitting one.
>>
>> Index: usr.bin/ftp/main.c
>> ===
>> RCS file: /cvs/src/usr.bin/ftp/main.c,v
>> retrieving revision 1.131
>> diff -u -p -u -r1.131 main.c
>> --- usr.bin/ftp/main.c 11 Feb 2020 18:41:39 - 1.131
>> +++ usr.bin/ftp/main.c 2 May 2020 19:31:40 -
>> @@ -483,27 +483,39 @@ main(volatile int argc, char *argv[])
>> (void)signal(SIGWINCH, setttywidth);
>>
>> if (argc > 0) {
>> +int i, needs_exec = 0;
>> if (isurl(argv[0])) {
>> -if (pipeout) {
>> #ifndef SMALL
>> -if (pledge("stdio rpath dns tty inet proc exec
>> fattr",
>> -NULL) == -1)
>> -err(1, "pledge");
>> -#else
>> -if (pledge("stdio rpath dns tty inet fattr",
>> -NULL) == -1)
>> -err(1, "pledge");
>> +for (i = 0; i < argc; ++i) {
>> +if (*(argv[i]) == '\0')
>> +errx(1, "empty URL not allowed");
>> +else if (strncasecmp("http:/", argv[i], 6) &&
>> +strncasecmp("https:/", argv[i], 7) &&
>> +strncasecmp("file:", argv[i], 5) &&
>> +argv[i][strlen(argv[i])] == '/')
>
> This should be strlen(...) - 1 right?
Indeed it should, thanks! Should I submit a new patch?
Sincerely,
Demi
OpenSSH-based socket forwarder
I have found myself using OpenSSH for its forwarding abilities, without actually using the remote shell feature. In these cases, the connection itself is over Xen shared memory, so I have no need for any of the cryptography. While allowing unencrypted SSH connections is obviously a bad idea, I would be very interested in adding support for using SSH as a pure forwarder, to allow forwarding sockets and X11 over an already-established, secure channel. While this is probably possible with libssh, libssh2, or other libraries, OpenSSH’s excellent security track-record makes it preferred here. I suggest that ssh(1) and sshd(8) act as the client and server of this protocol if invoked as forward-client(1) and forward-server(1), respectively. The protocol would be spoken over stdin/stdout. Would there be any interest in this from the OpenSSH maintainers? I have limited time, but would be willing to test patches. Sincerely, Demi
