[CRIU] [PATCH] security: check additional groups,v4

[Andrew Vagin]

On Tue, Jul 08, 2014 at 05:18:01PM +0400, 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.

offtopic: do we restore a group list?

> 
> 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))) {
> +		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);
>  }
> -- 
> 1.8.3.2
> 
> _______________________________________________
> CRIU mailing list
> CRIU at openvz.org
> https://lists.openvz.org/mailman/listinfo/criu