D5394: KAuth integration in document saving - vol. 2

[Fabian Vogt]

fvogt requested changes to this revision.
fvogt added a comment.
This revision now requires changes to proceed.


  Thanks!
  
  So far I only found two issues (see comments).
  Apart from that it would be great to see the application name and the target/source file in the polkit dialog, but I assume this is out of scope here.

INLINE COMMENTS

> katesecuretextbuffer.cpp:88
> +        // ensure file has the same owner and group as before
> +        setOwner(tempFile.fileName(), ownerId, groupId);
>      }

This is racy: If the newly set permissions allow someone to delete the file, it can be replaced with a symlink and the chown will take effect on the symlink target, which can be literally anything -> escalation.

This is not an issue for the rename call as if the file permissions allow deleting, they allow deleting for the destination file as well -> no escalation.

Solution: Use fchown.

> katesecuretextbuffer.cpp:92
> +    // rename temporary file to the target file
> +    return moveFile(tempFile.fileName(), targetFile);
>  }

The destructor of QTemporaryFile here tries to unlink the temporary file here, which fails if the rename was successful.

REPOSITORY
  R39 KTextEditor

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

To: martinkostolny, #ktexteditor, fvogt
Cc: elvisangelaccio, aacid, ivan, lbeltrame, fvogt, apol, anthonyfieroni, cullmann, ltoscano, dhaumann, graesslin, davidedmundson, palant, kwrite-devel, dfaure, #frameworks, head7, kfunk, sars
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20170416/2be36035/attachment.html>