From 29bb43cc46412366fc939c66331a916de07bfac4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sun, 18 Jun 2017 16:07:57 -0400 Subject: [PATCH 1/4] resolved: simplify alloc size calculation The allocation size was calculated in a complicated way, and for values close to the page size we would actually allocate less than requested. Reported by Chris Coulson . CVE-2017-9445 --- src/resolve/resolved-dns-packet.c | 8 +------- src/resolve/resolved-dns-packet.h | 2 -- 2 files changed, 1 insertion(+), 9 deletions(-) diff --git a/src/resolve/resolved-dns-packet.c b/src/resolve/resolved-dns-packet.c index 652970284..2034e3c8c 100644 --- a/src/resolve/resolved-dns-packet.c +++ b/src/resolve/resolved-dns-packet.c @@ -47,13 +47,7 @@ int dns_packet_new(DnsPacket **ret, DnsProtocol protocol, size_t mtu) { assert(ret); - if (mtu <= UDP_PACKET_HEADER_SIZE) - a = DNS_PACKET_SIZE_START; - else - a = mtu - UDP_PACKET_HEADER_SIZE; - - if (a < DNS_PACKET_HEADER_SIZE) - a = DNS_PACKET_HEADER_SIZE; + a = MAX(mtu, DNS_PACKET_HEADER_SIZE); /* round up to next page size */ a = PAGE_ALIGN(ALIGN(sizeof(DnsPacket)) + a) - ALIGN(sizeof(DnsPacket)); diff --git a/src/resolve/resolved-dns-packet.h b/src/resolve/resolved-dns-packet.h index 2c92392e4..3abcaf8cf 100644 --- a/src/resolve/resolved-dns-packet.h +++ b/src/resolve/resolved-dns-packet.h @@ -66,8 +66,6 @@ struct DnsPacketHeader { /* With EDNS0 we can use larger packets, default to 4096, which is what is commonly used */ #define DNS_PACKET_UNICAST_SIZE_LARGE_MAX 4096 -#define DNS_PACKET_SIZE_START 512 - struct DnsPacket { int n_ref; DnsProtocol protocol; -- 2.13.1 From cd3d8a7ebc01cd6913eaa9a591f7d606038a7588 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 27 Jun 2017 14:20:00 -0400 Subject: [PATCH 2/4] resolved: do not allocate packets with minimum size dns_packet_new() is sometimes called with mtu == 0, and in that case we should allocate more than the absolute minimum (which is the dns packet header size), otherwise we have to resize immediately again after appending the first data to the packet. This partially reverts the previous commit. --- src/resolve/resolved-dns-packet.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/resolve/resolved-dns-packet.c b/src/resolve/resolved-dns-packet.c index 2034e3c8c..9d806ab33 100644 --- a/src/resolve/resolved-dns-packet.c +++ b/src/resolve/resolved-dns-packet.c @@ -28,6 +28,9 @@ #define EDNS0_OPT_DO (1<<15) +#define DNS_PACKET_SIZE_START 512 +assert_cc(DNS_PACKET_SIZE_START > UDP_PACKET_HEADER_SIZE) + typedef struct DnsPacketRewinder { DnsPacket *packet; size_t saved_rindex; @@ -47,7 +50,14 @@ int dns_packet_new(DnsPacket **ret, DnsProtocol protocol, size_t mtu) { assert(ret); - a = MAX(mtu, DNS_PACKET_HEADER_SIZE); + /* When dns_packet_new() is called with mtu == 0, allocate more than the + * absolute minimum (which is the dns packet header size), to avoid + * resizing immediately again after appending the first data to the packet. + */ + if (mtu < UDP_PACKET_HEADER_SIZE) + a = DNS_PACKET_SIZE_START; + else + a = MAX(mtu, DNS_PACKET_HEADER_SIZE); /* round up to next page size */ a = PAGE_ALIGN(ALIGN(sizeof(DnsPacket)) + a) - ALIGN(sizeof(DnsPacket)); -- 2.13.1 From a03fc1acd66d23e239f2545e9a6887c7d0aad7c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 27 Jun 2017 16:59:06 -0400 Subject: [PATCH 3/4] resolved: define various packet sizes as unsigned This seems like the right thing to do, and apparently at least some compilers warn about signed/unsigned comparisons with DNS_PACKET_SIZE_MAX. --- src/resolve/resolved-dns-packet.c | 2 +- src/resolve/resolved-dns-packet.h | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/resolve/resolved-dns-packet.c b/src/resolve/resolved-dns-packet.c index 9d806ab33..e2285b440 100644 --- a/src/resolve/resolved-dns-packet.c +++ b/src/resolve/resolved-dns-packet.c @@ -28,7 +28,7 @@ #define EDNS0_OPT_DO (1<<15) -#define DNS_PACKET_SIZE_START 512 +#define DNS_PACKET_SIZE_START 512u assert_cc(DNS_PACKET_SIZE_START > UDP_PACKET_HEADER_SIZE) typedef struct DnsPacketRewinder { diff --git a/src/resolve/resolved-dns-packet.h b/src/resolve/resolved-dns-packet.h index 3abcaf8cf..5dff272fd 100644 --- a/src/resolve/resolved-dns-packet.h +++ b/src/resolve/resolved-dns-packet.h @@ -58,13 +58,13 @@ struct DnsPacketHeader { /* The various DNS protocols deviate in how large a packet can grow, but the TCP transport has a 16bit size field, hence that appears to be the absolute maximum. */ -#define DNS_PACKET_SIZE_MAX 0xFFFF +#define DNS_PACKET_SIZE_MAX 0xFFFFu /* RFC 1035 say 512 is the maximum, for classic unicast DNS */ -#define DNS_PACKET_UNICAST_SIZE_MAX 512 +#define DNS_PACKET_UNICAST_SIZE_MAX 512u /* With EDNS0 we can use larger packets, default to 4096, which is what is commonly used */ -#define DNS_PACKET_UNICAST_SIZE_LARGE_MAX 4096 +#define DNS_PACKET_UNICAST_SIZE_LARGE_MAX 4096u struct DnsPacket { int n_ref; -- 2.13.1