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
_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
