Bug 362805 - KF5Wallet can't open previuosly created wallet with error code -9
Summary: KF5Wallet can't open previuosly created wallet with error code -9
Status: RESOLVED FIXED
Alias: None
Product: kwalletmanager
Classification: Applications
Component: general (show other bugs)
Version: unspecified
Platform: openSUSE Linux
: NOR normal
Target Milestone: ---
Assignee: Valentin Rusu
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-05-08 08:49 UTC by Mykola Krachkovsky
Modified: 2020-05-18 17:47 UTC (History)
9 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
Patch to fix blowfish algorithm (1.29 KB, patch)
2016-05-08 16:52 UTC, Allen Winter
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Mykola Krachkovsky 2016-05-08 08:49:16 UTC
KWallet git version from 20160502T210733 commit 15e6feb works fine and opens kdewallet (only wallet).
KWallet git version from 20160507T105330 commit b3a95ba can't open the same wallet using same (and surely correct) password with error code -9.

Reproducible: Always
Comment 1 Bruno Friedmann 2016-05-08 12:10:30 UTC
Same here, kwalletmanager ( 4x work perfect ) 
my kwallet has no password 
kwalletmanager5 fail with error -9

related system log
May 08 14:02:50 dbus[2111]: [system] Activating service name='org.kde.kcontrol.kcmkwallet5' (using servicehelper)
May 08 14:02:50 org.kde.kcontrol.kcmkwallet5[5266]: QDBusArgument: read from a write-only object
May 08 14:02:50 org.kde.kcontrol.kcmkwallet5[5266]: QDBusArgument: read from a write-only object
May 08 14:02:50 org.kde.kcontrol.kcmkwallet5[5266]: QDBusArgument: read from a write-only object
May 08 14:02:50 dbus[2111]: [system] Successfully activated service 'org.kde.kcontrol.kcmkwallet5'
May 08 14:03:05 polkitd[2257]: Operator of unix-session:1 successfully authenticated as unix-user:bruno to gain TEMPORARY authorization for action org.kde.kcontrol.kcmkwallet5.save for system-bus-name::1.104 [/usr/bin/kcmshell5 kwalletconfig5] (owned by unix-user:bruno)
May 08 14:03:35 dbus[2111]: [system] Activating service name='org.kde.kcontrol.kcmkwallet5' (using servicehelper)
May 08 14:03:35 org.kde.kcontrol.kcmkwallet5[5317]: QDBusArgument: read from a write-only object
May 08 14:03:35 org.kde.kcontrol.kcmkwallet5[5317]: QDBusArgument: read from a write-only object
May 08 14:03:35 org.kde.kcontrol.kcmkwallet5[5317]: QDBusArgument: read from a write-only object
May 08 14:03:35 dbus[2111]: [system] Successfully activated service 'org.kde.kcontrol.kcmkwallet5'
May 08 14:03:39 org.kde.kwalletd5[2919]: Setting useNewHash to true
May 08 14:03:39 org.kde.kwalletd5[2919]: Wallet new enough, using new hash
May 08 14:03:39 org.kde.kwalletd5[2919]: fsize:  145769968  encrypted.size():  141760  blksz:  8
May 08 14:03:46 org.kde.kwalletd5[2919]: Setting useNewHash to true
May 08 14:03:46 org.kde.kwalletd5[2919]: Wallet new enough, using new hash
May 08 14:03:46 org.kde.kwalletd5[2919]: fsize:  1568022951  encrypted.size():  141760  blksz:  8
May 08 14:03:51 org.kde.kwalletd5[2919]: Setting useNewHash to true
May 08 14:03:51 org.kde.kwalletd5[2919]: Wallet new enough, using new hash
May 08 14:03:51 org.kde.kwalletd5[2919]: fsize:  3415025526  encrypted.size():  141760  blksz:  8
May 08 14:03:52 org.kde.kwalletd5[2919]: Setting useNewHash to true
May 08 14:03:52 org.kde.kwalletd5[2919]: Wallet new enough, using new hash
May 08 14:03:52 org.kde.kwalletd5[2919]: fsize:  145769968  encrypted.size():  141760  blksz:  8
May 08 14:03:54 org.kde.kwalletd5[2919]: Setting useNewHash to true
May 08 14:03:54 org.kde.kwalletd5[2919]: Wallet new enough, using new hash
May 08 14:03:54 org.kde.kwalletd5[2919]: fsize:  145769968  encrypted.size():  141760  blksz:  8
May 08 14:03:54 org.kde.kwalletd5[2919]: QXcbConnection: XCB error: 3 (BadWindow), sequence: 3128, resource id: 35651792, major code: 40 (TranslateCoords), minor code: 0
May 08 14:05:40 org.kde.kwalletd5[2919]: Setting useNewHash to true
May 08 14:05:40 org.kde.kwalletd5[2919]: Wallet new enough, using new hash
May 08 14:05:40 org.kde.kwalletd5[2919]: fsize:  145769968  encrypted.size():  141760  blksz:  8
Comment 2 Bruno Friedmann 2016-05-08 13:26:32 UTC
Valentin, if you are like me using kua/kuf repository you can still downgrade all wallet stuf to what oss propose. 
It will allow you at least be able to work.
Comment 3 Hrvoje Senjan 2016-05-08 16:21:46 UTC
Allen, can you check what happened with your last commit? Note that it also ended up in 5.22.0 tar.
Comment 4 Hrvoje Senjan 2016-05-08 16:24:48 UTC
Please compare also bug 344162, we had similar situation there.
Comment 5 Allen Winter 2016-05-08 16:42:35 UTC
are the problems happening on big endian or little endian systems?
you guys are using blowfish?

interestingly, if I remove the include <qglobal.h>
#if Q_BYTE_ORDER == Q_BIG_ENDIAN gives a warning but evaluates to true, so on a little endian system like mine the big endian code will be compiled. 

if I put back the include <qglobal.h>
#if Q_BYTE_ORDER == Q_BIG_ENDIAN evaluates to false on my system, as it should.

so... seems to be me we need to include <qglobal.h> but reverse the conditional Q_LITTLE_ENDIAN

I wish we had a make test in kwallet
Comment 6 Allen Winter 2016-05-08 16:46:20 UTC
yep. if you look at Bruce Schneier's blowfish.c you'll see code like this:
   #ifdef little_endian
	 data = ((data & 0xFF000000) >> 24) |
		((data & 0x00FF0000) >>  8) |
		((data & 0x0000FF00) <<  8) |
		((data & 0x000000FF) << 24);
   #endif
which is what we do with the shuffle macro.
Comment 7 Luca Beltrame 2016-05-08 16:46:56 UTC
Those are normal x86_64 systems, so little endian, I'd say.
Comment 8 Allen Winter 2016-05-08 16:52:39 UTC
Created attachment 98845 [details]
Patch to fix blowfish algorithm
Comment 9 Allen Winter 2016-05-08 16:52:58 UTC
I just attached a patch.  Can anyone  test it?
Comment 10 Allen Winter 2016-05-08 16:54:25 UTC
if you do test the patch, please remember to apply the patch to the un-reverted code.
in other works, make sure you have #include <QtCore/qglobal.h>
Comment 11 Mykola Krachkovsky 2016-05-08 21:01:39 UTC
Patch works for me. Thanks!
Comment 12 Allen Winter 2016-05-08 21:24:56 UTC
Git commit 87e774825b779ba846315a8b2ffe6479dd9f9814 by Allen Winter.
Committed on 08/05/2016 at 21:24.
Pushed by winterz into branch 'master'.

kwalletd/backend/blowfish.cc -fix Q_BYTE_ORDER for little endian

M  +5    -5    src/runtime/kwalletd/backend/blowfish.cc

http://commits.kde.org/kwallet-framework/87e774825b779ba846315a8b2ffe6479dd9f9814
Comment 13 Gabriel C 2016-05-09 13:51:50 UTC
Hello Allen ,

well I cannot see how this can work .. 

from my tests this breaks kwallet <=5.21.0.

meaning 

5.21.0 -> master is broken always gives you a -9 error
master setup and back to 5.21.0 is broken gives always -9 error

The Q_BYTE_ORDER == .. in code was right just br0ken by missing header files on <=5.21.0 so I cannot see how this can be fixed to keep compatibility..
Comment 14 Gabriel C 2016-05-09 16:05:49 UTC
Allen ,

this commit :

https://quickgit.kde.org/?p=kwallet.git&a=commitdiff&h=0d56c68d7a2204a987a5255096d004d5a696c0e5&hp=87e774825b779ba846315a8b2ffe6479dd9f9814

is not really right..

It does not matter how close one will look at it right now you get BIG ENDIAN algo on LITTLE ENDIAN becuase you are missing to include the header file..

the build  has now again lots :

warning: "Q_BIG_ENDIAN" is not defined [-Wundef]
 #if Q_BYTE_ORDER == Q_BIG_ENDIAN
                     ^
Comment 15 Allen Winter 2016-05-09 16:31:49 UTC
that's exactly what I was trying to fix that got us into this mess in the first place.
The code is now rolled-back to what it was before I messed with it.
well, except for a new comment.
Comment 16 Gabriel C 2016-05-09 16:52:59 UTC
yes , now is again broken in the way it was broken before your first fix. ( and this one was right )

I don't think there is a way to fix it and remain compatible with <=5.21.0 .. however you guys have to decide the right 'point' to break older installs ..

Best Regards
Comment 17 Maximiliano Curia 2016-06-21 21:01:44 UTC
With the addtion of the test case in:
https://quickgit.kde.org/?p=kwallet.git&a=commitdiff&h=7e7644b608e22a13e110284ed6c5426c1b493b43

It's clear now that the blowfish backend doesn't work in big endian, as shown in the buildds logs for mips and powerpc in Debian:
https://buildd.debian.org/status/package.php?p=kwallet-kf5

Adding back the include:
#include <QtCore/qglobal.h>

and changing the checks for #if Q_BYTE_ORDER == Q_LITTLE_ENDIAN
(effectively reverting the blowfish backend to the state in 
https://quickgit.kde.org/?p=kwallet.git&a=blob&h=e09d5422dfb404f2f15e74d693b24bb6cd8e1689&hb=87e774825b779ba846315a8b2ffe6479dd9f9814&f=src%2Fruntime%2Fkwalletd%2Fbackend%2Fblowfish.cc)
makes the test pass in little and big endian machines.

This changes nothing for little endian machines. And in big endian machines it should fix the blowfish backend.

Happy hacking,
Comment 18 David Faure 2020-04-26 21:19:00 UTC
Maximiliano Curia: I'm confused. Are you saying that you have a fix that unbreaks big endian systems without breaking little endian systems? That sounds good. Can you attach an actual patch?

Tests passing is good, but being able to open existing wallets is even more important. Would the change still support existing wallets created on big endian systems and on little endian systems?
Comment 19 David Faure 2020-05-17 11:03:36 UTC
Michael Pyne's comment https://phabricator.kde.org/D4335 says otherwise.

I'm afraid we have to keep the current mess for compat reasons.

Some distros (debian, neon, probably opensuse?) have applied the patch that tests for little endian [1]. Others (arch, upstream KDE) are doing 0==0 in that #if (or 1==1 with my recent commit to silence -Wundef). In both cases we go into those #if, on little endian machines.
I believe the original intent (see the old code at [2]) was to NOT go into those #if (which were for big endian) but that ship has sailed. Changing that would break all wallets.

On big endian, though, IIUC we have the choice between broken-unittest (without the patch) and broken-for-existing-users (with the patch). No good solution :(

[1] https://packaging.neon.kde.org/kde/kwallet.git/tree/debian/patches/blowfish_endianess.diff?h=Neon/unstable&id=572447bcb13e5f36247b3872a60d2959a3f82e12
[2] https://cgit.kde.org/kde-runtime.git/tree/kwalletd/backend/blowfish.cc?id=a6c3e399e8674914ccde5d80710de866cbb700da

Please correct me if I'm wrong in any of this.
What a mess!