Add support for sharing file descriptor between KIO slave and KAuth helper
ClosedPublic

Authored by chinmoyr on Jul 15 2017, 10:58 AM.

Details

Summary

When reading or writing a file with elevated privileges the helper will
open the required file (with elevated privileges) and it will share the
open file descriptor with file ioslave. Since the file referred to by the
shared file descriptor was opened by a privileged process, file ioslave
which is a normal user process will be able to modify the file.

This patch adds two classes, FdSender and FdReceiver. And as their
name suggest they facilitate sending and receiving of an open file
descriptor between a privileged and a normal process.

Diff Detail

Repository
R241 KIO
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
chinmoyr created this revision.Jul 15 2017, 10:58 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptJul 15 2017, 10:58 AM

The sequence would be, registering service in ioslave, setting euid of the helper process and sending the file descriptor over user's session bus

I don't fully know this code, but that doesn't sound right.

Your helper is running on the system bus, and shouldn't really have access to the user's session bus at all.
Your client side ioslave will have connections to both busses, but only be talking to the helper on the system bus.

With polkit you currently request something over DBus from a helper and you get a reply back. What you're describing you want is exactly that, except the reply happens a file descriptor. (which as you hint is a native DBus type) Why do you need to register a service in the ioslave?

src/ioslaves/file/sharefd.cpp
107 ↗(On Diff #16732)

If this is on the client side, what stops any other (non authorised) client listening to here?

And

The sequence would be, registering service in ioslave, setting euid of the helper process and sending the file descriptor over user's session bus

I don't fully know this code, but that doesn't sound right.

Your helper is running on the system bus, and shouldn't really have access to the user's session bus at all.
Your client side ioslave will have connections to both busses, but only be talking to the helper on the system bus.

You can set the effective uid of the root process to the users process to connect to user's session bus and then reset it and gain back the privilege.

With polkit you currently request something over DBus from a helper and you get a reply back. What you're describing you want is exactly that, except the reply happens a file descriptor. (which as you hint is a native DBus type) Why do you need to register a service in the ioslave?

QDBusUnixFileDescriptor object is required to send a file descriptor over dbus. The service must have a method accepting QDBusUnixFileDescriptor as an argument, QtDBus won't create it automatically. KAuth doesn't have any such method thats why the need for a separate service.

chinmoyr marked an inline comment as done.Jul 15 2017, 12:42 PM
chinmoyr added inline comments.
src/ioslaves/file/sharefd.cpp
107 ↗(On Diff #16732)

It doesn't matter. The job of client is to send the file descriptor and any other (unauthorised) client connected cannot see this fd.

This class isn't hooked up to anything. It's technically correct as an FD sender and receiver. What I want to see is how you use it, because that's extremely important to get right.

I can confirm to you that anonymous namespace sockets do not work on BSDs (which include macOS).

src/ioslaves/file/sharefd.cpp
112 ↗(On Diff #16732)

If SOCK_NONBLOCK is defined, OR it o SOCK_STREAM and then you don't have call setNonBlocking below.

117 ↗(On Diff #16732)

"binded" is not English. You mean "bound".

src/ioslaves/file/sharefd.h
33 ↗(On Diff #16732)

Use QString, not std::string.

This class isn't hooked up to anything. It's technically correct as an FD sender and receiver. What I want to see is how you use it, because that's extremely important to get right.

I think he's going to upload another patch that uses this code.

chinmoyr updated this revision to Diff 17012.Jul 22 2017, 10:39 AM
chinmoyr marked 5 inline comments as done.

std::string -> QString
binded -> bound
setNonBlock() - > SOCK_NONBLOCK

@thiago Used FdSender in the function senndFileDescriptor in D6197 [filehelper.cpp] and FdReceiver in D6830 [file_unix.cpp]

chinmoyr updated this revision to Diff 18298.Aug 17 2017, 5:29 PM
  • add stopListening method to FdReceiver
dfaure requested changes to this revision.Sep 3 2017, 8:32 AM
dfaure added a subscriber: dfaure.
dfaure added inline comments.
src/ioslaves/file/sharefd.cpp
32 ↗(On Diff #18502)

Either move it to a common header (if QString everywhere is OK), or use a different classname here, to avoid a classname clash at runtime (and document that it was ported to std::string, so we know why it was forked)

67 ↗(On Diff #18502)

Can you make the classname more descriptive? ShareFDMessageHeader maybe?
From the looks of it I initially thought this came from kio and not just from sharefd.*

95 ↗(On Diff #18502)

typo: receiver

147 ↗(On Diff #18502)

this if() serves no purpose (delete nullptr is a well-defined no-op)

159 ↗(On Diff #18502)

If FDSender is only used in the kauthhelper, and FDReceiver is only used in kio_file, why not split up this .cpp into two, and only compile the bit that's needed by each target?
(you'll need a private header for the shared classes of course, i.e. the sockaddr wrapper and the messageheader would go into sharefd_p.h)

It would also make the QString vs std::string API difference look less weird.

And then the files can match the classnames, too (fdsender.cpp/h and fdreceiver.cpp/h)

166 ↗(On Diff #18502)

you need std::endl when using std::cerr (repeats)

This revision now requires changes to proceed.Sep 3 2017, 8:32 AM
chinmoyr updated this revision to Diff 19757.Sep 21 2017, 4:57 PM
chinmoyr marked an inline comment as done.
  • separated FdReceiver and FdSender
  • KMSgHdr -> FDMessageHeader and KSockaddrUn -> SocketAddress and moved them to private header
  • removed stopListening() method. Qt can handle m_readNotifier's deletion

Question : Is it strange that kauth directory appears in this commit? And shall I rename isValid() in FdReceiver to isListening()?

chinmoyr updated this revision to Diff 19758.Sep 21 2017, 5:01 PM

Added license header

dfaure accepted this revision.Sep 24 2017, 8:57 AM

Yes isListening() would make sense.

The kauth subdir for fdsender.* is ok, it matches what you then do in https://phabricator.kde.org/D6197 where you actually use that file (well, you'll have to update that commit to change the filename in CMakeLists.txt)

It doesn't look like Thiago will have time to re-review this any time soon, so I'd say, let's get this in meanwhile.

This revision is now accepted and ready to land.Sep 24 2017, 8:57 AM
chinmoyr updated this revision to Diff 20232.Oct 2 2017, 10:58 AM

Changed isValid() -> isListening() in FdReceiver

dfaure accepted this revision.Oct 7 2017, 4:25 PM

This is all ready to push now, right?

Please do, once the 5.39 RC tags are there (should happen today or tomorrow).

This is all ready to push now, right?

Please do, once the 5.39 RC tags are there (should happen today or tomorrow).

This patch is. I have yet to add the new flag.

This revision was automatically updated to reflect the committed changes.
chinmoyr reopened this revision.Oct 29 2017, 4:10 PM
This revision is now accepted and ready to land.Oct 29 2017, 4:10 PM
chinmoyr updated this revision to Diff 24577.Tue, Jan 2, 11:20 AM
chinmoyr retitled this revision from [RFC] Add support for sharing file descriptor between KIO slave and KAuth helper to Add support for sharing file descriptor between KIO slave and KAuth helper.

Fixed compilation issue on FreeBSD.
Apparently I was passing a pointer to array in strcpy.

chinmoyr updated this revision to Diff 25215.Fri, Jan 12, 10:26 AM
chinmoyr edited the summary of this revision. (Show Details)

Summary update

This revision was automatically updated to reflect the committed changes.