From 751ca3f1de316ca79b60001334dbdf54077e1d01 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sun, 18 Jun 2017 15:53:15 -0400 Subject: [PATCH 1/4] test-resolved-packet: add a simple test for our allocation functions --- .gitignore | 1 + Makefile.am | 14 ++++++++++ src/resolve/meson.build | 9 ++++++ src/resolve/test-resolved-packet.c | 45 ++++++++++++++++++++++++++++++ 4 files changed, 69 insertions(+) create mode 100644 src/resolve/test-resolved-packet.c diff --git a/.gitignore b/.gitignore index 60eda2b8ce..bc47db6481 100644 --- a/.gitignore +++ b/.gitignore @@ -271,6 +271,7 @@ /test-replace-var /test-resolve /test-resolve-tables +/test-resolved-packet /test-ring /test-rlimit-util /test-sched-prio diff --git a/Makefile.am b/Makefile.am index c61f371a8e..a77495bf03 100644 --- a/Makefile.am +++ b/Makefile.am @@ -5758,6 +5758,7 @@ dist_zshcompletion_data += \ tests += \ test-dns-packet \ test-resolve-tables \ + test-resolved-packet \ test-dnssec manual_tests += \ @@ -5779,6 +5780,19 @@ test_resolve_tables_LDADD = \ $(GCRYPT_LIBS) \ -lm +test_resolved_packet_SOURCES = \ + src/resolve/test-resolved-packet.c \ + $(basic_dns_sources) + +test_resolved_packet_CFLAGS = \ + $(AM_CFLAGS) \ + $(GCRYPT_CFLAGS) + +test_resolved_packet_LDADD = \ + libsystemd-shared.la \ + $(GCRYPT_LIBS) \ + -lm + test_dns_packet_SOURCES = \ src/resolve/test-dns-packet.c \ $(basic_dns_sources) diff --git a/src/resolve/meson.build b/src/resolve/meson.build index f3c411ffee..fe228784fa 100644 --- a/src/resolve/meson.build +++ b/src/resolve/meson.build @@ -160,6 +160,15 @@ tests += [ libm], 'ENABLE_RESOLVED'], + [['src/resolve/test-resolved-packet.c', + basic_dns_sources, + dns_type_headers], + [], + [libgcrypt, + libgpg_error, + libm], + 'ENABLE_RESOLVED'], + [['src/resolve/test-dnssec.c', basic_dns_sources, dns_type_headers], diff --git a/src/resolve/test-resolved-packet.c b/src/resolve/test-resolved-packet.c new file mode 100644 index 0000000000..8b7da1408d --- /dev/null +++ b/src/resolve/test-resolved-packet.c @@ -0,0 +1,45 @@ +/*** + This file is part of systemd + + Copyright 2017 Zbigniew Jędrzejewski-Szmek + + systemd is free software; you can redistribute it and/or modify it + under the terms of the GNU Lesser General Public License as published by + the Free Software Foundation; either version 2.1 of the License, or + (at your option) any later version. + + systemd is distributed in the hope that it will be useful, but + WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public License + along with systemd; If not, see . +***/ + +#include "log.h" +#include "resolved-dns-packet.h" + +static void test_dns_packet_new(void) { + size_t i; + + for (i = 0; i < DNS_PACKET_SIZE_MAX + 2; i++) { + _cleanup_(dns_packet_unrefp) DnsPacket *p = NULL; + + assert_se(dns_packet_new(&p, DNS_PROTOCOL_DNS, i) == 0); + + log_debug("dns_packet_new: %zu → %zu", i, p->allocated); + assert_se(p->allocated >= MIN(DNS_PACKET_SIZE_MAX, i)); + } +} + +int main(int argc, char **argv) { + + log_set_max_level(LOG_DEBUG); + log_parse_environment(); + log_open(); + + test_dns_packet_new(); + + return 0; +} From db848813bae4d28c524b3b6a7dad135e426659ce 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 2/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 240ee448f4..821b66e266 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 2c92392e4d..3abcaf8cf3 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; From 88795538726a5bbfd9efc13d441cb05e1d7fc139 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 3/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 821b66e266..d1f0f760a4 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)); From 64a21fdaca7c93f1c30b21f6fdbd2261798b161a 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 4/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 d1f0f760a4..a486216d68 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 3abcaf8cf3..5dff272fd9 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;