wrong permission on xauthority file

[Yves-Alexis Perez]

By default, the Xauthority file is created with mode 644, enabling any user to hijack another user X screen.

Attached patch seems to fix the problem for me.

[Yves-Alexis Perez (corsac)]

Hmmh, btw it might be cleaner to use G_FILE_CREATE_PRIVATE in g_file_replace() instead of changing the mode afterward, I just discovered that.

[Robert Ancell (robert-ancell)]

I've updated to do that

[Yves-Alexis Perez (corsac)]

Seems that the bug is back, since you replaced g_file_replace() by g_file_set_contents() which actually replaces atomically the whole file but then respects umask, which is not touched before doing that...

It has been reported in Debian as http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=721744

I'm not sure if it's possible to reopen a bug, so I'll report a new one if it's not.

[Robert Ancell (robert-ancell)]

Hi,

I've released lightdm 1.4.2 and 1.6.1 which have this fix backported.

On Thu, Sep 5, 2013 at 7:58 AM, Yves-Alexis Perez <email address hidden> wrote:

> Seems that the bug is back, since you replaced g_file_replace() by
> g_file_set_contents() which actually replaces atomically the whole file
> but then respects umask, which is not touched before doing that...
>
> It has been reported in Debian as http://bugs.debian.org/cgi-
> bin/bugreport.cgi?bug=721744
>
> I'm not sure if it's possible to reopen a bug, so I'll report a new one
> if it's not.
>
> ** Bug watch added: Debian Bug tracker #721744
> http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=721744
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/685212
>
> Title:
> wrong permission on xauthority file
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/lightdm/+bug/685212/+subscriptions
>

[Yves-Alexis Perez (corsac)]

I'm sorry but that doesn't fix the security bug at all. It doesn't fix the permissions if they are currently bad, and even if the .Xauthority file doesn't exist, fopen() won't do better than set_file_contents(). Actually checking for the return code of the function might have helped too, but anyway.

In any case, care should be taken to:

- fix the permissions of existing .Xauthority files in a secure way
- correctly set the umask (or use a function which does it) before writing a new file

[Yves-Alexis Perez (corsac)]

On mer., 2013-09-04 at 22:24 +0000, Robert Ancell wrote:
> I've released lightdm 1.4.2 and 1.6.1 which have this fix backported.

As said on the bug, the fix doesn't actually work.

Regards,
--
Yves-Alexis

[Robert Ancell (robert-ancell)]

Sorry, this was a reply to an email regarding the temporary files being left behind when updating Xauthority. Confirmed the bug is back - am working on a fix.

[Robert Ancell (robert-ancell)]

Should be fixed in 1.4.3, 1.6.2, 1.7.14

[Yves-Alexis Perez (corsac)]

I guess it'd be best to also check for the current file permissions and change them if needed (or change them inconditionally if the file already exists).

I can try to cook a patch, I'm just not too sure how to be really clean wrt. links attacks etc, although at that point the file should be written as user so it's safer than if it was done as root.