On Sat, 11 Dec 2010 15:47:12 -0600
Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx> wrote:
> On Sat, Dec 11, 2010 at 10:17 AM, Jeff Layton <jlayton@xxxxxxxxx> wrote:
> > On Tue, 7 Dec 2010 11:11:12 -0600
> > shirishpargaonkar@xxxxxxxxx wrote:
> >
> >> From: Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx>
> >>
> >>
> >> 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@xxxxxxxxx>
> >> ---
> >> 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@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
