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

[Elvis Angelaccio]

On Tue, Feb 21, 2017 at 5:43 PM, Fabian Vogt <fabian at ritter-vogt.de> wrote:
> 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.
>
>> 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.
> Alternatives would be (I don't know how hard they would be to implement):
> - Saving clipboard contents, restoring again
> - Disabling pasting into the password field (event filter?)
>
> Nobody should ever need to paste into the password field of the lockscreen.
>
>> > 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.

For the record, Dolphin recently got the very same restriction:
https://cgit.kde.org/dolphin.git/commit/?id=0bdd8e0b0516555c6233fdc7901e9b417cf89791

> 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.
>
> Cheers,
> Fabian

Cheers,
Elvis

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