mgerstner added a comment.
In D12513#256845 <https://phabricator.kde.org/D12513#256845>, @aacid wrote: > @mgerstner I don't really understand why we need the chdir, renameat, etc. > > Dropping privileges to the minimum needed should be enough, shouldn't it? > > I mean at that point the only thing that can happen is that some user breaks files he can write to anyway, so why should we take extra precautions from that point on? Strictly speaking the `renameat()` in the target CWD is not necessary when the privdrop is in place. But the approach is mostly implemented already at the moment and it works reasonably well (given the temporary file is not unnecessarily reopened like it was the case). You have to decide how and where to create the temporary file and this is one valid approach that has the charme of supporting the atomic `renameat()`. Once the target directory has been entered all directory traversal questions are out of the way. If you choose a different approach then you will have to open the target file explicitly, which raises other questions like how to safely replace symlinks. Of course such an approach can also be implemented safely. In any case a prudent handling of the temporary file handling improves trust in and robustness of the code and provides additional barriers should errors slip in in the future by way of refactoring or extending the code. While this discussion here focuses on the CVE at hand, we have a broader discussion about the `savefile()` feature in an openSUSE bug review bug <https://bugzilla.suse.com/show_bug.cgi?id=1033055#c13>. I think the overall implementation can use some extra security checks and usability improvements. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D12513 To: cullmann, dfaure Cc: mgerstner, aacid, ngraham, fvogt, cullmann, #frameworks, michaelh, kevinapavew, bruns, demsking, sars, dhaumann
