<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/126524/">https://git.reviewboard.kde.org/r/126524/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On December 27th, 2015, 4:37 p.m. UTC, <b>David Edmundson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/126524/diff/1/?file=426085#file426085line74" style="color: black; font-weight: bold; text-decoration: underline;">sddmauthhelper.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">ActionReply SddmAuthHelper::save(const QVariantMap &args)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">74</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QFileInfo</span> <span class="n">newBackgroundFileInfo</span><span class="p">(</span><span class="n">iterator</span><span class="p">.</span><span class="n">value</span><span class="p">().</span><span class="n">toString</span><span class="p">());</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Ideally we should check the <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">calling</em> user can read this file, as you technically have a security bug.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Otherwise I could just select /etc/shadow as my background and suddenly it's available world readable.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">A distro/admin could theoretically set polkit up to allow any users to change the SDDM wallpaper. though TBH it'd never happen.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Polkit-Qt has that information available. KAuth does not seem to publicly.</p></pre>
</blockquote>
<p>On December 28th, 2015, 4:18 p.m. UTC, <b>Joshua Noack</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Not sure if I understand you correctly...
In addition to the KAuth dialog where the user needs to authenticate, a check if the user can read the file should be added?
Shouldn't the file chooser restrict the user here in the first place?</p></pre>
</blockquote>
<p>On December 28th, 2015, 4:48 p.m. UTC, <b>David Edmundson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">it needs to happen inside the helper, not on the client side.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">From a terminal I can manually start the sddm helper saying I want to set /etc/shadow as the background.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The helper will then check if the user is permitted to set the SDDM wallpaper in policykit, which is up to the sysadmin's policy
If that returns true, the helper will procede to copy the file as root.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">That's a security hole as it would potentially allow me to read any file.</p></pre>
</blockquote>
<p>On December 28th, 2015, 5:48 p.m. UTC, <b>Joshua Noack</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Okay, I looked up policykit and don't see any rule in /usr/share/polkit-1/ for it.
So... should I create a new rule (like org.kde.sddm.policy) ? But judging from your words, it seems to be already possible?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If you could tell me where the sysadmin sets his policies I would be grateful. :p</p></pre>
</blockquote>
<p>On December 28th, 2015, 6:49 p.m. UTC, <b>David Edmundson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">We already have a policy.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">See org.kde.kcontrol.kcmsddm.policy</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">our defaults are:
<defaults>
<allow_inactive>no</allow_inactive>
<allow_active>auth_admin_keep</allow_active>
</defaults></p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">which is good.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">However, we can't predict that every setup and every distro will have the same setup.
They could change it to be
<allow_active>yes</allow_active></p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">and then we have a problem.</p></pre>
</blockquote>
<p>On January 12th, 2016, 2:47 p.m. UTC, <b>David Edmundson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">https://git.reviewboard.kde.org/r/126724/ adds the API we need.</p></pre>
</blockquote>
<p>On January 13th, 2016, 8:21 a.m. UTC, <b>Joshua Noack</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Cool. When will the API changes be released? I don't have much interest in fiddling with self-compiled development libraries.</p></pre>
</blockquote>
<p>On January 13th, 2016, 3:15 p.m. UTC, <b>David Edmundson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Start of Feb.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If you want, I can continue this patch and add the extra bit of code?</p></pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I wouldn't mind, but there is at least one thing that should be fixed / improved. Currently the wallpaper is copied 1:1 to the sddm theme directory. But what happens when a file with the same name already exists? breeze.jpg is the first that comes to my mind. Selecting a user wallpaper named "breeze.jpg" would overwrite it, right? Maybe the name should be mangled first to ensure it's unique or copied to a separate directory...</p></pre>
<br />
<p>- Joshua</p>
<br />
<p>On December 28th, 2015, 3:44 p.m. UTC, Joshua Noack wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for Plasma and David Edmundson.</div>
<div>By Joshua Noack.</div>
<p style="color: grey;"><i>Updated Dec. 28, 2015, 3:44 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
sddm-kcm
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">For some reason sddm cannot handle absolute file paths to wallpapers
and also needs the wallpaper to be readable by others.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This is fixed by copying the wallpaper to the root directory of the
selected theme.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">On save the sddmauthhelper copies the background from the absolute path
into the theme directory and sets the "background" key of the
theme.user.conf to the copied file. If previously a different background was
set it is removed beforehand.</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Copies and removes backgrounds as intended.
The wallpaper is shown in sddm.</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>sddmauthhelper.cpp <span style="color: grey">(648b24c77e7570641d454fca9d121709a622bc36)</span></li>
<li>src/themeconfig.cpp <span style="color: grey">(bdd6dd29fd8eb052e2f2b2239b0c46ebbebec88c)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/126524/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>