[CRIU] [PATCH] security: check additional gids

[Pavel Emelyanov]

On 06/23/2014 11:04 AM, Ruslan Kuprieiev wrote:
> Currently, we only check if process gids match primary gid of user.
> But user may have some additional groups. So lets check them too.
> 
> Signed-off-by: Ruslan Kuprieiev <kupruser at gmail.com>
> ---
>   cr-service.c      |  3 ++-
>   crtools.c         |  3 ++-
>   include/crtools.h |  2 +-
>   security.c        | 74 
> +++++++++++++++++++++++++++++++++++++++++++++++--------
>   4 files changed, 69 insertions(+), 13 deletions(-)
> 
> diff --git a/cr-service.c b/cr-service.c
> index dc0ff5a..7802e71 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;

Spaces got corrupted.

> 
>       if (fstat(sk, &st)) {
>           pr_perror("Can't get socket stat");
> diff --git a/crtools.c b/crtools.c
> index 5684186..96ae590 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..3f3b73b 100644
> --- a/security.c
> +++ b/security.c
> @@ -1,4 +1,9 @@
>   #include <unistd.h>
> +#include <pwd.h>
> +#include <grp.h>
> +#include <limits.h>
> +#include <stdlib.h>
> +
>   #include "crtools.h"
>   #include "proc_parse.h"
>   #include "log.h"
> @@ -8,7 +13,9 @@
>   /*
>    * UID and GID of user requesting for C/R
>    */
> -static unsigned int cr_uid, cr_gid;
> +static unsigned int cr_uid;
> +static unsigned int cr_gids[NGROUPS_MAX];
> +static int cr_gids_num;
> 
>   /*
>    * Setup what user is requesting for dump (via rpc or using
> @@ -17,21 +24,68 @@ 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)
>   {
> +    struct passwd *pwd;
> +
>       pr_info("Restrict C/R with %u:%u uid\n", uid, gid);
>       cr_uid = uid;
> -    cr_gid = gid;
> +
> +    pwd = getpwuid(uid);
> +    if (!pwd) {
> +        pr_perror("Can't get user name");
> +        return -1;
> +    }
> +
> +    cr_gids_num = NGROUPS_MAX;
> +    if (getgrouplist(pwd->pw_name, gid, cr_gids, &cr_gids_num) < 0) {
> +        pr_perror("Can't get group list of the user");
> +        return -1;
> +    }
> +
> +    return 0;
>   }
> 
> -static bool check_ids(unsigned int crid, unsigned int rid, unsigned int 
> eid, unsigned int sid)
> +static bool check_uids(unsigned int crid, unsigned int rid, unsigned 
> int eid, unsigned int sid)
>   {
>       if (crid == 0)
>           return true;
>       if (crid == rid && crid == eid && crid == 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", crid, 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 cruid,
> +              unsigned int *crgids,
> +              unsigned int crgids_num,
> +              unsigned int rid,
> +              unsigned int eid,
> +              unsigned int sid)
> +{
> +    if (contains(crgids, crgids_num, 0))
> +        return true;
> +
> +    if (contains(crgids, crgids_num, rid) &&
> +        contains(crgids, crgids_num, eid) &&
> +        contains(crgids, crgids_num, sid))
> +        return true;
> +
> +    pr_err("GID mismatch. User(uid %u, primary gid %u) is absent in 
> (%u,%u,%u)",
> +                      cruid, crgids[0], rid, eid, sid);
>       return false;
>   }
> 
> @@ -46,7 +100,7 @@ static bool check_caps(u32 *inh, u32 *eff, u32 *prm)
>        * security model.
>        */
> 
> -    if (cr_uid == 0 && cr_gid == 0)
> +    if (cr_uid == 0 && contains(cr_gids, cr_gids_num, 0))
>           return true;
> 
>       for (i = 0; i < CR_CAP_SIZE; i++) {
> @@ -61,14 +115,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(cr_uid, creds->uids[0], creds->uids[1], 
> creds->uids[2]) &&
> +        check_gids(cr_uid, cr_gids, cr_gids_num, 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) &&
> +    return check_uids(cr_uid, creds->uid, creds->euid, creds->suid) &&
> +        check_gids(cr_uid, cr_gids, cr_gids_num, creds->gid, 
> creds->egid, creds->sgid) &&
>           check_caps(creds->cap_inh, creds->cap_eff, creds->cap_prm);
>   }
>