[CRIU] [PATCH] security: check additional groups

Ruslan Kuprieiev kupruser at gmail.com
Fri Jul 4 05:59:46 PDT 2014


On 04.07.2014 15:08, Pavel Emelyanov wrote:
> On 06/30/2014 06:35 PM, 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) check that user has all groups from the process grouplist.
>>
>> Signed-off-by: Ruslan Kuprieiev <kupruser at gmail.com>
>> ---
>>   cr-service.c         |  3 +-
>>   crtools.c            |  3 +-
>>   include/crtools.h    |  2 +-
>>   include/proc_parse.h |  2 ++
>>   proc_parse.c         | 34 +++++++++++++++++--
>>   security.c           | 94 ++++++++++++++++++++++++++++++++++++++++++++++------
>>   6 files changed, 122 insertions(+), 16 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 c3a2e27..c184dba 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/include/proc_parse.h b/include/proc_parse.h
>> index b153328..a13ec14 100644
>> --- a/include/proc_parse.h
>> +++ b/include/proc_parse.h
>> @@ -83,6 +83,8 @@ struct proc_pid_stat {
>>   struct proc_status_creds {
>>   	unsigned int uids[4];
>>   	unsigned int gids[4];
>> +	unsigned int *groups;
>> +	unsigned int ngroups;
>>   
>>   	u32 cap_inh[PROC_CAP_SIZE];
>>   	u32 cap_prm[PROC_CAP_SIZE];
>> diff --git a/proc_parse.c b/proc_parse.c
>> index f2ea897..2712432 100644
>> --- a/proc_parse.c
>> +++ b/proc_parse.c
>> @@ -689,6 +689,29 @@ static int ids_parse(char *str, unsigned int *arr)
>>   		return 0;
>>   }
>>   
>> +static int groups_parse(char *str, unsigned int **arr, unsigned int *arr_size)
>> +{
>> +	char *end;
>> +	unsigned int size = 0;
>> +	unsigned int *buf = NULL;
>> +
>> +	end = str - 1;
>> +	/* "Groups:" string has a space before '\n' */
>> +	while (*end != ' ' && *(end+1) != '\n') {

Oh! Typo! Will fix.

>> +		size++;
>> +		buf = xrealloc(buf, size*sizeof(*buf));
>> +		if (!buf)
>> +			return -1;
>> +
>> +		buf[size-1] = strtol(end + 1, &end, 10);
>> +	}
>> +
>> +	*arr = buf;
>> +	*arr_size = size;
>> +
>> +	return 0;
>> +}
>> +
>>   static int cap_parse(char *str, unsigned int *res)
>>   {
>>   	int i, ret;
>> @@ -715,7 +738,7 @@ int parse_pid_status(pid_t pid, struct proc_status_creds *cr)
>>   		return -1;
>>   	}
>>   
>> -	while (done < 6 && fgets(str, sizeof(str), f)) {
>> +	while (done < 7 && fgets(str, sizeof(str), f)) {
>>   		if (!strncmp(str, "Uid:", 4)) {
>>   			if (ids_parse(str + 5, cr->uids))
>>   				goto err_parse;
>> @@ -730,6 +753,13 @@ int parse_pid_status(pid_t pid, struct proc_status_creds *cr)
>>   			done++;
>>   		}
>>   
>> +		if (!strncmp(str, "Groups:", 7)) {
>> +			if (groups_parse(str + 8, &cr->groups, &cr->ngroups))
>> +				goto err_parse;
>> +
>> +			done++;
>> +		}
>> +
>>   		if (!strncmp(str, "CapInh:", 7)) {
>>   			if (cap_parse(str + 8, cr->cap_inh))
>>   				goto err_parse;
>> @@ -759,7 +789,7 @@ int parse_pid_status(pid_t pid, struct proc_status_creds *cr)
>>   		}
>>   	}
>>   
>> -	if (done != 6) {
>> +	if (done != 7) {
>>   err_parse:
>>   		pr_err("Error parsing proc status file\n");
>>   		fclose(f);
>> diff --git a/security.c b/security.c
>> index d4b4230..bbda294 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,24 +25,88 @@ 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)
>> +static bool check_uids(unsigned int rid, unsigned int eid, unsigned int sid)
> This routine just changes its name. Plz, rename with separate patch.

Ok.

>
>>   {
>> -	if (crid == 0)
>> +	if (cr_uid == 0)
>>   		return true;
>> -	if (crid == rid && crid == eid && crid == sid)
>> +	if (cr_uid == rid && cr_uid == eid && cr_uid == sid)
>>   		return true;
>>   
>> -	pr_err("UID/GID mismatch %u != (%u,%u,%u)\n", crid, rid, eid, sid);
>> +	pr_err("UID mismatch %u != (%u,%u,%u)\n", cr_uid, rid, eid, sid);
>>   	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,
>> +			unsigned int *groups, unsigned int ngroups)
>> +{
>> +	int i;
>> +
>> +	if (cr_gid == 0)
>> +		return true;
> If cr_gid != 0 it should be "included" into cr_groups too.

Well, if getgid() and pwd->pw_gid are the same(I suppose they are), then 
gid is
included into cr_groups after calling getgrouplist.

>> +
>> +	if (!(contains(cr_groups, cr_ngroups, rid) &&
>> +	    contains(cr_groups, cr_ngroups, eid)  &&
>> +	    contains(cr_groups, cr_ngroups, sid))) {
>> +		pr_err("GID mismatch. User is absent in (%u,%u,%u)",
>> +							rid, eid, sid);
>> +		return false;
>> +	}
>> +
>> +	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;
>> @@ -61,14 +133,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]) &&
>> +	return check_uids(creds->uids[0], creds->uids[1], creds->uids[2]) &&
>> +		check_gids(creds->gids[0], creds->gids[1], creds->gids[2], creds->groups, creds->ngroups) &&
>>   		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) &&
>> +	return check_uids(creds->uid, creds->euid, creds->suid) &&
>> +		check_gids(creds->gid, creds->egid, creds->sgid, creds->groups, creds->n_groups) &&
>>   		check_caps(creds->cap_inh, creds->cap_eff, creds->cap_prm);
>>   }
>>



More information about the CRIU mailing list