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

[Chinmoy Ranjan Pradhan]

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, lbeltrame, dfaure, michaelh, bruns

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

[Oswald Buddenhagen]

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
  https://phabricator.kde.org/D9966

To: chinmoyr, #frameworks, thiago, dfaure, ossi
Cc: kde-frameworks-devel, ngraham, fvogt, lbeltrame, dfaure, michaelh, bruns

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

[Chinmoy Ranjan Pradhan]

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, bruns

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

[Chinmoy Ranjan Pradhan]

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, bruns

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

[Chinmoy Ranjan Pradhan]

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
Cc: ngraham, fvogt, lbeltrame, dfaure, michaelh, bruns

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

[Oswald Buddenhagen]

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

[Chinmoy Ranjan Pradhan]

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, dfaure, michaelh

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

[Chinmoy Ranjan Pradhan]

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

[Chinmoy Ranjan Pradhan]

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

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

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

[Thiago Macieira]

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 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.

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

[Chinmoy Ranjan Pradhan]

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

[Chinmoy Ranjan Pradhan]

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 result in an 
error.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D9966?vs=26421=26446

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D9966

AFFECTED FILES
  src/ioslaves/file/fdreceiver.cpp
  src/ioslaves/file/file_unix.cpp

To: chinmoyr, #frameworks, thiago, dfaure, ossi
Cc: ngraham, fvogt, lbeltrame, dfaure, michaelh

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

[David Faure]

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, michaelh

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

[Oswald Buddenhagen]

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.
  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 ^^))

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

[David Faure]

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

[Chinmoy Ranjan Pradhan]

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
  https://phabricator.kde.org/D9966?vs=26403=26421

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D9966

AFFECTED FILES
  src/ioslaves/file/fdreceiver.cpp
  src/ioslaves/file/fdreceiver.h
  src/ioslaves/file/file_unix.cpp
  src/ioslaves/file/kauth/fdsender.cpp
  src/ioslaves/file/sharefd_p.h

To: chinmoyr, #frameworks, thiago, dfaure, ossi
Cc: ngraham, fvogt, lbeltrame, dfaure, michaelh

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

[Chinmoy Ranjan Pradhan]

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

[Fabian Vogt]

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 the 
mail are not fixed, this shouldn't get re-enabled.

INLINE COMMENTS

> thiago wrote in file.h:106
> 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.

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

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

[David Faure]

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

INLINE COMMENTS

> thiago wrote in 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.

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

> thiago wrote in fdreceiver.cpp:88
> Where's the support for other OSes?

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

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

[Thiago Macieira]

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 (!addr.address()) {

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

> fdreceiver.cpp:88
> +}
> +#else
> +#error Cannot get socket credentials!

Where's the support for other OSes?

> fdreceiver.h:45
>  int m_fileDes;
> +QString m_path;
>  };

Nitpick: always sort your members by size.

> file.h:106
>  bool privilegeOperationUnitTestMode();
> -PrivilegeOperationReturnValue execWithElevatedPrivilege(ActionType 
> action, const QVariant ,
> -const QVariant 
>  = QVariant(),
> -const QVariant 
>  = QVariant());
> -PrivilegeOperationReturnValue tryOpen(QFile , const QByteArray , 
> int flags, int mode);
> +PrivilegeOperationReturnValue execWithElevatedPrivilege(ActionType 
> action, const QVariantList , int errcode);
> +PrivilegeOperationReturnValue tryOpen(QFile , const QByteArray , 
> int flags, int mode, int errcode);

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.

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

[Chinmoy Ranjan Pradhan]

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 DETAIL
  https://phabricator.kde.org/D9966

AFFECTED FILES
  src/ioslaves/file/fdreceiver.cpp
  src/ioslaves/file/fdreceiver.h
  src/ioslaves/file/file.cpp
  src/ioslaves/file/file.h
  src/ioslaves/file/file_p.h
  src/ioslaves/file/file_unix.cpp
  src/ioslaves/file/file_win.cpp
  src/ioslaves/file/kauth/fdsender.cpp
  src/ioslaves/file/sharefd_p.h

To: chinmoyr, #frameworks, thiago, dfaure, ossi
Cc: ngraham, fvogt, lbeltrame, dfaure, michaelh

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

[David Faure]

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 
> failed, so a simple null-pointer check would be enough here. Plus I remember 
> some implementations of strlen breaking on null pointers...

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.

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

[Chinmoy Ranjan Pradhan]

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
  https://phabricator.kde.org/D9966?vs=26045=26220

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D9966

AFFECTED FILES
  src/ioslaves/file/fdreceiver.cpp
  src/ioslaves/file/fdreceiver.h
  src/ioslaves/file/file.cpp
  src/ioslaves/file/file.h
  src/ioslaves/file/file_p.h
  src/ioslaves/file/file_unix.cpp
  src/ioslaves/file/file_win.cpp
  src/ioslaves/file/kauth/fdsender.cpp
  src/ioslaves/file/sharefd_p.h

To: chinmoyr, #frameworks, thiago, dfaure
Cc: ngraham, fvogt, lbeltrame, dfaure, michaelh

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

[Chinmoy Ranjan Pradhan]

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

[Chinmoy Ranjan Pradhan]

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

[David Faure]

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
>  }
> +::unlink(m_path.toStdString().c_str());
>  }

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

> sharefd_p.h:51
>  {
> -return reinterpret_cast();
> +return (strlen(addr.sun_path) > 0) ? reinterpret_cast sockaddr*>() : nullptr;
>  }

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...

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D9966

To: chinmoyr, #frameworks, thiago, dfaure
Cc: ngraham, fvogt, lbeltrame, dfaure, michaelh

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

[Fabian Vogt]

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

[Chinmoy Ranjan Pradhan]

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 FILES
  src/ioslaves/file/fdreceiver.cpp
  src/ioslaves/file/fdreceiver.h
  src/ioslaves/file/file.cpp
  src/ioslaves/file/file.h
  src/ioslaves/file/file_p.h
  src/ioslaves/file/file_unix.cpp
  src/ioslaves/file/file_win.cpp
  src/ioslaves/file/kauth/fdsender.cpp
  src/ioslaves/file/sharefd_p.h

To: chinmoyr, #frameworks, thiago, fvogt
Cc: ngraham, fvogt, lbeltrame, dfaure, michaelh

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

[Chinmoy Ranjan Pradhan]

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
  https://phabricator.kde.org/D9966?vs=26028=26044

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D9966

AFFECTED FILES
  src/ioslaves/file/fdreceiver.cpp
  src/ioslaves/file/fdreceiver.h
  src/ioslaves/file/file.cpp
  src/ioslaves/file/file.h
  src/ioslaves/file/file_p.h
  src/ioslaves/file/file_unix.cpp
  src/ioslaves/file/file_win.cpp
  src/ioslaves/file/kauth/fdsender.cpp
  src/ioslaves/file/sharefd_p.h

To: chinmoyr, #frameworks, thiago, fvogt
Cc: ngraham, fvogt, lbeltrame, dfaure, michaelh

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

[Fabian Vogt]

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

INLINE COMMENTS

> file.cpp:239
>  if (!err.wasCanceled()) {
>  switch (errno) {
>  case EPERM:

`errno` might not be the expected value anymore.

> chinmoyr wrote in sharefd_p.h:66
> I think this is not needed. It's already there in FdReceiver.

Fair enough.

> chinmoyr wrote in sharefd_p.h:47
> @fvogt Is +1 for null byte required here?

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

  offsetof(struct sockaddr_un, sun_path)+strlen(addr.sun_path)+1```.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D9966

To: chinmoyr, #frameworks, thiago, fvogt
Cc: ngraham, fvogt, lbeltrame, dfaure, michaelh

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

[Chinmoy Ranjan Pradhan]

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 this is not needed. It's already there in FdReceiver.

> sharefd_p.h:47
>  {
> -return sizeof addr;
> +return offsetof(struct sockaddr_un, sun_path) + 
> strlen(addr.sun_path);
>  }

@fvogt Is +1 for null byte required here?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D9966

To: chinmoyr, #frameworks, thiago, fvogt
Cc: ngraham, fvogt, lbeltrame, dfaure, michaelh

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

[Chinmoy Ranjan Pradhan]

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

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D9966?vs=25941=26028

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D9966

AFFECTED FILES
  src/ioslaves/file/fdreceiver.cpp
  src/ioslaves/file/fdreceiver.h
  src/ioslaves/file/file.cpp
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp
  src/ioslaves/file/file_win.cpp
  src/ioslaves/file/kauth/fdsender.cpp
  src/ioslaves/file/sharefd_p.h

To: chinmoyr, #frameworks, thiago, fvogt
Cc: ngraham, fvogt, lbeltrame, dfaure, michaelh

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

[Fabian Vogt]

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 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.

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

> chinmoyr wrote in sharefd_p.h:66
> 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?

If that is the case you can do something like:

  #if !defined(__linux__) && !defined(__FreeBSD__)
  #error No secure implementation available
  #endif

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D9966

To: chinmoyr, #frameworks, thiago, fvogt
Cc: ngraham, fvogt, lbeltrame, dfaure

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

[Chinmoy Ranjan Pradhan]

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 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.

> fvogt wrote in sharefd_p.h:66
> Look at `man 7 unix`, section `Pathname socket ownership and permissions`.

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?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D9966

To: chinmoyr, #frameworks, thiago, fvogt
Cc: ngraham, fvogt, lbeltrame, dfaure

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

[Chinmoy Ranjan Pradhan]

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
  https://phabricator.kde.org/D9966

AFFECTED FILES
  src/ioslaves/file/fdreceiver.cpp
  src/ioslaves/file/fdreceiver.h
  src/ioslaves/file/file_unix.cpp
  src/ioslaves/file/kauth/fdsender.cpp
  src/ioslaves/file/sharefd_p.h

To: chinmoyr, #frameworks, thiago, fvogt
Cc: ngraham, fvogt, lbeltrame, dfaure

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

[Fabian Vogt]

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 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?

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.

> chinmoyr wrote in sharefd_p.h:66
> I didn't follow you here. Can you explain why working of this code on other 
> OSs, specifically FreeBsd and OSX, will be insecure?

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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D9966

To: chinmoyr, #frameworks, thiago, fvogt
Cc: ngraham, fvogt, lbeltrame, dfaure

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

[Chinmoy Ranjan Pradhan]

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 only one FdReceiver is created.

> fvogt wrote in file_unix.cpp:91
> 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.

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?

> fvogt wrote in sharefd_p.h:66
> 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.

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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D9966

To: chinmoyr, #frameworks, thiago, fvogt
Cc: ngraham, fvogt, lbeltrame, dfaure

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

[Chinmoy Ranjan Pradhan]

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

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D9966

AFFECTED FILES
  src/ioslaves/file/fdreceiver.cpp
  src/ioslaves/file/fdreceiver.h
  src/ioslaves/file/file_unix.cpp
  src/ioslaves/file/kauth/fdsender.cpp
  src/ioslaves/file/sharefd_p.h

To: chinmoyr, #frameworks, thiago, fvogt
Cc: ngraham, fvogt, lbeltrame, dfaure

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

[Fabian Vogt]

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 = -1;

or just move it above the `m_socketDes` assignment.

> fdreceiver.cpp:60
>  }
> +::unlink(m_path.toStdString().c_str());
>  }

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.

> file_unix.cpp:91
> +// value and reset it after deleting socket file.
> +int err = errno;
> +QFile::remove(sockPath);

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.

> sharefd_p.h:53
>  }
> +bool error() const
> +{

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.

> sharefd_p.h:66
> +::strncpy(a.sun_path, path.c_str(), sizeof(a.sun_path)-1);
> +}
>  return a;

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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D9966

To: chinmoyr, #frameworks, thiago, fvogt
Cc: ngraham, fvogt, lbeltrame, dfaure

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

[Chinmoy Ranjan Pradhan]

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
  https://phabricator.kde.org/D9966?vs=25602=25839

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D9966

AFFECTED FILES
  src/ioslaves/file/fdreceiver.cpp
  src/ioslaves/file/fdreceiver.h
  src/ioslaves/file/file_unix.cpp
  src/ioslaves/file/kauth/fdsender.cpp
  src/ioslaves/file/sharefd_p.h

To: chinmoyr, #frameworks, thiago
Cc: ngraham, fvogt, lbeltrame, dfaure

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

[David Faure]

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, #frameworks, thiago
Cc: ngraham, fvogt, lbeltrame, dfaure

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

[Fabian Vogt]

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` is attacker-controlled, which needs to be fixed.
  
  > (BTW, SocketAddress::length should return the actual length of the buffer,  
currently it adds ~100 '\0' bytes to the end)
  
  Is not fixed, is this intentional?
  
  > 2.Adds checks for socket credentials. Now a file descriptor will be 
received only if it was sent by a root owned process.
  
  Looks sensible, but it doesn't fix the other direction, which is:
  
  1. User asks the kauth helper to open a file as root
  2. The kauth helper receives the socket address
  3. file.so dies (reason does not matter)
  4. Any process can now create a socket with the address the kauth helper 
connects to and receive the fd
  
  IMO the correct fix (which only applies to linux, according to the manpage) 
is to use a pathname socket in `$XDG_RUNTIME_DIR` (or alternatively, somewhere 
returned by `mkdtemp`).

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D9966

To: chinmoyr, #frameworks, thiago
Cc: ngraham, fvogt, lbeltrame, dfaure

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

[Chinmoy Ranjan Pradhan]

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 descriptor will be received 
only if it was sent by a root owned process.
  
  These issues were mentioned in here : 
https://mail.kde.org/pipermail/kde-frameworks-devel/2018-January/055307.html

REPOSITORY
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D9966

AFFECTED FILES
  src/ioslaves/file/fdreceiver.cpp
  src/ioslaves/file/sharefd_p.h

To: chinmoyr, #frameworks, thiago
Cc: fvogt, lbeltrame, dfaure