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

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

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

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