<table><tr><td style="">maltek requested changes to this revision.<br />maltek added a comment.<br />This revision now requires changes to proceed.
</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/D14467">View Revision</a></tr></table><br /><div><div><p>I've gone over the code and found some issues. I haven't fully thought through the design on a conceptual level, because I assume Matthias already did.</p></div></div><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D14467#inline-123201">View Inline</a><span style="color: #4b4d51; font-weight: bold;">filehelper.cpp:74</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="p">}</span> <span style="color: #aa4000">else</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">target_fd</span> <span style="color: #aa2211">=</span> <span class="n">openat</span><span class="p">(</span><span class="n">parent_fd</span><span class="p">,</span> <span class="n">baseName</span><span class="p">.</span><span class="n">data</span><span class="p">(),</span> <span class="n">O_NOFOLLOW</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="p">}</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">This is still subject to race conditions. You're opening the file with O_NOFOLLOW and calling fstat on it, correctly. But then, you don't return this file descriptor to further work on it. So there's no way to know if the file checked here and the file that's changed later on are the same.</p>
<p style="padding: 0; margin: 8px;">(It's also leaking the opened file descriptor, currently.)</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D14467#inline-123202">View Inline</a><span style="color: #4b4d51; font-weight: bold;">filehelper.cpp:105</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">if</span> <span class="p">(</span><span class="n">setegid</span><span class="p">(</span><span class="n">newgid</span><span class="p">)</span> <span style="color: #aa2211">==</span> <span style="color: #aa2211">-</span><span style="color: #601200">1</span> <span style="color: #aa2211">||</span> <span class="n">seteuid</span><span class="p">(</span><span class="n">newuid</span><span class="p">)</span> <span style="color: #aa2211">==</span> <span style="color: #aa2211">-</span><span style="color: #601200">1</span><span class="p">)</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">return</span> <span style="color: #304a96">false</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="p">}</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">After this <tt style="background: #ebebeb; font-size: 13px;">return false</tt>, the process is in some undefined state with unknown groups, since there is no attempt to restore the original groups. The chance of these calls failing are quite slim, however - it can only really happen due to programming error when the program doesn't have privileges to call sete[ug]id. Personally, I'd just abort the program in such circumstances, but since it should never be happening, reasonable people might disagree.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D14467#inline-123209">View Inline</a><span style="color: #4b4d51; font-weight: bold;">filehelper.cpp:144</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; "> <span style="color: #aa4000">int</span> <span class="n">mode</span> <span style="color: #aa2211">=</span> <span class="n">arg2</span><span class="p">.</span><span class="n">toInt</span><span class="p">();</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span style="color: #aa4000">if</span> <span class="p">(</span><span class="n">chmod<span class="bright"></span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span class="n"><span class="bright">path</span></span><span class="p">.</span><span class="n">data</span><span class="p">(),</span> <span class="n">mode</span><span class="p">)</span> <span style="color: #aa2211">==</span> <span class="bright"></span><span style="color: #601200"><span class="bright">0</span></span><span class="p">)</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="bright"></span><span style="color: #aa4000"><span class="bright">return</span></span> <span class="n">reply</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="bright"> </span><span style="color: #aa4000">if</span> <span class="p">(<span class="bright"></span></span><span class="bright"></span><span class="n"><span class="bright">f</span>chmod<span class="bright">at</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span class="n"><span class="bright">parent_fd</span></span><span class="bright"></span><span class="p"><span class="bright">,</span></span><span class="bright"> </span><span class="n"><span class="bright">baseName</span></span><span class="p">.</span><span class="n">data</span><span class="p">(),</span> <span class="n">mode<span class="bright"></span></span><span class="bright"></span><span class="p"><span class="bright">,</span></span><span class="bright"> </span><span class="n"><span class="bright">AT_SYMLINK_NOFOLLOW</span></span><span class="p">)</span> <span style="color: #aa2211">==</span> <span class="bright"></span><span style="color: #aa2211"><span class="bright">-</span></span><span class="bright"></span><span style="color: #601200"><span class="bright">1</span></span><span class="p">)</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="bright"> </span> <span class="n">reply<span class="bright"></span></span><span class="bright"></span><span class="p"><span class="bright">.</span></span><span class="bright"></span><span class="n"><span class="bright">setError</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span class="n"><span class="bright">errno</span></span><span class="bright"></span><span class="p"><span class="bright">)</span>;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">AT_SYMLINK_NOFOLLOW is not implemented for fchmodat, so this will always fail with ENOTSUP. It seems the only safe way to do this is to open() the file with O_NOFOLLOW, and then use fchmod().</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D14467#inline-123204">View Inline</a><span style="color: #4b4d51; font-weight: bold;">filehelper.cpp:222</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">times</span><span class="p">[</span><span style="color: #601200">1</span><span class="p">].</span><span class="n">tv_sec</span> <span style="color: #aa2211">=</span> <span class="n">modtime</span> <span style="color: #aa2211">*</span> <span style="color: #601200">1000</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">times</span><span class="p">[</span><span style="color: #601200">1</span><span class="p">].</span><span class="n">tv_nsec</span> <span style="color: #aa2211">=</span> <span class="n">modtime</span> <span style="color: #aa2211">/</span> <span style="color: #601200">1000</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">if</span> <span class="p">(</span><span class="n">utimensat</span><span class="p">(</span><span class="n">parent_fd</span><span class="p">,</span> <span class="n">baseName</span><span class="p">.</span><span class="n">data</span><span class="p">(),</span> <span class="n">times</span><span class="p">,</span> <span class="n">AT_SYMLINK_NOFOLLOW</span><span class="p">)</span> <span style="color: #aa2211">==</span> <span style="color: #aa2211">-</span><span style="color: #601200">1</span><span class="p">)</span> <span class="p">{</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I *think* the divisions/multiplications are reversed here.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D14467#inline-123210">View Inline</a><span style="color: #4b4d51; font-weight: bold;">filehelper.cpp:231</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="p">}</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">close</span><span class="p">(</span><span class="n">parent_fd</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Somewhere here, I'd expect the privileges to be escalated again, to restore the state from before the call to dropPrivileges().</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R241 KIO</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D14467">https://phabricator.kde.org/D14467</a></div></div><br /><div><strong>To: </strong>chinmoyr, dfaure, ngraham, elvisangelaccio, Frameworks, Dolphin, maltek<br /><strong>Cc: </strong>maltek, mreeves, mgerstner, fvogt, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns<br /></div>