From b2f936ff24129c4f40b62f2a4a5410a24ad45ab4 Mon Sep 17 00:00:00 2001 From: Gerald Combs Date: Mon, 28 Apr 2003 04:03:26 +0000 Subject: Fix several buffer and integer overflow issues discovered by Timo Sirainen. tvbuff.c: Lots of existing code assumes that you can safely do the following: #define MAX_BUF 64 guint8 *buf[MAX_BUF]; ... tvb_get_nstringz0 (tvb, offset, MAX_BUF, buf, &bytes_copied); In reality, tvb_get_nstringz*() can potentially write one byte past "buf". Modify _tvb_get_nstringz() not to do that. packet-ppp.c: Check for a valid BAP suboption length. packet-mount.c: Fix a possible integer overflow in dissect_group(). svn path=/trunk/; revision=7590 --- epan/tvbuff.c | 34 ++++++++++++++++++++++------------ packet-mount.c | 24 +++++++++++++----------- packet-ppp.c | 11 ++++++++--- 3 files changed, 43 insertions(+), 26 deletions(-) diff --git a/epan/tvbuff.c b/epan/tvbuff.c index 4e8a2c2f0d..e45aac3c1e 100644 --- a/epan/tvbuff.c +++ b/epan/tvbuff.c @@ -9,7 +9,7 @@ * the data of a backing tvbuff, or can be a composite of * other tvbuffs. * - * $Id: tvbuff.c,v 1.41 2003/02/24 01:22:26 guy Exp $ + * $Id: tvbuff.c,v 1.42 2003/04/28 04:03:26 gerald Exp $ * * Copyright (c) 2000 by Gilbert Ramirez * @@ -1674,10 +1674,10 @@ tvb_format_text(tvbuff_t *tvb, gint offset, gint size) } /* Looks for a stringz (NUL-terminated string) in tvbuff and copies - * no more than maxlength number of bytes, including terminating NUL, to buffer. - * Returns length of string (not including terminating NUL), or -1 if the string was - * truncated in the buffer due to not having reached the terminating NUL. - * In this way, it acts like snprintf(). + * no more than (maxlength - 1) number of bytes, including terminating NUL, to + * buffer. Returns length of string (not including terminating NUL), or -1 if + * the string was truncated in the buffer due to not having reached the + * terminating NUL. * * When processing a packet where the remaining number of bytes is less * than maxlength, an exception is not thrown if the end of the packet @@ -1701,7 +1701,11 @@ _tvb_get_nstringz(tvbuff_t *tvb, gint offset, guint maxlength, guint8* buffer, check_offset_length(tvb, offset, 0, &abs_offset, &junk_length); if (maxlength == 0) { + *bytes_copied = 0; + return -1; + } else if (maxlength == 1) { buffer[0] = 0; + *bytes_copied = 1; return 0; } @@ -1715,16 +1719,22 @@ _tvb_get_nstringz(tvbuff_t *tvb, gint offset, guint maxlength, guint8* buffer, } /* This should not happen because check_offset_length() would - * have already thrown an exception if 'offset' were out-of-bounds. - */ + * have already thrown an exception if 'offset' were out-of-bounds. + */ g_assert(len != -1); + /* + * If we've been passed a negative number, maxlength will + * be huge. + */ + g_assert(maxlength <= G_MAXINT); + if ((guint)len < maxlength) { limit = len; decreased_max = TRUE; } else { - limit = maxlength; + limit = maxlength - 1; } stringlen = tvb_strnlen(tvb, abs_offset, limit); @@ -1750,10 +1760,10 @@ _tvb_get_nstringz(tvbuff_t *tvb, gint offset, guint maxlength, guint8* buffer, } /* Looks for a stringz (NUL-terminated string) in tvbuff and copies - * no more than maxlength number of bytes, including terminating NUL, to buffer. - * Returns length of string (not including terminating NUL), or -1 if the string was - * truncated in the buffer due to not having reached the terminating NUL. - * In this way, it acts like snprintf(). + * no more than (maxlength - 1) number of bytes, including terminating NUL, to + * buffer. Returns length of string (not including terminating NUL), or -1 if + * the string was truncated in the buffer due to not having reached the + * terminating NUL. * * When processing a packet where the remaining number of bytes is less * than maxlength, an exception is not thrown if the end of the packet diff --git a/packet-mount.c b/packet-mount.c index 8c48f0f9dd..843443a9f0 100644 --- a/packet-mount.c +++ b/packet-mount.c @@ -1,7 +1,7 @@ /* packet-mount.c * Routines for mount dissection * - * $Id: packet-mount.c,v 1.37 2002/11/14 02:31:26 guy Exp $ + * $Id: packet-mount.c,v 1.38 2003/04/28 04:03:24 gerald Exp $ * * Ethereal - Network traffic analyzer * By Gerald Combs @@ -240,17 +240,19 @@ dissect_group(tvbuff_t *tvb, int offset, packet_info *pinfo _U_, proto_tree *tre { int len,str_len; len=tvb_get_ntohl(tvb,offset); - str_len=tvb_get_nstringz(tvb,offset+4, - MAX_GROUP_NAME_LIST-5-group_names_len, - group_name_list+group_names_len); - if((group_names_len>=(MAX_GROUP_NAME_LIST-5))||(str_len<0)){ - strcpy(group_name_list+(MAX_GROUP_NAME_LIST-5),"..."); - group_names_len=MAX_GROUP_NAME_LIST-1; - } else { - group_names_len+=str_len; - group_name_list[group_names_len++]=' '; + if (group_names_len < MAX_GROUP_NAME_LIST - 5) { + str_len=tvb_get_nstringz(tvb,offset+4, + MAX_GROUP_NAME_LIST-5-group_names_len, + group_name_list+group_names_len); + if((group_names_len>=(MAX_GROUP_NAME_LIST-5))||(str_len<0)){ + strcpy(group_name_list+(MAX_GROUP_NAME_LIST-5),"..."); + group_names_len=MAX_GROUP_NAME_LIST; + } else { + group_names_len+=str_len; + group_name_list[group_names_len++]=' '; + } + group_name_list[group_names_len]=0; } - group_name_list[group_names_len]=0; offset = dissect_rpc_string(tvb, tree, hf_mount_groups_group, offset, NULL); diff --git a/packet-ppp.c b/packet-ppp.c index 8280a90726..aa28fc9a42 100644 --- a/packet-ppp.c +++ b/packet-ppp.c @@ -1,7 +1,7 @@ /* packet-ppp.c * Routines for ppp packet disassembly * - * $Id: packet-ppp.c,v 1.108 2003/02/07 20:09:33 guy Exp $ + * $Id: packet-ppp.c,v 1.109 2003/04/28 04:03:24 gerald Exp $ * * Ethereal - Network traffic analyzer * By Gerald Combs @@ -2205,9 +2205,14 @@ dissect_bap_phone_delta_opt(const ip_tcp_opt *optp, tvbuff_t *tvb, tvb_get_guint8(tvb, offset + 2)); break; case BAP_PHONE_DELTA_SUBOPT_SUBSC_NUM: - tvb_get_nstringz0(tvb, offset + 2, subopt_len - 2, buf); - proto_tree_add_text(suboption_tree, tvb, offset + 2, subopt_len - 2, + if (subopt_len >= 2) { + tvb_get_nstringz0(tvb, offset + 2, subopt_len - 2, buf); + proto_tree_add_text(suboption_tree, tvb, offset + 2, subopt_len - 2, "Subscriber Number: %s", buf); + } else { + proto_tree_add_text(suboption_tree, tvb, offset + 1, 1, + "Invalid suboption length: %u", subopt_len); + } break; case BAP_PHONE_DELTA_SUBOPT_PHONENUM_SUBADDR: tvb_get_nstringz0(tvb, offset + 2, subopt_len - 2, buf); -- cgit v1.2.3