Merge EDNS ECS option parsing security/bug fixes

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)

VERSION

@@ -1 +1 @@
-4.1.0-dev.1
+4.1.0-dev.4

src/analyzer/protocol/dns/DNS.cc

@@ -742,7 +742,10 @@ 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;
 					}
 
@@ -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<StringVal>("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<AddrVal>(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<StringVal>("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
 

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

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) {