D9966: [KIO] Fix issues with sharing of file descriptor

chinmoyr removed a dependent revision: D10410: Move the task of cleaning up of socket file to file ioslave and FdReceiver. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D9966 To: chinmoyr, #frameworks, thiago, dfaure, ossi Cc: kde-frameworks-devel, ngraham, fvogt,

D9966: [KIO] Fix issues with sharing of file descriptor

ossi added a comment. Restricted Application added a subscriber: kde-frameworks-devel. this dependency tree is a mess. please remove deps to abandoned changes or unabandon what you actually still need, and linearize the history. REPOSITORY R241 KIO REVISION DETAIL

D9966: [KIO] Fix issues with sharing of file descriptor

chinmoyr added a dependent revision: D10410: Move the task of cleaning up of socket file to file ioslave and FdReceiver. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D9966 To: chinmoyr, #frameworks, thiago, dfaure, ossi Cc: ngraham, fvogt, lbeltrame, dfaure, michaelh,

D9966: [KIO] Fix issues with sharing of file descriptor

chinmoyr removed a dependent revision: D10410: Move the task of cleaning up of socket file to file ioslave and FdReceiver. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D9966 To: chinmoyr, #frameworks, thiago, dfaure, ossi Cc: ngraham, fvogt, lbeltrame, dfaure, michaelh,

D9966: [KIO] Fix issues with sharing of file descriptor

chinmoyr abandoned this revision. chinmoyr added a comment. Changes split in D12291 and D10411 REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D9966 To: chinmoyr, #frameworks, thiago, dfaure, ossi

D9966: [KIO] Fix issues with sharing of file descriptor

ossi added a comment. looks good, though technically speaking this still fixes two separate issues. REPOSITORY R241 KIO BRANCH master REVISION DETAIL https://phabricator.kde.org/D9966 To: chinmoyr, #frameworks, thiago, dfaure, ossi Cc: ngraham, fvogt, lbeltrame, dfaure, michaelh

D9966: [KIO] Fix issues with sharing of file descriptor

chinmoyr added a dependent revision: D10410: Move the task of cleaning up of socket file to file ioslave and FdReceiver. REPOSITORY R241 KIO BRANCH master REVISION DETAIL https://phabricator.kde.org/D9966 To: chinmoyr, #frameworks, thiago, dfaure, ossi Cc: ngraham, fvogt, lbeltrame,

D9966: [KIO] Fix issues with sharing of file descriptor

chinmoyr added a comment. Is the patch good enough to be pushed? REPOSITORY R241 KIO BRANCH master REVISION DETAIL https://phabricator.kde.org/D9966 To: chinmoyr, #frameworks, thiago, dfaure, ossi Cc: ngraham, fvogt, lbeltrame, dfaure, michaelh

D9966: [KIO] Fix issues with sharing of file descriptor

chinmoyr added inline comments. INLINE COMMENTS > thiago wrote in fdreceiver.cpp:88 > __APPLE__ is there. My question is about OpenBSD, NetBSD and DragonflyBSD. > Are you breaking them? You may not know the answer, but you need to document > the issue if you intentionally cause a Failure To

D9966: [KIO] Fix issues with sharing of file descriptor

thiago added a comment. The patch that I can see accomplishes what the description says it should do. And I agree with the idea of the patch. INLINE COMMENTS > dfaure wrote in fdreceiver.cpp:88 > This is UNIX-only, but indeed OSX is missing. Does that support getpeereid? __APPLE__ is

D9966: [KIO] Fix issues with sharing of file descriptor

chinmoyr edited the summary of this revision. REPOSITORY R241 KIO BRANCH master REVISION DETAIL https://phabricator.kde.org/D9966 To: chinmoyr, #frameworks, thiago, dfaure, ossi Cc: ngraham, fvogt, lbeltrame, dfaure, michaelh

D9966: [KIO] Fix issues with sharing of file descriptor

chinmoyr updated this revision to Diff 26446. chinmoyr added a comment. Moved changes related to SocketAddress to https://phabricator.kde.org/D10273. Changed #error to #warning. So even if an OS does not have getpeereid or getsockopt compilation will still succeed but copying files will

D9966: [KIO] Fix issues with sharing of file descriptor

dfaure added a comment. Right, the port to QByteArray could be splitted out of this socket security fix commit. REPOSITORY R241 KIO BRANCH master REVISION DETAIL https://phabricator.kde.org/D9966 To: chinmoyr, #frameworks, thiago, dfaure, ossi Cc: ngraham, fvogt, lbeltrame, dfaure,

D9966: [KIO] Fix issues with sharing of file descriptor

ossi added a comment. In https://phabricator.kde.org/D9966#199979, @dfaure wrote: > Thiago: now it's ready for your review ;) if he wants to look past the already pointed out non-atomicity. i for one just refuse to look at this mess any further.

D9966: [KIO] Fix issues with sharing of file descriptor

dfaure added a comment. Thanks, looks ok to me now. Thiago: now it's ready for your review ;) REPOSITORY R241 KIO BRANCH master REVISION DETAIL https://phabricator.kde.org/D9966 To: chinmoyr, #frameworks, thiago, dfaure, ossi Cc: ngraham, fvogt, lbeltrame, dfaure, michaelh

D9966: [KIO] Fix issues with sharing of file descriptor

chinmoyr updated this revision to Diff 26421. chinmoyr added a comment. 1. Used QByteArray in SocketAddress. 2. Rearranged members of FdReceiver. 3. Removed changes not directly related to these fixes. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE

D9966: [KIO] Fix issues with sharing of file descriptor

chinmoyr reopened this revision. This revision is now accepted and ready to land. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D9966 To: chinmoyr, #frameworks, thiago, dfaure, ossi Cc: ngraham, fvogt, lbeltrame, dfaure, michaelh

D9966: [KIO] Fix issues with sharing of file descriptor

fvogt added a comment. In https://phabricator.kde.org/D9966#199916, @dfaure wrote: > The early push was so we don't release 5.43 (planned for today) without this security fix at all, in case of no answer... Timing is not that important. As long as the other issues mentioned in

D9966: [KIO] Fix issues with sharing of file descriptor

dfaure added a comment. Thiago: thanks for the review. You didn't say anything about the getsockopt/getpeereid usage so I assume this part looks ok? That's mostly what I wanted you to look at. The early push was so we don't release 5.43 (planned for today) without this security fix at

D9966: [KIO] Fix issues with sharing of file descriptor

thiago added a comment. Why do you ask for a rewview then not wait for the review? Belated -1. No actual review of the change done, because I can't tell what you've done. INLINE COMMENTS > fdreceiver.cpp:34 > { > +SocketAddress addr(m_path.toStdString()); > +if

D9966: [KIO] Fix issues with sharing of file descriptor

This revision was automatically updated to reflect the committed changes. Closed by commit R241:8d8f3025dffd: [KIO] Fix issues with sharing of file descriptor (authored by chinmoyr). REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D9966?vs=26220=26403 REVISION

D9966: [KIO] Fix issues with sharing of file descriptor

dfaure accepted this revision. dfaure added a comment. This revision is now accepted and ready to land. Thiago, Ossi, looks ok to you too? (Tagging tomorrow...) INLINE COMMENTS > dfaure wrote in sharefd_p.h:51 > Is strlen needed? It seems to me that sun_path will be null if make_address

D9966: [KIO] Fix issues with sharing of file descriptor

chinmoyr updated this revision to Diff 26220. chinmoyr added a comment. 1. Added null pointer check in place of strlen (in SocketAddress::address()) 2. Used QFile::encodeName to convert file path to 8-bit for use in unlink(). REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE

D9966: [KIO] Fix issues with sharing of file descriptor

chinmoyr marked 2 inline comments as done. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D9966 To: chinmoyr, #frameworks, thiago, dfaure, ossi Cc: ngraham, fvogt, lbeltrame, dfaure, michaelh

D9966: [KIO] Fix issues with sharing of file descriptor

chinmoyr added a reviewer: ossi. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D9966 To: chinmoyr, #frameworks, thiago, dfaure, ossi Cc: ngraham, fvogt, lbeltrame, dfaure, michaelh

D9966: [KIO] Fix issues with sharing of file descriptor

dfaure requested changes to this revision. dfaure added a comment. This revision now requires changes to proceed. I don't feel confident approving the security-related fixes in here. Maybe Thiago or Oswald could have a look... INLINE COMMENTS > fdreceiver.cpp:61 > } > +

D9966: [KIO] Fix issues with sharing of file descriptor

fvogt resigned from this revision. fvogt added a comment. Works for me. I'll leave the final review to someone else (@dfaure?). REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D9966 To: chinmoyr, #frameworks, thiago Cc: ngraham, fvogt, lbeltrame, dfaure, michaelh

D9966: [KIO] Fix issues with sharing of file descriptor

chinmoyr updated this revision to Diff 26045. chinmoyr added a comment. Fix typo. (int->bool in file_p.h line 59) REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D9966?vs=26044=26045 BRANCH master REVISION DETAIL https://phabricator.kde.org/D9966 AFFECTED

D9966: [KIO] Fix issues with sharing of file descriptor

chinmoyr updated this revision to Diff 26044. chinmoyr added a comment. 1. Added +1 to SocketAddress::length for null byte. 2. Modified PrivilegeOperationReturnValue to use with switch-case and if-else construct. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE

D9966: [KIO] Fix issues with sharing of file descriptor

fvogt requested changes to this revision. fvogt added a comment. This revision now requires changes to proceed. Almost done! I just noticed some small issues with reading errno after calling `execWithElevatedPrivileges(...)` (I only left a single comment, the other places are basically

D9966: [KIO] Fix issues with sharing of file descriptor

chinmoyr marked an inline comment as done. chinmoyr added inline comments. INLINE COMMENTS > fvogt wrote in sharefd_p.h:66 > If that is the case you can do something like: > > #if !defined(__linux__) && !defined(__FreeBSD__) > #error No secure implementation available > #endif I think

D9966: [KIO] Fix issues with sharing of file descriptor

chinmoyr updated this revision to Diff 26028. chinmoyr added a comment. Changed parameters of execWithElevatedPrivileges(), tryChangeAttr() : now they accept action type, list of arguments and error code. tryOpen(): accepts error code as an additional parameter. @dfaure please have

D9966: [KIO] Fix issues with sharing of file descriptor

fvogt requested changes to this revision. fvogt added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > chinmoyr wrote in file_unix.cpp:117 > @fvogt I think the errno assignment should be fine here? > Except this one case, the function call whose errno value we

D9966: [KIO] Fix issues with sharing of file descriptor

chinmoyr added inline comments. INLINE COMMENTS > file_unix.cpp:117 > +errno = err; > + > if (auto err = execWithElevatedPrivilege(OPEN, path, flags, mode)) { @fvogt I think the errno assignment should be fine here? Except this one case, the function call whose errno value we are

D9966: [KIO] Fix issues with sharing of file descriptor

chinmoyr updated this revision to Diff 25941. chinmoyr added a comment. 1.Change errno value just before execWithPrivilege call REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D9966?vs=25897=25941 BRANCH master REVISION DETAIL

D9966: [KIO] Fix issues with sharing of file descriptor

fvogt requested changes to this revision. fvogt added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > chinmoyr wrote in file_unix.cpp:91 > It is the only case for which this hack seems necessary. For all other cases > a library call (to perform a file

D9966: [KIO] Fix issues with sharing of file descriptor

chinmoyr marked 2 inline comments as done. chinmoyr added inline comments. INLINE COMMENTS > fvogt wrote in fdreceiver.cpp:60 > A quick question: Is it possible that there are two FdReceivers with the same > pid? > In that case they would end up removing each other's sockets. No. Currently

D9966: [KIO] Fix issues with sharing of file descriptor

chinmoyr updated this revision to Diff 25897. chinmoyr added a comment. 1.Return nullptr when address() is called for an invalid socket path 2.Create SocketAddress object before socket . REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D9966?vs=25839=25897

D9966: [KIO] Fix issues with sharing of file descriptor

fvogt requested changes to this revision. fvogt added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > fdreceiver.cpp:42 > +std::cerr << "Invalid socket address" << std::endl; > +return; > +} Missing ::close(m_socketDes); m_socketDes =

D9966: [KIO] Fix issues with sharing of file descriptor

chinmoyr updated this revision to Diff 25839. chinmoyr added a comment. 1.Check for overflow condition. In such case file operation will terminate. 2.Return actual length of buffer 3.Use QStandardPaths::RuntimeLocation for socket creation REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE

D9966: [KIO] Fix issues with sharing of file descriptor

dfaure added a comment. Note: if someone wants to use XDG_RUNTIME_DIR with Qt, use QStandardPaths::RuntimeLocation (which includes the proper security checks for that directory, and fallbacks). REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D9966 To: chinmoyr,

D9966: [KIO] Fix issues with sharing of file descriptor

fvogt added a comment. Thanks for the quick reaction! > 1.Fixes buffer overflow due to strcpy. Looks good, but I would prefer an exception or abort instead of silent truncation. Also note that this makes it possible to delete an arbitrary file on non-linux platforms if `path`

D9966: [KIO] Fix issues with sharing of file descriptor

chinmoyr created this revision. chinmoyr added reviewers: Frameworks, thiago. Restricted Application added a project: Frameworks. chinmoyr requested review of this revision. REVISION SUMMARY This patch, 1.Fixes buffer overflow due to strcpy. 2.Adds checks for socket credentials. Now a file