Re: [PATCH v2 2/3] pagemap: hide physical addresses from non-privileged users

[Konstantin Khlebnikov <khlebnikov@yandex-team.ru>]

On 12.05.2015 18:06, Linus Torvalds wrote:
> On Tue, May 12, 2015 at 2:43 AM, Konstantin Khlebnikov
> <khlebnikov@yandex-team.ru> wrote:
>> @@ -1260,6 +1269,8 @@ static ssize_t pagemap_read(struct file *file, char __user *buf,
>>          if (!count)
>>                  goto out_task;
>>
>> +       /* do not disclose physical addresses: attack vector */
>> +       pm.show_pfn = capable(CAP_SYS_ADMIN);
>>          pm.v2 = soft_dirty_cleared;
>>          pm.len = (PAGEMAP_WALK_SIZE >> PAGE_SHIFT);
>>          pm.buffer = kmalloc(pm.len * PM_ENTRY_BYTES, GFP_TEMPORARY);
>
> NO! Dammit, no, no, no!
>
> How many times must people do this major security faux-pas before we learn?

Oops. Sorry. I guess everybody must do that mistake at least once.
That's my first time. =)


So, in this case existing call of mm_access() from pagemap_read()
is a bug too because it checks CAP_SYS_PTRACE for current task.

I'll rework it in the same way as /proc/*/[s]maps.

>
> WE DO NOT CHECK CURRENT CAPABILITIES AT READ/WRITE TIME!
>
> It's a bug. It's a security issue. It's not how Unix capabilities work!
>
> Capabilities are checked at open time.:
>
>> @@ -1335,9 +1346,6 @@ out:
>>
>>   static int pagemap_open(struct inode *inode, struct file *file)
>>   {
>> -       /* do not disclose physical addresses: attack vector */
>> -       if (!capable(CAP_SYS_ADMIN))
>> -               return -EPERM;
>
> THIS  is where you are supposed to check for capabilities. The place
> where you removed it!
>
> The reason we check capabilities at open time, and open time ONLY is
> because that is really very integral to the whole Unix security model.
> Otherwise, you get into this situation:
>
>   - unprivileged process opens file
>
>   - unprivileged process tricks suid process to do the actual access for it
>
> where the traditional model is to just force a "write()" by opening
> the file as stderr, and then executing a suid process (traditionally
> "sudo") that writes an error message to it.
>
> So *don't* do permission checks using read/write time credentials.
> They are wrong.
>
> Now, if there is some reason that you really can't do it when opening
> the file, and you actually need to use capability information at
> read/write time, you use the "file->f_cred" field, which is the
> open-time capabilities. So you _can_ do permission checks at
> read/write time, but you have to use the credentials of the opener,
> not "current".
>
> So in this case, I guess you could use
>
>          pm.show_pfn = file_ns_capable(file, &init_user_ns, CAP_SYS_ADMIN);
>
> if you really need to do this at read time, and cannot fill in that
> "show_pfn" at open-time.
>
>                          Linus
>


-- 
Konstantin

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>