[quagga-dev 10184] [PATCH] zebra: fix sockaddr_dl length assumptions (BZ#737)

David Lamparter equinox at opensourcerouting.org
Tue Jan 15 16:18:25 GMT 2013


Quagga makes bad assumptions about sockaddr_dl (on NetBSD, but possibly
on other systems as well).  Particularly, sizeof(struct sockaddr_dl)
returns a size that does not include the full sdl_data field, leading to
not enough data being copied.  This breaks IPv6 RAs in particular, as
a broken mac address from sockaddr_dl will be included in the packets.

From: Matthias-Christian Ott <ott at mirix.org>
Tested-by: Uwe Toenjes <6bone at 6bone.informatik.uni-leipzig.de>
[further simplified + more comments]
Signed-off-by: David Lamparter <equinox at opensourcerouting.org>
---

applying this version now.  the "we have no sdl_len" path is a bit unfortunate,
if we hit it on anything other than Solaris (with sdl_data[244]), it'll break.

-David

---
 configure.ac          |    1 +
 lib/if.h              |    8 +++++++-
 lib/zclient.c         |    2 +-
 zebra/kernel_socket.c |   15 ++++++++++++---
 zebra/zserv.c         |    2 +-
 5 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/configure.ac b/configure.ac
index b69c99d..a4d97bf 100755
--- a/configure.ac
+++ b/configure.ac
@@ -1464,6 +1464,7 @@ AC_CHECK_TYPES([struct sockaddr, struct sockaddr_in,
 AC_CHECK_MEMBERS([struct sockaddr.sa_len,
 	struct sockaddr_in.sin_len, struct sockaddr_un.sun_len,
 	struct sockaddr_in6.sin6_scope_id,
+	struct sockaddr_dl.sdl_len,
 	struct if6_aliasreq.ifra_lifetime,
 	struct nd_opt_adv_interval.nd_opt_ai_type],
 	[], [], QUAGGA_INCLUDES)
diff --git a/lib/if.h b/lib/if.h
index 841ce51..2116e12 100644
--- a/lib/if.h
+++ b/lib/if.h
@@ -103,7 +103,13 @@ struct interface
 
   /* Hardware address. */
 #ifdef HAVE_STRUCT_SOCKADDR_DL
-  struct sockaddr_dl sdl;
+  union {
+    /* note that sdl_storage is never accessed, it only exists to make space.
+     * all actual uses refer to sdl - but use sizeof(sdl_storage)!  this fits
+     * best with C aliasing rules. */
+    struct sockaddr_dl sdl;
+    struct sockaddr_storage sdl_storage;
+  };
 #else
   unsigned short hw_type;
   u_char hw_addr[INTERFACE_HWADDR_MAX];
diff --git a/lib/zclient.c b/lib/zclient.c
index 61c6f73..d316596 100644
--- a/lib/zclient.c
+++ b/lib/zclient.c
@@ -734,7 +734,7 @@ zebra_interface_if_set_value (struct stream *s, struct interface *ifp)
   ifp->mtu6 = stream_getl (s);
   ifp->bandwidth = stream_getl (s);
 #ifdef HAVE_STRUCT_SOCKADDR_DL
-  stream_get (&ifp->sdl, s, sizeof (ifp->sdl));
+  stream_get (&ifp->sdl, s, sizeof (ifp->sdl_storage));
 #else
   ifp->hw_addr_len = stream_getl (s);
   if (ifp->hw_addr_len)
diff --git a/zebra/kernel_socket.c b/zebra/kernel_socket.c
index cde36bd..73fabd4 100644
--- a/zebra/kernel_socket.c
+++ b/zebra/kernel_socket.c
@@ -477,13 +477,22 @@ ifm_read (struct if_msghdr *ifm)
 
       /*
        * XXX sockaddr_dl contents can be larger than the structure
-       * definition, so the user of the stored structure must be
-       * careful not to read off the end.
-       *
+       * definition.  There are 2 big families here:
+       *  - BSD has sdl_len + sdl_data[16] + overruns sdl_data
+       *    we MUST use sdl_len here or we'll truncate data.
+       *  - Solaris has no sdl_len, but sdl_data[244]
+       *    presumably, it's not going to run past that, so sizeof()
+       *    is fine here.
        * a nonzero ifnlen from RTA_NAME_GET() means sdl is valid
        */
       if (ifnlen)
+      {
+#ifdef HAVE_STRUCT_SOCKADDR_DL_SDL_LEN
+	memcpy (&ifp->sdl, sdl, sdl->sdl_len);
+#else
 	memcpy (&ifp->sdl, sdl, sizeof (struct sockaddr_dl));
+#endif /* HAVE_STRUCT_SOCKADDR_DL_SDL_LEN */
+      }
 
       if_add_update (ifp);
     }
diff --git a/zebra/zserv.c b/zebra/zserv.c
index 9e47f23..cb8dbcb 100644
--- a/zebra/zserv.c
+++ b/zebra/zserv.c
@@ -153,7 +153,7 @@ zserv_encode_interface (struct stream *s, struct interface *ifp)
   stream_putl (s, ifp->mtu6);
   stream_putl (s, ifp->bandwidth);
 #ifdef HAVE_STRUCT_SOCKADDR_DL
-  stream_put (s, &ifp->sdl, sizeof (ifp->sdl));
+  stream_put (s, &ifp->sdl, sizeof (ifp->sdl_storage));
 #else
   stream_putl (s, ifp->hw_addr_len);
   if (ifp->hw_addr_len)
-- 
1.7.8.6





More information about the Quagga-dev mailing list