What to do with potential workflow-changing / breaking changes?

[Martin Gräßlin]

Am 2017-02-21 17:43, schrieb Fabian Vogt:
> Am Dienstag, 21. Februar 2017, 16:55:36 CET schrieb Martin Gr??lin:
>> Am 2017-02-21 11:07, schrieb Fabian Vogt:
>> 
>> > For the kscreenlocker patch, the better option would be to just disable
>> > the
>> > option to show the password (which is IMO a security issue by itself)
>> > again
>> > and the best option to disable pasting.
>> 
>> That would also break a feature. So given what you wrote above that
>> should not be allowed as well. Given that you find that acceptable you
>> do the mistake of considering one feature more important as the other.
> 
> Only a few use that functionality and it is a security issue in itself, 
> as
> someone could toggle that to make a partially typed password visible or 
> to
> wait until someone types it in, if that someone does not notice the 
> change.
> 
> The clipboard breakage affects all users, whether they use the feature 
> or
> not.

You don't know, I don't know. We both haven't done a user survey to 
figure out. Thus we cannot base assumptions on it.

I don't need the clipboard after the screen locks as it means either:
* at least five minutes away
* or I go explicitly away

Whatever I have in the clipboard would be lost due to my short-term 
memory.

Users are different. To me the clipboard change is a no-brainer and 
nothing which affects the usability of the system. To you it is.
> 
>> Given the emerge of touch screens I consider the possibility to see 
>> the
>> password as highly important. (Yes I'm aware that we don't have an
>> onscreen keyboard on the lockscreen, I'm working on that and have a
>> branch which could go in now that we depend on 5.7)
> 
>> > Instead, it did something that breaks
>> > even beyond the screen locker: It erases the clipboard!
>> 
>> Just pointing out that this also prevents against brute force attacks
>> through pasting the clipboard. Such attacks existed for example 
>> against
>> Android. As the screenlocker maintainer I'm highly interested in
>> reducing the attack surface.
> 
> Would it though? You could just type in a long string and copy it and
> you circumvented the clipboard erasure.

It needs more time, though.

> Alternatives would be (I don't know how hard they would be to 
> implement):
> - Saving clipboard contents, restoring again

Pointless as we cannot restore in an app which is going to close down. 
It needs klipper to be able to restore.

> - Disabling pasting into the password field (event filter?)

That was my initial thought as well, but I didn't find anything in the 
Qt API to achieve it.

> 
> Nobody should ever need to paste into the password field of the 
> lockscreen.

Agreed :-) For the Wayland case I'm considering to do the "magic" 
completely in KWin by disallowing clipboard access to kscreenlocker.

> 
>> > That kate/kwrite cannot be run as root anymore is fine for everyone,
>> > but the
>> > way the patch implements it is not ok. When run using "kdesu kate" or
>> > similar,
>> > the user just gets nothing as feedback! I'm working on a patch to open
>> > a
>> > dialog box as the original user but it's not implemented in a readable
>> > way
>> > yet.
>> 
>> I'm sorry, I didn't consider kdesu(do) at all. It's a tool I never 
>> use.
>> If I need root I use konsole or polkit. But not kdesu. That was an
>> oversight, which also nobody during review noticed. I'm sorry I didn't
>> consider that one can start a gui application as root through gui.
> 
> Ok, that's the only issue I have with this patch. If a dialog box 
> appears
> and explains to the user why whatever he just tried to do does not work
> anymore and how to work around that, it's not a usability issue 
> anymore.
> In my opinion it's equally important to implement the same restriction 
> in
> konsole and dolphin as well. Other apps aren't that likely to be run as 
> root,
> so it's not that important for now.
> 
> A diff of what I have so far it appended to this mail, maybe it's 
> useful.
> It's probably easier for you to make this actually mergeable.

The diff has a problem: it's X11 specific without checking whether we 
are on X11 (and that early we just don't know. On my system (modern 
session) both WAYLAND_DISPLAY and DISPLAY are available.)

I think the detection needs to go into kdesu(do). That would also ensure 
that we detect the Wayland case which currently also just exits.

I just tried to see what happens:
kdesudo dolphin
Invalid MIT-MAGIC-COOKIE-1 keyQXcbConnection: Could not connect to 
display :0

That is kdesudo/kdesu could detect whether the process directly failed 
and show an error message: "Did you try starting a GUI application as 
root? Several applications protect against it and disallow it. If you 
want to edit a file consider using sudoedit..."

It would mean that we need to change kate to not return 0, but maybe 
-EPERM?

Cheers
Martin


> 
> Cheers,
> Fabian
> 
> diff --git a/kate/main.cpp b/kate/main.cpp
> index 9fcda33a4..2a04a5507 100644
> --- a/kate/main.cpp
> +++ b/kate/main.cpp
> @@ -64,8 +64,16 @@ int main(int argc, char **argv)
>       * Check whether we are running as root
>       **/
>      if (getuid() == 0) {
> -        std::cout << "Executing Kate as root is not possible. To edit
> files as root use:" << std::endl;
> -        std::cout << "SUDO_EDITOR=kate sudoedit <file>" << std::endl;
> +        int dialog_ret = system("xauth extract - \"$DISPLAY\" | sudo
> -Eu#${KDE_SESSION_UID} -- sh -c 'export XAUTHORITY=$(mktemp); xauth
> merge - && kdialog --error \"Running kate as root is not possible.\nTo
> edit files as root, use:\nSUDO_EDITOR=kate sudoedit <file>\"; rm
> $XAUTHORITY'");
> +
> +        /**
> +         * Fallback to message to cout
> +         **/
> +        if (dialog_ret != 0) {
> +            std::cout << "Executing Kate as root is not possible. To
> edit files as root use:" << std::endl;
> +            std::cout << "SUDO_EDITOR=kate sudoedit <file>" << 
> std::endl;
> +        }
> +
>          return 0;
>      }
>  #endif