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

[Ruslan Kuprieiev]

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.
I'm actually not sure, that chowing is better than setting sgid bit. 
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;
>> +}
>>