[CRIU] [PATCH] security: check additional groups,v4
Ruslan Kuprieiev
kupruser at gmail.com
Wed Jul 9 07:28:47 PDT 2014
On 09.07.2014 16:42, Pavel Emelyanov wrote:
> On 07/08/2014 05:22 PM, Ruslan Kuprieiev wrote:
>> Forgot to mention, I forgot to add "if (root) return true" in
>> check_groups() in v3.
>>
>> On 08.07.2014 16:18, Ruslan Kuprieiev wrote:
>>> Currently, we only check if process gids match primary gid of user.
>>> But process and user have additional groups too. So lets:
>>> 1) check that process rgid,egid and sgid are in the user's grouplist.
>>> 2) on restore check that user has all groups from the images.
>>>
>>> Signed-off-by: Ruslan Kuprieiev <kupruser at gmail.com>
>>> ---
>>> cr-service.c | 3 +-
>>> crtools.c | 3 +-
>>> include/crtools.h | 2 +-
>>> security.c | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
>>> 4 files changed, 99 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/cr-service.c b/cr-service.c
>>> index 64ce751..9504c14 100644
>>> --- a/cr-service.c
>>> +++ b/cr-service.c
>>> @@ -178,7 +178,8 @@ static int setup_opts_from_req(int sk, CriuOpts *req)
>>> return -1;
>>> }
>>>
>>> - restrict_uid(ids.uid, ids.gid);
>>> + if (restrict_uid(ids.uid, ids.gid))
>>> + return -1;
>>>
>>> if (fstat(sk, &st)) {
>>> pr_perror("Can't get socket stat");
>>> diff --git a/crtools.c b/crtools.c
>>> index b662fff..28d343e 100644
>>> --- a/crtools.c
>>> +++ b/crtools.c
>>> @@ -173,7 +173,8 @@ int main(int argc, char *argv[])
>>> BUILD_BUG_ON(PAGE_SIZE != PAGE_IMAGE_SIZE);
>>>
>>> cr_pb_init();
>>> - restrict_uid(getuid(), getgid());
>>> + if (restrict_uid(getuid(), getgid()))
>>> + return 1;
>>>
>>> if (argc < 2)
>>> goto usage;
>>> diff --git a/include/crtools.h b/include/crtools.h
>>> index c4ae580..75047fc 100644
>>> --- a/include/crtools.h
>>> +++ b/include/crtools.h
>>> @@ -24,7 +24,7 @@ extern int cr_check(void);
>>> extern int cr_exec(int pid, char **opts);
>>> extern int cr_dedup(void);
>>>
>>> -extern void restrict_uid(unsigned int uid, unsigned int gid);
>>> +extern int restrict_uid(unsigned int uid, unsigned int gid);
>>> struct proc_status_creds;
>>> extern bool may_dump(struct proc_status_creds *);
>>> struct _CredsEntry;
>>> diff --git a/security.c b/security.c
>>> index d4b4230..d17c726 100644
>>> --- a/security.c
>>> +++ b/security.c
>>> @@ -1,14 +1,22 @@
>>> #include <unistd.h>
>>> +#include <pwd.h>
>>> +#include <grp.h>
>>> +#include <limits.h>
>>> +#include <stdlib.h>
>>> +#include <string.h>
>>> +
>>> #include "crtools.h"
>>> #include "proc_parse.h"
>>> #include "log.h"
>>> +#include "xmalloc.h"
>>>
>>> #include "protobuf/creds.pb-c.h"
>>>
>>> /*
>>> - * UID and GID of user requesting for C/R
>>> + * UID, GID and groups of user requesting for C/R
>>> */
>>> static unsigned int cr_uid, cr_gid;
>>> +static unsigned int cr_ngroups, *cr_groups;
>>>
>>> /*
>>> * Setup what user is requesting for dump (via rpc or using
>>> @@ -17,11 +25,36 @@ static unsigned int cr_uid, cr_gid;
>>> * access to. (Or implement some trickier security policy).
>>> */
>>>
>>> -void restrict_uid(unsigned int uid, unsigned int gid)
>>> +int restrict_uid(unsigned int uid, unsigned int gid)
>>> {
>>> - pr_info("Restrict C/R with %u:%u uid\n", uid, gid);
>>> + struct passwd *pwd;
>>> + unsigned int buf[NGROUPS_MAX];
>>> + int nbuf;
>>> +
>>> + pr_info("Restrict C/R with %u:%u uid:gid\n", uid, gid);
>>> cr_uid = uid;
>>> cr_gid = gid;
>>> +
>>> + pwd = getpwuid(uid);
>>> + if (!pwd) {
>>> + pr_perror("Can't get password file entry");
>>> + return -1;
>>> + }
>>> +
>>> + nbuf = NGROUPS_MAX;
>>> + if (getgrouplist(pwd->pw_name, pwd->pw_gid, buf, &nbuf) < 0) {
>>> + pr_perror("Can't get group list");
>>> + return -1;
>>> + }
>>> +
>>> + cr_ngroups = nbuf;
>>> + cr_groups = xmalloc(cr_ngroups*sizeof(*cr_groups));
>>> + if (!cr_groups)
>>> + return -1;
>>> +
>>> + memcpy(cr_groups, buf, cr_ngroups*sizeof(*cr_groups));
>>> +
>>> + return 0;
>>> }
>>>
>>> static bool check_ids(unsigned int crid, unsigned int rid, unsigned int eid, unsigned int sid)
>>> @@ -35,6 +68,61 @@ static bool check_ids(unsigned int crid, unsigned int rid, unsigned int eid, uns
>>> return false;
>>> }
>>>
>>> +static bool contains(unsigned int *crgids, unsigned int crgids_num, unsigned int gid)
>>> +{
>>> + int i;
>>> +
>>> + for (i = 0; i < crgids_num; ++i) {
>>> + if (crgids[i] == gid)
>>> + return true;
>>> + }
>>> +
>>> + return false;
>>> +}
>>> +
>>> +static bool check_gids(unsigned int rid, unsigned int eid, unsigned int sid)
>>> +{
>>> + if (cr_gid == 0)
>>> + return true;
>>> +
>>> + if (!(contains(cr_groups, cr_ngroups, rid) &&
>>> + contains(cr_groups, cr_ngroups, eid) &&
>>> + contains(cr_groups, cr_ngroups, sid))) {
> I still worry about cr_gid is not checked against rid, eid and sid. Are
> you 100% sure that cr_groups will include one? Can we put the check here
> for it being true?
Well, non-root cant get gid of group, that he is not present in.
getgrouplist reads /etc/group and gets _all_ groups of the user.
So, i'm quite sure.
Stiil, I can excessively check cr_gid agains rid, eid and sid and
set BUG_ON() if something wrong.
Ok?
>>> + pr_err("GID mismatch. User is absent in (%u,%u,%u)",
>>> + rid, eid, sid);
>>> + return false;
>>> + }
>>> +
>>> + return true;
>>> +}
>>> +
>>> +/*
>>> + * There is no need to check groups on dump, because if uids and gids match
>>> + * then groups will match too. Btw, getting groups on dump is problematic.
>>> + * We can't parse proc, as it contains only first 32 groups. And we can't use
>>> + * getgrouplist, as it reads /etc/group which depends on the namespace.
>>> + *
>>> + * On restore we're getting groups from imgs and can check if user didn't add
>>> + * wrong groups by modifying images.
>>> + */
>>> +static bool check_groups(unsigned int *groups, unsigned int ngroups)
>>> +{
>>> + int i;
>>> +
>>> + if (cr_gid == 0)
>>> + return true;
>>> +
>>> + for (i = 0; i < ngroups; ++i) {
>>> + if (!contains(cr_groups, cr_ngroups, groups[i])) {
>>> + pr_err("GID mismatch. User is absent in %u group\n",
>>> + groups[i]);
>>> + return false;
>>> + }
>>> + }
>>> +
>>> + return true;
>>> +}
>>> +
>>> static bool check_caps(u32 *inh, u32 *eff, u32 *prm)
>>> {
>>> int i;
>>> @@ -62,13 +150,14 @@ static bool check_caps(u32 *inh, u32 *eff, u32 *prm)
>>> bool may_dump(struct proc_status_creds *creds)
>>> {
>>> return check_ids(cr_uid, creds->uids[0], creds->uids[1], creds->uids[2]) &&
>>> - check_ids(cr_gid, creds->gids[0], creds->gids[1], creds->gids[2]) &&
>>> + check_gids(creds->gids[0], creds->gids[1], creds->gids[2]) &&
>>> check_caps(creds->cap_inh, creds->cap_eff, creds->cap_prm);
>>> }
>>>
>>> bool may_restore(CredsEntry *creds)
>>> {
>>> return check_ids(cr_uid, creds->uid, creds->euid, creds->suid) &&
>>> - check_ids(cr_gid, creds->gid, creds->egid, creds->sgid) &&
>>> + check_gids(creds->gid, creds->egid, creds->sgid) &&
>>> + check_groups(creds->groups, creds->n_groups) &&
>>> check_caps(creds->cap_inh, creds->cap_eff, creds->cap_prm);
>>> }
>> .
>>
More information about the CRIU
mailing list