diff options
author | Guy Harris <guy@alum.mit.edu> | 2005-10-04 10:23:40 +0000 |
---|---|---|
committer | Guy Harris <guy@alum.mit.edu> | 2005-10-04 10:23:40 +0000 |
commit | 2a8e47b11a19e61a9df3e7322e54cd0c00dbd25b (patch) | |
tree | 9cd7888f1053b91641991667ae98a5dbe813787d /epan | |
parent | 72dd04b43c973bb1f22bedf682909237efed2c1c (diff) |
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
Diffstat (limited to 'epan')
-rw-r--r-- | epan/dissectors/packet-dcerpc-lsa.c | 4 | ||||
-rw-r--r-- | epan/dissectors/packet-dcerpc-samr.c | 5 | ||||
-rw-r--r-- | epan/dissectors/packet-dcerpc-spoolss.c | 12 | ||||
-rw-r--r-- | epan/dissectors/packet-smb.c | 8 | ||||
-rw-r--r-- | epan/dissectors/packet-windows-common.c | 113 | ||||
-rw-r--r-- | epan/dissectors/packet-windows-common.h | 3 |
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 |