On Tue, Oct 16, 2012 at 9:26 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> On Tue, 16 Oct 2012 09:09:57 -0500
> Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx> wrote:
>
>> 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>
>
> Thanks, I had another question too on a related note...
>
> The idmap code basically does double caching. You call into the keys
> API to do the mapping. If it doesn't have a mapping then it will call
> into userspace to establish one. The keys API will then cache that
> result in a "struct key". You're then taking that payload and caching
> it again in a rbtree.
>
> What's the rationale for that? Why not just use they keys API cache
> directly and get rid of all of the rbtrees? It seems like that would be
> a lot simpler...
>
> --
> Jeff Layton <jlayton@xxxxxxxxxx>
Jeff,
Would not we end up making an upcall for every uid/gid to sid
and sid to uid/gid mapping needed? That would be expensive.
The search in cached kernel cifs_sid_id structure would certainly
be faster and if either of the caches does not have mapping, it would
end up in making a winbind library to the server in either cases.
Is keys API cache bigger and sticks longer and searches quicker?
How would keys API cache entry look like? Right now, cifs client
asks for either sid or id (uid or gid) and makes that association
in the cache entries. Would keys API cache make/keep same
association and return kernel that entire mapping?
Regards,
Shirish
--
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
