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
> +#include
> #include
> #include
> #include
> +#include
>
> #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_A
