Changeset View
Standalone View
src/ioslaves/file/fdreceiver.cpp
| Show First 20 Lines • Show All 65 Lines • ▼ Show 20 Line(s) | |||||
| 66 | { | 66 | { | ||
| 67 | return m_socketDes >= 0 && m_readNotifier; | 67 | return m_socketDes >= 0 && m_readNotifier; | ||
| 68 | } | 68 | } | ||
| 69 | 69 | | |||
| 70 | void FdReceiver::receiveFileDescriptor() | 70 | void FdReceiver::receiveFileDescriptor() | ||
| 71 | { | 71 | { | ||
| 72 | int client = ::accept(m_socketDes, NULL, NULL); | 72 | int client = ::accept(m_socketDes, NULL, NULL); | ||
| 73 | if (client > 0) { | 73 | if (client > 0) { | ||
| 74 | // Receive fd only if socket owner is root (our setuid helper) | ||||
| 75 | bool acceptConnection = true; | ||||
| 76 | #if defined(__linux__) | ||||
| 77 | ucred cred; | ||||
| 78 | socklen_t len = sizeof(cred); | ||||
| 79 | if (getsockopt(client, SOL_SOCKET, SO_PEERCRED, &cred, &len) != 0 || cred.uid != 0) { | ||||
| 80 | acceptConnection = false; | ||||
| 81 | } | ||||
| 82 | #elif defined(__FreeBSD__) || defined(__APPLE__) | ||||
| 83 | uid_t uid; | ||||
| 84 | gid_t gid; | ||||
| 85 | if (getpeereid(m_socketDes, &uid, &gid) != 0 && uid != 0) { | ||||
| 86 | acceptConnection = false; | ||||
| 87 | } | ||||
| 88 | #else | ||||
| 89 | #warning Cannot get socket credentials! | ||||
ossi: i wonder whether that shouldn't come with an unconditional acceptConnection = true; then - now… | |||||
I don't think making acceptConnection unconditionally true is a good idea. At least it will make this patch look horrible. chinmoyr: I don't think making acceptConnection unconditionally true is a good idea. At least it will… | |||||
i don't see why that would be horrible; as i pointed out multiple times already, this change is redundant. one correction, though: add a code comment here rather than extending the commit message. getsockopt() is standard, but the actual options aren't. you could change the ifdef to SO_PEERCRED itself, but that wouldn't actually add any portability. ossi: i don't see why that would be horrible; as i pointed out multiple times already, this change is… | |||||
I meant adding "acceptConnection = true;" after #warning would look weird. Obviously that's not even an issue and I shouldn't have mentioned it. There is a discussion[1] going on related to a similar change in ktexteditor. Because ktexteditor also uses polkit to save files in read-only location, one of the suggestions to improve this process, in case the owner of target is not root, was to either ignore the operation or drop privileges to owner/group of the directory. Now in KIO the kauth helper performs every operation as root. So if in future it is decided to do a privilege drop before performing any file operation on non-root targets then this change will likely be a hindrance. After considering the fact that this is also redundant, now I am not really feeling confident about this change. Just out of curiosity, I want to know (although I feel weird for asking this) what was your reason for accepting this patch? chinmoyr: >i don't see why that would be horrible
I meant adding "acceptConnection = true;" after… | |||||
i initially didn't notice the problem we're currently discussing. ossi: i initially didn't notice the problem we're currently discussing.
but more generally: it's a… | |||||
| 90 | #endif | ||||
| 91 | if (acceptConnection) { | ||||
| 74 | FDMessageHeader msg; | 92 | FDMessageHeader msg; | ||
| 75 | if (::recvmsg(client, msg.message(), 0) == 2) { | 93 | if (::recvmsg(client, msg.message(), 0) == 2) { | ||
| 76 | ::memcpy(&m_fileDes, CMSG_DATA(msg.cmsgHeader()), sizeof m_fileDes); | 94 | ::memcpy(&m_fileDes, CMSG_DATA(msg.cmsgHeader()), sizeof m_fileDes); | ||
| 77 | } | 95 | } | ||
i'd append "(our setuid helper)" to that - i wondered for a moment why the limitation. ossi: i'd append "(our setuid helper)" to that - i wondered for a moment why the limitation. | |||||
| 96 | } | ||||
| 78 | ::close(client); | 97 | ::close(client); | ||
| 79 | } | 98 | } | ||
| 80 | } | 99 | } | ||
| 81 | 100 | | |||
| 82 | int FdReceiver::fileDescriptor() const | 101 | int FdReceiver::fileDescriptor() const | ||
| 83 | { | 102 | { | ||
| 84 | return m_fileDes; | 103 | return m_fileDes; | ||
| 85 | } | 104 | } | ||
i wonder whether that shouldn't come with an unconditional acceptConnection = true; then - now the compilation succeeds, but it will fail to operate. given that the whole feature is redundant with moving the socket to a safe place (something you really should mention in the commit message), this seems like a rather pointless limitation.