[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 16:21, Pavel Emelyanov wrote:
> 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()?

Oh! Good point! So, lets just call setfs*g*id(0)(because our uid is 0 
anyway) inside restrict_uid(), right?

>
>> 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 :)

No, setting sgid on criu binary.

>
>> 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
>> .
>>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openvz.org/pipermail/criu/attachments/20140915/c09d070f/attachment.html>