D12291: Accept file descriptor only from root owned process
chinmoyr abandoned this revision. chinmoyr added a comment. 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. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D12291 To: chinmoyr, #frameworks, dfaure, ossi Cc: kde-frameworks-devel, ossi, michaelh, ngraham, bruns
D12291: Accept file descriptor only from root owned process
chinmoyr updated this revision to Diff 35010. chinmoyr added a comment. Accept socket connection where getsockopt() is not present REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D12291?vs=34966=35010 BRANCH master REVISION DETAIL https://phabricator.kde.org/D12291 AFFECTED FILES src/ioslaves/file/fdreceiver.cpp To: chinmoyr, #frameworks, dfaure, ossi Cc: kde-frameworks-devel, ossi, michaelh, ngraham, bruns
D12291: Accept file descriptor only from root owned process
ossi added inline comments. INLINE COMMENTS > chinmoyr wrote in fdreceiver.cpp:89 > > i don't see why that would be horrible > > 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? > > [1]: https://bugzilla.suse.com/show_bug.cgi?id=1033055#c13 i initially didn't notice the problem we're currently discussing. but more generally: it's a second layer of security, just in case somebody accidentally f*cks up the perms of their runtime dir (not something to be particularly concerned about; you'd certainly have bigger problems in this case). it might also help detecting configuration problems (though for that you'd have to add reasonable error reporting). and if done right, it's (currently) harmless, and i didn't feel like arguing over it. but i myself would just drop it. REPOSITORY R241 KIO BRANCH master REVISION DETAIL https://phabricator.kde.org/D12291 To: chinmoyr, #frameworks, dfaure, ossi Cc: kde-frameworks-devel, ossi, michaelh, ngraham, bruns
D12291: Accept file descriptor only from root owned process
chinmoyr added inline comments. INLINE COMMENTS > ossi wrote in 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. > i don't see why that would be horrible 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? [1]: https://bugzilla.suse.com/show_bug.cgi?id=1033055#c13 REPOSITORY R241 KIO BRANCH master REVISION DETAIL https://phabricator.kde.org/D12291 To: chinmoyr, #frameworks, dfaure, ossi Cc: kde-frameworks-devel, ossi, michaelh, ngraham, bruns
D12291: Accept file descriptor only from root owned process
ossi added inline comments. INLINE COMMENTS > chinmoyr wrote in fdreceiver.cpp:89 > I don't think making acceptConnection unconditionally true is a good idea. At > least it will make this patch look horrible. > Since getsocketopt conforms to posix, how about we check for __ unix __ , and > posix compliance instead of __ linux __ and upon failure inform the user to > install a posix compliant libc? 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. REPOSITORY R241 KIO BRANCH master REVISION DETAIL https://phabricator.kde.org/D12291 To: chinmoyr, #frameworks, dfaure, ossi Cc: kde-frameworks-devel, ossi, michaelh, ngraham, bruns
D12291: Accept file descriptor only from root owned process
chinmoyr added inline comments. INLINE COMMENTS > ossi wrote in 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. I don't think making acceptConnection unconditionally true is a good idea. At least it will make this patch look horrible. Since getsocketopt conforms to posix, how about we check for __ unix __ , and posix compliance instead of __ linux __ and upon failure inform the user to install a posix compliant libc? REPOSITORY R241 KIO BRANCH master REVISION DETAIL https://phabricator.kde.org/D12291 To: chinmoyr, #frameworks, dfaure, ossi Cc: kde-frameworks-devel, ossi, michaelh, ngraham, bruns
D12291: Accept file descriptor only from root owned process
ossi added inline comments. INLINE COMMENTS > fdreceiver.cpp:89 > +#else > +#warning Cannot get socket credentials! > +#endif 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. REPOSITORY R241 KIO BRANCH master REVISION DETAIL https://phabricator.kde.org/D12291 To: chinmoyr, #frameworks, dfaure, ossi Cc: kde-frameworks-devel, ossi, michaelh, ngraham, bruns
D12291: Accept file descriptor only from root owned process
ossi accepted this revision. This revision is now accepted and ready to land. REPOSITORY R241 KIO BRANCH master REVISION DETAIL https://phabricator.kde.org/D12291 To: chinmoyr, #frameworks, dfaure, ossi Cc: kde-frameworks-devel, ossi, michaelh, ngraham, bruns
D12291: Accept file descriptor only from root owned process
chinmoyr added a comment. @ossi would you mind reviewing this patch? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D12291 To: chinmoyr, #frameworks, dfaure Cc: kde-frameworks-devel, ossi, michaelh, ngraham, bruns
D12291: Accept file descriptor only from root owned process
chinmoyr updated this revision to Diff 34966. chinmoyr added a comment. Restricted Application added a subscriber: kde-frameworks-devel. I only realized now that after making the previous change I forgot to rebase. So uploading correct patch. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D12291?vs=33776=34966 BRANCH master REVISION DETAIL https://phabricator.kde.org/D12291 AFFECTED FILES src/ioslaves/file/fdreceiver.cpp To: chinmoyr, #frameworks, dfaure Cc: kde-frameworks-devel, ossi, michaelh, ngraham, bruns
D12291: Accept file descriptor only from root owned process
chinmoyr updated this revision to Diff 33776. chinmoyr marked an inline comment as done. chinmoyr added a comment. Updated the comment REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D12291?vs=32410=33776 BRANCH master REVISION DETAIL https://phabricator.kde.org/D12291 AFFECTED FILES src/ioslaves/file/fdreceiver.cpp To: chinmoyr, #frameworks, dfaure Cc: ossi, michaelh, ngraham, bruns
D12291: Accept file descriptor only from root owned process
ossi added a comment.
as i certainly mentioned somewhere else already, this is redundant with
putting the socket in a safe place. but fair enough ...
INLINE COMMENTS
> fdreceiver.cpp:67
> if (client > 0) {
> -FDMessageHeader msg;
> -if (::recvmsg(client, msg.message(), 0) == 2) {
> -::memcpy(_fileDes, CMSG_DATA(msg.cmsgHeader()), sizeof
> m_fileDes);
> +// Receive fd only if socket owner is root
> +bool acceptConnection = false;
i'd append "(our setuid helper)" to that - i wondered for a moment why the
limitation.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D12291
To: chinmoyr, #frameworks, dfaure
Cc: ossi, michaelh, ngraham, bruns
D12291: Accept file descriptor only from root owned process
chinmoyr created this revision. chinmoyr added reviewers: Frameworks, dfaure. Restricted Application added a project: Frameworks. chinmoyr requested review of this revision. REVISION SUMMARY 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. REPOSITORY R241 KIO BRANCH master REVISION DETAIL https://phabricator.kde.org/D12291 AFFECTED FILES src/ioslaves/file/fdreceiver.cpp To: chinmoyr, #frameworks, dfaure Cc: michaelh, bruns
