Review Request 115497: Replace SHA with PBKDF2-SHA512+Salt

[Michael Pyne]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/115497/#review49315
-----------------------------------------------------------


I've had a chance to download the patch and restart the desktop with the patched code.

It didn't eat my wallet, which is the best news. ;)

I've stared at the code for about an hour now and only saw one other issue (and that's only in the edge case that we can't write a salt file). I think it looks OK so I'd leave the decision to commit up to you, or you can see if valir has other comments.


kwalletd/backend/kwalletbackend.cc
<https://git.reviewboard.kde.org/r/115497/#comment34813>

    I think this should only be set to KWALLET_VERSION_MINOR if _useNewHash is true; otherwise when we read in this file later we'll try to use the PBKDF2 hash to decrypt it instead of the old hash which will fail.



kwalletd/backend/kwalletbackend.cc
<https://git.reviewboard.kde.org/r/115497/#comment34814>

    I think that this should be PBKDF2_SHA512_KEYSIZE, strictly speaking.
    
    Of course, in this case it turns out to be the exact same number (448 / 8 == 56). Or maybe it's better to warn the future developer when the KEYSIZE macro is defined not to change the PBKDF2 key size without also looking at the Blowfish persist handler.


- Michael Pyne


On Feb. 7, 2014, 5:39 p.m., Àlex Fiestas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115497/
> -----------------------------------------------------------
> 
> (Updated Feb. 7, 2014, 5:39 p.m.)
> 
> 
> Review request for KDE Runtime, Teo Mrnjavac and Valentin Rusu.
> 
> 
> Repository: kde-runtime
> 
> 
> Description
> -------
> 
> 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.
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt 275a6c7 
>   cmake/modules/FindLibGcrypt.cmake PRE-CREATION 
>   kwalletd/backend/kwalletbackend.cc e4d461c 
>   kwalletd/backend/kwalletbackend.h 83ebf7f 
>   kwalletd/backend/backendpersisthandler.cpp bdef6ca 
>   kwalletd/backend/CMakeLists.txt 5a5837c 
> 
> Diff: https://git.reviewboard.kde.org/r/115497/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Àlex Fiestas
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20140209/d0f6f418/attachment.htm>