On Tue, Oct 16, 2012 at 6:19 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> 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@xxxxxxxxxx
> Cc: Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx>
> Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> ---
> fs/cifs/cifsacl.c | 49 ++++++++++++++++++++-----------------------------
> 1 file changed, 20 insertions(+), 29 deletions(-)
>
> diff --git a/fs/cifs/cifsacl.c b/fs/cifs/cifsacl.c
> index 2ee5c54..0955d23 100644
> --- 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;
> }
> --
> 1.7.11.7
>
Looks correct. Cleaner.
Reviewed-by: Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
