Differential D9966

[KIO] Fix issues with sharing of file descriptor
AbandonedPublic
Actions

Authored by chinmoyr on Jan 18 2018, 4:37 PM.

Details

Reviewers
thiago
dfaure
ossi
fvogt
Group Reviewers
Frameworks
Commits
R241:8d8f3025dffd: [KIO] Fix issues with sharing of file descriptor
Summary

This patch changes FdReceiver to create pathname socket in user's runtime directory and receive file descriptor only
from a root owned process.

Issues mentioned in here : https://mail.kde.org/pipermail/kde-frameworks-devel/2018-January/055307.html

Diff Detail

Repository
R241 KIO
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
There are a very large number of changes, so older changes are hidden. Show Older Changes
chinmoyr added a comment.Jan 23 2018, 6:29 PM

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

fvogt requested changes to this revision.Jan 23 2018, 7:07 PM
fvogt added inline comments.
src/ioslaves/file/fdreceiver.cpp
48

Missing

::close(m_socketDes);
m_socketDes = -1;

or just move it above the m_socketDes assignment.

62

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.

src/ioslaves/file/file_unix.cpp
92

That looks like a hack. If errno is that important, save and keep it in a variable. Every call into a library can change errno, it's extremely volatile.

src/ioslaves/file/sharefd_p.h
53 ↗(On Diff #25839)

Just a suggestion: Remove error() and just return nullptr in address() in the error case.
That way it's not possible to use it accidentially.

66 ↗(On Diff #25839)

The socket permissions only work that way for linux, so if other OSs can use this code as well it's insecure and should #error out.

This revision now requires changes to proceed.Jan 23 2018, 7:07 PM
chinmoyr updated this revision to Diff 25897.Jan 24 2018, 5:20 PM

1.Return nullptr when address() is called for an invalid socket path
2.Create SocketAddress object before socket .

chinmoyr marked 2 inline comments as done.Jan 24 2018, 5:44 PM
chinmoyr added inline comments.
src/ioslaves/file/fdreceiver.cpp
62

No. Currently only one FdReceiver is created.

src/ioslaves/file/file_unix.cpp
92

It is the only case for which this hack seems necessary. For all other cases a library call (to perform a file operation) is immediately followed by a call to helper. IMO the chances of errno changing to something unrelated in between these two calls are very slim (is it even possible?)
Although errno is important, saving it for every call will result in unnecessary code. Can't we make an exception for this case?

src/ioslaves/file/sharefd_p.h
66 ↗(On Diff #25839)

I didn't follow you here. Can you explain why working of this code on other OSs, specifically FreeBsd and OSX, will be insecure?

fvogt requested changes to this revision.Jan 24 2018, 6:21 PM
fvogt added inline comments.
src/ioslaves/file/file_unix.cpp
92

I don't see how this could ever work. Even the line immediately below errno = err can change errno.
You must not assume that errno does not change if you call a function. Save it immediately after the function which errno you are interested in returns.
The famous "Could not perform operation: Success" - kind of error messages happens exactly because of bugs like these.

src/ioslaves/file/sharefd_p.h
66 ↗(On Diff #25839)

Look at man 7 unix, section Pathname socket ownership and permissions.

This revision now requires changes to proceed.Jan 24 2018, 6:21 PM
chinmoyr updated this revision to Diff 25941.Jan 25 2018, 4:05 PM

1.Change errno value just before execWithPrivilege call

chinmoyr added inline comments.Jan 25 2018, 4:55 PM
src/ioslaves/file/file_unix.cpp
110

@fvogt I think the errno assignment should be fine here?
Except this one case, the function call whose errno value we are interested in is immediately followed by execWithElevatedPrivilege (which checks errno as the first thing). So I was saying earlier that saving errno there isn't necessary because there is no (other) library call between the failed function call and execWithElevatedPrivilege.

src/ioslaves/file/sharefd_p.h
66 ↗(On Diff #25839)

I see, it mentions some BSD-derived system ignore read/write permissions on socket file. But in FreeBSD documentation it clearly specifies destination of connect() should be writable.
So shall I error out if OS is not linux or freebsd or mac os?

fvogt requested changes to this revision.Jan 25 2018, 7:04 PM
fvogt added inline comments.
src/ioslaves/file/file_unix.cpp
110

This looks very much like code smell. Currently you treat errno as a hidden function parameter.
IMO you should make it explicit.

src/ioslaves/file/sharefd_p.h
66 ↗(On Diff #25839)

If that is the case you can do something like:

#if !defined(__linux__) && !defined(__FreeBSD__)
#error No secure implementation available
#endif
This revision now requires changes to proceed.Jan 25 2018, 7:04 PM
chinmoyr updated this revision to Diff 26028.Jan 27 2018, 9:27 AM

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 a look at it.

chinmoyr marked an inline comment as done.Jan 27 2018, 9:38 AM
chinmoyr added inline comments.
src/ioslaves/file/sharefd_p.h
47 ↗(On Diff #26028)

@fvogt Is +1 for null byte required here?

66 ↗(On Diff #25839)

I think this is not needed. It's already there in FdReceiver.

fvogt requested changes to this revision.Jan 27 2018, 10:39 AM

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 identical)

src/ioslaves/file/file.cpp
239 ↗(On Diff #26028)

errno might not be the expected value anymore.

src/ioslaves/file/sharefd_p.h
47 ↗(On Diff #26028)

Not on most platforms, but man unix says for portability:

offsetof(struct sockaddr_un, sun_path)+strlen(addr.sun_path)+1```.
66 ↗(On Diff #25839)

Fair enough.

This revision now requires changes to proceed.Jan 27 2018, 10:39 AM
chinmoyr updated this revision to Diff 26044.Jan 27 2018, 11:38 AM
  1. Added +1 to SocketAddress::length for null byte.
  2. Modified PrivilegeOperationReturnValue to use with switch-case and if-else construct.
chinmoyr updated this revision to Diff 26045.Jan 27 2018, 11:43 AM

Fix typo. (int->bool in file_p.h line 59)

fvogt resigned from this revision.Jan 27 2018, 11:49 AM

Works for me. I'll leave the final review to someone else (@dfaure?).

dfaure requested changes to this revision.Jan 28 2018, 7:35 PM

I don't feel confident approving the security-related fixes in here. Maybe Thiago or Oswald could have a look...

src/ioslaves/file/fdreceiver.cpp
62

This will break if the path contains non-ascii characters.

Either use QFile::remove, or use a QByteArray (or std::string) everywhere to avoid a conversion from 16-bit to 8-bit, or third option, do the conversion properly here, using QFile::encodeName(m_path).

src/ioslaves/file/sharefd_p.h
51 ↗(On Diff #26045)

Is strlen needed? It seems to me that sun_path will be null if make_address failed, so a simple null-pointer check would be enough here. Plus I remember some implementations of strlen breaking on null pointers...

This revision now requires changes to proceed.Jan 28 2018, 7:35 PM
chinmoyr updated this revision to Diff 26220.Jan 30 2018, 6:24 PM
  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().
chinmoyr added a reviewer: ossi.Jan 30 2018, 6:26 PM
chinmoyr marked 2 inline comments as done.
dfaure accepted this revision.Feb 2 2018, 11:42 PM

Thiago, Ossi, looks ok to you too?

(Tagging tomorrow...)

src/ioslaves/file/sharefd_p.h
51 ↗(On Diff #26045)

Looks good.

My own comment was partly nonsense, I see now (sun_path can't be a null pointer). But still this change is for the better, it makes it faster.

This revision is now accepted and ready to land.Feb 2 2018, 11:42 PM
Closed by commit R241:8d8f3025dffd: [KIO] Fix issues with sharing of file descriptor (authored by chinmoyr). · Explain WhyFeb 3 2018, 1:47 AM
This revision was automatically updated to reflect the committed changes.
thiago added a comment.Feb 3 2018, 2:43 AM

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.

src/ioslaves/file/fdreceiver.cpp
34

Don't use toStdString(). I know this is what it used to do, but you can take the opportunity to fix the issue.

87

Where's the support for other OSes?

src/ioslaves/file/fdreceiver.h
45 ↗(On Diff #26403)

Nitpick: always sort your members by size.

src/ioslaves/file/file.h
106 ↗(On Diff #26403)

This change should be in a separate commit. I can't tell what you're doing specifically to fix the bug with so much noise in this commit.

dfaure added a comment.Feb 3 2018, 9:25 AM

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 all, in case of no answer...

src/ioslaves/file/fdreceiver.cpp
34

Right, this should be QFile::encodeName(m_path) and using QByteArray in SocketAddress rather than std::string.

87

This is UNIX-only, but indeed OSX is missing. Does that support getpeereid?

fvogt added a comment.Feb 3 2018, 10:26 AM
In 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 the mail are not fixed, this shouldn't get re-enabled.

src/ioslaves/file/file.h
106 ↗(On Diff #26403)

The fixes in this commit uncovered a design issue in the code which this fixes.
It could probably be split though.

chinmoyr reopened this revision.Feb 3 2018, 10:51 AM
This revision is now accepted and ready to land.Feb 3 2018, 10:51 AM
chinmoyr updated this revision to Diff 26421.Feb 3 2018, 10:56 AM
  1. Used QByteArray in SocketAddress.
  2. Rearranged members of FdReceiver.
  3. Removed changes not directly related to these fixes.
dfaure added a comment.Feb 3 2018, 11:01 AM

Thanks, looks ok to me now.

Thiago: now it's ready for your review ;)

ossi added a comment.Feb 3 2018, 11:45 AM
In 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.
https://community.kde.org/Policies/Commit_Policy#Commit_complete_changesets (i find https://wiki.qt.io/Commit_Policy point 8 clearer (yeah, i wrote it down myself ^^))

dfaure added a comment.Feb 3 2018, 12:50 PM

Right, the port to QByteArray could be splitted out of this socket security fix commit.

chinmoyr updated this revision to Diff 26446.Feb 3 2018, 4:31 PM

Moved changes related to SocketAddress to D10273.
Changed #error to #warning. So even if an OS does not have getpeereid or getsockopt compilation will still succeed but copying files will result in an error.

chinmoyr edited the summary of this revision. (Show Details)Feb 3 2018, 4:33 PM
thiago added a comment.Feb 3 2018, 5:35 PM

The patch that I can see accomplishes what the description says it should do. And I agree with the idea of the patch.

src/ioslaves/file/fdreceiver.cpp
87

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 Build From Sources.

chinmoyr added inline comments.Feb 3 2018, 5:39 PM
src/ioslaves/file/fdreceiver.cpp
87

I have replaced #error with #warning
so there shouldn't be any breakage.

chinmoyr added a comment.Feb 9 2018, 4:29 PM

Is the patch good enough to be pushed?

ossi added a comment.Mar 4 2018, 11:46 AM

looks good, though technically speaking this still fixes two separate issues.

chinmoyr abandoned this revision.Apr 17 2018, 5:58 PM

Changes split in D12291 and D10411

ossi added a comment.May 27 2018, 11:10 AM

this dependency tree is a mess. please remove deps to abandoned changes or unabandon what you actually still need, and linearize the history.

Restricted Application added a subscriber: kde-frameworks-devel. · View Herald TranscriptMay 27 2018, 11:10 AM

Revision Contents
Changeset List

Diff 26446

View Options

src/ioslaves/file/fdreceiver.cpp

Loading...
View Options

src/ioslaves/file/file_unix.cpp

Loading...