[CRIU] [PATCH 1/2] security: chown imgs to 0, 0 when creating, and check owner, group and mode when reading

[Pavel Emelyanov]

On 09/15/2014 05:01 PM, Ruslan Kuprieiev wrote:
> On 15.09.2014 15:11, Pavel Emelyanov wrote:
>> On 09/13/2014 02:12 PM, Ruslan Kuprieiev wrote:
>>> Signed-off-by: Ruslan Kuprieiev <kupruser at gmail.com>
>>> ---
>>>   image.c           | 12 ++++++++++++
>>>   include/crtools.h |  1 +
>>>   security.c        | 43 +++++++++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 56 insertions(+)
>>>
>>> diff --git a/image.c b/image.c
>>> index 566073b..9f49887 100644
>>> --- a/image.c
>>> +++ b/image.c
>>> @@ -218,6 +218,18 @@ int open_image_at(int dfd, int type, unsigned long flags, ...)
>>>   		goto err;
>>>   	}
>>>   
>>> +	if (flags == O_RDONLY) {
>>> +		if (!check_file_ids(ret)) {
>>> +			pr_err("User has no rights to open image %s\n", path);
>>> +			goto err;
>>> +		}
>>> +	} else {
>>> +		if (fchown(ret, 0, 0)) {
>> This looks strange. Chown is root-only operation. If we're not root,
>> this will fail, if we are -- this is pointless.
> 
> We are always root. If we set suid bit, criu process will have root uid, 
> but group will be equal to
> user group(keep in mind CR_FD_PERM=rw-rw-r--). So, either we need to not 
> only set suid bit, but
> also sgid, or just chown images.

Or call setfsuid()?

> I'm actually not sure, that chowing is better than setting sgid bit. 

On images? We must make sure user doesn't modify them, suid bit won't help
with it :)

> But, it allows logs, stats, pidfiles
> to be modified by user group, which, i guess, may be (somehow?) useful.
> 
>>> +			pr_perror("Can't chown image %s to uid 0, gid 0", path);
>>> +			goto err;
>>> +		}
>>> +	}
>>> +
>>>   	if (fdset_template[type].magic == RAW_IMAGE_MAGIC)
>>>   		goto skip_magic;
>>>   
>>> diff --git a/include/crtools.h b/include/crtools.h
>>> index 75047fc..0f73f27 100644
>>> --- a/include/crtools.h
>>> +++ b/include/crtools.h
>>> @@ -29,5 +29,6 @@ struct proc_status_creds;
>>>   extern bool may_dump(struct proc_status_creds *);
>>>   struct _CredsEntry;
>>>   extern bool may_restore(struct _CredsEntry *);
>>> +extern bool check_file_ids(int fd);
>>>   
>>>   #endif /* __CR_CRTOOLS_H__ */
>>> diff --git a/security.c b/security.c
>>> index a801005..75086dc 100644
>>> --- a/security.c
>>> +++ b/security.c
>>> @@ -4,6 +4,7 @@
>>>   #include <limits.h>
>>>   #include <stdlib.h>
>>>   #include <string.h>
>>> +#include <sys/stat.h>
>>>   
>>>   #include "crtools.h"
>>>   #include "proc_parse.h"
>>> @@ -164,3 +165,45 @@ bool may_restore(CredsEntry *creds)
>>>   		check_groups(creds->groups, creds->n_groups) &&
>>>   		check_caps(creds->cap_inh, creds->cap_eff, creds->cap_prm);
>>>   }
>>> +
>>> +static char *mode_str(char *buf, mode_t mode)
>>> +{
>>> +	buf[0] = (mode & S_IRUSR) ? 'r' : '-';
>>> +	buf[1] = (mode & S_IWUSR) ? 'w' : '-';
>>> +	buf[2] = (mode & S_IXUSR) ? 'x' : '-';
>>> +	buf[3] = (mode & S_IRGRP) ? 'r' : '-';
>>> +	buf[4] = (mode & S_IWGRP) ? 'w' : '-';
>>> +	buf[5] = (mode & S_IXGRP) ? 'x' : '-';
>>> +	buf[6] = (mode & S_IROTH) ? 'r' : '-';
>>> +	buf[7] = (mode & S_IWOTH) ? 'w' : '-';
>>> +	buf[8] = (mode & S_IXOTH) ? 'x' : '-';
>>> +	buf[9] = '\0';
>>> +
>>> +	return buf;
>>> +}
>>> +
>>> +bool check_file_ids(int fd)
>>> +{
>>> +	struct stat st;
>>> +	char buf[10];
>>> +
>>> +	if (cr_uid == 0 && cr_gid == 0)
>>> +		return true;
>>> +
>>> +	if (fstat(fd, &st)) {
>>> +		pr_perror("Can't stat file");
>>> +		return false;
>>> +	}
>>> +
>>> +	if (!(st.st_mode & CR_FD_PERM)) {
>>> +		pr_err("File mode %s != %s\n", mode_str(buf, st.st_mode), mode_str(buf, CR_FD_PERM));
>>> +		return false;
>>> +	}
>>> +
>>> +	if (st.st_uid != 0 || st.st_gid != 0) {
>>> +		pr_err("File uid/gid (%d,%d) != (0,0)\n", st.st_uid, st.st_gid);
>>> +		return false;
>>> +	}
>>> +
>>> +	return true;
>>> +}
>>>
> 
> _______________________________________________
> CRIU mailing list
> CRIU at openvz.org
> https://lists.openvz.org/mailman/listinfo/criu
> .
>