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
It works
| No Linters Available |
| No Unit Test Coverage |
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.
A somewhat simpler version, just delay the close(2) call. Based on @davidedmundson
suggestion.
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.
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.
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"
Update comments as suggested.
I'll push this, and ask the users that reported the issue to also test the
fixed version.
Thanks.
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."
Thanks!
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