Re: accountsservice: insufficient path check in user_change_icon_file_authorized_cb()

[Jakub Wilk <jwilk () jwilk net>]

* 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