ksmbd: validate inherited ACE SID length

smb_inherit_dacl() walks the parent directory DACL loaded from the
security descriptor xattr. It verifies that each ACE contains the fixed
SID header before using it, but does not verify that the variable-length
SID described by sid.num_subauth is fully contained in the ACE.

A malformed inheritable ACE can advertise more subauthorities than are
present in the ACE. compare_sids() may then read past the ACE.
smb_set_ace() also clamps the copied destination SID, but used the
unchecked source SID count to compute the inherited ACE size. That could
advance the temporary inherited ACE buffer pointer and nt_size accounting
past the allocated buffer.

Fix this by validating the parent ACE SID count and SID length before
using the SID during inheritance. Compute the inherited ACE size from the
copied SID so the size matches the bounded destination SID. Reject the
inherited DACL if size accumulation would overflow smb_acl.size or the
security descriptor allocation size.

Fixes: e2f34481b24d ("cifsd: add server-side procedures for SMB3")
Signed-off-by: Shota Zaizen <s@zaizen.me>
Acked-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>

diff --git a/fs/smb/server/smbacl.c b/fs/smb/server/smbacl.c
index 4bbc2c27e6805ee12173a21d1cd13c8e5d7df349..c1d1f34581d69d0665d95a34d13ded291ae14b9b 100644 (file)
--- a/fs/smb/server/smbacl.c
+++ b/fs/smb/server/smbacl.c
@@ -1068,7 +1068,26 @@ static void smb_set_ace(struct smb_ace *ace, const struct smb_sid *sid, u8 type,
        ace->flags = flags;
        ace->access_req = access_req;
        smb_copy_sid(&ace->sid, sid);
-       ace->size = cpu_to_le16(1 + 1 + 2 + 4 + 1 + 1 + 6 + (sid->num_subauth * 4));
+       ace->size = cpu_to_le16(1 + 1 + 2 + 4 + 1 + 1 + 6 +
+                               (ace->sid.num_subauth * 4));
+}
+
+static int smb_append_inherited_ace(struct smb_ace **ace, int *nt_size,
+                                   u16 *ace_cnt, const struct smb_sid *sid,
+                                   u8 type, u8 flags, __le32 access_req)
+{
+       int ace_size;
+
+       smb_set_ace(*ace, sid, type, flags, access_req);
+       ace_size = le16_to_cpu((*ace)->size);
+       /* pdacl->size is __le16 and includes struct smb_acl. */
+       if (check_add_overflow(*nt_size, ace_size, nt_size) ||
+           *nt_size > U16_MAX - (int)sizeof(struct smb_acl))
+               return -EINVAL;
+
+       (*ace_cnt)++;
+       *ace = (struct smb_ace *)((char *)*ace + ace_size);
+       return 0;
 }
 
 int smb_inherit_dacl(struct ksmbd_conn *conn,
@@ -1157,6 +1176,12 @@ int smb_inherit_dacl(struct ksmbd_conn *conn,
                                CIFS_SID_BASE_SIZE)
                        break;
 
+               if (parent_aces->sid.num_subauth > SID_MAX_SUB_AUTHORITIES ||
+                   pace_size < offsetof(struct smb_ace, sid) +
+                               CIFS_SID_BASE_SIZE +
+                               sizeof(__le32) * parent_aces->sid.num_subauth)
+                       break;
+
                aces_size -= pace_size;
 
                flags = parent_aces->flags;
@@ -1186,22 +1211,24 @@ int smb_inherit_dacl(struct ksmbd_conn *conn,
                }
 
                if (is_dir && creator && flags & CONTAINER_INHERIT_ACE) {
-                       smb_set_ace(aces, psid, parent_aces->type, inherited_flags,
-                                   parent_aces->access_req);
-                       nt_size += le16_to_cpu(aces->size);
-                       ace_cnt++;
-                       aces = (struct smb_ace *)((char *)aces + le16_to_cpu(aces->size));
+                       rc = smb_append_inherited_ace(&aces, &nt_size, &ace_cnt,
+                                                     psid, parent_aces->type,
+                                                     inherited_flags,
+                                                     parent_aces->access_req);
+                       if (rc)
+                               goto free_aces_base;
                        flags |= INHERIT_ONLY_ACE;
                        psid = creator;
                } else if (is_dir && !(parent_aces->flags & NO_PROPAGATE_INHERIT_ACE)) {
                        psid = &parent_aces->sid;
                }
 
-               smb_set_ace(aces, psid, parent_aces->type, flags | inherited_flags,
-                           parent_aces->access_req);
-               nt_size += le16_to_cpu(aces->size);
-               aces = (struct smb_ace *)((char *)aces + le16_to_cpu(aces->size));
-               ace_cnt++;
+               rc = smb_append_inherited_ace(&aces, &nt_size, &ace_cnt, psid,
+                                             parent_aces->type,
+                                             flags | inherited_flags,
+                                             parent_aces->access_req);
+               if (rc)
+                       goto free_aces_base;
 pass:
                parent_aces = (struct smb_ace *)((char *)parent_aces + pace_size);
        }
@@ -1211,7 +1238,7 @@ pass:
                struct smb_acl *pdacl;
                struct smb_sid *powner_sid = NULL, *pgroup_sid = NULL;
                int powner_sid_size = 0, pgroup_sid_size = 0, pntsd_size;
-               int pntsd_alloc_size;
+               size_t pntsd_alloc_size;
 
                if (parent_pntsd->osidoffset) {
                        powner_sid = (struct smb_sid *)((char *)parent_pntsd +
@@ -1224,8 +1251,19 @@ pass:
                        pgroup_sid_size = 1 + 1 + 6 + (pgroup_sid->num_subauth * 4);
                }
 
-               pntsd_alloc_size = sizeof(struct smb_ntsd) + powner_sid_size +
-                       pgroup_sid_size + sizeof(struct smb_acl) + nt_size;
+               if (check_add_overflow(sizeof(struct smb_ntsd),
+                                      (size_t)powner_sid_size,
+                                      &pntsd_alloc_size) ||
+                   check_add_overflow(pntsd_alloc_size,
+                                      (size_t)pgroup_sid_size,
+                                      &pntsd_alloc_size) ||
+                   check_add_overflow(pntsd_alloc_size, sizeof(struct smb_acl),
+                                      &pntsd_alloc_size) ||
+                   check_add_overflow(pntsd_alloc_size, (size_t)nt_size,
+                                      &pntsd_alloc_size)) {
+                       rc = -EINVAL;
+                       goto free_aces_base;
+               }
 
                pntsd = kzalloc(pntsd_alloc_size, KSMBD_DEFAULT_GFP);
                if (!pntsd) {