Re: KRandom regression + fix

[Frederik Schwarzer]

Am 27.04.2016 23:45 schrieb Albert Astals Cid:
El dimecres, 27 d’abril de 2016, a les 11:42:46 CEST, Frederik 
Schwarzer va

escriure:

Am 27.04.2016 08:48 schrieb Johannes Huber:



> thanks for the patch. When i read "randon numbers were predictable"
> instantly
> a alarm bell rings in my head. Is this a security issue?

The docs of rand() state that you should not use it for serious 
business

like cryptography
http://en.cppreference.com/w/cpp/numeric/random/rand
and the most serious business I could see within KDE was PIN 
generation

for Bluetooth pairing. But you can never know who is using it for what
outside of the KDE infrastructure.

Since I am neither a core developer (just maintaining a game which was
beaten by the consequences of this issue) nor a crypto guy, I cannot
really assess the severity of such a regression but my first thoughts
were:
- why is there no unit test cathing this?


Because noone wrote one (obvious answer)


Yes, of course that's the obvious answer. :)
I asked because the answer could have been something along the lines of 
"because KRandom is old; do not use it; we have something new in 
frameworks" or so.



From my "i know nothing about random numbers", i guess it's hard to 
write a
unit test for a sequente of random numbers, you can get ten "3" in a 
row and

it's still a valid random sequence.


srand() is the same as srand(1) so it uses a fixed seed. Thus two 
initialisations produce the same sequence. Not sure though if this can 
be done in a unit test.




- should KRandom api doc pass through the note of not using it for
serious business in general?


Probably makes sense adopting what rand() says, yes. Would you propose 
a

patch?


Already did: https://git.reviewboard.kde.org/r/127767/
Comments about the wording welcome. :)

Regards,
Frederik

Re: KRandom regression + fix

[Albert Astals Cid]

El dimecres, 27 d’abril de 2016, a les 11:42:46 CEST, Frederik Schwarzer va 
escriure:
> Am 27.04.2016 08:48 schrieb Johannes Huber:
> 
> Hi Johannes,
> 
> >> KRandom saw a regression in KCoreAddon's 5.21.0 release, which impacts
> >> a
> >> wide range of applications and use cases. Since the rand() was not
> >> seeded, the numbers generated were predictable, which is ugly in games
> >> and probably alarming for bluetooth pairing.
> > 
> > thanks for the patch. When i read "randon numbers were predictable"
> > instantly
> > a alarm bell rings in my head. Is this a security issue?
> 
> The docs of rand() state that you should not use it for serious business
> like cryptography
> http://en.cppreference.com/w/cpp/numeric/random/rand
> and the most serious business I could see within KDE was PIN generation
> for Bluetooth pairing. But you can never know who is using it for what
> outside of the KDE infrastructure.
> 
> Since I am neither a core developer (just maintaining a game which was
> beaten by the consequences of this issue) nor a crypto guy, I cannot
> really assess the severity of such a regression but my first thoughts
> were:
> - why is there no unit test cathing this?

Because noone wrote one (obvious answer)

From my "i know nothing about random numbers", i guess it's hard to write a 
unit test for a sequente of random numbers, you can get ten "3" in a row and 
it's still a valid random sequence.

> - should KRandom api doc pass through the note of not using it for
> serious business in general?

Probably makes sense adopting what rand() says, yes. Would you propose a 
patch?

Cheers,
  Albert

> 
> That's why I CC'ed kde-core-devel.
> 
> Regards,
> Frederik

Re: KRandom regression + fix

[Ivan Čukić]

If used for crypto, it is a security issue.

Now, while I don't expect anyone who implements the crypto stuff to use
KRandom, I do agree that KRandom docs should explicitly state this.

Cheers,
Ivan

Re: KRandom regression + fix

[Frederik Schwarzer]

Am 27.04.2016 08:48 schrieb Johannes Huber:

Hi Johannes,

KRandom saw a regression in KCoreAddon's 5.21.0 release, which impacts 
a

wide range of applications and use cases. Since the rand() was not
seeded, the numbers generated were predictable, which is ugly in games
and probably alarming for bluetooth pairing.


thanks for the patch. When i read "randon numbers were predictable" 
instantly

a alarm bell rings in my head. Is this a security issue?


The docs of rand() state that you should not use it for serious business 
like cryptography

http://en.cppreference.com/w/cpp/numeric/random/rand
and the most serious business I could see within KDE was PIN generation 
for Bluetooth pairing. But you can never know who is using it for what 
outside of the KDE infrastructure.


Since I am neither a core developer (just maintaining a game which was 
beaten by the consequences of this issue) nor a crypto guy, I cannot 
really assess the severity of such a regression but my first thoughts 
were:

- why is there no unit test cathing this?
- should KRandom api doc pass through the note of not using it for 
serious business in general?


That's why I CC'ed kde-core-devel.

Regards,
Frederik

Re: Moving CI server documentation to community.ko? (was: Re: KDE CI enforcing ECMEnableSanitizers.cmake/KDECompilerSettings.cmake?)

[Ben Cooksley]

On Wed, Apr 27, 2016 at 4:40 AM, Friedrich W. H. Kossebau
 wrote:
> Am Montag, 25. April 2016, 23:26:45 CEST schrieb Albert Astals Cid:
>> El dilluns, 25 d’abril de 2016, a les 10:59:13 CEST, Friedrich W. H.
>> Kossebau
>> va escriure:
>> > Hi,
>> >
>> > can anyone shed full light on KDE CI and the usage of ASan?
>> >
>> > Currently e.g. all tests of Marble are failing, with the message
>> > "ASan runtime does not come first in initial library list; you should
>> > either link runtime to your application or manually preload it with
>> > LD_PRELOAD."
>> >
>> > https://build.kde.org/job/marble%20master%20kf5-qt5/
>> > PLATFORM=Linux,compiler=gcc/26/testReport/
>> >
>> > As Marble tries to be optionally Qt-only by tradition, also the current
>> > Qt5(/ KF5)-based version has lots of custom CMake logic, for full
>> > independence, and only falls back to using ECM macros/settings when it
>> > comes to Plasma & other KDE system integration code.
>> >
>> > Which currently also means all tests are not influenced anywhere by ECM
>> > macros. Incl. KDECompilerSettings.cmake (and thus
>> > ECMEnableSanitizers.cmake). So the code from ECMEnableSanitizers.cmake
>> > which handles the
>> > "ECM_ENABLE_SANITIZERS" cmake argument is never run.
>> >
>> > Where currently -DECM_ENABLE_SANITIZERS='address' seems to be passed in
>> > the
>> > kf5-qt5 configuration:
>> > https://quickgit.kde.org/?p=sysadmin%2Fci-builder-tools.git=blob=build
>> > %2Fkf5-qt5.cfg
>> >
>> > Is this the reason that all the Marble tests fail on KDE CI, with the
>> > given
>> > error?
>> >
>> > So do any projects which are build on KDE CI need to have
>> > ECMEnableSanitizers.cmake included?
>> >
>> > Would it make sense to have ASan as an option to be turned off?
>>
>> It's compile time, it's off for your project, but you're linking against KF5
>> libraries that have ASAN compiled in.
>>
>> > And is that possible, or is ASan usage viral (if deps built on CI have it,
>> > it needs to be used)?
>>
>> Yes ASan usage is viral-ish, if you're using a library that was compiled
>> with ASAN you either need to compile your binary with asan too or pass the
>> LD_PRELOAD as the error says, that may be tricky since it needs a full
>> path.
>> > While using ASan seems to be useful for improved test coverage, this
>> > requirement still would need to be explained and documented somewhere,
>> > please.
>>
>> Where would you document it?
>
> Right now AFAIK the only documentation about CI is at https://
> sysadmin.kde.org/services/continuous-integration/
>
> Though perhaps it makes sense to move that over to somewhere below https://
> community.kde.org/Infrastructure, both to improve finding it and
> to lower the burden for non-sysadmin people to maintain the pages (possibly
> still need higher edit restrictions to minimize wrong information there).
>
> Sysadmin & else, what do you think?

I believe Scarlett has been documenting things at
https://phabricator.kde.org/w/build-kde-org/

>
> Cheers
> Friedrich
>
>

Regards,
Ben

Re: KDE CI enforcing ECMEnableSanitizers.cmake/KDECompilerSettings.cmake?

[Ben Cooksley]

On Wed, Apr 27, 2016 at 2:49 PM, Friedrich W. H. Kossebau
 wrote:
> Am Mittwoch, 27. April 2016, 01:29:06 CEST schrieb Jan Kundrát:
>> On Wednesday, 27 April 2016 01:21:22 CEST, Albert Astals Cid wrote:
>> > No, Qt5 is not built with ASAN on CI.
>
> Okay, good to know.
> (next to the failing tests I also remembered to have seen ASAN_OPTIONS
> detect_leaks=0 as env setting with the qt5 builds on CI. So that is just
> default env setup and those vars ignored otherwise, okay).
>
> Next to documenting things, can we start with some rule what gets or should be
> built with ASAN, so people know what to expect?
> I would assume: any KDE software based on C++/C. And then there might be a few
> exceptions, for whatever reasons (built screwed, incompatible, etc).

Everything that uses ECM gets built with ASAN basically.
As a general rule, most KF5 using software uses ASAN because they all use ECM.

>
>> > It is strange that your Qt5-only tests are failing, may it be that they
>> > are
>> > loading some plugin that is linked against some KF5 lib?
>
> For Marble plugins only if something is not like it should be:
> for one, the current build on CI even gets "-DQTONLY=TRUE" passed, which is
> turned into the cmake var "set(WITH_KF5 FALSE)", and all KF5 deps are looked
> for with "macro_optional_find_package(KF5 QUIET COMPONENTS something)", so
> should always yield NOT_FOUND (looking at it I found a bug which still made
> KF5DocTools to be found, but now fixed and should not have any effect on
> linking to KF5 libs).
> More, the only things being explicitely linked to KF5 libs are KF5 thumbnailer
> plugin, KRunner plugin, and the KF5-enhanced Marble app variant. So nothing
> which should be used in the tests.
>
> Ah, Phonon! That is linked by at least the RoutingPlugin. And Phonon gets
> instrumented with ASAN:
> https://build.kde.org/job/phonon%20master%20kf5-qt5/
> PLATFORM=Linux,compiler=gcc/5/console
>
> That might explain what we see currently.
>
>> Qt guesses what platform one is running on in order to blend with it. In
>> KDE and under the Plasma desktop, this involves loading
>> plugins/platformthemes/KDEPlatformTheme.so which belongs to KF5's
>> frameworkintegration.
>>
>> Is the KDE CI setting some variables which might trigger loading of these
>> plugins [edited]?
>
> Good idea, that might be indeed other intrusion path for ASAN deps.
> @Scarlett, can you tell?

Please see my other mail to Jan.

>
> Cheers
> Friedrich

Regards,
Ben

Re: KDE CI enforcing ECMEnableSanitizers.cmake/KDECompilerSettings.cmake?

[Ben Cooksley]

On Wed, Apr 27, 2016 at 11:29 AM, Jan Kundrát  wrote:
> On Wednesday, 27 April 2016 01:21:22 CEST, Albert Astals Cid wrote:
>>
>> It is strange that your Qt5-only tests are failing, may it be that they
>> are loading some plugin that is linked against some KF5 lib?
>
>
> Qt guesses what platform one is running on in order to blend with it. In KDE
> and under the Plasma desktop, this involves loading
> plugins/platformthemes/KDEPlatformTheme.so which belongs to KF5's
> frameworkintegration.
>
> Is the KDE CI setting some variables which might trigger loading of these
> variables?

Please see line 715 of
https://quickgit.kde.org/?p=sysadmin%2Fci-master-config.git=blob=3df963348dedf66567249530ddc7de4f0a1e0b76=8bdb20f0189da74e9875be45822e7f9aa872543e=tools%2Fkdecilib.py

>
> Cheers,
> Jan

Regards,
Ben

>
> --
> Trojitá, a fast Qt IMAP e-mail client -- http://trojita.flaska.net/