[B.A.T.M.A.N.] [PATCH 1/5] batctl: Fix crash when parsing unknown TVLVs

[B.A.T.M.A.N.] [PATCH 1/5] batctl: Fix crash when parsing unknown TVLVs

[Sven Eckelmann]

batctl tcpdump has an array with all known TVLVs and versions. The correct
parser for the TVLV is chosen by getting the pointer from the address
calculated by version and type. Unfortunately, the version and type was never
validated to ensure that not an unknown TVLV (like mcast) was received.

This missing validation makes it possible to crash batctl by injecting packets
with an unknown type and/or version. batctl will try to get the parser, fetch a
NULL pointer or random data and then try to dereferenced it. This is usually
handled by the operating system with a segfault. But this might be exploitable
in rare situations.

An approach to handle this problem is by combining the simple selection step
with the validation step. Only valid version+type will return a parser function
pointer and the requesting function will only call the parser function pointer
when it got one.

This regression was introduced by 4c39fb823b86036df40187f8bd342fe5398c28ef
("batctl: tcpdump - parse TVLV containers").

Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 tcpdump.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 53 insertions(+), 14 deletions(-)

diff --git a/tcpdump.c b/tcpdump.c
index ad5469b..c3c847e 100644
--- a/tcpdump.c
+++ b/tcpdump.c
@@ -171,16 +171,53 @@ static void batctl_tvlv_parse_roam_v1(void *buff,
 
 typedef void (*batctl_tvlv_parser_t)(void *buff, ssize_t buff_len);
 
-/* location [i][j] contains the parsing function for TVLV of type 'i' and
- * version 'j + 1'
- */
-batctl_tvlv_parser_t tvlv_parsers[][1] = {
-	[BATADV_TVLV_GW][0] = batctl_tvlv_parse_gw_v1,
-	[BATADV_TVLV_DAT][0] = batctl_tvlv_parse_dat_v1,
-	[BATADV_TVLV_NC][0] = batctl_tvlv_parse_nc_v1,
-	[BATADV_TVLV_TT][0] = batctl_tvlv_parse_tt_v1,
-	[BATADV_TVLV_ROAM][0] = batctl_tvlv_parse_roam_v1,
-};
+static batctl_tvlv_parser_t tvlv_parser_get(uint8_t type, uint8_t version)
+{
+	switch (type) {
+	case BATADV_TVLV_GW:
+		switch (version) {
+		case 1:
+			return batctl_tvlv_parse_gw_v1;
+		default:
+			return NULL;
+		}
+
+	case BATADV_TVLV_DAT:
+		switch (version) {
+		case 1:
+			return batctl_tvlv_parse_dat_v1;
+		default:
+			return NULL;
+		}
+
+	case BATADV_TVLV_NC:
+		switch (version) {
+		case 1:
+			return batctl_tvlv_parse_nc_v1;
+		default:
+			return NULL;
+		}
+
+	case BATADV_TVLV_TT:
+		switch (version) {
+		case 1:
+			return batctl_tvlv_parse_tt_v1;
+		default:
+			return NULL;
+		}
+
+	case BATADV_TVLV_ROAM:
+		switch (version) {
+		case 1:
+			return batctl_tvlv_parse_roam_v1;
+		default:
+			return NULL;
+		}
+
+	default:
+		return NULL;
+	}
+}
 
 static void dump_batman_ucast_tvlv(unsigned char *packet_buff, ssize_t buff_len,
 				   int read_opt, int time_printed)
@@ -223,8 +260,9 @@ static void dump_batman_ucast_tvlv(unsigned char *packet_buff, ssize_t buff_len,
 		tvlv_hdr = (struct batadv_tvlv_hdr *)ptr;
 		len = ntohs(tvlv_hdr->len);
 
-		parser = tvlv_parsers[tvlv_hdr->type][tvlv_hdr->version - 1];
-		parser(tvlv_hdr + 1, len);
+		parser = tvlv_parser_get(tvlv_hdr->type, tvlv_hdr->version);
+		if (parser)
+			parser(tvlv_hdr + 1, len);
 
 		/* go to the next container */
 		ptr = (uint8_t *)(tvlv_hdr + 1) + len;
@@ -651,8 +689,9 @@ static void dump_batman_iv_ogm(unsigned char *packet_buff, ssize_t buff_len, int
 		tvlv_hdr = (struct batadv_tvlv_hdr *)ptr;
 		len = ntohs(tvlv_hdr->len);
 
-		parser = tvlv_parsers[tvlv_hdr->type][tvlv_hdr->version - 1];
-		parser(tvlv_hdr + 1, len);
+		parser = tvlv_parser_get(tvlv_hdr->type, tvlv_hdr->version);
+		if (parser)
+			parser(tvlv_hdr + 1, len);
 
 		/* go to the next container */
 		ptr = (uint8_t *)(tvlv_hdr + 1) + len;
-- 
2.1.3

[B.A.T.M.A.N.] [PATCH 2/5] batctl: Parse only TVLV when header is available

[Sven Eckelmann]

The TVLV must only start parsing an header when at least one TVLV header is
available. Otherwise data behind the received data might be accessed.

This regression was introduced by 4c39fb823b86036df40187f8bd342fe5398c28ef
("batctl: tcpdump - parse TVLV containers").

Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 tcpdump.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tcpdump.c b/tcpdump.c
index c3c847e..3e57544 100644
--- a/tcpdump.c
+++ b/tcpdump.c
@@ -256,7 +256,7 @@ static void dump_batman_ucast_tvlv(unsigned char *packet_buff, ssize_t buff_len,
 
 	ptr = (uint8_t *)(tvlv_packet + 1);
 
-	while (tvlv_len > 0) {
+	while (tvlv_len >= (ssize_t)sizeof(*tvlv_hdr)) {
 		tvlv_hdr = (struct batadv_tvlv_hdr *)ptr;
 		len = ntohs(tvlv_hdr->len);
 
@@ -685,7 +685,7 @@ static void dump_batman_iv_ogm(unsigned char *packet_buff, ssize_t buff_len, int
 
 	ptr = (uint8_t *)(batman_ogm_packet + 1);
 
-	while (tvlv_len > 0) {
+	while (tvlv_len >= (ssize_t)sizeof(*tvlv_hdr)) {
 		tvlv_hdr = (struct batadv_tvlv_hdr *)ptr;
 		len = ntohs(tvlv_hdr->len);
 
-- 
2.1.3

Re: [B.A.T.M.A.N.] [PATCH 2/5] batctl: Parse only TVLV when header is available

[Marek Lindner]

[-- Attachment #1: Type: text/plain, Size: 518 bytes --]

On Wednesday 12 November 2014 18:58:26 Sven Eckelmann wrote:
> The TVLV must only start parsing an header when at least one TVLV header is
> available. Otherwise data behind the received data might be accessed.
> 
> This regression was introduced by 4c39fb823b86036df40187f8bd342fe5398c28ef
> ("batctl: tcpdump - parse TVLV containers").
> 
> Signed-off-by: Sven Eckelmann <sven@narfation.org>
> ---
>  tcpdump.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Applied in revision e5c2e7f.

Thanks,
Marek

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

[B.A.T.M.A.N.] [PATCH 3/5] batctl: Only parse TVLV data when length is valid

[Sven Eckelmann]

The length in a TVLV field must be smaller or equal to the length of the
remaining read data. Otherwise the parser can read outside of the
valid data region.

This regression was introduced by 4c39fb823b86036df40187f8bd342fe5398c28ef
("batctl: tcpdump - parse TVLV containers").

Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 tcpdump.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/tcpdump.c b/tcpdump.c
index 3e57544..443f671 100644
--- a/tcpdump.c
+++ b/tcpdump.c
@@ -258,15 +258,21 @@ static void dump_batman_ucast_tvlv(unsigned char *packet_buff, ssize_t buff_len,
 
 	while (tvlv_len >= (ssize_t)sizeof(*tvlv_hdr)) {
 		tvlv_hdr = (struct batadv_tvlv_hdr *)ptr;
+
+		/* data after TVLV header */
+		ptr = (uint8_t *)(tvlv_hdr + 1);
+		tvlv_len -= sizeof(*tvlv_hdr);
+
 		len = ntohs(tvlv_hdr->len);
+		LEN_CHECK(tvlv_len, (size_t)len, "BAT UCAST TVLV");
 
 		parser = tvlv_parser_get(tvlv_hdr->type, tvlv_hdr->version);
 		if (parser)
-			parser(tvlv_hdr + 1, len);
+			parser(ptr, len);
 
 		/* go to the next container */
-		ptr = (uint8_t *)(tvlv_hdr + 1) + len;
-		tvlv_len -= sizeof(*tvlv_hdr) + len;
+		ptr += len;
+		tvlv_len -= len;
 	}
 }
 
@@ -687,15 +693,21 @@ static void dump_batman_iv_ogm(unsigned char *packet_buff, ssize_t buff_len, int
 
 	while (tvlv_len >= (ssize_t)sizeof(*tvlv_hdr)) {
 		tvlv_hdr = (struct batadv_tvlv_hdr *)ptr;
+
+		/* data after TVLV header */
+		ptr = (uint8_t *)(tvlv_hdr + 1);
+		tvlv_len -= sizeof(*tvlv_hdr);
+
 		len = ntohs(tvlv_hdr->len);
+		LEN_CHECK(tvlv_len, (size_t)len, "BAT IV OGM TVLV");
 
 		parser = tvlv_parser_get(tvlv_hdr->type, tvlv_hdr->version);
 		if (parser)
-			parser(tvlv_hdr + 1, len);
+			parser(ptr, len);
 
 		/* go to the next container */
-		ptr = (uint8_t *)(tvlv_hdr + 1) + len;
-		tvlv_len -= sizeof(*tvlv_hdr) + len;
+		ptr += len;
+		tvlv_len -= len;
 	}
 }
 
-- 
2.1.3

Re: [B.A.T.M.A.N.] [PATCH 3/5] batctl: Only parse TVLV data when length is valid

[Marek Lindner]

[-- Attachment #1: Type: text/plain, Size: 554 bytes --]

On Wednesday 12 November 2014 18:58:27 Sven Eckelmann wrote:
> The length in a TVLV field must be smaller or equal to the length of the
> remaining read data. Otherwise the parser can read outside of the
> valid data region.
> 
> This regression was introduced by 4c39fb823b86036df40187f8bd342fe5398c28ef
> ("batctl: tcpdump - parse TVLV containers").
> 
> Signed-off-by: Sven Eckelmann <sven@narfation.org>
> ---
>  tcpdump.c | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)

Applied in revision 79f3061.

Thanks,
Marek

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

[B.A.T.M.A.N.] [PATCH 4/5] batctl: Parse non-variable TVLV only with correct length

[Sven Eckelmann]

The TVLV length has to be verified before the tvlv parser can start. Otherwise
the tvlv parser might access invalid data.

Doing the same for zero length TVLVs is just good practice and might help
finding problems easier.

This regression was introduced by 4c39fb823b86036df40187f8bd342fe5398c28ef
("batctl: tcpdump - parse TVLV containers").

Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 tcpdump.c | 34 ++++++++++++++++++++++++++++------
 1 file changed, 28 insertions(+), 6 deletions(-)

diff --git a/tcpdump.c b/tcpdump.c
index 443f671..361deb3 100644
--- a/tcpdump.c
+++ b/tcpdump.c
@@ -100,12 +100,17 @@ static int print_time(void)
 	return 1;
 }
 
-static void batctl_tvlv_parse_gw_v1(void *buff,
-				    ssize_t (buff_len)__attribute__((unused)))
+static void batctl_tvlv_parse_gw_v1(void *buff, ssize_t buff_len)
 {
 	struct batadv_tvlv_gateway_data *tvlv = buff;
 	uint32_t down, up;
 
+	if (buff_len != sizeof(*tvlv)) {
+		fprintf(stderr, "Warning - dropping received %s packet as it is not the correct size (%zu): %zu\n",
+			"TVLV GWv1", sizeof(*tvlv), buff_len);
+		return;
+	}
+
 	down = ntohl(tvlv->bandwidth_down);
 	up = ntohl(tvlv->bandwidth_up);
 
@@ -114,14 +119,26 @@ static void batctl_tvlv_parse_gw_v1(void *buff,
 }
 
 static void batctl_tvlv_parse_dat_v1(void (*buff)__attribute__((unused)),
-				     ssize_t (buff_len)__attribute__((unused)))
+				     ssize_t buff_len)
 {
+	if (buff_len != 0) {
+		fprintf(stderr, "Warning - dropping received %s packet as it is not the correct size (0): %zu\n",
+			"TVLV DATv1", buff_len);
+		return;
+	}
+
 	printf("\tTVLV DATv1: enabled\n");
 }
 
 static void batctl_tvlv_parse_nc_v1(void (*buff)__attribute__((unused)),
-				    ssize_t (buff_len)__attribute__((unused)))
+				    ssize_t buff_len)
 {
+	if (buff_len != 0) {
+		fprintf(stderr, "Warning - dropping received %s packet as it is not the correct size (0): %zu\n",
+			"TVLV NCv1", buff_len);
+		return;
+	}
+
 	printf("\tTVLV NCv1: enabled\n");
 }
 
@@ -159,11 +176,16 @@ static void batctl_tvlv_parse_tt_v1(void *buff,
 	}
 }
 
-static void batctl_tvlv_parse_roam_v1(void *buff,
-				      ssize_t (buff_len)__attribute__((unused)))
+static void batctl_tvlv_parse_roam_v1(void *buff, ssize_t buff_len)
 {
 	struct batadv_tvlv_roam_adv *tvlv = buff;
 
+	if (buff_len != sizeof(*tvlv)) {
+		fprintf(stderr, "Warning - dropping received %s packet as it is not the correct size (%zu): %zu\n",
+			"TVLV ROAMv1", sizeof(*tvlv), buff_len);
+		return;
+	}
+
 	printf("\tTVLV ROAMv1: client %s, VLAN ID %d\n",
 	       get_name_by_macaddr((struct ether_addr *)tvlv->client, NO_FLAGS),
 	       BATADV_PRINT_VID(ntohs(tvlv->vid)));
-- 
2.1.3

Re: [B.A.T.M.A.N.] [PATCH 4/5] batctl: Parse non-variable TVLV only with correct length

[Marek Lindner]

[-- Attachment #1: Type: text/plain, Size: 632 bytes --]

On Wednesday 12 November 2014 18:58:28 Sven Eckelmann wrote:
> The TVLV length has to be verified before the tvlv parser can start.
> Otherwise the tvlv parser might access invalid data.
> 
> Doing the same for zero length TVLVs is just good practice and might help
> finding problems easier.
> 
> This regression was introduced by 4c39fb823b86036df40187f8bd342fe5398c28ef
> ("batctl: tcpdump - parse TVLV containers").
> 
> Signed-off-by: Sven Eckelmann <sven@narfation.org>
> ---
>  tcpdump.c | 34 ++++++++++++++++++++++++++++------
>  1 file changed, 28 insertions(+), 6 deletions(-)

Applied in revision 0505524.

Thanks,
Marek

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

[B.A.T.M.A.N.] [PATCH 5/5] batctl: Only parse big enough TVLV TTv1

[Sven Eckelmann]

A TTv1 has a constant header part and two variable parts. One is the defined by
the number of VLANs and the rest are the changes. The TVLV can only be parsed
when there is enough room for the constant header. Also the number of VLANs
must be validated. Otherwise the TVLV parser can read invalid data outside of
the buffer.

This regression was introduced by 4c39fb823b86036df40187f8bd342fe5398c28ef
("batctl: tcpdump - parse TVLV containers").

Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 tcpdump.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/tcpdump.c b/tcpdump.c
index 361deb3..50ba010 100644
--- a/tcpdump.c
+++ b/tcpdump.c
@@ -142,13 +142,15 @@ static void batctl_tvlv_parse_nc_v1(void (*buff)__attribute__((unused)),
 	printf("\tTVLV NCv1: enabled\n");
 }
 
-static void batctl_tvlv_parse_tt_v1(void *buff,
-				    ssize_t (buff_len)__attribute__((unused)))
+static void batctl_tvlv_parse_tt_v1(void *buff, ssize_t buff_len)
 {
 	struct batadv_tvlv_tt_data *tvlv = buff;
 	struct batadv_tvlv_tt_vlan_data *vlan;
 	int i, num_vlan, num_entry;
 	const char *type;
+	size_t vlan_len;
+
+	LEN_CHECK(buff_len, sizeof(*tvlv), "TVLV TTv1")
 
 	if (tvlv->flags & BATADV_TT_OGM_DIFF)
 		type = "OGM DIFF";
@@ -160,7 +162,10 @@ static void batctl_tvlv_parse_tt_v1(void *buff,
 		type = "UNKNOWN";
 
 	num_vlan = ntohs(tvlv->num_vlan);
-	buff_len -= sizeof(*tvlv) + sizeof(*vlan) * num_vlan;
+	vlan_len = sizeof(*tvlv) + sizeof(*vlan) * num_vlan;
+	LEN_CHECK(buff_len, vlan_len, "TVLV TTv1 VLAN")
+
+	buff_len -= vlan_len;
 	num_entry = buff_len / sizeof(struct batadv_tvlv_tt_change);
 
 	printf("\tTVLV TTv1: %s [%c] ttvn=%hhu vlan_num=%hu entry_num=%hu\n",
-- 
2.1.3

Re: [B.A.T.M.A.N.] [PATCH 5/5] batctl: Only parse big enough TVLV TTv1

[Marek Lindner]

[-- Attachment #1: Type: text/plain, Size: 710 bytes --]

On Wednesday 12 November 2014 18:58:29 Sven Eckelmann wrote:
> A TTv1 has a constant header part and two variable parts. One is the defined
> by the number of VLANs and the rest are the changes. The TVLV can only be
> parsed when there is enough room for the constant header. Also the number
> of VLANs must be validated. Otherwise the TVLV parser can read invalid data
> outside of the buffer.
> 
> This regression was introduced by 4c39fb823b86036df40187f8bd342fe5398c28ef
> ("batctl: tcpdump - parse TVLV containers").
> 
> Signed-off-by: Sven Eckelmann <sven@narfation.org>
> ---
>  tcpdump.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)

Applied in revision 5fda4d5.

Thanks,
Marek

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

Re: [B.A.T.M.A.N.] [PATCH 1/5] batctl: Fix crash when parsing unknown TVLVs

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 1434 bytes --]

Hi,

On 12/11/14 18:58, Sven Eckelmann wrote:
> batctl tcpdump has an array with all known TVLVs and versions. The correct
> parser for the TVLV is chosen by getting the pointer from the address
> calculated by version and type. Unfortunately, the version and type was never
> validated to ensure that not an unknown TVLV (like mcast) was received.
> 
> This missing validation makes it possible to crash batctl by injecting packets
> with an unknown type and/or version. batctl will try to get the parser, fetch a
> NULL pointer or random data and then try to dereferenced it. This is usually
> handled by the operating system with a segfault. But this might be exploitable
> in rare situations.
> 
> An approach to handle this problem is by combining the simple selection step
> with the validation step. Only valid version+type will return a parser function
> pointer and the requesting function will only call the parser function pointer
> when it got one.
> 
> This regression was introduced by 4c39fb823b86036df40187f8bd342fe5398c28ef
> ("batctl: tcpdump - parse TVLV containers").
> 
> Signed-off-by: Sven Eckelmann <sven@narfation.org>


For the whole series:

Acked-by: Antonio Quartulli <antonio@meshcoding.com>

Thank you very much for your work Sven!

@Marek: the offending patch is in next, therefore this patchset should
be merged there as well.


Cheers,

-- 
Antonio Quartulli


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [B.A.T.M.A.N.] [PATCH 1/5] batctl: Fix crash when parsing unknown TVLVs

[Marek Lindner]

[-- Attachment #1: Type: text/plain, Size: 1348 bytes --]

On Wednesday 12 November 2014 18:58:25 Sven Eckelmann wrote:
> batctl tcpdump has an array with all known TVLVs and versions. The correct
> parser for the TVLV is chosen by getting the pointer from the address
> calculated by version and type. Unfortunately, the version and type was
> never validated to ensure that not an unknown TVLV (like mcast) was
> received.
> 
> This missing validation makes it possible to crash batctl by injecting
> packets with an unknown type and/or version. batctl will try to get the
> parser, fetch a NULL pointer or random data and then try to dereferenced
> it. This is usually handled by the operating system with a segfault. But
> this might be exploitable in rare situations.
> 
> An approach to handle this problem is by combining the simple selection step
> with the validation step. Only valid version+type will return a parser
> function pointer and the requesting function will only call the parser
> function pointer when it got one.
> 
> This regression was introduced by 4c39fb823b86036df40187f8bd342fe5398c28ef
> ("batctl: tcpdump - parse TVLV containers").
> 
> Signed-off-by: Sven Eckelmann <sven@narfation.org>
> ---
>  tcpdump.c | 67
> ++++++++++++++++++++++++++++++++++++++++++++++++++------------- 1 file
> changed, 53 insertions(+), 14 deletions(-)

Applied in revision 140882b.

Thanks,
Marek

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