Add a "tftp_strnlen()" routine that

	1) checks to make sure that the terminating '\0' is found in the
	   string, and throws a BoundsError exception if it isn't (TFTP
	   packets should fit in a single frame, so if the '\0' isn't
	   found, that's an error);

	2) adds 1 to the length to include the trailing '\0';

and use it to find all string lengths, so that we properly handle short
or malformed frames.

svn path=/trunk/; revision=2778

1 files changed, 52 insertions, 22 deletions

diff --git a/packet-tftp.c b/packet-tftp.c
index 5161344e6d..2c51258b90 100644
--- a/packet-tftp.c
+++ b/packet-tftp.c
@@ -5,7 +5,7 @@
  * Craig Newell <CraigN@cheque.uq.edu.au>
  *	RFC2347 TFTP Option Extension
  *
- * $Id: packet-tftp.c,v 1.19 2000/12/02 08:41:08 guy Exp $
+ * $Id: packet-tftp.c,v 1.20 2000/12/24 20:33:04 guy Exp $
  *
  * Ethereal - Network traffic analyzer
  * By Gerald Combs <gerald@zing.org>
@@ -87,6 +87,7 @@ static const value_string tftp_error_code_vals[] = {
 };
 
 static void tftp_dissect_options(tvbuff_t *tvb, int offset, proto_tree *tree);
+static gint tftp_strnlen(tvbuff_t *tvb, gint offset);
 
 static void
 dissect_tftp(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree)
@@ -151,28 +152,28 @@ dissect_tftp(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree)
 	    
 	  switch (opcode) {
 	  case RRQ:
-	    i1 = tvb_strnlen(tvb, offset, -1);
+	    i1 = tftp_strnlen(tvb, offset);
 	    proto_tree_add_item(tftp_tree, hf_tftp_source_file,
-			    tvb, offset, i1 + 1, FALSE);
-	    offset += i1 + 1;
+			    tvb, offset, i1, FALSE);
+	    offset += i1;
 
-	    i1 = tvb_strnlen(tvb, offset, -1);
+	    i1 = tftp_strnlen(tvb, offset);
 	    ti = proto_tree_add_item(tftp_tree, hf_tftp_transfer_type,
-			    tvb, offset, i1 + 1, FALSE);
-	    offset += i1 + 1;
+			    tvb, offset, i1, FALSE);
+	    offset += i1;
 
 	    tftp_dissect_options(tvb, offset, tftp_tree);
 	    break;
 	  case WRQ:
-	    i1 = tvb_strnlen(tvb, offset, -1);
+	    i1 = tftp_strnlen(tvb, offset);
 	    proto_tree_add_item(tftp_tree, hf_tftp_destination_file,
-			    tvb, offset, i1 + 1, FALSE);
-	    offset += i1 + 1;
+			    tvb, offset, i1, FALSE);
+	    offset += i1;
 
-	    i1 = tvb_strnlen(tvb, offset, -1);
+	    i1 = tftp_strnlen(tvb, offset);
 	    ti = proto_tree_add_item(tftp_tree, hf_tftp_transfer_type,
-			    tvb, offset, i1 + 1, FALSE);
-	    offset += i1 + 1;
+			    tvb, offset, i1, FALSE);
+	    offset += i1;
 
 	    tftp_dissect_options(tvb, offset, tftp_tree);
 	    break;
@@ -193,9 +194,9 @@ dissect_tftp(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree)
 			    FALSE);
 	    offset += 2;
 
-	    i1 = tvb_strnlen(tvb, offset, -1);
+	    i1 = tftp_strnlen(tvb, offset);
 	    proto_tree_add_item(tftp_tree, hf_tftp_error_string, tvb, offset,
-	        i1 + 1, FALSE);
+	        i1, FALSE);
 	    break;
 	  case OACK:
 	    tftp_dissect_options(tvb, offset, tftp_tree);
@@ -212,19 +213,48 @@ dissect_tftp(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree)
 static void
 tftp_dissect_options(tvbuff_t *tvb, int offset, proto_tree *tree)
 {
-	int i1, i2;
+	int option_len, value_len;
+	int value_offset;
 
 	while (tvb_offset_exists(tvb, offset)) {
-	  i1 = tvb_strnlen(tvb, offset, -1);	/* length of option */
-	  i2 = tvb_strnlen(tvb, offset+i1+1, -1);	/* length of value */
-	  proto_tree_add_text(tree, tvb, offset, i1+i2+2,
+	  option_len = tftp_strnlen(tvb, offset);	/* length of option */
+	  value_offset = offset + option_len;
+	  value_len = tftp_strnlen(tvb, value_offset);	/* length of value */
+	  proto_tree_add_text(tree, tvb, offset, option_len+value_len,
 	          "Option: %.*s = %.*s",
-		  i1, tvb_get_ptr(tvb, offset, i1),
-		  i2, tvb_get_ptr(tvb, offset+i1+1, i2));
-	  offset += i1 + i2 + 2;
+		  option_len - 1, tvb_get_ptr(tvb, offset, option_len - 1),
+		  value_len - 1, tvb_get_ptr(tvb, value_offset, value_len - 1));
+	  offset += option_len + value_len;
 	}
 }
 
+/*
+ * Find the length of a null-terminated string in a TFTP packet; if we
+ * don't find the '\0', throw an exception, as it means that either
+ * we didn't capture enough of the frame, or the frame is malformed.
+ *
+ * XXX - we'd like to know *which* exception to throw....
+ *
+ * XXX - this should probably be a standard tvbuff accessor.
+ *
+ * Add 1 to the resulting length, so that it includes the '\0'.
+ */
+static gint
+tftp_strnlen(tvbuff_t *tvb, gint offset)
+{
+	gint len;
+
+	len = tvb_strnlen(tvb, offset, -1);
+	if (len == -1) {
+	    /*
+	     * No '\0' found before the end of the tvbuff; throw an
+	     * exception.
+	     */
+	    THROW(BoundsError);
+	}
+	return len + 1;
+}
+
 void
 proto_register_tftp(void)
 {