----------------------------------------------------------- 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 > >
