<table><tr><td style="">mgerstner added a comment.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D12513">View Revision</a></tr></table><br /><div><div><blockquote style="border-left: 3px solid #8C98B8;
          color: #6B748C;
          font-style: italic;
          margin: 4px 0 12px 0;
          padding: 8px 12px;
          background-color: #F8F9FC;">
<div style="font-style: normal;
          padding-bottom: 4px;">In <a href="https://phabricator.kde.org/D12513#256845" style="background-color: #e7e7e7;
          border-color: #e7e7e7;
          border-radius: 3px;
          padding: 0 4px;
          font-weight: bold;
          color: black;text-decoration: none;">D12513#256845</a>, <a href="https://phabricator.kde.org/p/aacid/" style="
              border-color: #f1f7ff;
              color: #19558d;
              background-color: #f1f7ff;
                border: 1px solid transparent;
                border-radius: 3px;
                font-weight: bold;
                padding: 0 4px;">@aacid</a> wrote:</div>
<div style="margin: 0;
          padding: 0;
          border: 0;
          color: rgb(107, 116, 140);"><p><a href="https://phabricator.kde.org/p/mgerstner/" style="
              border-color: #f1f7ff;
              color: #19558d;
              background-color: #f1f7ff;
                border: 1px solid transparent;
                border-radius: 3px;
                font-weight: bold;
                padding: 0 4px;">@mgerstner</a> I don't really understand why we need the chdir, renameat, etc.</p>

<p>Dropping privileges to the minimum needed should be enough, shouldn't it?</p>

<p>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?</p></div>
</blockquote>

<p>Strictly speaking the <tt style="background: #ebebeb; font-size: 13px;">renameat()</tt> 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 <tt style="background: #ebebeb; font-size: 13px;">renameat()</tt>. Once the target directory has been entered all directory traversal questions are out of the way.</p>

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

<p>While this discussion here focuses on the CVE at hand, we have a broader discussion about the <tt style="background: #ebebeb; font-size: 13px;">savefile()</tt> feature in an <a href="https://bugzilla.suse.com/show_bug.cgi?id=1033055#c13" class="remarkup-link" target="_blank" rel="noreferrer">openSUSE bug review bug</a>. I think the overall implementation can use some extra security checks and usability improvements.</p></div></div><br /><div><strong>REPOSITORY</strong><div><div>R39 KTextEditor</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D12513">https://phabricator.kde.org/D12513</a></div></div><br /><div><strong>To: </strong>cullmann, dfaure<br /><strong>Cc: </strong>mgerstner, aacid, ngraham, fvogt, cullmann, Frameworks, michaelh, kevinapavew, bruns, demsking, sars, dhaumann<br /></div>