<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/115497/">https://git.reviewboard.kde.org/r/115497/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I think the salt selection should be improved (or at least stored with the passwords on disk in case the user chooses a different login name). You might also want to consider 'scrypt' instead of PBKDF2 (both are available in libgcrypt).</pre>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/115497/diff/1/?file=242022#file242022line129" style="color: black; font-weight: bold; text-decoration: underline;">kwalletd/backend/kwalletbackend.cc</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">129</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">GCRY_KDF_PBKDF2</span><span class="p">,</span> <span class="n">GCRY_MD_SHA512</span><span class="p">,</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">libgcrypt supports the "scrypt" key derivation function since 1.6.0, and we require 1.6.1.
Since scrypt is superior in almost all respects to PBKDF2 I'd recommend using it instead since we're improving KDFs here anyways. More info at http://www.tarsnap.com/scrypt.html
There are "side channel" attacks possible with scrypt by its design that don't occur with PBKDF2, but there shouldn't be malicious code running on the user's system at the same time as they're opening the wallet anyways (if that's the case we have bigger problems).
The "bcrypt" algorithm is apparently even better for this particular use case, but it doesn't appear to be available in libgcrypt.</pre>
</div>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/115497/diff/1/?file=242022#file242022line130" style="color: black; font-weight: bold; text-decoration: underline;">kwalletd/backend/kwalletbackend.cc</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">130</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">passwd</span><span class="o">-></span><span class="n">pw_name</span><span class="p">,</span> <span class="n">strlen</span><span class="p">(</span><span class="n">passwd</span><span class="o">-></span><span class="n">pw_name</span><span class="p">),</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">The salt here seems to be based off of the user's login-name, which can change (for instance, someday my KDE git user account will change away from "kde-svn"... I've just been lazy so far ;).
Beyond that the salt should really be random bytes otherwise you could still build rainbow tables of the top-100 most popular user names, for instance.
Using random bytes would complicate this since you'd have to actually store the salt and re-load it to re-derive the right key, but it's the right way to do it.
In any event it's probably more important to ensure that there's no data loss if the user tries to open their wallet under a different UNIX login, even if we have to use a plain constant string as the salt.</pre>
</div>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/115497/diff/1/?file=242022#file242022line635" style="color: black; font-weight: bold; text-decoration: underline;">kwalletd/backend/kwalletbackend.cc</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">bool Backend::entryDoesNotExist(const QString& folder, const QString& entry) const {</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">635</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">passwrod2PBKDF2_SHA512</span><span class="p">(</span><span class="n">password</span><span class="p">,</span> <span class="n">_newPassHash</span><span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Seems to be no error checking here, if this fails and we overwrite the hashed passwords on disk, couldn't there be data loss when we try to re-open them (when the user can't guess the key)?</pre>
</div>
<br />
<p>- Michael Pyne</p>
<br />
<p>On February 5th, 2014, 3:10 p.m. UTC, Àlex Fiestas wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDE Runtime, Teo Mrnjavac and Valentin Rusu.</div>
<div>By Àlex Fiestas.</div>
<p style="color: grey;"><i>Updated Feb. 5, 2014, 3:10 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kde-runtime
</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;">Uses the MINOR_VERSION (which until now it was 0) to upgrade the hash from SHA to PBKDF2-SHA512+salt.
I would have loved to completely replace it once the wallet is ported to the new hashing but because
of kwalletd code that is not possible without a bigger rewrite.
There are 2 reasons for this patch:
1-We avoid using our own implementation of SHA
2-We use a modern hashing technique
I'm cooking more patches to use the system user password to open the wallet, we want that password to be
hashed using PBKDF2_SHA512 for security reasons.</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>CMakeLists.txt <span style="color: grey">(275a6c7)</span></li>
<li>cmake/modules/FindLibGcrypt.cmake <span style="color: grey">(PRE-CREATION)</span></li>
<li>kwalletd/backend/CMakeLists.txt <span style="color: grey">(5a5837c)</span></li>
<li>kwalletd/backend/backendpersisthandler.cpp <span style="color: grey">(bdef6ca)</span></li>
<li>kwalletd/backend/kwalletbackend.h <span style="color: grey">(83ebf7f)</span></li>
<li>kwalletd/backend/kwalletbackend.cc <span style="color: grey">(e4d461c)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/115497/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>