<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/127833/">https://git.reviewboard.kde.org/r/127833/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 23rd, 2016, 11:20 a.m. UTC, <b>David Faure</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;">Please note that:
* the Q_BYTE_ORDER change was reverted, since it made the wallet storage incompatible with previous releases. This needs further analysis in order to improve the code while retaining compat.
* kwallet is definitely usable with a home install, I'm doing that right now. I do have the distro KF5 packages installed though, so maybe the polkit stuff got installed at the right place into the system as well. Anyhow it means it should be fixable with a one-time copy operation as root.</pre>
</blockquote>
<p>On May 24th, 2016, 3:12 a.m. UTC, <b>Michael Pyne</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;">That reminds me, I made an autotest for the Blowfish cipher that would hopefully catch such errors in the future. I've committed the autotests now (they pass, hopefully they don't break CI!).
With that autotest I think that Allen Winter's last Q_BYTE_ORDER change had actually been correct (to enable the relevant code sections when Q_BYTE_ORDER == Q_LITTLE_ENDIAN), but his first commit (to #include qglobal.h) would have generated incorrect Wallets which would then not match with KWallet when it was fixed.
Either way someone's systems are being broken here, because the current code unilaterally enables bit-shuffling always no matter if the CPU is big-endian or little-endian. But I don't have a big-endian machine to test on to make sure a fix would (or would not) work.</pre>
</blockquote>
<p>On June 25th, 2016, 5:42 p.m. UTC, <b>Pino Toscano</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;">This indeed shows that the current situation (kwallet 5.23) is broken on big endian platforms; looking at the failures we got when it was uploaded to Debian few days ago, https://buildd.debian.org/status/logs.php?pkg=kwallet-kf5&ver=5.23.0-1&suite=sid, we've got failures (related to this) on mips, powerpc, and hppa (s390x is not built yet, but it will fail there too) -- all of them are big endian.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I can help in testing patches fixing the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">blowfishtest</code> unit test on those platforms.</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">the current code unilaterally enables bit-shuffling always no matter if the CPU is big-endian or little-endian.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">A simple fix could then be use QtGlobal again, but invert the byte order conditions like:
-#if Q_BYTE_ORDER == Q_BIG_ENDIAN
+#if Q_BYTE_ORDER == Q_LITTLE_ENDIAN
this would preserve the compatibility with little endian platforms, although breaking wallets on big endian machines.</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;">If you can test a patch, I'd recommend Allen Winter's last fix attempt before the whole thing was reverted, at commit 87e774825b779ba846315a8b2ffe6479dd9f9814. This should implement your suggestion already, it was only reverted since there continued to be complaints of brokenness.
It is my feeling that his patch was correct, and that the problems noted were by users who had recreated a wallet during the 34-hour window where the cipher was broken for all (due to commit b3a95ba0540e01a9bb10db53fc449cc49ce9a9e8 activating Q_BYTE_ORDER checks in reverse). If wallets generated in this window are broken then they would always be treated as corrupt by correct code. But note that the current autotest only checks the cipher for compliance against Blowfish's own test vectors, not whether wallets generated using it can be reopened later.</pre>
<br />
<p>- Michael</p>
<br />
<p>On May 8th, 2016, 12:10 a.m. UTC, Michael Pyne 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 KDE Frameworks.</div>
<div>By Michael Pyne.</div>
<p style="color: grey;"><i>Updated May 8, 2016, 12:10 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kwallet
</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;">This is a collection of minor fixes:
An uninit variable usage was noted by Coverity (CID 1289177) for a CBC crypto function, though it only happens for encryption lengths that would not be hit in practice. I troubleshot this in December but forgot to make a RR, but IIRC the lengths that would cause problems are 7 bytes or less -- but it's still better to fix.
The other Coverity fix is to avoid a needless dup(2) of an opened socket since it's immediately turned into a FILE* object anyways (CID 1353007). This avoids a minor resource leak of a file descriptor.
Finally, some of the ciphers use Qt checks for endianness, and need to actually include the header that does this instead of relying on other parts of the code incidentally pulling in the needed #includes.</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;">Everything still compiles -- I'm limited in my ability to test since I'm still using KDE4's KWallet (as the KF5 stuff seems to require polkit to actually work, which isn't possible with a homedir install like mine).</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>src/runtime/kwalletd/backend/cbc.cc <span style="color: grey">(4c13466)</span></li>
<li>src/runtime/kwalletd/main.cpp <span style="color: grey">(90c60d8)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/127833/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>