From abb4f0be03597e385524b6a81ec76531ee1fed62 Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Mon, 7 Dec 2020 13:14:17 -0800 Subject: [PATCH 1/2] Fix EDNS ECS option parsing bugs * The parsing of IPv6 addresses tried to fill a stack-buffer with as much data as supplied in the Option even if it was in excess of the desired prefix or maximum IPv6 address size. This could result in an overflow of that stack-buffer. * The parsing of IPv4 addresses would overwrite the storage used for that address as many times as there were bytes in the Option in excess of the desired prefix length or maximum IPv4 address size. This could cause the resulting IPv4 address to be derived from the incorrect data. * Upon encountering unexpected/excessive option-length or source-prefix parameters, the data pointer used for parsing was also not always advanced to the start of the next alleged option's data. Assuming all other parsing code correctly guards against invalid input, there's no further harm from that other than the subsequent parsing being more likely to encounter unexpected values and emitting more Weirds. Credit to OSS-Fuzz for discovery https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28336 (Link to details becomes public 30 days after patch release) --- src/analyzer/protocol/dns/DNS.cc | 58 +++++++++++++++++++++++++++++--- 1 file changed, 54 insertions(+), 4 deletions(-) diff --git a/src/analyzer/protocol/dns/DNS.cc b/src/analyzer/protocol/dns/DNS.cc index d8c81f8571..93bb855917 100644 --- a/src/analyzer/protocol/dns/DNS.cc +++ b/src/analyzer/protocol/dns/DNS.cc @@ -742,9 +742,12 @@ bool DNS_Interpreter::ParseRR_EDNS(detail::DNS_MsgInfo* msg, case detail::TYPE_ECS: { // must be 4 bytes + variable number of octets for address - if ( option_len <= 4 ) { + if ( option_len <= 4 ) + { + analyzer->Weird("EDNS_ECS_invalid_option_len"); + data += option_len; break; - } + } detail::EDNS_ECS opt{}; uint16_t ecs_family = ExtractShort(data, option_len); @@ -758,27 +761,73 @@ bool DNS_Interpreter::ParseRR_EDNS(detail::DNS_MsgInfo* msg, // padding with 0 bits to pad to the end of the last octet needed. if ( ecs_family == L3_IPV4 ) { + if ( opt.ecs_src_pfx_len > 32 ) + { + analyzer->Weird("EDNS_ECS_invalid_addr_v4_prefix", + util::fmt("%" PRIu16 " bits", opt.ecs_src_pfx_len)); + data += option_len; + break; + } + + if ( opt.ecs_src_pfx_len > option_len * 8 ) + { + analyzer->Weird("EDNS_ECS_invalid_addr_v4", + util::fmt("need %" PRIu16 " bits, have %d bits", + opt.ecs_src_pfx_len, option_len * 8)); + data += option_len; + break; + } + opt.ecs_family = make_intrusive("v4"); uint32_t addr = 0; - for (uint16_t shift_factor = 3; option_len > 0; option_len--) + uint16_t shift_factor = 3; + int bits_left = opt.ecs_src_pfx_len; + + while ( bits_left > 0 ) { addr |= data[0] << (shift_factor * 8); data++; shift_factor--; + option_len--; + bits_left -= 8; } + addr = htonl(addr); opt.ecs_addr = make_intrusive(addr); } else if ( ecs_family == L3_IPV6 ) { + if ( opt.ecs_src_pfx_len > 128 ) + { + analyzer->Weird("EDNS_ECS_invalid_addr_v6_prefix", + util::fmt("%" PRIu16 " bits", opt.ecs_src_pfx_len)); + data += option_len; + break; + } + + if ( opt.ecs_src_pfx_len > option_len * 8 ) + { + analyzer->Weird("EDNS_ECS_invalid_addr_v6", + util::fmt("need %" PRIu16 " bits, have %d bits", + opt.ecs_src_pfx_len, option_len * 8)); + data += option_len; + break; + } + opt.ecs_family = make_intrusive("v6"); uint32_t addr[4] = { 0 }; - for (uint16_t i = 0, shift_factor = 15; option_len > 0; option_len--) + uint16_t shift_factor = 15; + int bits_left = opt.ecs_src_pfx_len; + int i = 0; + + while ( bits_left > 0 ) { addr[i / 4] |= data[0] << ((shift_factor % 4) * 8); data++; i++; shift_factor--; + option_len--; + bits_left -= 8; } for (uint8_t i = 0; i < 4; i++) @@ -799,6 +848,7 @@ bool DNS_Interpreter::ParseRR_EDNS(detail::DNS_MsgInfo* msg, msg->BuildHdrVal(), msg->BuildEDNS_ECS_Val(&opt) ); + data += option_len; break; } // END EDNS ECS From 7c27d4c1e7621ae15079943d82b1bfbd623b72b2 Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Tue, 8 Dec 2020 12:57:15 -0800 Subject: [PATCH 2/2] Add test case to cover weird EDNS ECS parsing situations --- .../weird.log | 15 +++++++++++++++ testing/btest/Traces/dns-edns-ecs-bad.pcap | Bin 0 -> 1804 bytes testing/btest/Traces/dns-edns-ecs-weirds.pcap | Bin 0 -> 660 bytes .../scripts/base/protocols/dns/dns-edns-ecs.zeek | 6 ++++++ 4 files changed, 21 insertions(+) create mode 100644 testing/btest/Baseline/scripts.base.protocols.dns.dns-edns-ecs/weird.log create mode 100644 testing/btest/Traces/dns-edns-ecs-bad.pcap create mode 100644 testing/btest/Traces/dns-edns-ecs-weirds.pcap diff --git a/testing/btest/Baseline/scripts.base.protocols.dns.dns-edns-ecs/weird.log b/testing/btest/Baseline/scripts.base.protocols.dns.dns-edns-ecs/weird.log new file mode 100644 index 0000000000..11b2698348 --- /dev/null +++ b/testing/btest/Baseline/scripts.base.protocols.dns.dns-edns-ecs/weird.log @@ -0,0 +1,15 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +#separator \x09 +#set_separator , +#empty_field (empty) +#unset_field - +#path weird +#open XXXX-XX-XX-XX-XX-XX +#fields ts uid id.orig_h id.orig_p id.resp_h id.resp_p name addl notice peer source +#types time string addr port addr port string string bool string string +XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 74.125.47.13 57157 192.168.90.10 53 EDNS_ECS_invalid_addr_v4 need 32 bits, have 24 bits F zeek DNS +XXXXXXXXXX.XXXXXX ClEkJM2Vm5giqnMf4h 74.125.73.76 55744 192.168.90.10 53 EDNS_ECS_invalid_addr_v4_prefix 255 bits F zeek DNS +XXXXXXXXXX.XXXXXX C4J4Th3PJpwUYZZ6gc 2a00:1450:4013:c03::10a 46433 2001:470:765b::a25:53 53 EDNS_ECS_invalid_addr_v6_prefix 255 bits F zeek DNS +XXXXXXXXXX.XXXXXX CtPZjS20MLrsMUOJi2 2a00:1450:4013:c06::105 63369 2001:470:765b::a25:53 53 EDNS_ECS_invalid_addr_v6 need 66 bits, have 56 bits F zeek DNS +XXXXXXXXXX.XXXXXX CUM0KZ3MLUfNB0cl11 2a00:1450:400c:c00::106 54430 2001:470:765b::a25:53 53 EDNS_ECS_invalid_option_len - F zeek DNS +#close XXXX-XX-XX-XX-XX-XX diff --git a/testing/btest/Traces/dns-edns-ecs-bad.pcap b/testing/btest/Traces/dns-edns-ecs-bad.pcap new file mode 100644 index 0000000000000000000000000000000000000000..be4f7a90fb7d25aa1e039aa76cdb1b7362545e05 GIT binary patch literal 1804 zcmca|c+)~A1{MYcU}Rtfa*QvWkKcQTlc5vH0bwv;0+WnwcN2gNMsGm_HTLfg4BTK* zAi+Obfsv&^UXVlnqD6ct+ZFjo45p{L7`>G``0ZPP8W`RHF%w@&MM*(oN=jy4dc3g_ zOm7N|0i=QA#z1|&zC03+t_76yiXpw%D@2E#I93K(GC1N#vrjA0Rj zGsyw9u;NU1kU)cxC5bteRYfeB$;Fu*NvTEIsX3{Y%&94*3=9IGte`0XRM@~kesYD_ zLwu4%j%~1BtUiai9yLkAMIj7W#zhk*iVF`GuxdCDZy+%+fRd-}A?CqeZg5}^0^%cS u$dwzW&~if|D%n_&>WCW08&MP+LC{ImFElSBNW=cr~NdToZ1%Q$b z3=G^191QFXj0y}_ZDoyt7J9ehv+$KN$imORylOptKo;IS0J3mK{T>UTX(U~)5=InP5y+U_PW?fApsB$%zmAQIpp%)<;)0JMcmfsv)4 zEE>Y&Qe_C*g21Yh?hg_Kla>z36B^LM`f)OIj3``a output # @TEST-EXEC: btest-diff output +# Test-case for malformed messages: +# @TEST-EXEC: zeek -b -C -r $TRACES/dns-edns-ecs-bad.pcap %INPUT +# @TEST-EXEC: zeek -b -C -r $TRACES/dns-edns-ecs-weirds.pcap %INPUT base/frameworks/notice/weird +# @TEST-EXEC: btest-diff weird.log + @load policy/protocols/dns/auth-addl event dns_EDNS_ecs(c: connection, msg: dns_msg, opt: dns_edns_ecs) {