Create the xauth-<uid>-<display> under /run instead of /tmp
Needs ReviewPublic

Authored by antlarr on Nov 9 2016, 2:40 PM.

Details

Reviewers
dfaure
Summary

Use QStandardPaths::RuntimeLocation instead of /tmp as the location
for the XAUTHORITY file. This is runtime information and should not
be stored in /tmp.

Diff Detail

Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
antlarr updated this revision to Diff 8038.Nov 9 2016, 2:40 PM
antlarr retitled this revision from to Create the xauth-<uid>-<display> under /run instead of /tmp.
antlarr updated this object.
antlarr edited the test plan for this revision. (Show Details)
antlarr added a reviewer: aacid.
aacid edited edge metadata.Nov 9 2016, 6:05 PM

I really know nothing about this, my only involvement with this change was settiing it's umask "properly-ish".

You'll have to find someone else that really understands this code, sorry.

antlarr edited reviewers, added: dfaure; removed: aacid.Nov 10 2016, 7:50 AM
antlarr added a subscriber: aacid.Nov 10 2016, 8:06 AM
In D3317#61881, @aacid wrote:

I really know nothing about this, my only involvement with this change was settiing it's umask "properly-ish".

Ah, ok

You'll have to find someone else that really understands this code, sorry.

Thanks anyway, Albert. I removed you and added dfaure as reviewer in the hope he knows what to do here :)

dfaure edited edge metadata.Nov 10 2016, 8:21 AM

Did you test this with kdesu? According to the comment, the whole point here is to save a file that will be used by kdesu, i.e. as another user. Since RuntimeLocation is normally per-user, that other user might not have access to the file
(well ok, root would, but I suppose kdesu also allows logging in as another user?)

(I also don't know much about this code, it's from Luboš Luňák, see bug 75492 -- which might help giving you instructions to test this.)

In D3317#61947, @dfaure wrote:

Did you test this with kdesu? According to the comment, the whole point here is to save a file that will be used by kdesu, i.e. as another user. Since RuntimeLocation is normally per-user, that other user might not have access to the file
(well ok, root would, but I suppose kdesu also allows logging in as another user?)

Hehe, yes, but only in part. RuntimeLocation is normally per-user, so other users don't have access to that file. Agreed. But kdesu already has code to overcome this.
If you look at kdesu/src/kcookie.cpp, it reads the xauth cookie (look for proc.start(QStringLiteral("xauth") ), then in kdesu/src/stubprocess.cpp (look for writeLine(displayAuth());) it's written to the stdin of kdesu_stub (the process actually started by kdesu), which writes a new xauth file with that cookie in kdesu/src/kdesu_stub.c (look for mkstemp(xauthority); ).

I tested this both with root and another regular user, with "kdesu -u foo kcalc" which created a /tmp/xauth.XXXX5icBWi file while kcalc was run, and kdesu_stub removed it once the app was finished. So I think that's ok.

(I also don't know much about this code, it's from Luboš Luňák, see bug 75492 -- which might help giving you instructions to test this.)

After a long read (that bug is old, and is also a mixture of different unrelated bugreports, afaics) I managed to find something that fails, but note that it fails both before and after applying this patch.

If you run "kdesu -u foo dolphin", dolphin shows up, but doesn't show any file. Instead it shows the message: "Could not start process Can not create socket for launching io-slave for protocol 'file' "

After debugging this a bit, I found out the reason for not being able to create the socket is it's trying to use $XDG_RUNTIME_DIR (which was set to /run/user/1000/ a directory where the new user doesn't have access permissions).

Note that the usual case (using kdesu to run dolphin or some other application as root) "works" fine because actually, QStandardPaths::writableLocation checks that the owner of XDG_RUNTIME_DIR should be the same as the current uid, and if it's not, returns QString(), which results in the sockets being generated in / (yes, /kdeinit5__0 and /klauncherJ14914.1.slave-socket).

I'll provide a patch for kdesu_stub to unset XDG_RUNTIME_DIR if it doesn't have access to that directory. In that case, QStandardPaths::writableLocation will create a /tmp/runtime-<username> directory and use that as runtime dir.

Thanks for the great investigation. I'm surprised by "created in /", this sounds like a bug, it could very well be /tmp instead, no? (i.e. testing for the empty string return value).

Apart from that it sounds ok indeed.

@antlarr Did you ever figure out the bug where stuff gets created in '/'? Do you need help with that? Did we both completely forget about this? Is it a downstream patch meanwhile?