Review Request 126524: Fixed custom wallpaper being not respected by sddm.

[Joshua Noack]

> On Dec. 27, 2015, 4:37 p.m., David Edmundson wrote:
> > sddmauthhelper.cpp, line 74
> > <https://git.reviewboard.kde.org/r/126524/diff/1/?file=426085#file426085line74>
> >
> >     Ideally we should check the *calling* user can read this file, as you technically have a security bug.
> >     
> >     Otherwise I could just select /etc/shadow as my background and suddenly it's available world readable.
> >     
> >     A distro/admin could theoretically set polkit up to allow any users to change the SDDM wallpaper. though TBH it'd never happen.
> >     
> >     Polkit-Qt has that information available. KAuth does not seem to publicly.
> 
> Joshua Noack wrote:
>     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?
> 
> David Edmundson wrote:
>     it needs to happen inside the helper, not on the client side.
>     
>     From a terminal I can manually start the sddm helper saying I want to set /etc/shadow as the background.
>     
>     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.
>     
>     That's a security hole as it would potentially allow me to read any file.
> 
> Joshua Noack wrote:
>     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?
>     
>     If you could tell me where the sysadmin sets his policies I would be grateful. :p
> 
> David Edmundson wrote:
>     We already have a policy.
>     
>     See org.kde.kcontrol.kcmsddm.policy
>     
>     our defaults are:
>           <defaults>
>              <allow_inactive>no</allow_inactive>
>              <allow_active>auth_admin_keep</allow_active>
>           </defaults>
>     
>     
>     which is good.
>     
>     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>
>              
>     and then we have a problem.
> 
> David Edmundson wrote:
>     https://git.reviewboard.kde.org/r/126724/ adds the API we need.
> 
> Joshua Noack wrote:
>     Cool. When will the API changes be released? I don't have much interest in fiddling with self-compiled development libraries.
> 
> David Edmundson wrote:
>     Start of Feb.
>     
>     If you want, I can continue this patch and add the extra bit of code?

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


- Joshua


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126524/#review90132
-----------------------------------------------------------


On Dec. 28, 2015, 3:44 p.m., Joshua Noack wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126524/
> -----------------------------------------------------------
> 
> (Updated Dec. 28, 2015, 3:44 p.m.)
> 
> 
> Review request for Plasma and David Edmundson.
> 
> 
> Repository: sddm-kcm
> 
> 
> Description
> -------
> 
> For some reason sddm cannot handle absolute file paths to wallpapers
> and also needs the wallpaper to be readable by others.
> 
> This is fixed by copying the wallpaper to the root directory of the
> selected theme.
> 
> 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.
> 
> 
> Diffs
> -----
> 
>   sddmauthhelper.cpp 648b24c77e7570641d454fca9d121709a622bc36 
>   src/themeconfig.cpp bdd6dd29fd8eb052e2f2b2239b0c46ebbebec88c 
> 
> Diff: https://git.reviewboard.kde.org/r/126524/diff/
> 
> 
> Testing
> -------
> 
> Copies and removes backgrounds as intended.
> The wallpaper is shown in sddm.
> 
> 
> Thanks,
> 
> Joshua Noack
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20160113/02f9ab60/attachment-0001.html>