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

[Fabian Vogt]

Am Dienstag, 21. Februar 2017, 20:11:04 CET schrieb Martin Gr??lin:
> >> 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.

Triggering such a vulnerability doesn't take a long time really,
typing asdf<ctrl-a><ctrl-c><ctrl-v><ctrl-v><ctrl-a><ctrl-c><ctrl-v><ctrl-v>...
results in a length of 2^(1+x/4) with x being the count of keypresses.
I just tried to do that in klipper. Funnily enough that broke it, it only
renders the selected part of the text properly:
https://i.imgur.com/n2RKuIl.png

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

Is there no built-in API in QClipboard for that or is this the issue that
the clipboard on X11 is a weird shared buffer?

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

Wouldn't an event filter capturing Ctrl-V (or however it is implemented
inside Qt to determine the key combo) and middle mouse work?
I'm not sure whether that works with QML, I'm still used to good ol'
QWidgets...

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

Sounds good!

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

Yup, as it's not possible to run applications as root on wayland anyway
I didn't bother to figure out something. By doing 2>/dev/null that
should work sufficiently on wayland as well, using the cout fallback.

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

What about other methods of launching it? You mentioned polkit, for
example.

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

As it's the goal to disallow applications from running graphically as
root anyway, maybe kwin is the best place? I'm not too familiar with
the X11 protocol, but maybe it's possible to detect a new X11 connection
and display the warning message to the user and if he selects "Cancel"
the connection is terminated.
There is a window between the connection getting established and the
user making a decision, I'm not sure how critical that would be.

Cheers,
Fabian

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