cifs: validate full SID length in security descriptors

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>
Signed-off-by: Steve French <stfrench@microsoft.com>

diff --git a/fs/smb/client/cifsacl.c b/fs/smb/client/cifsacl.c
index 786dbbc43c5b9991eb13074fbe03a27b4bc44ca1..42a3115359dac5faedfc8ea8327d372fbb250662 100644 (file)
--- 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 @@ finalize_dacl:
        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 */