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;
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);
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
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; } }
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
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)));
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
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",
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
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,
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
b.a.t.m.a.n@lists.open-mesh.org
-
Antonio Quartulli -
Marek Lindner -
Sven Eckelmann