On Thu, Aug 29, 2019 at 05:31:04PM +0200, Claudio Jeker wrote:
> I don't think this is the right way to do this. The consumer of rtinfo
> need to check the values based on their needs. Ideally we add some helpers
> to make that easier. I think it is close to impossible to properly
> validate the sockaddrs in rtm_xaddrs() since that function is missing
> needed context.
For me it is not perfectly clear at which places the routing code
deals with user input. So I think it is better to do all the size
checks when the user data enters the kernel. Placing the checks
at multiple places would also mean the we need error handling
everywhere.
So my plan is to check the sizes in a general way at the beginning.
If the specific code looks at the address family, it knows that the
struct is complete; e.g. MPLS igores non AF_MPLS adresses. It
would be easier to add sa_af checks here and there than to care
about sa_len.
A problem is that user land sometimes provides only partial addresses.
RTAX_NETMASK is such an example, so I do not check it here. Route
labels do not have an address family, I only depend on RTAX_LABEL.
Perhaps we can figure out where userland actually provides partial
addresses and add more errors in this switch. I consider this diff
only a start. Ports may send even more crap than I found in our
tools already.
So I think a central size check is good, more specific code can add
more checks if needed.
New diff with more comments. RTAX_GATEWAY may be AF_LINK, so also
check that. RTAX_IFP from userland is very broken, so I have a
very weak check. We should make it stricter when fixes have been
deployed.
This change needs a lot of testing.
bluhm
Index: net/rtsock.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/net/rtsock.c,v
retrieving revision 1.291
diff -u -p -r1.291 rtsock.c
--- net/rtsock.c 28 Aug 2019 22:36:41 -0000 1.291
+++ net/rtsock.c 29 Aug 2019 23:52:53 -0000
@@ -1350,6 +1350,11 @@ rtm_xaddrs(caddr_t cp, caddr_t cplim, st
struct sockaddr *sa;
int i;
+ /*
+ * Parse address bits, split address storage in chunks, and
+ * set info pointers. Use sa_len for traversing the memory
+ * and check that we stay within in the limit.
+ */
bzero(rtinfo->rti_info, sizeof(rtinfo->rti_info));
for (i = 0; i < sizeof(rtinfo->rti_addrs) * 8; i++) {
if ((rtinfo->rti_addrs & (1 << i)) == 0)
@@ -1361,6 +1366,104 @@ rtm_xaddrs(caddr_t cp, caddr_t cplim, st
return (EINVAL);
rtinfo->rti_info[i] = sa;
ADVANCE(cp, sa);
+ }
+ /*
+ * Check that the address family is suitable for the route address
+ * type. Check that each address has a size that fits its family
+ * and its length is within the size. Strings within addresses must
+ * be NUL terminated.
+ */
+ for (i = 0; i < RTAX_MAX; i++) {
+ size_t len, maxlen, size;
+
+ sa = rtinfo->rti_info[i];
+ if (sa == NULL)
+ continue;
+ maxlen = size = 0;
+ switch (i) {
+ case RTAX_DST:
+ case RTAX_GATEWAY:
+ case RTAX_SRC:
+ switch (sa->sa_family) {
+ case AF_INET:
+ size = sizeof(struct sockaddr_in);
+ break;
+ case AF_LINK:
+ size = sizeof(struct sockaddr_dl);
+ break;
+#ifdef INET6
+ case AF_INET6:
+ size = sizeof(struct sockaddr_in6);
+ break;
+#endif
+#ifdef MPLS
+ case AF_MPLS:
+ size = sizeof(struct sockaddr_mpls);
+ break;
+#endif
+ }
+ break;
+ case RTAX_IFP:
+ if (sa->sa_family != AF_LINK)
+ return (EAFNOSUPPORT);
+ /*
+ * 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;
+ case RTAX_IFA:
+ switch (sa->sa_family) {
+ case AF_INET:
+ size = sizeof(struct sockaddr_in);
+ break;
+#ifdef INET6
+ case AF_INET6:
+ size = sizeof(struct sockaddr_in6);
+ break;
+#endif
+ default:
+ return (EAFNOSUPPORT);
+ }
+ break;
+ case RTAX_LABEL:
+ maxlen = RTLABEL_LEN;
+ size = sizeof(struct sockaddr_rtlabel);
+ break;
+#ifdef BFD
+ case RTAX_BFD:
+ size = sizeof(struct sockaddr_bfd);
+ break;
+#endif
+ case RTAX_DNS:
+ maxlen = RTDNS_LEN;
+ size = sizeof(struct sockaddr_rtdns);
+ break;
+ case RTAX_STATIC:
+ maxlen = RTSTATIC_LEN;
+ size = sizeof(struct sockaddr_rtstatic);
+ break;
+ case RTAX_SEARCH:
+ maxlen = RTSEARCH_LEN;
+ size = sizeof(struct sockaddr_rtsearch);
+ break;
+ }
+ if (size) {
+ /* memory for the full struct must not be provided */
+ if (sa->sa_len < size)
+ return (EINVAL);
+ }
+ if (maxlen) {
+ /* this should not happen */
+ if (2 + maxlen > size)
+ return (EINVAL);
+ /* strings must be NUL terminated within the struct */
+ len = strnlen(sa->sa_data, maxlen);
+ if (len >= maxlen || 2 + len >= sa->sa_len)
+ return (EINVAL);
+ break;
+ }
}
return (0);
}
