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
