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.
>
> 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!
> --
> 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
