zebra: fix sockaddr_dl length assumptions (BZ#737)

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@mirix.org>
Tested-by: Uwe Toenjes <6bone@6bone.informatik.uni-leipzig.de>
[further simplified + more comments]
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>

diff --git a/configure.ac b/configure.ac
index 92c831ab47b82a510de19500f80b5185e3c25d95..937c79ce69612f54576eec263c911fbaf93f2ac9 100755 (executable)
--- a/configure.ac
+++ b/configure.ac
@@ -1469,6 +1469,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 841ce51ec597c6b6107330dee5c59599c2462c11..2116e12ebe3ca727deb0a1c0a1c9243e93fe6d36 100644 (file)
--- 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 61c6f73003b92f08a7d10743229ba416fa6bc535..d3165962d070199aab176698e2d70721eb0c4271 100644 (file)
--- 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 cde36bd034447eeb6b683308dcc438b0cf2af42e..73fabd4c0622a8209b2d9c28e2c7563f626b7dc2 100644 (file)
--- 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 9e47f23fe826abaf329ae89414d928f724a590ee..cb8dbcb3a6daaa5275ed947a0737e4e5035f6cea 100644 (file)
--- 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)