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

[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)
> +                        return 0;
> +        }
> +
>          icmp6_nd_notify(nd, event);
>  

Zbyszek