Avoid giving an stderr to kwallet

Authored by maximilianocuria on May 4 2018, 8:41 PM.



The fixes for CVE-2018-10380 introduced a regression for most users not
using kde, and some for kde sessions. In particular the reorder of the
close calls and creating a new socket caused that the socket is always
assigned the file descriptor 2, aka stderr.

BUG: 393856

Test Plan

It works

Diff Detail

R107 KWallet PAM Integration
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.
Restricted Application added a project: Plasma. · View Herald TranscriptMay 4 2018, 8:41 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
maximilianocuria requested review of this revision.May 4 2018, 8:41 PM

Thanks for investigating.

Might be cleaner to explicitly open the stderr FD earlier and not do anything with it, which would force the socket() call to give us an unused one.

But if that doesn't work...+1 from me.

maximilianocuria updated this revision to Diff 33663.EditedMay 4 2018, 9:41 PM

A somewhat simpler version, just delay the close(2) call. Based on @davidedmundson

I was testing this and trying to look for some documentation about the use of
the fds in pam modules. I'm pretty sure that you can't keep them, but I haven't found
anything that talks about this.

I think I like the previous version better.

Another approach could be to simply drop privileges first, then open the socket, and close the fds after that.

rdieter added a subscriber: rdieter.May 5 2018, 5:18 AM
aacid added a comment.May 5 2018, 9:27 AM

Are you sure this is a fix or is it just a hunch? i.e. can you reproduce the bug without this patch? (i can't, it works fine for me)

I can reproduce the issue in openbox and cinnamon. I think this is related to qt5ct which always tries to print things to stderr. I guess having the debug messages enabled would also trigger the issue.

The problem is no longer present with either version of the patch.

aacid accepted this revision.May 5 2018, 9:37 AM

Ok, then please commit (5,8, 5.12 and then merge 5.12 to master)

If you're unsure how to do the commits tell me and i can do it for you.

Ideally i'd like a more verbose comment for the "keep stderr open" comment, maybe "keep stderr open so socket doesn't returns us that fd"

This revision is now accepted and ready to land.May 5 2018, 9:37 AM

Update comments as suggested.

I'll push this, and ask the users that reported the issue to also test the
fixed version.


This revision was automatically updated to reflect the committed changes.

When this was getting a security review for Ubuntu from Seth Arnold, he wanted to ask the following (without having to make an account here):

"Why is stderr closed on line 429? Most programs can have unexpected results when these descriptors are not available."


I'm not sure, but I think that pam could be called from a process that has the stdin/stdout/stderr redirected to pipes, so disconnecting those for a long lived process would avoid a possible broken pipe. Now, if that's so, why it's not clossing stdin and stdout, I don't know.

Also, why hardcode the 64 instead of closing all the available fds (see https://github.com/openssh/openssh-portable/blob/master/openbsd-compat/bsd-closefrom.c as an example).

I guess there is room for improvement, but the change here follows the same logic of the original behavior. The original code starts kwalled without an stderr, and so does this version.

Happy hacking,

Btw, Debian users confirmed that this fixes the issue: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=897687

And in the "Kwallet does not work" thread here: https://lists.debian.org/debian-kde/2018/05/threads.html#00001