[systemd-devel] [PATCH 03/11] sd-icmp6-nd: Update Router Advertisement handling

2015-01-13 Thread Patrik Flykt 
As the IPv6 prefixes are needed, update the ICMPv6 Router Advertisement
code to dynamically allocate a suitably sized buffer. Iterate through
the ICMPv6 options one by one returning error if the option length is
too big to fit the buffer.
---
 src/libsystemd-network/sd-icmp6-nd.c | 75 +++-
 1 file changed, 65 insertions(+), 10 deletions(-)

diff --git a/src/libsystemd-network/sd-icmp6-nd.c 
b/src/libsystemd-network/sd-icmp6-nd.c
index fbaf093..c9b390e 100644
--- a/src/libsystemd-network/sd-icmp6-nd.c
+++ b/src/libsystemd-network/sd-icmp6-nd.c
@@ -18,9 +18,11 @@
 ***/
 
 #include netinet/icmp6.h
+#include netinet/ip6.h
 #include string.h
 #include stdbool.h
 #include netinet/in.h
+#include sys/ioctl.h
 
 #include socket-util.h
 #include refcnt.h
@@ -38,6 +40,10 @@ enum icmp6_nd_state {
 ICMP6_ROUTER_ADVERTISMENT_LISTEN= 11,
 };
 
+#define IP6_MIN_MTU 1280
+#define ICMP6_ND_RECV_SIZE (IP6_MIN_MTU - sizeof(struct ip6_hdr))
+#define ICMP6_OPT_LEN_UNITS 8
+
 struct sd_icmp6_nd {
 RefCount n_ref;
 
@@ -179,45 +185,94 @@ int sd_icmp6_nd_new(sd_icmp6_nd **ret) {
 return 0;
 }
 
+static int icmp6_ra_parse(sd_icmp6_nd *nd, struct nd_router_advert *ra,
+ssize_t len) {
+void *opt;
+struct nd_opt_hdr *opt_hdr;
+
+assert_return(nd, -EINVAL);
+assert_return(ra, -EINVAL);
+
+len -= sizeof(*ra);
+if (len  ICMP6_OPT_LEN_UNITS)
+return 0;
+
+opt = ra + 1;
+opt_hdr = opt;
+
+while (len  len = opt_hdr-nd_opt_len * ICMP6_OPT_LEN_UNITS) {
+
+if (!opt_hdr-nd_opt_len)
+return -ENOMSG;
+
+switch (opt_hdr-nd_opt_type) {
+
+}
+
+len -= opt_hdr-nd_opt_len * ICMP6_OPT_LEN_UNITS;
+opt = (void *)((char *)opt +
+opt_hdr-nd_opt_len * ICMP6_OPT_LEN_UNITS);
+opt_hdr = opt;
+}
+
+return 0;
+}
+
 static int icmp6_router_advertisment_recv(sd_event_source *s, int fd,
   uint32_t revents, void *userdata)
 {
 sd_icmp6_nd *nd = userdata;
+int r, buflen;
 ssize_t len;
-struct nd_router_advert ra;
+_cleanup_free_ struct nd_router_advert *ra = NULL;
 int event = ICMP6_EVENT_ROUTER_ADVERTISMENT_NONE;
 
 assert(s);
 assert(nd);
 assert(nd-event);
 
-/* only interested in Managed/Other flag */
-len = read(fd, ra, sizeof(ra));
-if ((size_t)len  sizeof(ra))
+r = ioctl(fd, FIONREAD, buflen);
+if (r  0 || buflen = 0)
+buflen = ICMP6_ND_RECV_SIZE;
+
+ra = malloc0(buflen);
+if (!ra)
+return -ENOMEM;
+
+len = read(fd, ra, buflen);
+if (len  0) {
+log_icmp6_nd(nd, Could not receive message from UDP socket: 
%m);
 return 0;
+}
 
-if (ra.nd_ra_type != ND_ROUTER_ADVERT)
+if (ra-nd_ra_type != ND_ROUTER_ADVERT)
 return 0;
 
-if (ra.nd_ra_code != 0)
+if (ra-nd_ra_code != 0)
 return 0;
 
 nd-timeout = sd_event_source_unref(nd-timeout);
 
 nd-state = ICMP6_ROUTER_ADVERTISMENT_LISTEN;
 
-if (ra.nd_ra_flags_reserved  ND_RA_FLAG_OTHER )
+if (ra-nd_ra_flags_reserved  ND_RA_FLAG_OTHER )
 event = ICMP6_EVENT_ROUTER_ADVERTISMENT_OTHER;
 
-if (ra.nd_ra_flags_reserved  ND_RA_FLAG_MANAGED)
+if (ra-nd_ra_flags_reserved  ND_RA_FLAG_MANAGED)
 event = ICMP6_EVENT_ROUTER_ADVERTISMENT_MANAGED;
 
 log_icmp6_nd(nd, Received Router Advertisement flags %s/%s,
- (ra.nd_ra_flags_reserved  ND_RA_FLAG_MANAGED)? MANAGED:
+ (ra-nd_ra_flags_reserved  ND_RA_FLAG_MANAGED)? 
MANAGED:
  none,
- (ra.nd_ra_flags_reserved  ND_RA_FLAG_OTHER)? OTHER:
+ (ra-nd_ra_flags_reserved  ND_RA_FLAG_OTHER)? OTHER:
  none);
 
+if (event != ICMP6_EVENT_ROUTER_ADVERTISMENT_NONE) {
+r = icmp6_ra_parse(nd, ra, len);
+if (r  0)
+return 0;
+}
+
 icmp6_nd_notify(nd, event);
 
 return 0;
-- 
2.1.4

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH 03/11] sd-icmp6-nd: Update Router Advertisement handling

2015-01-13 Thread Zbigniew Jędrzejewski-Szmek 
On Tue, Jan 13, 2015 at 02:02:13PM +0200, Patrik Flykt wrote:
 As the IPv6 prefixes are needed, update the ICMPv6 Router Advertisement
 code to dynamically allocate a suitably sized buffer. Iterate through
 the ICMPv6 options one by one returning error if the option length is
 too big to fit the buffer.
 ---
  src/libsystemd-network/sd-icmp6-nd.c | 75 
 +++-
  1 file changed, 65 insertions(+), 10 deletions(-)
 
 diff --git a/src/libsystemd-network/sd-icmp6-nd.c 
 b/src/libsystemd-network/sd-icmp6-nd.c
 index fbaf093..c9b390e 100644
 --- a/src/libsystemd-network/sd-icmp6-nd.c
 +++ b/src/libsystemd-network/sd-icmp6-nd.c
 @@ -18,9 +18,11 @@
  ***/
  
  #include netinet/icmp6.h
 +#include netinet/ip6.h
  #include string.h
  #include stdbool.h
  #include netinet/in.h
 +#include sys/ioctl.h
  
  #include socket-util.h
  #include refcnt.h
 @@ -38,6 +40,10 @@ enum icmp6_nd_state {
  ICMP6_ROUTER_ADVERTISMENT_LISTEN= 11,
  };
  
 +#define IP6_MIN_MTU 1280
 +#define ICMP6_ND_RECV_SIZE (IP6_MIN_MTU - sizeof(struct ip6_hdr))
 +#define ICMP6_OPT_LEN_UNITS 8
 +
  struct sd_icmp6_nd {
  RefCount n_ref;
  
 @@ -179,45 +185,94 @@ int sd_icmp6_nd_new(sd_icmp6_nd **ret) {
  return 0;
  }
  
 +static int icmp6_ra_parse(sd_icmp6_nd *nd, struct nd_router_advert *ra,
 +ssize_t len) {
 +void *opt;
 +struct nd_opt_hdr *opt_hdr;
 +
 +assert_return(nd, -EINVAL);
 +assert_return(ra, -EINVAL);
 +
 +len -= sizeof(*ra);
 +if (len  ICMP6_OPT_LEN_UNITS)
 +return 0;
 +
 +opt = ra + 1;
 +opt_hdr = opt;
 +
 +while (len  len = opt_hdr-nd_opt_len * ICMP6_OPT_LEN_UNITS) {
Isn't the first part of the check implied by the second?

Also, if len is ever  0, would this imply that a buffer overlow happened?
Maybe assert(len = 0);

 +
 +if (!opt_hdr-nd_opt_len)
 +return -ENOMSG;
We usually use == 0 for comparison of integers to 0. Makes it easier to
distinguish from booleans.

 +switch (opt_hdr-nd_opt_type) {
 +
 +}
 +
 +len -= opt_hdr-nd_opt_len * ICMP6_OPT_LEN_UNITS;
 +opt = (void *)((char *)opt +
 +opt_hdr-nd_opt_len * ICMP6_OPT_LEN_UNITS);
 +opt_hdr = opt;
 +}
Maybe add a check for trailing garbage and log_warning() or lower priority?

 +
 +return 0;
 +}
 +
  static int icmp6_router_advertisment_recv(sd_event_source *s, int fd,
uint32_t revents, void *userdata)
  {
  sd_icmp6_nd *nd = userdata;
 +int r, buflen;
  ssize_t len;
 -struct nd_router_advert ra;
 +_cleanup_free_ struct nd_router_advert *ra = NULL;
  int event = ICMP6_EVENT_ROUTER_ADVERTISMENT_NONE;
  
  assert(s);
  assert(nd);
  assert(nd-event);
  
 -/* only interested in Managed/Other flag */
 -len = read(fd, ra, sizeof(ra));
 -if ((size_t)len  sizeof(ra))
 +r = ioctl(fd, FIONREAD, buflen);
 +if (r  0 || buflen = 0)
 +buflen = ICMP6_ND_RECV_SIZE;
buflen might be used unitialized here.

 +
 +ra = malloc0(buflen);
Normal malloc should suffice.

 +if (!ra)
 +return -ENOMEM;
 +
 +len = read(fd, ra, buflen);
 +if (len  0) {
 +log_icmp6_nd(nd, Could not receive message from UDP socket: 
 %m);
  return 0;
 +}
  
 -if (ra.nd_ra_type != ND_ROUTER_ADVERT)
 +if (ra-nd_ra_type != ND_ROUTER_ADVERT)
  return 0;
  
 -if (ra.nd_ra_code != 0)
 +if (ra-nd_ra_code != 0)
  return 0;
  
  nd-timeout = sd_event_source_unref(nd-timeout);
  
  nd-state = ICMP6_ROUTER_ADVERTISMENT_LISTEN;
  
 -if (ra.nd_ra_flags_reserved  ND_RA_FLAG_OTHER )
 +if (ra-nd_ra_flags_reserved  ND_RA_FLAG_OTHER )
  event = ICMP6_EVENT_ROUTER_ADVERTISMENT_OTHER;
  
 -if (ra.nd_ra_flags_reserved  ND_RA_FLAG_MANAGED)
 +if (ra-nd_ra_flags_reserved  ND_RA_FLAG_MANAGED)
  event = ICMP6_EVENT_ROUTER_ADVERTISMENT_MANAGED;
  
  log_icmp6_nd(nd, Received Router Advertisement flags %s/%s,
 - (ra.nd_ra_flags_reserved  ND_RA_FLAG_MANAGED)? 
 MANAGED:
 + (ra-nd_ra_flags_reserved  ND_RA_FLAG_MANAGED)? 
 MANAGED:
   none,
Maybe remove the parentheses, and forgo line-wrapping. This sould be clearer 
that way.

 - (ra.nd_ra_flags_reserved  ND_RA_FLAG_OTHER)? OTHER:
 + (ra-nd_ra_flags_reserved  ND_RA_FLAG_OTHER)? OTHER:
   none);
  
 +if (event != ICMP6_EVENT_ROUTER_ADVERTISMENT_NONE) {
 +r = icmp6_ra_parse(nd, ra, len);
 +if (r  0)
 +