diff --git a/CHANGES b/CHANGES index d0b6b976bb..e2c617cac5 100644 --- a/CHANGES +++ b/CHANGES @@ -1,4 +1,32 @@ +4.1.0-dev.4 | 2020-12-15 08:00:44 -0800 + + * Add test case to cover weird EDNS ECS parsing situations (Jon Siwek, Corelight) + + * Fix EDNS ECS option parsing bugs (Jon Siwek, Corelight) + + * 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) + 4.1.0-dev.1 | 2020-12-14 22:27:57 -0800 * Fix local.zeek compatibility test (Jon Siwek, Corelight) diff --git a/VERSION b/VERSION index 914c96fb13..1b31fb0da7 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -4.1.0-dev.1 +4.1.0-dev.4 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 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 0000000000..be4f7a90fb Binary files /dev/null and b/testing/btest/Traces/dns-edns-ecs-bad.pcap differ diff --git a/testing/btest/Traces/dns-edns-ecs-weirds.pcap b/testing/btest/Traces/dns-edns-ecs-weirds.pcap new file mode 100644 index 0000000000..6491a0b8d4 Binary files /dev/null and b/testing/btest/Traces/dns-edns-ecs-weirds.pcap differ diff --git a/testing/btest/scripts/base/protocols/dns/dns-edns-ecs.zeek b/testing/btest/scripts/base/protocols/dns/dns-edns-ecs.zeek index 384014db25..772b83c90d 100644 --- a/testing/btest/scripts/base/protocols/dns/dns-edns-ecs.zeek +++ b/testing/btest/scripts/base/protocols/dns/dns-edns-ecs.zeek @@ -1,6 +1,12 @@ +# Test-case for valid message format: # @TEST-EXEC: zeek -b -C -r $TRACES/dns-edns-ecs.pcap %INPUT > 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) {