b.a.t.m.a.n.lists.open-mesh.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/6] batctl: tcpdump: Fix problems detected during fuzzing
@ 2024-01-27 12:48 Sven Eckelmann
  2024-01-27 12:48 ` [PATCH 1/6] batctl: tcpdump: Fix missing sanity check for batman-adv header Sven Eckelmann
                   ` (6 more replies)
  0 siblings, 7 replies; 8+ messages in thread
From: Sven Eckelmann @ 2024-01-27 12:48 UTC (permalink / raw)
  To: b.a.t.m.a.n; +Cc: Sven Eckelmann, Marco Dalla Torre

While many parts of batctl are rather simple, tcpdump is one of the most
complex parts - which unfortunately is also dealing all the time
with potentially harmful input. It is therefore a good idea to perform
some tests to figure out how bad the current state of the code is. The
findings will be presented here - including some information how other
people can reproduce these problems.

With afl++, it is possible to fuzz batctl tcpdump and find parsing errors
(easier). But it needs an entry point to actually send data to. So for
simplicity, a fuzzme subcommand was added which just gets new data from
afl++ and then runs the main ethernet parsing function.

  diff --git a/split_pcap.py b/split_pcap.py
  new file mode 100755
  index 0000000000000000000000000000000000000000..11a1f5ce8ec60fac141693b0449d5c3955f9ad28
  --- /dev/null
  +++ b/split_pcap.py
  @@ -0,0 +1,21 @@
  +#!/usr/bin/env python3
  +
  +import sys
  +
  +from hashlib import sha256
  +from scapy.utils import rdpcap
  +
  +
  +def main():
  +    for pcap in sys.argv[1:]:
  +        packets = rdpcap(pcap)
  +        for packet in packets:
  +            m = sha256()
  +            m.update(packet.load)
  +            fname = m.hexdigest()
  +            with open(fname, "wb") as f:
  +                f.write(packet.load)
  +
  +
  +if __name__ == "__main__":
  +    main()
  diff --git a/tcpdump.c b/tcpdump.c
  index 5e7c76c69bd192d7485958aafabc0e9264b41b90..d340af986f99bdf2e8e6dae0d91a641bc80e82a2 100644
  --- a/tcpdump.c
  +++ b/tcpdump.c
  @@ -1556,3 +1556,41 @@ static int tcpdump(struct state *state __maybe_unused, int argc, char **argv)
   
   COMMAND(SUBCOMMAND, tcpdump, "td", 0, NULL,
   	"<interface>       \ttcpdump layer 2 traffic on the given interface");
  +
  +__AFL_FUZZ_INIT();
  +
  +static int fuzzme(struct state *state __maybe_unused, int argc, char **argv)
  +{
  +	dump_level = dump_level_all;
  +
  +#ifdef __AFL_HAVE_MANUAL_CONTROL
  +	__AFL_INIT();
  +#endif
  +
  +	unsigned char *buf = __AFL_FUZZ_TESTCASE_BUF;
  +	while (__AFL_LOOP(10000)) {
  +		int len = __AFL_FUZZ_TESTCASE_LEN;
  +
  +		/* safety check from tcpdump */
  +		if ((size_t)len < sizeof(struct ether_header))
  +			continue;
  +
  +		/* move into new buffer to allow ASAN to detect invalid memory access */
  +		unsigned char *p = malloc(len);
  +		if (!p)
  +			continue;
  +
  +		memcpy(p, buf, len);
  +
  +		/* function under test */
  +		parse_eth_hdr(p, len, 0, 0);
  +
  +		/* drop buffer from asan */
  +		free(p);
  +	}
  +
  +	return 0;
  +}
  +
  +COMMAND(SUBCOMMAND, fuzzme, "fz", 0, NULL,

To build the fuzzing test, it is necessary to build batctl slightly
differently:

  make clean
  export AFL_USE_ASAN=1; CC=afl-clang-fast make V=s

And the some input files (containing raw ethernet fames have to be
generated from existing pcaps):

  mkdir in
  cd in
  ../split_pcap.py ~/wireshark-batman-adv/tests/*
  cd ..

And then multiple afl++ fuzzer instances can be started.

  if [ -z "${STY}" ]; then
      echo "must be started inside a screen session" >&2
      exit 1
  fi

  for i in $(seq 1 $(nproc)); do
      start_mode=-M
      [ "${i}" = "1" ] || start_mode=-S
      screen afl-fuzz "${start_mode}" "fuzzer${i}" -i in -o out ./batctl fuzzme
  done

The crashes can then be analyzed further by sending them to the fuzzme
subcommand:

   ./batctl fuzzme < out/fuzzer1/crashes/id:000000,sig:06,src:000528,time:12,execs:23992,op:havoc,rep:8

Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
Sven Eckelmann (6):
      batctl: tcpdump: Fix missing sanity check for batman-adv header
      batctl: tcpdump: Add missing throughput header length check
      batctl: tcpdump: Fix IPv4 header length check
      batctl: tcpdump: Add missing ICMPv6 Neighbor Advert length check
      batctl: tcpdump: Add missing ICMPv6 Neighbor Solicit length check
      batctl: tcpdump: Fix ICMPv4 inner IPv4 header length check

 tcpdump.c | 28 ++++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)
---
base-commit: a57de3183e67ec27cf96f1761e69d542e6dfac03
change-id: 20240127-tcpdump_fuzzing-736774906f60

Best regards,
-- 
Sven Eckelmann <sven@narfation.org>


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/6] batctl: tcpdump: Fix missing sanity check for batman-adv header
  2024-01-27 12:48 [PATCH 0/6] batctl: tcpdump: Fix problems detected during fuzzing Sven Eckelmann
@ 2024-01-27 12:48 ` Sven Eckelmann
  2024-01-27 12:49 ` [PATCH 2/6] batctl: tcpdump: Add missing throughput header length check Sven Eckelmann
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Sven Eckelmann @ 2024-01-27 12:48 UTC (permalink / raw)
  To: b.a.t.m.a.n; +Cc: Sven Eckelmann

parse_eth_hdr() is assuming that every ETH_P_BATMAN ethernet packet has a
valid, minimal batman-adv header (packet_type, version, ttl) attached. But
it doesn't actually check if the received buffer has enough bytes to access
the two bytes packet_type + version. So it is possible that it tries to
read outside of the received data.

Fixes: 3bdfc388e74b ("implement simple tcpdump, first only batman packets")
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 tcpdump.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tcpdump.c b/tcpdump.c
index d340af9..d15c32e 100644
--- a/tcpdump.c
+++ b/tcpdump.c
@@ -1167,6 +1167,9 @@ static void parse_eth_hdr(unsigned char *packet_buff, ssize_t buff_len, int read
 			dump_vlan(packet_buff, buff_len, read_opt, time_printed);
 		break;
 	case ETH_P_BATMAN:
+		/* check for batman-adv packet_type + version */
+		LEN_CHECK(buff_len, sizeof(*eth_hdr) + 2, "BAT HEADER")
+
 		batman_ogm_packet = (struct batadv_ogm_packet *)(packet_buff + ETH_HLEN);
 
 		if ((read_opt & COMPAT_FILTER) &&

-- 
2.39.2


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 2/6] batctl: tcpdump: Add missing throughput header length check
  2024-01-27 12:48 [PATCH 0/6] batctl: tcpdump: Fix problems detected during fuzzing Sven Eckelmann
  2024-01-27 12:48 ` [PATCH 1/6] batctl: tcpdump: Fix missing sanity check for batman-adv header Sven Eckelmann
@ 2024-01-27 12:49 ` Sven Eckelmann
  2024-01-27 12:49 ` [PATCH 3/6] batctl: tcpdump: Fix IPv4 " Sven Eckelmann
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Sven Eckelmann @ 2024-01-27 12:49 UTC (permalink / raw)
  To: b.a.t.m.a.n; +Cc: Sven Eckelmann

dump_batman_icmp() is only doing a length check for the original ICMP
packet length. But the throughput packet (which is also handled by this
function) is accessed without doing an additional length check. So it is
possible that it tries to read outside of the received data.

Fixes: f109b3473f86 ("batctl: introduce throughput meter support")
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 tcpdump.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tcpdump.c b/tcpdump.c
index d15c32e..9bb4b9e 100644
--- a/tcpdump.c
+++ b/tcpdump.c
@@ -911,7 +911,6 @@ static void dump_batman_icmp(unsigned char *packet_buff, ssize_t buff_len, int r
 	LEN_CHECK((size_t)buff_len - sizeof(struct ether_header), sizeof(struct batadv_icmp_packet), "BAT ICMP");
 
 	icmp_packet = (struct batadv_icmp_packet *)(packet_buff + sizeof(struct ether_header));
-	tp = (struct batadv_icmp_tp_packet *)icmp_packet;
 
 	if (!time_printed)
 		print_time();
@@ -942,6 +941,10 @@ static void dump_batman_icmp(unsigned char *packet_buff, ssize_t buff_len, int r
 			(size_t)buff_len - sizeof(struct ether_header));
 		break;
 	case BATADV_TP:
+		LEN_CHECK((size_t)buff_len - sizeof(struct ether_header), sizeof(*tp), "BAT TP");
+
+		tp = (struct batadv_icmp_tp_packet *)icmp_packet;
+
 		printf("%s: ICMP TP type %s (%hhu), id %hhu, seq %u, ttl %2d, v %d, length %zu\n",
 		       name, tp->subtype == BATADV_TP_MSG ? "MSG" :
 			     tp->subtype == BATADV_TP_ACK ? "ACK" : "N/A",

-- 
2.39.2


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 3/6] batctl: tcpdump: Fix IPv4 header length check
  2024-01-27 12:48 [PATCH 0/6] batctl: tcpdump: Fix problems detected during fuzzing Sven Eckelmann
  2024-01-27 12:48 ` [PATCH 1/6] batctl: tcpdump: Fix missing sanity check for batman-adv header Sven Eckelmann
  2024-01-27 12:49 ` [PATCH 2/6] batctl: tcpdump: Add missing throughput header length check Sven Eckelmann
@ 2024-01-27 12:49 ` Sven Eckelmann
  2024-01-27 12:49 ` [PATCH 4/6] batctl: tcpdump: Add missing ICMPv6 Neighbor Advert " Sven Eckelmann
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Sven Eckelmann @ 2024-01-27 12:49 UTC (permalink / raw)
  To: b.a.t.m.a.n; +Cc: Sven Eckelmann

dump_ip() is directly accessing the header in the header length check and
assumes that ihl can be trusted. But when when ihl is set to something less
than 5 then it would not even be possible to store the basic IPv4 header in
it. But dump_ip would have still accepted it because it didn't check if
there are at least enough bytes available to read the basic IPv4 header. So
it is possible that it tries to read outside of the received data.

Fixes: 75d68356f3fa ("[batctl] tcpdump - add basic IPv4 support")
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 tcpdump.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tcpdump.c b/tcpdump.c
index 9bb4b9e..3fdd7c3 100644
--- a/tcpdump.c
+++ b/tcpdump.c
@@ -694,7 +694,9 @@ static void dump_ip(unsigned char *packet_buff, ssize_t buff_len,
 	struct icmphdr *icmphdr;
 
 	iphdr = (struct iphdr *)packet_buff;
+	LEN_CHECK((size_t)buff_len, sizeof(*iphdr), ip_string);
 	LEN_CHECK((size_t)buff_len, (size_t)(iphdr->ihl * 4), ip_string);
+	LEN_CHECK((size_t)(iphdr->ihl * 4), sizeof(*iphdr), ip_string);
 
 	if (!time_printed)
 		print_time();

-- 
2.39.2


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 4/6] batctl: tcpdump: Add missing ICMPv6 Neighbor Advert length check
  2024-01-27 12:48 [PATCH 0/6] batctl: tcpdump: Fix problems detected during fuzzing Sven Eckelmann
                   ` (2 preceding siblings ...)
  2024-01-27 12:49 ` [PATCH 3/6] batctl: tcpdump: Fix IPv4 " Sven Eckelmann
@ 2024-01-27 12:49 ` Sven Eckelmann
  2024-01-27 12:49 ` [PATCH 5/6] batctl: tcpdump: Add missing ICMPv6 Neighbor Solicit " Sven Eckelmann
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Sven Eckelmann @ 2024-01-27 12:49 UTC (permalink / raw)
  To: b.a.t.m.a.n; +Cc: Sven Eckelmann, Marco Dalla Torre

dump_ipv6() is doing a length check for the original ICMPv6 header length.
But the neighbor advertisement (which is also handled by this function) is
accessed without doing an additional length check. So it is possible that
it tries to read outside of the received data.

Fixes: 35b37756f4a3 ("add IPv6 support to tcpdump parser")
Cc: Marco Dalla Torre <marco.dallato@gmail.com>
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 tcpdump.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tcpdump.c b/tcpdump.c
index 3fdd7c3..2ae3909 100644
--- a/tcpdump.c
+++ b/tcpdump.c
@@ -659,6 +659,8 @@ static void dump_ipv6(unsigned char *packet_buff, ssize_t buff_len,
 			       nd_nas_target, buff_len);
 			break;
 		case ND_NEIGHBOR_ADVERT:
+			LEN_CHECK((size_t)buff_len - (size_t)(sizeof(struct ip6_hdr)),
+				  sizeof(*nd_advert), "ICMPv6 Neighbor Advertisement");
 			nd_advert = (struct nd_neighbor_advert *)icmphdr;
 			inet_ntop(AF_INET6, &(nd_advert->nd_na_target),
 				  nd_nas_target, 40);

-- 
2.39.2


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 5/6] batctl: tcpdump: Add missing ICMPv6 Neighbor Solicit length check
  2024-01-27 12:48 [PATCH 0/6] batctl: tcpdump: Fix problems detected during fuzzing Sven Eckelmann
                   ` (3 preceding siblings ...)
  2024-01-27 12:49 ` [PATCH 4/6] batctl: tcpdump: Add missing ICMPv6 Neighbor Advert " Sven Eckelmann
@ 2024-01-27 12:49 ` Sven Eckelmann
  2024-01-27 12:49 ` [PATCH 6/6] batctl: tcpdump: Fix ICMPv4 inner IPv4 header " Sven Eckelmann
  2024-01-27 12:51 ` [PATCH 0/6] batctl: tcpdump: Fix problems detected during fuzzing Sven Eckelmann
  6 siblings, 0 replies; 8+ messages in thread
From: Sven Eckelmann @ 2024-01-27 12:49 UTC (permalink / raw)
  To: b.a.t.m.a.n; +Cc: Sven Eckelmann, Marco Dalla Torre

dump_ipv6() is doing a length check for the original ICMPv6 header length.
But the neighbor solicitation (which is also handled by this function) is
accessed without doing an additional length check. So it is possible that
it tries to read outside of the received data.

Fixes: 35b37756f4a3 ("add IPv6 support to tcpdump parser")
Cc: Marco Dalla Torre <marco.dallato@gmail.com>
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 tcpdump.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tcpdump.c b/tcpdump.c
index 2ae3909..c253755 100644
--- a/tcpdump.c
+++ b/tcpdump.c
@@ -652,6 +652,8 @@ static void dump_ipv6(unsigned char *packet_buff, ssize_t buff_len,
 			       (size_t)buff_len - sizeof(struct icmp6_hdr));
 			break;
 		case ND_NEIGHBOR_SOLICIT:
+			LEN_CHECK((size_t)buff_len - (size_t)(sizeof(struct ip6_hdr)),
+				  sizeof(*nd_neigh_sol), "ICMPv6 Neighbor Solicitation");
 			nd_neigh_sol = (struct nd_neighbor_solicit *)icmphdr;
 			inet_ntop(AF_INET6, &(nd_neigh_sol->nd_ns_target),
 				  nd_nas_target, 40);

-- 
2.39.2


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 6/6] batctl: tcpdump: Fix ICMPv4 inner IPv4 header length check
  2024-01-27 12:48 [PATCH 0/6] batctl: tcpdump: Fix problems detected during fuzzing Sven Eckelmann
                   ` (4 preceding siblings ...)
  2024-01-27 12:49 ` [PATCH 5/6] batctl: tcpdump: Add missing ICMPv6 Neighbor Solicit " Sven Eckelmann
@ 2024-01-27 12:49 ` Sven Eckelmann
  2024-01-27 12:51 ` [PATCH 0/6] batctl: tcpdump: Fix problems detected during fuzzing Sven Eckelmann
  6 siblings, 0 replies; 8+ messages in thread
From: Sven Eckelmann @ 2024-01-27 12:49 UTC (permalink / raw)
  To: b.a.t.m.a.n; +Cc: Sven Eckelmann

dump_ip() is doing a length check for the inner (inside ICMP) IPv4 header
length. But it is just assuming that the inner ICMPv4 header has ihl set to
5 - without actually checking for this. The more complex IPv4 header length
check for the outer IPv4 header is missing before it tries to access the
UDP header using the inner ihl IPv4 header length information. So it is
possible that it tries to read outside of the received data.

Fixes: 75d68356f3fa ("[batctl] tcpdump - add basic IPv4 support")
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 tcpdump.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/tcpdump.c b/tcpdump.c
index c253755..c6aca27 100644
--- a/tcpdump.c
+++ b/tcpdump.c
@@ -730,12 +730,20 @@ static void dump_ip(unsigned char *packet_buff, ssize_t buff_len,
 				(size_t)buff_len - (iphdr->ihl * 4));
 			break;
 		case ICMP_DEST_UNREACH:
-			LEN_CHECK((size_t)buff_len - (iphdr->ihl * 4) - sizeof(struct icmphdr),
-				sizeof(struct iphdr) + 8, "ICMP DEST_UNREACH");
-
 			switch (icmphdr->code) {
 			case ICMP_PORT_UNREACH:
+				LEN_CHECK((size_t)buff_len - (iphdr->ihl * 4) - sizeof(struct icmphdr),
+					  sizeof(struct iphdr), "ICMP DEST_UNREACH");
+
+				/* validate inner IP header information */
 				tmp_iphdr = (struct iphdr *)(((char *)icmphdr) + sizeof(struct icmphdr));
+				LEN_CHECK((size_t)buff_len - (iphdr->ihl * 4) - sizeof(struct icmphdr),
+					  (size_t)(tmp_iphdr->ihl * 4), "ICMP DEST_UNREACH");
+				LEN_CHECK((size_t)(tmp_iphdr->ihl * 4), sizeof(*iphdr), "ICMP DEST_UNREACH");
+
+				LEN_CHECK((size_t)buff_len - (iphdr->ihl * 4) - sizeof(struct icmphdr) - (tmp_iphdr->ihl * 4),
+					  sizeof(*tmp_udphdr), "ICMP DEST_UNREACH");
+
 				tmp_udphdr = (struct udphdr *)(((char *)tmp_iphdr) + (tmp_iphdr->ihl * 4));
 
 				printf("%s: ICMP ", ipdst);

-- 
2.39.2


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 0/6] batctl: tcpdump: Fix problems detected during fuzzing
  2024-01-27 12:48 [PATCH 0/6] batctl: tcpdump: Fix problems detected during fuzzing Sven Eckelmann
                   ` (5 preceding siblings ...)
  2024-01-27 12:49 ` [PATCH 6/6] batctl: tcpdump: Fix ICMPv4 inner IPv4 header " Sven Eckelmann
@ 2024-01-27 12:51 ` Sven Eckelmann
  6 siblings, 0 replies; 8+ messages in thread
From: Sven Eckelmann @ 2024-01-27 12:51 UTC (permalink / raw)
  To: b.a.t.m.a.n; +Cc: Marco Dalla Torre


[-- Attachment #1.1: Type: text/plain, Size: 527 bytes --]

On Saturday, 27 January 2024 13:48:58 CET Sven Eckelmann wrote:
> While many parts of batctl are rather simple, tcpdump is one of the most
> complex parts - which unfortunately is also dealing all the time
> with potentially harmful input. It is therefore a good idea to perform
> some tests to figure out how bad the current state of the code is. The
> findings will be presented here - including some information how other
> people can reproduce these problems.

Attached are also the actual reproducers.

Kind regards,
	Sven

[-- Attachment #1.2: 0001-parse_eth_hdr-missing-bat_hdr_len-check --]
[-- Type: application/octet-stream, Size: 62 bytes --]

[-- Attachment #1.3: 0002-dump_batman_icmp-tp_len_check --]
[-- Type: application/octet-stream, Size: 34 bytes --]

[-- Attachment #1.4: 0003-dump_ip-missing-ihl-validation --]
[-- Type: application/octet-stream, Size: 65 bytes --]

[-- Attachment #1.5: 0004-dump_ipv6-neigh-advert-len_check --]
[-- Type: application/octet-stream, Size: 62 bytes --]

[-- Attachment #1.6: 0005-dump_ipv6-neigh-solicit-len_check --]
[-- Type: application/octet-stream, Size: 63 bytes --]

[-- Attachment #1.7: 0006-dump_ip-unreachable-len_check --]
[-- Type: application/octet-stream, Size: 86 bytes --]

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2024-01-27 12:54 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-01-27 12:48 [PATCH 0/6] batctl: tcpdump: Fix problems detected during fuzzing Sven Eckelmann
2024-01-27 12:48 ` [PATCH 1/6] batctl: tcpdump: Fix missing sanity check for batman-adv header Sven Eckelmann
2024-01-27 12:49 ` [PATCH 2/6] batctl: tcpdump: Add missing throughput header length check Sven Eckelmann
2024-01-27 12:49 ` [PATCH 3/6] batctl: tcpdump: Fix IPv4 " Sven Eckelmann
2024-01-27 12:49 ` [PATCH 4/6] batctl: tcpdump: Add missing ICMPv6 Neighbor Advert " Sven Eckelmann
2024-01-27 12:49 ` [PATCH 5/6] batctl: tcpdump: Add missing ICMPv6 Neighbor Solicit " Sven Eckelmann
2024-01-27 12:49 ` [PATCH 6/6] batctl: tcpdump: Fix ICMPv4 inner IPv4 header " Sven Eckelmann
2024-01-27 12:51 ` [PATCH 0/6] batctl: tcpdump: Fix problems detected during fuzzing Sven Eckelmann

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).