Re: [PATCH] cifs: Support for an upcall to map SID to an uid and a gid

[Jeff Layton <jlayton@samba.org>]

On Sat, 11 Dec 2010 15:47:12 -0600
Shirish Pargaonkar <shirishpargaonkar@gmail.com> wrote:

> On Sat, Dec 11, 2010 at 10:17 AM, Jeff Layton <jlayton@samba.org> wrote:
> > On Tue,  7 Dec 2010 11:11:12 -0600
> > shirishpargaonkar@gmail.com wrote:
> >
> >> From: Shirish Pargaonkar <shirishpargaonkar@gmail.com>
> >>
> >>
> >> Use a cifs upcall to map a SID to either an uid or a gid using
> >> winbind.
> >>
> >> There is a corrosponding patch for the cifs.upcall binary in cifs-utils rpm
> >> that is being posted also.
> >>
> >> A new type of key, cifs_acl_key_type, is used.
> >> To map a SID, which can be either a Onwer SID or a Group SID, key
> >> description starts with the string "os" or "gs" followed by SID converted
> >> to a string. Without "os" or "gs", cifs.upcall does not know whether
> >> SID needs to be mapped to either an uid or a gid.
> >>
> >> Once a key is instantiated, get rid of it since cifs does not need to
> >> use in any way.
> >> winbind does the id mapping and looks up name for the newly mapped
> >> SID/uid or SID/gid combination.
> >>
> >> For now, cifs.upcall is only used to map a SID to an id (uid or gid) but
> >> it would be used to obtain an SID (string) for an id.
> >>
> >> An entry such as this
> >>
> >> create  cifs.cifs_acl   *       *               /usr/sbin/cifs.upcall %k
> >>
> >> is needed in the file /etc/request-key.conf.
> >>
> >>
> >> Signed-off-by: Shirish Pargaonkar <shirishpargaonkar@gmail.com>
> >> ---
> >>  fs/cifs/cifsacl.c |  117 ++++++++++++++++++++++++++++++++++++++++++++++++-----
> >>  fs/cifs/cifsacl.h |    8 ++++
> >>  fs/cifs/cifsfs.c  |    7 +++
> >>  3 files changed, 122 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/fs/cifs/cifsacl.c b/fs/cifs/cifsacl.c
> >> index a520091..d3ac6c8 100644
> >> --- a/fs/cifs/cifsacl.c
> >> +++ b/fs/cifs/cifsacl.c
> >> @@ -23,6 +23,9 @@
> >>
> >>  #include <linux/fs.h>
> >>  #include <linux/slab.h>
> >> +#include <linux/string.h>
> >> +#include <keys/user-type.h>
> >> +#include <linux/key-type.h>
> >>  #include "cifspdu.h"
> >>  #include "cifsglob.h"
> >>  #include "cifsacl.h"
> >> @@ -52,6 +55,102 @@ static const struct cifs_sid sid_authusers = {
> >>  /* group users */
> >>  static const struct cifs_sid sid_user = {1, 2 , {0, 0, 0, 0, 0, 5}, {} };
> >>
> >> +static int
> >> +cifs_acl_key_instantiate(struct key *key, const void *data, size_t datalen)
> >> +{
> >> +     char *payload;
> >> +
> >> +     payload = kmalloc(datalen, GFP_KERNEL);
> >> +     if (!payload)
> >> +             return -ENOMEM;
> >> +
> >> +     memcpy(payload, data, datalen);
> >> +     key->payload.data = payload;
> >> +     return 0;
> >> +}
> >> +
> >> +static void
> >> +cifs_acl_key_destroy(struct key *key)
> >> +{
> >> +     kfree(key->payload.data);
> >> +}
> >> +
> >> +struct key_type cifs_acl_key_type = {
> >> +     .name        = "cifs.cifs_acl",
> >> +     .instantiate = cifs_acl_key_instantiate,
> >> +     .destroy     = cifs_acl_key_destroy,
> >> +     .describe    = user_describe,
> >> +     .match       = user_match,
> >> +};
> >> +
> >
> > Nit: these don't have so much to do with ACL's per-se, as much as
> > idmapping. Maybe these functions and the key type should be called
> > "cifs_idmap_*" and "cifs.idmap" ? It might be clearer to admins what
> > this is for.
> 
> Yes, will change the name as you suggested, that sounds right, rather
> that acl.
> 
> >
> >> +static void
> >> +sid_to_str(struct cifs_sid *sidptr, char *sidstr)
> >> +{
> >> +     int i;
> >> +     unsigned long saval;
> >> +     char *strptr;
> >> +
> >> +     strptr = sidstr;
> >> +
> >> +     sprintf(strptr, "%s", "S");
> >> +     strptr = sidstr + strlen(sidstr);
> >> +
> >> +     sprintf(strptr, "-%d", sidptr->revision);
> >> +     strptr = sidstr + strlen(sidstr);
> >> +
> >> +     for (i = 0; i < 6; ++i) {
> >> +             if (sidptr->authority[i]) {
> >> +                     sprintf(strptr, "-%d", sidptr->authority[i]);
> >> +                     strptr = sidstr + strlen(sidstr);
> >> +             }
> >> +     }
> >> +
> >> +     for (i = 0; i < sidptr->num_subauth; ++i) {
> >> +             saval = le32_to_cpu(sidptr->sub_auth[i]);
> >> +             sprintf(strptr, "-%ld", saval);
> >> +             strptr = sidstr + strlen(sidstr);
> >> +     }
> >> +}
> >> +
> >> +static int
> >> +sid_to_id(struct cifs_sid *psid, struct cifs_fattr *fattr, uint sidtype)
> >> +{
> >> +     int rc = 0;
> >> +     char *sidstr, *strptr;
> >> +     struct key *idkey;
> >> +
> >> +     sidstr = kzalloc(SIDLEN, GFP_KERNEL);
> >> +     if (!sidstr)
> >> +             return -ENOMEM;
> >> +     strptr = sidstr;
> >> +
> >> +     if (sidtype == SIDOWNER)
> >> +             sprintf(strptr, "%s", "os:");
> >> +     else if (sidtype == SIDGROUP)
> >> +             sprintf(strptr, "%s", "gs:");
> >> +     else {
> >> +             rc = -EINVAL;
> >> +             goto idresolve_err;
> >> +     }
> >> +     strptr = sidstr + strlen(sidstr);
> >> +
> >> +     sid_to_str(psid, strptr);
> >> +
> >> +     idkey = request_key(&cifs_acl_key_type, sidstr, "");
> >> +     if (IS_ERR(idkey))
> >> +             cFYI(1, "%s: idkey error: %d\n", __func__, -ENOKEY);
> >> +     else {
> >> +             if (sidtype == SIDOWNER)
> >> +                     fattr->cf_uid = *(unsigned long *)idkey->payload.value;
> >> +             else if (sidtype == SIDGROUP)
> >> +                     fattr->cf_gid = *(unsigned long *)idkey->payload.value;
> >> +             key_put(idkey);
> >> +     }
> >> +
> >> +idresolve_err:
> >> +     kfree(sidstr);
> >> +     return rc;
> >> +}
> >>
> >
> > What about security? With this code, it'll be possible for a user to
> > "stuff" the cache with idmapping. In the situation where the kernel is
> > trying to enforce permissions on the client, it'll be possible to
> > trick it into mapping a sid to a uid of your choosing. I think you need
> > to guard against that.
> 
> That means cifs module will have to know what was the range of ids for
> uids and gids to be allocated by winbind as dictated by entries in
> smb.conf and if a returned id happens not to be in that range, discard it
> and assign default id of 0 (root).
> winbind know the range from smb.conf, it could be any smb.conf file,
> not necessarily under /etc/samba/smb.conf.
> How would cifs module know that range.
> Not sure if winbind has an API to query the ranges.  And even if it had,
> it is possible that winbind deamon itself is not running.
> 

Maybe I wasn't clear... The danger here is that I have the ability to
put info of my own choosing into my keyring via an add_key() syscall
from userspace.

You need to do something like the override_creds trick that dns_query
does.

> >
> > You'll also be doing an upcall every time a different user needs to map
> > a SID to a UID/GID.
> >
> > Finally, calling into the keys API every time you want to map an ID
> > sounds rather inefficient. This might take quite some time if you were
> > doing "ls -l" on a large directory.
> >
> > Would it be better to consider taking the info in the key and
> > populating a different cache? You could register it with a shrinker and
> > prune off LRU entries if memory gets tight.
> >
> 
> Will look into this.  One thing that concerns me is if a cached etnry
> for a SID with its name and an id (either an uid or a gid), if that SID
> now represents a different object and has differernt name, would
> not cached info be incorrect?  Not sure if this can ever happen
> or how would it happen and if it does, what would be a trigger
> for a cache revalidation and purges!
> 

Sure, mappings can change. But, you still have the same problem with
what you're proposing in these patches. The userspace program isn't
setting a timeout on the key. Once a mapping is put in the keyring,
it's there until it's revoked. You probably want to set a max TTL for
the entries in the cache regardless of what scheme is used.

idmap lookups need to be pretty fast as you'll be calling them a lot.
Consider the case of "ls -l" on a large directory. Some slowness for
the upcall to populate the entry in the cache is probably acceptable.
Slowness on subsequent cached lookups of that same entry will be
painful.

-- 
Jeff Layton <jlayton@samba.org>