Limiting file ioslave to recieve file descriptor only from root owned process helps to make sure
that no malicious process interfere while a privilege operation is in progress by sending garbage
data to the socket.
Details
- Reviewers
dfaure ossi - Group Reviewers
Frameworks
Diff Detail
- Repository
- R241 KIO
- Branch
- master
- Lint
No Linters Available - Unit
No Unit Test Coverage
as i certainly mentioned somewhere else already, this is redundant with putting the socket in a safe place. but fair enough ...
src/ioslaves/file/fdreceiver.cpp | ||
---|---|---|
74–95 | i'd append "(our setuid helper)" to that - i wondered for a moment why the limitation. |
I only realized now that after making the previous change I forgot to rebase.
So uploading correct patch.
src/ioslaves/file/fdreceiver.cpp | ||
---|---|---|
89 | 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. |
src/ioslaves/file/fdreceiver.cpp | ||
---|---|---|
89 | I don't think making acceptConnection unconditionally true is a good idea. At least it will make this patch look horrible. |
src/ioslaves/file/fdreceiver.cpp | ||
---|---|---|
89 | 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. |
src/ioslaves/file/fdreceiver.cpp | ||
---|---|---|
89 |
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? |
src/ioslaves/file/fdreceiver.cpp | ||
---|---|---|
89 | i initially didn't notice the problem we're currently discussing. |
As of now this change is not of any importance. If in future a situation arises where we need it I will reopen this revision.