[CRIU] [PATCH 1/2] security: set fs gid to 0 and check img ids and mode when reading

[Ruslan Kuprieiev]

On 15.09.2014 17:19, Pavel Emelyanov wrote:
> On 09/15/2014 09:50 AM, Ruslan Kuprieiev wrote:
>
>> +bool check_file_ids(int fd)
>> +{
>> +	struct stat st;
>> +	char buf[10];
>> +
>> +	if (cr_uid == 0 && cr_gid == 0)
>> +		return true;
> If current is root, everything is allowed.
> ACK
>
> Otherwise...
>
>> +
>> +	if (fstat(fd, &st)) {
>> +		pr_perror("Can't stat file");
>> +		return false;
>> +	}
>> +
>> +	if (!(st.st_mode & CR_FD_PERM)) {
> If image file's permission miss _all_ bits from CR_FD_PERM mask -- don't allow
> even to open it.
>
> This looks wrong. What if st_mode is rw--w--w-? Then anyone can modify images,
> but user will still be able to restore from them :)

Oh, sorry. I've messed up everything again =(.

>
>> +		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) {
> Images belonging to non-root are prohibited. This is also strange. The intention
> was -- r/o images belonging to root are allowed for restore w/o restrictions. Images
> belonging to user, or others-writable root images are allowed for restore only if
> they result in tasks belonging to the user, that calls the restore.

Hm... I was thinking that it is not safe, but after a bit more thinking, 
I agree with you.

Thanks! Will fix and resend.

>
>> +		pr_err("File uid/gid (%d,%d) != (0,0)\n", st.st_uid, st.st_gid);
>> +		return false;
>> +	}
>> +
>> +	return true;
>> +}
>>