Security descriptor parse bug

[Sridhar Venkatakrishnan]

Hi,

I'm working on a VFS module we've developed that provides NT security
semantics and support for alternate data streams. We are currently storing
the security descriptors in the extended attributes after marshalling the
SEC_DESC structure. While doing this we encountered the following problem :

NT seems to be sending a few ( 4 ) extra bytes as padding when it sends an
ACE containing a well known SID. Samba unmarshalls the ACE's properly if
they are sent from XP, but gets the sizes wrong if they are sent from NT as
the extra bytes sent by NT are thrown away. The result is that the offsets
that are stored in the SEC_DESC structure turn out to be wrong. When samba
tries to marshall a structure with the incorrect offsets it runs out of
memory and sends an NT_STATUS_ACCESS_DENIED.

I've attached a patch that should fix this problem.

What the patch basically does is calculate the actual number of bytes read
for an ACL and then re-calculates the offsets in the security descriptor.

Sridhar
-------------- next part --------------
--- parse_sec.c	2004-11-16 08:33:24.000000000 +0530
+++ parse_sec.new.c	2004-12-12 17:20:44.000000000 +0530
@@ -53,6 +53,7 @@
 {
 	uint32 old_offset;
 	uint32 offset_ace_size;
+	uint32 ace_length;
 
 	if (psa == NULL)
 		return False;
@@ -94,8 +95,18 @@
 			return False;
 	}
 
+	ace_length = prs_offset(ps) - old_offset;
+	
 	if(!prs_uint16_post("size ", ps, depth, &psa->size, offset_ace_size, old_offset))
 		return False;
+	
+	/* Now handle the fact that NT seems to send extra bytes for some well-known 
+	 * sids. We have to handle this while unmarshalling because then we dont have 
+	 * inconsistent security descriptor structures
+	 */
+	if(UNMARSHALLING(ps) && (ace_length != psa->size)) 
+		psa->size = ace_length;
+	
 	return True;
 }
 
@@ -111,8 +123,10 @@
 	unsigned int i;
 	uint32 old_offset;
 	uint32 offset_acl_size;
+	uint32 acl_parse_size = 0;
 	SEC_ACL *psa;
 
+
 	/*
 	 * Note that the size is always a multiple of 4 bytes due to the
 	 * nature of the data structure.  Therefore the prs_align() calls
@@ -148,6 +162,9 @@
 	if(!prs_uint32("num_aces ", ps, depth, &psa->num_aces))
 		return False;
 
+	/* Count the number of bytes read into the acl structure */ 
+	acl_parse_size = sizeof(uint16) * 2 + sizeof(uint32);
+
 	if (UNMARSHALLING(ps)) {
 		/*
 		 * Even if the num_aces is zero, allocate memory as there's a difference
@@ -163,15 +180,58 @@
 		slprintf(tmp, sizeof(tmp)-1, "ace_list[%02d]: ", i);
 		if(!sec_io_ace(tmp, &psa->ace[i], ps, depth))
 			return False;
+		
+		/* Handle the fact that ace sizes of well known sids differ when sent from 
+		 * NT and Win2K/XP */
+		acl_parse_size += psa->ace[i].size;		
 	}
 
 	if(!prs_uint16_post("size     ", ps, depth, &psa->size, offset_acl_size, old_offset))
 		return False;
 
+	/* Reset the acl size to the actual size based on the format we're storing it in */	
+	if(UNMARSHALLING(ps) && (acl_parse_size != psa->size))
+		psa->size = acl_parse_size;
+	
 	return True;
 }
 
 /*******************************************************************
+ * Resets offsets in the security descriptor while unmarshalling to 
+ * ensure that the offsets stored in the security descriptor are not
+ * invalid.
+ * 
+ * Assumes that the security descriptor structure has been 
+ * unmarshalled and that the differences in the size of the acls has
+ * been calculated
+ ******************************************************************/
+BOOL _sec_reset_sec_desc_offsets(SEC_DESC *psd, prs_struct *ps, uint32 dacl_diff, uint32 sacl_diff)
+{
+	if(!psd || MARSHALLING(ps))
+		return False;
+		
+	if(psd->off_owner_sid > psd->off_dacl)
+		psd->off_owner_sid -= dacl_diff;	
+	
+	if(psd->off_owner_sid > psd->off_sacl)
+		psd->off_owner_sid -= sacl_diff;	
+	
+	if(psd->off_grp_sid > psd->off_dacl)
+		psd->off_grp_sid -= dacl_diff;	
+
+	if(psd->off_grp_sid > psd->off_sacl)
+		psd->off_grp_sid -= sacl_diff;	
+	
+	if(psd->off_dacl > psd->off_sacl)
+		psd->off_dacl -= sacl_diff;	
+	else
+		psd->off_sacl -= dacl_diff;	
+	
+	return True;
+}
+
+
+/*******************************************************************
  Reads or writes a SEC_DESC structure.
  If reading and the *ppsd = NULL, allocates the structure.
 ********************************************************************/
@@ -181,7 +241,8 @@
 	uint32 old_offset;
 	uint32 max_offset = 0; /* after we're done, move offset to end */
 	uint32 tmp_offset = 0;
-	
+	uint32 diff_dacl = 0;
+	uint32 diff_sacl = 0;
 	SEC_DESC *psd;
 
 	if (ppsd == NULL)
@@ -282,8 +343,10 @@
 		tmp_offset = prs_offset(ps);
 		if(!prs_set_offset(ps, old_offset + psd->off_sacl))
 			return False;
+		diff_sacl = prs_offset(ps);
 		if(!sec_io_acl("sacl", &psd->sacl, ps, depth))
 			return False;
+		diff_sacl = prs_offset(ps) - diff_sacl - psd->sacl->size;	
 		max_offset = MAX(max_offset, prs_offset(ps));
 		if (!prs_set_offset(ps,tmp_offset))
 			return False;
@@ -294,8 +357,10 @@
 		tmp_offset = prs_offset(ps);
 		if(!prs_set_offset(ps, old_offset + psd->off_dacl))
 			return False;
+		diff_dacl = prs_offset(ps);
 		if(!sec_io_acl("dacl", &psd->dacl, ps, depth))
 			return False;
+		diff_dacl = prs_offset(ps) - diff_dacl - psd->dacl->size;	
 		max_offset = MAX(max_offset, prs_offset(ps));
 		if (!prs_set_offset(ps,tmp_offset))
 			return False;
@@ -303,6 +368,12 @@
 
 	if(!prs_set_offset(ps, max_offset))
 		return False;
+		
+	if(UNMARSHALLING(ps) && (diff_dacl || diff_sacl)) {
+		if(!_sec_reset_sec_desc_offsets(*ppsd, ps, diff_dacl, diff_sacl))
+			return False;
+	}
+	
 	return True;
 }