From cfd794827e3243c681590845063309fa628cfef4 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 19 Mar 2014 22:07:12 +0000 Subject: [PATCH] netlink: Set "unused" in ICMP header (SECURITY) Previously, the "unused" field in our ICMP messages was left uninitialised (!) This is a security problem, at least in principle, as the field would as a result contain bits of previous packets. In practice, the information leaked could be IP options, TCP ports and sequence numbers, or UDP ports, length and/or checksum, or similar information for other protocols, so the impact is limited. Set the field to 0. Also, make provision for netlink_icmp_simple's callers to be able to specify a different value, if desired. Signed-off-by: Ian Jackson --- debian/changelog | 1 + netlink.c | 23 ++++++++++++++--------- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/debian/changelog b/debian/changelog index 613963e..33b7c0b 100644 --- a/debian/changelog +++ b/debian/changelog @@ -7,6 +7,7 @@ secnet (0.3.1~~unstable) unstable; urgency=low * Fix formatting error in p-t-p startup message. * Additions to the test-example suite. * SECURITY: Fixes to MTU and fragmentation handling. + * SECURITY: Correctly set "unused" ICMP header field. -- diff --git a/netlink.c b/netlink.c index 61841b0..87b6671 100644 --- a/netlink.c +++ b/netlink.c @@ -210,7 +210,7 @@ struct icmphdr { uint8_t type; uint8_t code; uint16_t check; - union { + union icmpinfofield { uint32_t unused; struct { uint8_t pointer; @@ -224,6 +224,8 @@ struct icmphdr { } echo; } d; }; + +static const union icmpinfofield icmp_noinfo; static void netlink_packet_deliver(struct netlink *st, struct netlink_client *client, @@ -358,7 +360,8 @@ static uint16_t netlink_icmp_reply_len(struct buffer_if *buf) comes from. NULL indicates the host. */ static void netlink_icmp_simple(struct netlink *st, struct buffer_if *buf, struct netlink_client *client, - uint8_t type, uint8_t code) + uint8_t type, uint8_t code, + union icmpinfofield info) { struct icmphdr *h; uint16_t len; @@ -367,7 +370,7 @@ static void netlink_icmp_simple(struct netlink *st, struct buffer_if *buf, struct iphdr *iph=(struct iphdr *)buf->start; len=netlink_icmp_reply_len(buf); h=netlink_icmp_tmpl(st,ntohl(iph->saddr),len); - h->type=type; h->code=code; + h->type=type; h->code=code; h->d=info; memcpy(buf_append(&st->icmp,len),buf->start,len); netlink_icmp_csum(h); netlink_packet_deliver(st,NULL,&st->icmp); @@ -525,7 +528,7 @@ static void netlink_packet_deliver(struct netlink *st, "(s=%s, d=%s)\n", st->name, s, d); free(s); free(d); netlink_icmp_simple(st,buf,client,ICMP_TYPE_UNREACHABLE, - ICMP_CODE_NET_UNREACHABLE); + ICMP_CODE_NET_UNREACHABLE, icmp_noinfo); BUF_FREE(buf); } } else { @@ -542,7 +545,7 @@ static void netlink_packet_deliver(struct netlink *st, free(s); free(d); netlink_icmp_simple(st,buf,client,ICMP_TYPE_UNREACHABLE, - ICMP_CODE_NET_PROHIBITED); + ICMP_CODE_NET_PROHIBITED, icmp_noinfo); BUF_FREE(buf); } else { if (best_quality>0) { @@ -552,8 +555,10 @@ static void netlink_packet_deliver(struct netlink *st, BUF_ASSERT_FREE(buf); } else { /* Generate ICMP destination unreachable */ - netlink_icmp_simple(st,buf,client,ICMP_TYPE_UNREACHABLE, - ICMP_CODE_NET_UNREACHABLE); /* client==NULL */ + netlink_icmp_simple(st,buf,client,/* client==NULL */ + ICMP_TYPE_UNREACHABLE, + ICMP_CODE_NET_UNREACHABLE, + icmp_noinfo); BUF_FREE(buf); } } @@ -574,7 +579,7 @@ static void netlink_packet_forward(struct netlink *st, if (iph->ttl<=1) { /* Generate ICMP time exceeded */ netlink_icmp_simple(st,buf,client,ICMP_TYPE_TIME_EXCEEDED, - ICMP_CODE_TTL_EXCEEDED); + ICMP_CODE_TTL_EXCEEDED,icmp_noinfo); BUF_FREE(buf); return; } @@ -629,7 +634,7 @@ static void netlink_packet_local(struct netlink *st, } else { /* Send ICMP protocol unreachable */ netlink_icmp_simple(st,buf,client,ICMP_TYPE_UNREACHABLE, - ICMP_CODE_PROTOCOL_UNREACHABLE); + ICMP_CODE_PROTOCOL_UNREACHABLE,icmp_noinfo); BUF_FREE(buf); return; } -- 2.30.2