[RFC PATCH] cifs: validate full SID length in security descriptors

[Qihang <q.h.hack.winter@gmail.com>]

parse_sid() only verified that the fixed SID header fit in the
returned security descriptor, but did not verify that the full SID
body described by num_subauth was present.

A malicious server can return a truncated owner or group SID whose
header lies within the descriptor buffer while sub_auth[] extends
past the end of the allocation, leading to an out-of-bounds read
when the client later parses or copies that SID.

Validate the full SID body in parse_sid(), centralize owner/group SID
lookup and bounds checking in sid_from_sd(), and use that validation
in parse_sec_desc(), build_sec_desc(), and copy_sec_desc() before
sub_auth[] is accessed.

Signed-off-by: Qihang <q.h.hack.winter@gmail.com>
---
 fs/smb/client/cifsacl.c | 196 ++++++++++++++++++++++++++--------------
 1 file changed, 129 insertions(+), 67 deletions(-)

diff --git a/fs/smb/client/cifsacl.c b/fs/smb/client/cifsacl.c
index 786dbbc43c5b..42a3115359da 100644
--- a/fs/smb/client/cifsacl.c
+++ b/fs/smb/client/cifsacl.c
@@ -275,6 +275,73 @@ cifs_copy_sid(struct smb_sid *dst, const struct smb_sid *src)
 	return size;
 }
 
+static int parse_sid(const struct smb_sid *psid, const char *end_of_acl)
+{
+	unsigned int sid_len;
+
+	/* SID must contain the fixed header before num_subauth is trusted. */
+	if (end_of_acl < (const char *)psid + CIFS_SID_BASE_SIZE) {
+		cifs_dbg(VFS, "ACL too small to parse SID %p\n", psid);
+		return -EINVAL;
+	}
+	if (psid->num_subauth > SID_MAX_SUB_AUTHORITIES) {
+		cifs_dbg(VFS, "SID contains too many subauthorities %u\n",
+			 psid->num_subauth);
+		return -EINVAL;
+	}
+
+	sid_len = CIFS_SID_BASE_SIZE + psid->num_subauth * sizeof(__le32);
+	if (end_of_acl < (const char *)psid + sid_len) {
+		cifs_dbg(VFS, "ACL too small to parse SID %p\n", psid);
+		return -EINVAL;
+	}
+
+#ifdef CONFIG_CIFS_DEBUG2
+	if (psid->num_subauth) {
+		int i;
+
+		cifs_dbg(FYI, "SID revision %d num_auth %d\n",
+			 psid->revision, psid->num_subauth);
+
+		for (i = 0; i < psid->num_subauth; i++) {
+			cifs_dbg(FYI, "SID sub_auth[%d]: 0x%x\n",
+				 i, le32_to_cpu(psid->sub_auth[i]));
+		}
+
+		cifs_dbg(FYI, "RID 0x%x\n",
+			 le32_to_cpu(psid->sub_auth[psid->num_subauth - 1]));
+	}
+#endif
+
+	return 0;
+}
+
+static int sid_from_sd(const struct smb_ntsd *pntsd, __u32 secdesclen,
+		       __u32 sid_offset, struct smb_sid **sid)
+{
+	struct smb_sid *psid;
+	char *end_of_acl;
+
+	if (secdesclen < sizeof(struct smb_ntsd)) {
+		cifs_dbg(VFS, "ACL too small to parse security descriptor\n");
+		return -EINVAL;
+	}
+	end_of_acl = (char *)pntsd + secdesclen;
+
+	if (sid_offset < sizeof(struct smb_ntsd) ||
+	    sid_offset > secdesclen - CIFS_SID_BASE_SIZE) {
+		cifs_dbg(VFS, "Server returned illegal SID offset\n");
+		return -EINVAL;
+	}
+
+	psid = (struct smb_sid *)((char *)pntsd + sid_offset);
+	if (parse_sid(psid, end_of_acl))
+		return -EINVAL;
+
+	*sid = psid;
+	return 0;
+}
+
 static int
 id_to_sid(unsigned int cid, uint sidtype, struct smb_sid *ssid)
 {
@@ -515,14 +582,14 @@ exit_cifs_idmap(void)
 }
 
 /* copy ntsd, owner sid, and group sid from a security descriptor to another */
-static __u32 copy_sec_desc(const struct smb_ntsd *pntsd,
-				struct smb_ntsd *pnntsd,
-				__u32 sidsoffset,
-				struct smb_sid *pownersid,
-				struct smb_sid *pgrpsid)
+static int copy_sec_desc(const struct smb_ntsd *pntsd, struct smb_ntsd *pnntsd,
+			 __u32 sidsoffset, __u32 secdesclen,
+			 __u32 *pnsecdesclen, struct smb_sid *pownersid,
+			 struct smb_sid *pgrpsid)
 {
 	struct smb_sid *owner_sid_ptr, *group_sid_ptr;
 	struct smb_sid *nowner_sid_ptr, *ngroup_sid_ptr;
+	int rc;
 
 	/* copy security descriptor control portion */
 	pnntsd->revision = pntsd->revision;
@@ -533,28 +600,34 @@ static __u32 copy_sec_desc(const struct smb_ntsd *pntsd,
 	pnntsd->gsidoffset = cpu_to_le32(sidsoffset + sizeof(struct smb_sid));
 
 	/* copy owner sid */
-	if (pownersid)
+	if (pownersid) {
 		owner_sid_ptr = pownersid;
-	else
-		owner_sid_ptr = (struct smb_sid *)((char *)pntsd +
-				le32_to_cpu(pntsd->osidoffset));
+	} else {
+		rc = sid_from_sd(pntsd, secdesclen,
+				 le32_to_cpu(pntsd->osidoffset), &owner_sid_ptr);
+		if (rc)
+			return rc;
+	}
 	nowner_sid_ptr = (struct smb_sid *)((char *)pnntsd + sidsoffset);
 	cifs_copy_sid(nowner_sid_ptr, owner_sid_ptr);
 
 	/* copy group sid */
-	if (pgrpsid)
+	if (pgrpsid) {
 		group_sid_ptr = pgrpsid;
-	else
-		group_sid_ptr = (struct smb_sid *)((char *)pntsd +
-				le32_to_cpu(pntsd->gsidoffset));
+	} else {
+		rc = sid_from_sd(pntsd, secdesclen,
+				 le32_to_cpu(pntsd->gsidoffset), &group_sid_ptr);
+		if (rc)
+			return rc;
+	}
 	ngroup_sid_ptr = (struct smb_sid *)((char *)pnntsd + sidsoffset +
 					sizeof(struct smb_sid));
 	cifs_copy_sid(ngroup_sid_ptr, group_sid_ptr);
 
-	return sidsoffset + (2 * sizeof(struct smb_sid));
+	*pnsecdesclen = sidsoffset + (2 * sizeof(struct smb_sid));
+	return 0;
 }
 
-
 /*
    change posix mode to reflect permissions
    pmode is the existing mode (we only want to overwrite part of this
@@ -1232,38 +1305,6 @@ static int set_chmod_dacl(struct smb_acl *pdacl, struct smb_acl *pndacl,
 	return 0;
 }
 
-static int parse_sid(struct smb_sid *psid, char *end_of_acl)
-{
-	/* BB need to add parm so we can store the SID BB */
-
-	/* validate that we do not go past end of ACL - sid must be at least 8
-	   bytes long (assuming no sub-auths - e.g. the null SID */
-	if (end_of_acl < (char *)psid + 8) {
-		cifs_dbg(VFS, "ACL too small to parse SID %p\n", psid);
-		return -EINVAL;
-	}
-
-#ifdef CONFIG_CIFS_DEBUG2
-	if (psid->num_subauth) {
-		int i;
-		cifs_dbg(FYI, "SID revision %d num_auth %d\n",
-			 psid->revision, psid->num_subauth);
-
-		for (i = 0; i < psid->num_subauth; i++) {
-			cifs_dbg(FYI, "SID sub_auth[%d]: 0x%x\n",
-				 i, le32_to_cpu(psid->sub_auth[i]));
-		}
-
-		/* BB add length check to make sure that we do not have huge
-			num auths and therefore go off the end */
-		cifs_dbg(FYI, "RID 0x%x\n",
-			 le32_to_cpu(psid->sub_auth[psid->num_subauth-1]));
-	}
-#endif
-
-	return 0;
-}
-
 static bool dacl_offset_valid(unsigned int acl_len, __u32 dacloffset)
 {
 	if (acl_len < sizeof(struct smb_acl))
@@ -1284,23 +1325,25 @@ static int parse_sec_desc(struct cifs_sb_info *cifs_sb,
 	int rc = 0;
 	struct smb_sid *owner_sid_ptr, *group_sid_ptr;
 	struct smb_acl *dacl_ptr; /* no need for SACL ptr */
-	char *end_of_acl = ((char *)pntsd) + acl_len;
-	__u32 dacloffset;
+	char *end_of_acl;
+	__u32 dacloffset, osidoffset, gsidoffset;
 
 	if (pntsd == NULL)
 		return smb_EIO(smb_eio_trace_null_pointers);
+	if (acl_len < (int)sizeof(struct smb_ntsd)) {
+		cifs_dbg(VFS, "ACL too small to parse security descriptor\n");
+		return -EINVAL;
+	}
+	end_of_acl = ((char *)pntsd) + acl_len;
 
-	owner_sid_ptr = (struct smb_sid *)((char *)pntsd +
-				le32_to_cpu(pntsd->osidoffset));
-	group_sid_ptr = (struct smb_sid *)((char *)pntsd +
-				le32_to_cpu(pntsd->gsidoffset));
+	osidoffset = le32_to_cpu(pntsd->osidoffset);
+	gsidoffset = le32_to_cpu(pntsd->gsidoffset);
 	dacloffset = le32_to_cpu(pntsd->dacloffset);
 	cifs_dbg(NOISY, "revision %d type 0x%x ooffset 0x%x goffset 0x%x sacloffset 0x%x dacloffset 0x%x\n",
-		 pntsd->revision, pntsd->type, le32_to_cpu(pntsd->osidoffset),
-		 le32_to_cpu(pntsd->gsidoffset),
+		 pntsd->revision, pntsd->type, osidoffset, gsidoffset,
 		 le32_to_cpu(pntsd->sacloffset), dacloffset);
 /*	cifs_dump_mem("owner_sid: ", owner_sid_ptr, 64); */
-	rc = parse_sid(owner_sid_ptr, end_of_acl);
+	rc = sid_from_sd(pntsd, acl_len, osidoffset, &owner_sid_ptr);
 	if (rc) {
 		cifs_dbg(FYI, "%s: Error %d parsing Owner SID\n", __func__, rc);
 		return rc;
@@ -1312,9 +1355,9 @@ static int parse_sec_desc(struct cifs_sb_info *cifs_sb,
 		return rc;
 	}
 
-	rc = parse_sid(group_sid_ptr, end_of_acl);
+	rc = sid_from_sd(pntsd, acl_len, gsidoffset, &group_sid_ptr);
 	if (rc) {
-		cifs_dbg(FYI, "%s: Error %d mapping Owner SID to gid\n",
+		cifs_dbg(FYI, "%s: Error %d parsing Group SID\n",
 			 __func__, rc);
 		return rc;
 	}
@@ -1354,8 +1397,15 @@ static int build_sec_desc(struct smb_ntsd *pntsd, struct smb_ntsd *pnntsd,
 	struct smb_sid *nowner_sid_ptr = NULL, *ngroup_sid_ptr = NULL;
 	struct smb_acl *dacl_ptr = NULL;  /* no need for SACL ptr */
 	struct smb_acl *ndacl_ptr = NULL; /* no need for SACL ptr */
-	char *end_of_acl = ((char *)pntsd) + secdesclen;
+	char *end_of_acl;
 	u16 size = 0;
+	__u32 osidoffset, gsidoffset;
+
+	if (secdesclen < sizeof(struct smb_ntsd)) {
+		cifs_dbg(VFS, "ACL too small to parse security descriptor\n");
+		return -EINVAL;
+	}
+	end_of_acl = ((char *)pntsd) + secdesclen;
 
 	dacloffset = le32_to_cpu(pntsd->dacloffset);
 	if (dacloffset) {
@@ -1370,10 +1420,18 @@ static int build_sec_desc(struct smb_ntsd *pntsd, struct smb_ntsd *pnntsd,
 			return rc;
 	}
 
-	owner_sid_ptr = (struct smb_sid *)((char *)pntsd +
-			le32_to_cpu(pntsd->osidoffset));
-	group_sid_ptr = (struct smb_sid *)((char *)pntsd +
-			le32_to_cpu(pntsd->gsidoffset));
+	osidoffset = le32_to_cpu(pntsd->osidoffset);
+	gsidoffset = le32_to_cpu(pntsd->gsidoffset);
+	rc = sid_from_sd(pntsd, secdesclen, osidoffset, &owner_sid_ptr);
+	if (rc) {
+		cifs_dbg(FYI, "%s: Error %d parsing Owner SID\n", __func__, rc);
+		return rc;
+	}
+	rc = sid_from_sd(pntsd, secdesclen, gsidoffset, &group_sid_ptr);
+	if (rc) {
+		cifs_dbg(FYI, "%s: Error %d parsing Group SID\n", __func__, rc);
+		return rc;
+	}
 
 	if (pnmode && *pnmode != NO_CHANGE_64) { /* chmod */
 		ndacloffset = sizeof(struct smb_ntsd);
@@ -1389,8 +1447,10 @@ static int build_sec_desc(struct smb_ntsd *pntsd, struct smb_ntsd *pnntsd,
 
 		sidsoffset = ndacloffset + le16_to_cpu(ndacl_ptr->size);
 		/* copy the non-dacl portion of secdesc */
-		*pnsecdesclen = copy_sec_desc(pntsd, pnntsd, sidsoffset,
-				NULL, NULL);
+		rc = copy_sec_desc(pntsd, pnntsd, sidsoffset, secdesclen,
+				   pnsecdesclen, NULL, NULL);
+		if (rc)
+			return rc;
 
 		*aclflag |= CIFS_ACL_DACL;
 	} else {
@@ -1467,8 +1527,10 @@ static int build_sec_desc(struct smb_ntsd *pntsd, struct smb_ntsd *pnntsd,
 
 		sidsoffset = ndacloffset + le16_to_cpu(ndacl_ptr->size);
 		/* copy the non-dacl portion of secdesc */
-		*pnsecdesclen = copy_sec_desc(pntsd, pnntsd, sidsoffset,
-				nowner_sid_ptr, ngroup_sid_ptr);
+		rc = copy_sec_desc(pntsd, pnntsd, sidsoffset, secdesclen,
+				   pnsecdesclen, nowner_sid_ptr, ngroup_sid_ptr);
+		if (rc)
+			goto chown_chgrp_exit;
 
 chown_chgrp_exit:
 		/* errors could jump here. So make sure we return soon after this */
-- 
2.39.5 (Apple Git-154)