[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-panel-devel
Subject: Re: Review Request 121429: Use out-of-band communication between ksld and greeter
From: Àlex_Fiestas <afiestas () kde ! org>
Date: 2014-12-22 14:37:34
Message-ID: 20141222143734.16952.45949 () probe ! kde ! org
[Download RAW message or body]
--===============5389524803283398043==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit
> On des. 15, 2014, 10:45 p.m., Àlex Fiestas wrote:
> > Code looks good.
> >
> > Could you perhaps add an integration test for this? Since we are "abstracted" by \
> > the socket it should be possible. If it is too much work feel free to push it.
>
> Martin Gräßlin wrote:
> what do you want the integration test to test? I certainly can start the daemon but \
> I'm not sure what it would give us as the only way to return from it requires a \
> password. And that's what the test application in tests already does.
> Àlex Fiestas wrote:
> Well, this patch adds a lot of new logic that can be tested, since it does not have \
> unit test (and doing them in ksmserver migh prove difficult) we can test the code \
> with an integraiton test.
> I see lots of socket logic
> I see logic in addAllowedWindow
> And also the biggest method setKsldSocket which has 2 lambdas that (afaik) can't be \
> tested in any other way.
> Martin Gräßlin wrote:
> The point is I don't see how to do an integration test for it. If we pull up \
> everything the screen is locked, like locked. It needs a damn password to be \
> entered to get unlocked. I just don't see how this could be integration tested.
> If you see how to integration test it please provide the code for it.
As I said, if it is too much work just push it :p.
- Àlex
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121429/#review72103
-----------------------------------------------------------
On des. 15, 2014, 9:29 a.m., Martin Gräßlin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/121429/
> -----------------------------------------------------------
>
> (Updated des. 15, 2014, 9:29 a.m.)
>
>
> Review request for Plasma, Àlex Fiestas and David Edmundson.
>
>
> Repository: plasma-workspace
>
>
> Description
> -------
>
> The screenlocker_greet needs to tell the parent ksld process which
> windows it created. Ksld sends input events to these windows. So
> far this was based on an X property on the window. Unfortunately
> ksld didn't validate whether the windows tagged with this property
> belong to the screenlocker_greet process it started.
>
> With this change the communication for announcing windows is moved
> away from the X11 protocol and instead a custom Wayland protocol is
> used.
>
> Ksld starts a KWaylandServer when the greet process gets started. It
> creates anonymous unix sockets for the connection and passes one
> filedescriptor to the started greeter process.
>
> The check for the X property is removed in ksld and instead only
> windows ids passed through the Wayland socket connection are
> accepted.
>
>
> Diffs
> -----
>
> ksmserver/screenlocker/ksldapp.cpp 22698ce37e9d4be17126111b3ded8133f7c3baa6
> ksmserver/screenlocker/lockwindow.h 9938d201269c89a24c9c0bd6275aa5f731bb5535
> ksmserver/screenlocker/lockwindow.cpp 3aa963a59e21636862f5ca59e220bbea3bd41ff9
> ksmserver/screenlocker/protocols/ksld.xml PRE-CREATION
> ksmserver/screenlocker/waylandserver.h PRE-CREATION
> ksmserver/screenlocker/waylandserver.cpp PRE-CREATION
> ksmserver/screenlocker/greeter/greeterapp.h \
> b92b13b63365a9026dba5d71b772dcd8c9ee3d3b \
> ksmserver/screenlocker/greeter/greeterapp.cpp \
> 30d1821bdba38028959f3457e900a1b32e628192 ksmserver/screenlocker/greeter/main.cpp \
> 12e570107d0cba851b8978131d730b27924529bb ksmserver/screenlocker/ksldapp.h \
> 095424c9845c134aa156917aeb6c8ddf31e8d25a CMakeLists.txt \
> c6d89c14b05f5639937aee5692d305fa2faed974 ksmserver/screenlocker/CMakeLists.txt \
> 5378a10df2be70cee95b5612c23046eae639f610 \
> ksmserver/screenlocker/greeter/CMakeLists.txt \
> 10c473488f08354096f68784b9240392a444af5f
> Diff: https://git.reviewboard.kde.org/r/121429/diff/
>
>
> Testing
> -------
>
> Running ksmserver with the patch. Lock/unlock working, my exploit is failing.
>
>
> Thanks,
>
> Martin Gräßlin
>
>
--===============5389524803283398043==
MIME-Version: 1.0
Content-Type: text/html; charset="utf-8"
Content-Transfer-Encoding: 8bit
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 \
solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;"> \
<tr> <td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/121429/">https://git.reviewboard.kde.org/r/121429/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On desembre 15th, 2014, 10:45 p.m. UTC, <b>Àlex \
Fiestas</b> wrote:</p> <blockquote style="margin-left: 1em; border-left: 2px solid \
#d0d0d0; padding-left: 10px;"> <pre style="white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">Code looks good. </p> <p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Could \
you perhaps add an integration test for this? Since we are "abstracted" by the socket \
it should be possible. If it is too much work feel free to push it.</p></pre> \
</blockquote>
<p>On desembre 16th, 2014, 7 a.m. UTC, <b>Martin Gräßlin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">what \
do you want the integration test to test? I certainly can start the daemon but I'm \
not sure what it would give us as the only way to return from it requires a password. \
And that's what the test application in tests already does.</p></pre> </blockquote>
<p>On desembre 16th, 2014, 11:09 p.m. UTC, <b>Àlex Fiestas</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Well, \
this patch adds a lot of new logic that can be tested, since it does not have unit \
test (and doing them in ksmserver migh prove difficult) we can test the code with an \
integraiton test.</p> <p style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: inherit;">I see lots of socket logic I see logic \
in addAllowedWindow And also the biggest method setKsldSocket which has 2 lambdas \
that (afaik) can't be tested in any other way.</p></pre> </blockquote>
<p>On desembre 18th, 2014, 8:45 a.m. UTC, <b>Martin Gräßlin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The \
point is I don't see how to do an integration test for it. If we pull up everything \
the screen is locked, like locked. It needs a damn password to be entered to get \
unlocked. I just don't see how this could be integration tested.</p> <p \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">If you see how to integration test it please provide the code for \
it.</p></pre> </blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">As I \
said, if it is too much work just push it :p.</p></pre> <br />
<p>- Àlex</p>
<br />
<p>On desembre 15th, 2014, 9:29 a.m. UTC, Martin Gräßlin wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: \
1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; \
-webkit-border-radius: 6px;"> <tr>
<td>
<div>Review request for Plasma, Àlex Fiestas and David Edmundson.</div>
<div>By Martin Gräßlin.</div>
<p style="color: grey;"><i>Updated des. 15, 2014, 9:29 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
plasma-workspace
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" \
style="border: 1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">The screenlocker_greet needs to tell the parent ksld \
process which windows it created. Ksld sends input events to these windows. So
far this was based on an X property on the window. Unfortunately
ksld didn't validate whether the windows tagged with this property
belong to the screenlocker_greet process it started.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">With this change the communication for announcing \
windows is moved away from the X11 protocol and instead a custom Wayland protocol is
used.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">Ksld starts a KWaylandServer when the greet process \
gets started. It creates anonymous unix sockets for the connection and passes one
filedescriptor to the started greeter process.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">The check for the X property is removed in ksld and \
instead only windows ids passed through the Wayland socket connection are
accepted.</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">Running ksmserver with the patch. Lock/unlock working, \
my exploit is failing.</p></pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>ksmserver/screenlocker/ksldapp.cpp <span style="color: \
grey">(22698ce37e9d4be17126111b3ded8133f7c3baa6)</span></li>
<li>ksmserver/screenlocker/lockwindow.h <span style="color: \
grey">(9938d201269c89a24c9c0bd6275aa5f731bb5535)</span></li>
<li>ksmserver/screenlocker/lockwindow.cpp <span style="color: \
grey">(3aa963a59e21636862f5ca59e220bbea3bd41ff9)</span></li>
<li>ksmserver/screenlocker/protocols/ksld.xml <span style="color: \
grey">(PRE-CREATION)</span></li>
<li>ksmserver/screenlocker/waylandserver.h <span style="color: \
grey">(PRE-CREATION)</span></li>
<li>ksmserver/screenlocker/waylandserver.cpp <span style="color: \
grey">(PRE-CREATION)</span></li>
<li>ksmserver/screenlocker/greeter/greeterapp.h <span style="color: \
grey">(b92b13b63365a9026dba5d71b772dcd8c9ee3d3b)</span></li>
<li>ksmserver/screenlocker/greeter/greeterapp.cpp <span style="color: \
grey">(30d1821bdba38028959f3457e900a1b32e628192)</span></li>
<li>ksmserver/screenlocker/greeter/main.cpp <span style="color: \
grey">(12e570107d0cba851b8978131d730b27924529bb)</span></li>
<li>ksmserver/screenlocker/ksldapp.h <span style="color: \
grey">(095424c9845c134aa156917aeb6c8ddf31e8d25a)</span></li>
<li>CMakeLists.txt <span style="color: \
grey">(c6d89c14b05f5639937aee5692d305fa2faed974)</span></li>
<li>ksmserver/screenlocker/CMakeLists.txt <span style="color: \
grey">(5378a10df2be70cee95b5612c23046eae639f610)</span></li>
<li>ksmserver/screenlocker/greeter/CMakeLists.txt <span style="color: \
grey">(10c473488f08354096f68784b9240392a444af5f)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/121429/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
--===============5389524803283398043==--
_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic