Don't ensure all the bytes of a security descriptor exist before calling

"dissect_nt_sec_desc()".  Add a Boolean argument to
"dissect_nt_sec_desc()" to indicate whether a length was passed to it
(so we don't treat -1 as a special value; we want to stop treating -1 as
a special length value, and, in fact, want to stop treating *any*
negative length values specially, so that we don't have to worry about
passing arbitrary 32-bit values from packets as lengths), and have
"dissect_nt_sec_desc()" initially create the protocol tree item for the
security descriptor with a length of "go to the end of the tvbuff", and
set the length once we're done dissecting it - and, if the length was
specified, check at *that* point, *after* we've dissected the security
descriptor, whether we have the entire security descriptor in the
tvbuff.

That means that we don't have to worry about overflows after
"dissect_nt_sec_desc()" returns - if the length was so large that we
would have gotten an overflow, we'd have thrown an exception in the
"tvb_ensure_bytes_exist()" call at the end of "dissect_nt_sec_desc()".

Do sanity checks on offsets within the security descriptor, so we know
the item referred to by the offset is after the fixed-length portion of
the descriptor.

svn path=/trunk/; revision=16113

6 files changed, 104 insertions, 41 deletions

diff --git a/epan/dissectors/packet-dcerpc-lsa.c b/epan/dissectors/packet-dcerpc-lsa.c
index 9332ba47c5..a933db15ca 100644
--- a/epan/dissectors/packet-dcerpc-lsa.c
+++ b/epan/dissectors/packet-dcerpc-lsa.c
@@ -385,9 +385,9 @@ lsa_dissect_sec_desc_buf_data(tvbuff_t *tvb, int offset,
 	offset = dissect_ndr_uint32 (tvb, offset, pinfo, tree, drep,
 				     hf_lsa_sd_size, &len);
 
-	tvb_ensure_bytes_exist(tvb, offset, len);
 	dissect_nt_sec_desc(
-		tvb, offset, pinfo, tree, drep, len, &lsa_access_mask_info);
+		tvb, offset, pinfo, tree, drep, TRUE, len,
+		&lsa_access_mask_info);
 
 	offset += len;
 
diff --git a/epan/dissectors/packet-dcerpc-samr.c b/epan/dissectors/packet-dcerpc-samr.c
index 0d2204c5f8..cb672e0422 100644
--- a/epan/dissectors/packet-dcerpc-samr.c
+++ b/epan/dissectors/packet-dcerpc-samr.c
@@ -259,11 +259,10 @@ sam_dissect_SAM_SECURITY_DESCRIPTOR_data(tvbuff_t *tvb, int offset,
                                     hf_samr_sd_size, &len);
 
        dissect_nt_sec_desc(
-               tvb, offset, pinfo, tree, drep, len, &samr_connect_access_mask_info);
+               tvb, offset, pinfo, tree, drep, TRUE, len,
+               &samr_connect_access_mask_info);
 
        offset += len;
-       if (offset < old_offset)
-               THROW(ReportedBoundsError);
 
        return offset;
 }
diff --git a/epan/dissectors/packet-dcerpc-spoolss.c b/epan/dissectors/packet-dcerpc-spoolss.c
index 42ce9c0b95..9dd5a95538 100644
--- a/epan/dissectors/packet-dcerpc-spoolss.c
+++ b/epan/dissectors/packet-dcerpc-spoolss.c
@@ -2275,17 +2275,13 @@ static int dissect_PRINTER_INFO_2(tvbuff_t *tvb, int offset,
 	 * lacking the "len" argument, so that won't work.
 	 */
 
-	/* TODO: I think the length is only used to fix up the hex display
-	   pane.  We should be able to use proto_item_set_len() to avoid
-	   having to calculate the length. -tpot */
-
 	offset = dissect_ndr_uint32(
 		tvb, offset, pinfo, NULL, drep, hf_offset,
 		&secdesc_offset);
 
 	dissect_nt_sec_desc(
 		tvb, secdesc_offset, pinfo, tree, drep, 
-		tvb_length_remaining(tvb, secdesc_offset),
+		FALSE, -1,
 		&spoolss_printer_access_mask_info);
 
 	offset = dissect_printer_attributes(tvb, offset, pinfo, tree, drep);
@@ -2335,7 +2331,7 @@ static int dissect_PRINTER_INFO_3(tvbuff_t *tvb, int offset,
 	
 	offset = dissect_nt_sec_desc(
 		tvb, offset, pinfo, tree, drep, 
-		tvb_length_remaining(tvb, offset), 
+		FALSE, -1,
 		&spoolss_printer_access_mask_info);
 
 	return offset;
@@ -3413,7 +3409,7 @@ dissect_SEC_DESC_BUF(tvbuff_t *tvb, int offset, packet_info *pinfo,
                 hf_secdescbuf_len, &len);
 	
 	dissect_nt_sec_desc(
-		tvb, offset, pinfo, subtree, drep, len, 
+		tvb, offset, pinfo, subtree, drep, TRUE, len, 
 		&spoolss_printer_access_mask_info);
 
 	offset += len;	
@@ -4538,7 +4534,7 @@ dissect_spoolss_JOB_INFO_2(tvbuff_t *tvb, int offset, packet_info *pinfo,
 
 	dissect_nt_sec_desc(
 		tvb, secdesc_offset, pinfo, subtree, drep,
-		tvb_length_remaining(tvb, secdesc_offset),
+		FALSE, -1,
 		&spoolss_job_access_mask_info);
 
 	offset = dissect_job_status(tvb, offset, pinfo, subtree, drep);
diff --git a/epan/dissectors/packet-smb.c b/epan/dissectors/packet-smb.c
index 278069f6b0..18ae8d8d8b 100644
--- a/epan/dissectors/packet-smb.c
+++ b/epan/dissectors/packet-smb.c
@@ -7283,8 +7283,8 @@ dissect_nt_trans_data_request(tvbuff_t *tvb, packet_info *pinfo, int offset, pro
 		/* security descriptor */
 		if(ntd->sd_len){
 		        offset = dissect_nt_sec_desc(
-				tvb, offset, pinfo, tree, NULL, ntd->sd_len, 
-				NULL);
+				tvb, offset, pinfo, tree, NULL, TRUE,
+				ntd->sd_len, NULL);
 		}
 
 		/* extended attributes */
@@ -7302,7 +7302,7 @@ dissect_nt_trans_data_request(tvbuff_t *tvb, packet_info *pinfo, int offset, pro
 		break;
 	case NT_TRANS_SSD:
 		offset = dissect_nt_sec_desc(
-			tvb, offset, pinfo, tree, NULL, bc, NULL);
+			tvb, offset, pinfo, tree, NULL, TRUE, bc, NULL);
 		break;
 	case NT_TRANS_NOTIFY:
 		break;
@@ -7814,7 +7814,7 @@ dissect_nt_trans_data_response(tvbuff_t *tvb, packet_info *pinfo,
 		 * somewhere.
 		 */
 		offset = dissect_nt_sec_desc(
-			tvb, offset, pinfo, tree, NULL, len, NULL);
+			tvb, offset, pinfo, tree, NULL, TRUE, len, NULL);
 		break;
 	}
 	case NT_TRANS_GET_USER_QUOTA:
diff --git a/epan/dissectors/packet-windows-common.c b/epan/dissectors/packet-windows-common.c
index 7a46b2b122..c3b75fe655 100644
--- a/epan/dissectors/packet-windows-common.c
+++ b/epan/dissectors/packet-windows-common.c
@@ -2004,23 +2004,23 @@ dissect_nt_sec_desc_type(tvbuff_t *tvb, int offset, proto_tree *parent_tree)
 
 int
 dissect_nt_sec_desc(tvbuff_t *tvb, int offset, packet_info *pinfo,
-		    proto_tree *parent_tree, guint8 *drep, int len, 
+		    proto_tree *parent_tree, guint8 *drep,
+		    gboolean len_supplied, int len, 
 		    struct access_mask_info *ami)
 {
 	proto_item *item = NULL;
 	proto_tree *tree = NULL;
 	guint16 revision;
-	int old_offset = offset;
+	int start_offset = offset;
+	int item_offset, end_offset;
 	guint32 owner_sid_offset;
 	guint32 group_sid_offset;
 	guint32 sacl_offset;
 	guint32 dacl_offset;
 
-	if(parent_tree){
-		item = proto_tree_add_text(parent_tree, tvb, offset, len,
-					   "NT Security Descriptor");
-		tree = proto_item_add_subtree(item, ett_nt_sec_desc);
-	}
+	item = proto_tree_add_text(parent_tree, tvb, offset, -1,
+				   "NT Security Descriptor");
+	tree = proto_item_add_subtree(item, ett_nt_sec_desc);
 
 	/* revision */
 	revision = tvb_get_letohs(tvb, offset);
@@ -2028,7 +2028,6 @@ dissect_nt_sec_desc(tvbuff_t *tvb, int offset, packet_info *pinfo,
 		tvb, offset, 2, revision);
 	offset += 2;
 
-
 	switch(revision){
 	case 1:  /* only version we will ever see of this structure?*/
 	  /* type */
@@ -2036,52 +2035,120 @@ dissect_nt_sec_desc(tvbuff_t *tvb, int offset, packet_info *pinfo,
 
 	  /* offset to owner sid */
 	  owner_sid_offset = tvb_get_letohl(tvb, offset);
-	  proto_tree_add_text(tree, tvb, offset, 4, "Offset to owner SID: %u", owner_sid_offset);
+	  if(owner_sid_offset != 0 && owner_sid_offset < 20){
+	    /* Bogus value - points into fixed portion of descriptor */
+	    proto_tree_add_text(tree, tvb, offset, 4, "Offset to owner SID: %u (bogus, must be >= 20)", owner_sid_offset);
+	    owner_sid_offset = 0;
+	  } else
+	    proto_tree_add_text(tree, tvb, offset, 4, "Offset to owner SID: %u", owner_sid_offset);
 	  offset += 4;
 
 	  /* offset to group sid */
 	  group_sid_offset = tvb_get_letohl(tvb, offset);
-	  proto_tree_add_text(tree, tvb, offset, 4, "Offset to group SID: %u", group_sid_offset);
+	  if(group_sid_offset != 0 && group_sid_offset < 20){
+	    /* Bogus value - points into fixed portion of descriptor */
+	    proto_tree_add_text(tree, tvb, offset, 4, "Offset to group SID: %u (bogus, must be >= 20)", group_sid_offset);
+	    group_sid_offset = 0;
+	  } else
+	    proto_tree_add_text(tree, tvb, offset, 4, "Offset to group SID: %u", group_sid_offset);
 	  offset += 4;
 
 	  /* offset to sacl */
 	  sacl_offset = tvb_get_letohl(tvb, offset);
-	  proto_tree_add_text(tree, tvb, offset, 4, "Offset to SACL: %u", sacl_offset);
+	  if(sacl_offset != 0 && sacl_offset < 20){
+	    /* Bogus value - points into fixed portion of descriptor */
+	    proto_tree_add_text(tree, tvb, offset, 4, "Offset to SACL: %u (bogus, must be >= 20)", sacl_offset);
+	    sacl_offset = 0;
+	  } else
+	    proto_tree_add_text(tree, tvb, offset, 4, "Offset to SACL: %u", sacl_offset);
 	  offset += 4;
 
 	  /* offset to dacl */
 	  dacl_offset = tvb_get_letohl(tvb, offset);
-	  proto_tree_add_text(tree, tvb, offset, 4, "Offset to DACL: %u", dacl_offset);
+	  if(dacl_offset != 0 && dacl_offset < 20){
+	    /* Bogus value - points into fixed portion of descriptor */
+	    proto_tree_add_text(tree, tvb, offset, 4, "Offset to DACL: %u (bogus, must be >= 20)", dacl_offset);
+	    dacl_offset = 0;
+	  } else
+	    proto_tree_add_text(tree, tvb, offset, 4, "Offset to DACL: %u", dacl_offset);
 	  offset += 4;
 
+	  end_offset = offset;
+
 	  /*owner SID*/
 	  if(owner_sid_offset){
-	    if (len == -1)
-	      offset = dissect_nt_sid(tvb, offset, tree, "Owner", NULL, -1);
-	    else
-	      dissect_nt_sid(
-		      tvb, old_offset+owner_sid_offset, tree, "Owner", NULL, -1);
+	    item_offset = start_offset+owner_sid_offset;
+	    if (item_offset < start_offset) {
+	      /*
+	       * Overflow - throw an exception.
+	       */
+	      THROW(ReportedBoundsError);
+	    }
+	    offset = dissect_nt_sid(tvb, item_offset, tree, "Owner", NULL, -1);
+	    if (offset > end_offset)
+	      end_offset = offset;
 	  }
 
 	  /*group SID*/
 	  if(group_sid_offset){
-	    dissect_nt_sid(
-		    tvb, old_offset+group_sid_offset, tree, "Group", NULL, -1);
+	    item_offset = start_offset+group_sid_offset;
+	    if (item_offset < start_offset) {
+	      /*
+	       * Overflow - throw an exception.
+	       */
+	      THROW(ReportedBoundsError);
+	    }
+	    offset = dissect_nt_sid(tvb, item_offset, tree, "Group", NULL, -1);
+	    if (offset > end_offset)
+	      end_offset = offset;
 	  }
 
 	  /* sacl */
 	  if(sacl_offset){
-	    dissect_nt_acl(tvb, old_offset+sacl_offset, pinfo, tree, 
-			   drep, "System (SACL)", ami);
+	    item_offset = start_offset+sacl_offset;
+	    if (item_offset < start_offset) {
+	      /*
+	       * Overflow - throw an exception.
+	       */
+	      THROW(ReportedBoundsError);
+	    }
+	    offset = dissect_nt_acl(tvb, item_offset, pinfo, tree, 
+				    drep, "System (SACL)", ami);
+	    if (offset > end_offset)
+	      end_offset = offset;
 	  }
 
 	  /* dacl */
 	  if(dacl_offset){
-	    dissect_nt_acl(tvb, old_offset+dacl_offset, pinfo, tree, 
-			   drep, "User (DACL)", ami);
+	    item_offset = start_offset+dacl_offset;
+	    if (item_offset < start_offset) {
+	      /*
+	       * Overflow - throw an exception.
+	       */
+	      THROW(ReportedBoundsError);
+	    }
+	    offset = dissect_nt_acl(tvb, item_offset, pinfo, tree, 
+				    drep, "User (DACL)", ami);
+	    if (offset > end_offset)
+	      end_offset = offset;
 	  }
+	  break;
 
+	default:
+	  end_offset = offset;
+	  break;
+	}
+	if (len_supplied) {
+	  /* Make sure the length isn't too large (so that we get an
+	     overflow) */
+	  tvb_ensure_bytes_exist(tvb, start_offset, len);
+	} else {
+	  /* The length of the security descriptor is the difference
+	     between the starting offset and the offset past the last
+	     item in the descriptor. */
+	  len = end_offset - start_offset;
 	}
+	proto_item_set_len(item, len);
 
 	return offset+len;
 }
diff --git a/epan/dissectors/packet-windows-common.h b/epan/dissectors/packet-windows-common.h
index 8591614c25..ef9da37bbf 100644
--- a/epan/dissectors/packet-windows-common.h
+++ b/epan/dissectors/packet-windows-common.h
@@ -173,7 +173,8 @@ dissect_nt_access_mask(tvbuff_t *tvb, gint offset, packet_info *pinfo,
 
 int
 dissect_nt_sec_desc(tvbuff_t *tvb, int offset, packet_info *pinfo,
-		    proto_tree *parent_tree, guint8 *drep, int len, 
+		    proto_tree *parent_tree, guint8 *drep,
+		    gboolean len_supplied, int len,
 		    struct access_mask_info *ami);
 
 void