Review Request 120577: Remove shutdown option from lockscreen's look and feel

Wed Oct 15 08:57:24 UTC 2014

> On Oct. 14, 2014, 9:02 a.m., Martin Klapetek wrote:
> > Could we possibly get "Suspend" button instead? Suspend stores the session state into memory and then restores it without any issues (and we lock screen on wakeup anyway); many times I have found my laptop with locked screen and I had to unlock, click the suspend button in the panel only to get it locked again and suspended.
> 
> Martin Gräßlin wrote:
>     I expected this question to come :-) - I'm not sure whether it's a valid use case, at least for laptops. Closing the lid should send it to suspend, the suspend button should still work (if not, we should fix it). So there are already quite some decent ways to get the system to suspend when locked. Especially with lid closing it does what it's supposed to do.
>     
>     In fact the functionality should even be available through the Change Session functionality as that drops you to the login manager where you can suspend. Sure not as comfortable as directly exposing it. But we don't need to expose every option which might make sense ;-)
> 
> Kai Uwe Broulik wrote:
>     Although closing the lid apparently reliably suspends the device, even when locked, my gut makes me not trust it. :/
> 
> Martin Gräßlin wrote:
>     > Although closing the lid apparently reliably suspends the device, even when locked, my gut makes me not trust it. :/
>     
>     At least my notebook(s) have a visible LED indicating it is in sleep mode.
> 
> Martin Klapetek wrote:
>     There are still usecases however where either the user does not want/need to close the lid and/or sets his laptop to not suspend on the lid close (personally I have that disabled when on charger). Plus there is the whole non-laptop world. It's not about exposing every option that makes sense, it's about replacing one senseless with one that makes more sense ;)
> 
> Martin Gräßlin wrote:
>     > It's not about exposing every option that makes sense, it's about replacing one senseless with one that makes more sense ;)
>     
>     I don't see it that way. The option got added in a strange way shortly before the 5.0 release with the maintainers of the screenlocker not having a chance to review it (let's say it simple: if I would have reviewed the code, it would not have gone in, in the first place). So I'm comparing to the last state where it was working which is 4.11 and there the screen locker did not expose such an option and I want to go back to that working state. Let's look from there whether it's a valid option to expose the suspend option and not from the broken state.
>     
>     To me it's not a valid use case to expose suspend in the lock screen. To a certain degree it circumvents security. Suspend/Hibernate is allowed on a per-user logged in base. That's implemented by checking whether suspend/hibernate is supported at runtime. By exposing it in the screenlocker it circumvents the check. A not logged in user (or a user who is not allowed to supsend in his session) is able to suspend (multi user system just needs to switch to a logged session which allows suspend). Thus I think from a security perspective the option may not be there in the first place.
> 
> Martin Klapetek wrote:
>     Oh come on, that's hardly any security circumvention. By that logic we should not show battery state, current user's date/time, user name and user picture in the lock screen either because it's exposing things which other users might have restricted access to.
>     
>     Plus you said in the very first comment that it's possible to go around this to the login screen and from there anyone can suspend...
> 
> Martin Gräßlin wrote:
>     > Plus you said in the very first comment that it's possible to go around this to the login screen and from there anyone can suspend...
>     
>     yes and no. In a multi-user system you can of course configure the system in a way to not allow suspend/hibernate from login manager.
>     
>     It all boils down to: why do we go all the hassle to check whether this options should be enabled from a security point of view, when we allow it for not logged-in users. If we go for allow for not logged in users, we can remove quite a decent amount of code...
> 
> Kai Uwe Broulik wrote:
>     Having a suspend button is a security issue but closing the lid, which causes it to suspend, is not?
> 
> Martin Gräßlin wrote:
>     > Having a suspend button is a security issue but closing the lid, which causes it to suspend, is not?
>     
>     no, as that is controlled by logind. If the system is configured to not allow it, logind won't suspend the system if the lid closes.
> 
> Kai Uwe Broulik wrote:
>     So, the button should also honor that policy?
> 
> Martin Klapetek wrote:
>     > So, the button should also honor that policy?
>     
>     The thing is (I think) that having the button exposed for user A would allow user B to suspend the session while user B has it restricted. But I think that at that very point closing the lid would simply work because it's user A that's logged in and it's his active sesison. No?
>     
>     (I still don't see any security issue in user B suspending the machine from user's A session though, there will be no data leak nor will user B gain anything by doing that)
> 
> Martin Gräßlin wrote:
>     > So, the button should also honor that policy?
>     
>     yes, but our API doesn't allow that. Our API checks for "is current logged in user allowed to suspend", while we need "is not logged in user allowed to suspend". So supporting that would need quite a change in our implementation, it's not as simple as just swapping the buttons.
> 
> Martin Gräßlin wrote:
>     to be precise the documentation from logind:
>     
>     > CanPowerOff(), CanReboot(), CanSuspend(), CanHibernate(), CanHybridSleep() tests whether the system supports the respective operation and whether the calling user is eligible for the desired operation. Returns one of "na", "yes", "no" or "challenge". If "na" is returned the operation is not available because hardware, kernel or drivers do not support it. If "yes" is returned the operation is supported and the user may execute the operation without further authentication. If "no" is returned the operation is available but the user is not allowed to execute the operation. If "challenge" is returned the operation is available, but only after authorization.
>     
>     This means to me that we cannot test for not logged in user.
>     
>     Now concerning Martin's lid argument: that's then a security vulnerability. Let's please not get to the point where we allow adding further because there is a not fixed one.
> 
> Martin Klapetek wrote:
>     Why exactly is that a security vulnerability though? User B would not breach into user A's session. It's an annoyance at most.
> 
> Martin Gräßlin wrote:
>     > Why exactly is that a security vulnerability though? User B would not breach into user A's session. It's an annoyance at most.
>     
>     really? Do I have to explain that?
>     
>     Ok scenario: lab with multiple systems. Each employee in the lab is allowed to login in on each system but has one main system for her work. Only the employee who owns the system is allowed to suspend it, for the other employees it's just meant to be able to quickly check something in the Internet etc. Now employee A uses her system to do a week long calculation for research while being on vacations, employee B wants to sabotize the work by interrupting the calculation (let's assume the lab is setup in a way that you cannot just unplug the system).
> 
> Martin Klapetek wrote:
>     Yes, that's a very valid scenairo, I would just not consider it a "security vulnerability" as the system security as such is not compromised in this case.
> 
> Martin Gräßlin wrote:
>     that's then a definition problem ;-) In my book it counts as "unauthorized access" which is "privelege escalation" and such a "security vulnerability".
> 
> Lukáš Tinkl wrote:
>     Just for the record, we can distinguish quite safely between "can the system suspend" and "is the user allowed to suspend". CanSuspend() (from login1 iface) returns "yes", "no" or "challenge". If "challenge" is returned the operation is available, but only after authorization. See http://www.freedesktop.org/wiki/Software/systemd/logind/
> 
> Martin Gräßlin wrote:
>     yeah, that's the documentation I quoted ;-) Highlight: "and whether the calling user is eligible for the desired operation." Maybe I missed an important part, but to me it looks like that's not possible to distinguish.
> 
> Martin Klapetek wrote:
>     (One thing I forgot to add to the lab scenairo - the ongoing work would be just "suspended" rather than "interrupted" and would happily continue upon resuming from suspend)
> 
> Martin Gräßlin wrote:
>     which means a week lost - so let's add another constraint to the scenario: paper submission shortly afterwards.
> 
> Martin Klapetek wrote:
>     Easy, such critical calculations are being done on a dedicated server with no plasma5 lockscreens :P
> 
> Martin Gräßlin wrote:
>     I have worked in a lab being Plasma powered and the researchers used all available computing power including their own systems, yes.
>     
>     If you want to refuse my arguments, that's fine, but please don't ridiculate them. Security is not a laughable matter.

Well you're just going to the extreme with your use-cases by adding artificial constraints and I'm sure I could come up with a use-case that would fit just as well in my side of argument. 

> Security is not a laughable matter.

It's not. Nor would this be compromising the security of the system. So I'd like to stay away from argumenting with security in this case, that is all.

- Martin

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

On Oct. 14, 2014, 11:45 a.m., Martin Gräßlin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120577/
> -----------------------------------------------------------
> 
> (Updated Oct. 14, 2014, 11:45 a.m.)
> 
> 
> Review request for Plasma and Aleix Pol Gonzalez.
> 
> 
> Bugs: 339453
>     https://bugs.kde.org/show_bug.cgi?id=339453
> 
> 
> Repository: plasma-workspace
> 
> 
> Description
> -------
> 
> Logging out from the locked screen is impossible. Logging out means
> interaction with the session due to the session manager. The session
> manager asks all applications to quit and applications are allowed to
> ask for example saving changes. The session manager stopps the
> logout in this case and asks the window manager to focus this window
> and the session manager will only continue the logout once the
> application resolved the situation. At any time in this process the
> user is still able to abort the logout!
> 
> Switching to the application which needs interaction is impossible,
> though as the screen is still locked. The result is a seemingly
> frozen system as the logout cannot continue and there is no indication
> what is going on.
> 
> Of course the lock screen cannot unlock the session for the logout as
> that would circumvent the security. If the lock screen would unlock
> one would just have to click the button and abort the logout really
> fast to have the system unlocked. So this is clearly not an option.
> 
> The result is: we cannot implement this functionality in a working
> and secure manner, so it's better to remove it.
> 
> 
> Diffs
> -----
> 
>   lookandfeel/contents/lockscreen/LockScreen.qml 7d730cf1ebd8241dfe1c00a2bef86ec4a3f0212d 
> 
> Diff: https://git.reviewboard.kde.org/r/120577/diff/
> 
> 
> Testing
> -------
> 
> run kscreenlocker_greeter --testing and locked the screen - button is gone.
> 
> 
> Thanks,
> 
> Martin Gräßlin
> 
>

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