cifs: fix potential buffer overrun in cifs.idmap handling code

The userspace cifs.idmap program generally works with the wbclient libs
to generate binary SIDs in userspace. That program defines the struct
that holds these values as having a max of 15 subauthorities. The kernel
idmapping code however limits that value to 5.

When the kernel copies those values around though, it doesn't sanity
check the num_subauths value handed back from userspace or from the
server. It's possible therefore for userspace to hand us back a bogus
num_subauths value (or one that's valid, but greater than 5) that could
cause the kernel to walk off the end of the cifs_sid->sub_auths array.

Fix this by defining a new routine for copying sids and using that in
all of the places that copy it. If we end up with a sid that's longer
than expected then this approach will just lop off the "extra" subauths,
but that's basically what the code does today already. Better approaches
might be to fix this code to reject SIDs with >5 subauths, or fix it
to handle the subauths array dynamically.

At the same time, change the kernel to check the length of the data
returned by userspace. If it's shorter than struct cifs_sid, reject it
and return -EIO. If that happens we'll end up with fields that are
basically uninitialized.

Long term, it might make sense to redefine cifs_sid using a flexarray at
the end, to allow for variable-length subauth lists, and teach the code
to handle the case where the subauths array being passed in from
userspace is shorter than 5 elements.

Note too, that I don't consider this a security issue since you'd need
a compromised cifs.idmap program. If you have that, you can do all sorts
of nefarious stuff. Still, this is probably reasonable for stable.

Cc: stable@kernel.org
Reviewed-by: Shirish Pargaonkar <shirishpargaonkar@gmail.com>
Signed-off-by: Jeff Layton <jlayton@redhat.com>

diff --git a/fs/cifs/cifsacl.c b/fs/cifs/cifsacl.c
index fc783e2..0fb15bb 100644 (file)
--- a/fs/cifs/cifsacl.c
+++ b/fs/cifs/cifsacl.c
@@ -225,6 +225,13 @@ sid_to_str(struct cifs_sid *sidptr, char *sidstr)
 }
 
 static void
+cifs_copy_sid(struct cifs_sid *dst, const struct cifs_sid *src)
+{
+       memcpy(dst, src, sizeof(*dst));
+       dst->num_subauth = min_t(u8, src->num_subauth, NUM_SUBAUTHS);
+}
+
+static void
 id_rb_insert(struct rb_root *root, struct cifs_sid *sidptr,
                struct cifs_sid_id **psidid, char *typestr)
 {
@@ -248,7 +255,7 @@ id_rb_insert(struct rb_root *root, struct cifs_sid *sidptr,
                }
        }
 
-       memcpy(&(*psidid)->sid, sidptr, sizeof(struct cifs_sid));
+       cifs_copy_sid(&(*psidid)->sid, sidptr);
        (*psidid)->time = jiffies - (SID_MAP_RETRY + 1);
        (*psidid)->refcount = 0;
 
@@ -354,7 +361,7 @@ id_to_sid(unsigned long cid, uint sidtype, struct cifs_sid *ssid)
         * any fields of the node after a reference is put .
         */
        if (test_bit(SID_ID_MAPPED, &psidid->state)) {
-               memcpy(ssid, &psidid->sid, sizeof(struct cifs_sid));
+               cifs_copy_sid(ssid, &psidid->sid);
                psidid->time = jiffies; /* update ts for accessing */
                goto id_sid_out;
        }
@@ -370,14 +377,14 @@ id_to_sid(unsigned long cid, uint sidtype, struct cifs_sid *ssid)
                if (IS_ERR(sidkey)) {
                        rc = -EINVAL;
                        cFYI(1, "%s: Can't map and id to a SID", __func__);
+               } else if (sidkey->datalen < sizeof(struct cifs_sid)) {
+                       rc = -EIO;
+                       cFYI(1, "%s: Downcall contained malformed key "
+                               "(datalen=%hu)", __func__, sidkey->datalen);
                } else {
                        lsid = (struct cifs_sid *)sidkey->payload.data;
-                       memcpy(&psidid->sid, lsid,
-                               sidkey->datalen < sizeof(struct cifs_sid) ?
-                               sidkey->datalen : sizeof(struct cifs_sid));
-                       memcpy(ssid, &psidid->sid,
-                               sidkey->datalen < sizeof(struct cifs_sid) ?
-                               sidkey->datalen : sizeof(struct cifs_sid));
+                       cifs_copy_sid(&psidid->sid, lsid);
+                       cifs_copy_sid(ssid, &psidid->sid);
                        set_bit(SID_ID_MAPPED, &psidid->state);
                        key_put(sidkey);
                        kfree(psidid->sidstr);
@@ -396,7 +403,7 @@ id_to_sid(unsigned long cid, uint sidtype, struct cifs_sid *ssid)
                        return rc;
                }
                if (test_bit(SID_ID_MAPPED, &psidid->state))
-                       memcpy(ssid, &psidid->sid, sizeof(struct cifs_sid));
+                       cifs_copy_sid(ssid, &psidid->sid);
                else
                        rc = -EINVAL;
        }
@@ -675,8 +682,6 @@ int compare_sids(const struct cifs_sid *ctsid, const struct cifs_sid *cwsid)
 static void copy_sec_desc(const struct cifs_ntsd *pntsd,
                                struct cifs_ntsd *pnntsd, __u32 sidsoffset)
 {
-       int i;
-
        struct cifs_sid *owner_sid_ptr, *group_sid_ptr;
        struct cifs_sid *nowner_sid_ptr, *ngroup_sid_ptr;
 
@@ -692,26 +697,14 @@ static void copy_sec_desc(const struct cifs_ntsd *pntsd,
        owner_sid_ptr = (struct cifs_sid *)((char *)pntsd +
                                le32_to_cpu(pntsd->osidoffset));
        nowner_sid_ptr = (struct cifs_sid *)((char *)pnntsd + sidsoffset);
-
-       nowner_sid_ptr->revision = owner_sid_ptr->revision;
-       nowner_sid_ptr->num_subauth = owner_sid_ptr->num_subauth;
-       for (i = 0; i < 6; i++)
-               nowner_sid_ptr->authority[i] = owner_sid_ptr->authority[i];
-       for (i = 0; i < 5; i++)
-               nowner_sid_ptr->sub_auth[i] = owner_sid_ptr->sub_auth[i];
+       cifs_copy_sid(nowner_sid_ptr, owner_sid_ptr);
 
        /* copy group sid */
        group_sid_ptr = (struct cifs_sid *)((char *)pntsd +
                                le32_to_cpu(pntsd->gsidoffset));
        ngroup_sid_ptr = (struct cifs_sid *)((char *)pnntsd + sidsoffset +
                                        sizeof(struct cifs_sid));
-
-       ngroup_sid_ptr->revision = group_sid_ptr->revision;
-       ngroup_sid_ptr->num_subauth = group_sid_ptr->num_subauth;
-       for (i = 0; i < 6; i++)
-               ngroup_sid_ptr->authority[i] = group_sid_ptr->authority[i];
-       for (i = 0; i < 5; i++)
-               ngroup_sid_ptr->sub_auth[i] = group_sid_ptr->sub_auth[i];
+       cifs_copy_sid(ngroup_sid_ptr, group_sid_ptr);
 
        return;
 }
@@ -1120,8 +1113,7 @@ static int build_sec_desc(struct cifs_ntsd *pntsd, struct cifs_ntsd *pnntsd,
                                kfree(nowner_sid_ptr);
                                return rc;
                        }
-                       memcpy(owner_sid_ptr, nowner_sid_ptr,
-                                       sizeof(struct cifs_sid));
+                       cifs_copy_sid(owner_sid_ptr, nowner_sid_ptr);
                        kfree(nowner_sid_ptr);
                        *aclflag = CIFS_ACL_OWNER;
                }
@@ -1139,8 +1131,7 @@ static int build_sec_desc(struct cifs_ntsd *pntsd, struct cifs_ntsd *pnntsd,
                                kfree(ngroup_sid_ptr);
                                return rc;
                        }
-                       memcpy(group_sid_ptr, ngroup_sid_ptr,
-                                       sizeof(struct cifs_sid));
+                       cifs_copy_sid(group_sid_ptr, ngroup_sid_ptr);
                        kfree(ngroup_sid_ptr);
                        *aclflag = CIFS_ACL_GROUP;
                }