[prev in list] [next in list] [prev in thread] [next in thread] 

List:       oss-security
Subject:    Re: [oss-security] accountsservice: insufficient path check in user_change_icon_file_authorized_cb()
From:       Jakub Wilk <jwilk () jwilk ! net>
Date:       2018-07-20 15:47:24
Message-ID: 20180720154723.5rgdhg2n5nj5bgce () jwilk ! net
[Download RAW message or body]

* Matthias Gerstner <mgerstner@suse.de>, 2018-07-02, 16:38:
>>>I think the easiest way to fix this is to normalize the user supplied 
>>>filename e.g. using realpath()
>>
>>Using realpath(3) for access control is almost always a mistake: this 
>>function expands symlinks, including attacker-controlled symlinks.
>
>can you elaborate what your main worry of using realpath is in this 
>context?

AIUI, in your original patch, canonicalized path was used for prefix 
check, but then the orignal was stored. If you used realpath() for 
canonicalization, the attacker could make a symlink that points to 
/usr/share/icons/moo.png, so that the check passes, and then switch 
the symlink to something else.

But in the patch that went upstream[0], it's the canonicalized path that 
is stored, which is probably a good idea anyway.

Another problem with realpath(), unrelated to symlinks, is that if it's 
run as root, it could reveal to the attacker whether an 
otherwise-inaccessible directory exists. For example, 
realpath("/home/bob/foobar/../../../usr/share/icons/moo.png", ...) would 
succeed iff /home/bob/foobar/ existed.


[0] https://cgit.freedesktop.org/accountsservice/commit/?id=f9abd359f71a5bce421b9ae23432f539a067847a

-- 
Jakub Wilk
[prev in list] [next in list] [prev in thread] [next in thread]