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

[Àlex Fiestas]

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

(Updated Feb. 13, 2014, 2:31 p.m.)


Status
--

This change has been marked as submitted.


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/CMakeLists.txt 5a5837c 
  kwalletd/backend/backendpersisthandler.cpp bdef6ca 
  kwalletd/backend/kwalletbackend.h 83ebf7f 
  kwalletd/backend/kwalletbackend.cc e4d461c 

Diff: https://git.reviewboard.kde.org/r/115497/diff/


Testing
---


Thanks,

Àlex Fiestas

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

[Commit Hook]

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


This review has been submitted with commit 
c546c2517edc517eabdb8750e625964657152767 by Àlex Fiestas to branch master.

- Commit Hook


On Feb. 11, 2014, 9:46 a.m., Àlex Fiestas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115497/
> ---
> 
> (Updated Feb. 11, 2014, 9:46 a.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/CMakeLists.txt 5a5837c 
>   kwalletd/backend/backendpersisthandler.cpp bdef6ca 
>   kwalletd/backend/kwalletbackend.h 83ebf7f 
>   kwalletd/backend/kwalletbackend.cc e4d461c 
> 
> Diff: https://git.reviewboard.kde.org/r/115497/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Àlex Fiestas
> 
>

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

[Àlex Fiestas]

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

(Updated Feb. 11, 2014, 9:46 a.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 (updated)
-

  CMakeLists.txt 275a6c7 
  cmake/modules/FindLibGcrypt.cmake PRE-CREATION 
  kwalletd/backend/CMakeLists.txt 5a5837c 
  kwalletd/backend/backendpersisthandler.cpp bdef6ca 
  kwalletd/backend/kwalletbackend.h 83ebf7f 
  kwalletd/backend/kwalletbackend.cc e4d461c 

Diff: https://git.reviewboard.kde.org/r/115497/diff/


Testing
---


Thanks,

Àlex Fiestas

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

[Valentin Rusu]

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

Ship it!


One the issue around minor version check is done, feel free to commit it!

- Valentin Rusu


On Feb. 10, 2014, 5:43 p.m., Àlex Fiestas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115497/
> ---
> 
> (Updated Feb. 10, 2014, 5:43 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/CMakeLists.txt 5a5837c 
>   kwalletd/backend/backendpersisthandler.cpp bdef6ca 
>   kwalletd/backend/kwalletbackend.h 83ebf7f 
>   kwalletd/backend/kwalletbackend.cc e4d461c 
> 
> Diff: https://git.reviewboard.kde.org/r/115497/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Àlex Fiestas
> 
>

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

[Valentin Rusu]

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



kwalletd/backend/kwalletbackend.cc


The error logic should be more strict here, I think. In fact we loose the 
"unknown version" return code.


- Valentin Rusu


On Feb. 10, 2014, 5:43 p.m., Àlex Fiestas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115497/
> ---
> 
> (Updated Feb. 10, 2014, 5:43 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/CMakeLists.txt 5a5837c 
>   kwalletd/backend/backendpersisthandler.cpp bdef6ca 
>   kwalletd/backend/kwalletbackend.h 83ebf7f 
>   kwalletd/backend/kwalletbackend.cc e4d461c 
> 
> Diff: https://git.reviewboard.kde.org/r/115497/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Àlex Fiestas
> 
>

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

[Valentin Rusu]

> On Feb. 5, 2014, 7:18 p.m., Michael Pyne wrote:
> > kwalletd/backend/kwalletbackend.cc, line 635
> > 
> >
> > 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)?
> 
> Àlex Fiestas wrote:
> Added a runtime check to decide if we shuold swap or not the hashes, also 
> checking it in BlowfishPersistHandler::write.
> 
> This will fix the following usecase:
> -KWallet uses SHA1 to read
> -KWallet uses PBKDF2 to write, but BKDF2 hash is null
> 
> For other cases we can't add much fallback since Backend::setPassword 
> returns void, and no other code using it checks for errors in anyway.

feel free to add a return value to it, if that's needed.


- Valentin


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


On Feb. 10, 2014, 5:43 p.m., Àlex Fiestas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115497/
> ---
> 
> (Updated Feb. 10, 2014, 5:43 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/CMakeLists.txt 5a5837c 
>   kwalletd/backend/backendpersisthandler.cpp bdef6ca 
>   kwalletd/backend/kwalletbackend.h 83ebf7f 
>   kwalletd/backend/kwalletbackend.cc e4d461c 
> 
> Diff: https://git.reviewboard.kde.org/r/115497/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Àlex Fiestas
> 
>

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

[Valentin Rusu]

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


- Valentin Rusu


On Feb. 10, 2014, 5:43 p.m., Àlex Fiestas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115497/
> ---
> 
> (Updated Feb. 10, 2014, 5:43 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/CMakeLists.txt 5a5837c 
>   kwalletd/backend/backendpersisthandler.cpp bdef6ca 
>   kwalletd/backend/kwalletbackend.h 83ebf7f 
>   kwalletd/backend/kwalletbackend.cc e4d461c 
> 
> Diff: https://git.reviewboard.kde.org/r/115497/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Àlex Fiestas
> 
>

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

[Àlex Fiestas]

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

(Updated Feb. 10, 2014, 5:43 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 (updated)
-

  CMakeLists.txt 275a6c7 
  cmake/modules/FindLibGcrypt.cmake PRE-CREATION 
  kwalletd/backend/CMakeLists.txt 5a5837c 
  kwalletd/backend/backendpersisthandler.cpp bdef6ca 
  kwalletd/backend/kwalletbackend.h 83ebf7f 
  kwalletd/backend/kwalletbackend.cc e4d461c 

Diff: https://git.reviewboard.kde.org/r/115497/diff/


Testing
---


Thanks,

Àlex Fiestas

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

[Michael Pyne]

> On Feb. 7, 2014, 5:14 a.m., Michael Pyne wrote:
> > kwalletd/backend/kwalletbackend.cc, line 387
> > 
> >
> > Again, might want to add error-checking here. If the salt can't be 
> > saved for whatever reason then we don't want to destroy an existing 
> > old-style wallet by mistake.
> > 
> > It looks like it would be as simple as returning an empty QByteArray if 
> > an error is detected.
> 
> Àlex Fiestas wrote:
> The check is done in Backend::setPassword, if the resulting salt is empty 
> then we do not use the new hash.
> 
> In Backend::createAndSaveSalt I'm not sure I can add any extra check 
> besides the return of QIODevice::write, but if that files you have bigger 
> problems... Also from what I see in gcrypt code gcry_random_bytes can't 
> return null, it doesn't seem to have any error reporting.

QIODevice::write was the thing I was thinking of.

QIODevice::close() too, although it turns out it has no way of returning an 
error if one occurs. :(

You're right that this is a pretty minor edge case, I only brought it up 
because of how important the data in question is.


- Michael


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


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

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


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


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

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

[Àlex Fiestas]

---
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 (updated)
-

  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

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

[Àlex Fiestas]

> On Feb. 7, 2014, 5:14 a.m., Michael Pyne wrote:
> > kwalletd/backend/kwalletbackend.cc, line 387
> > 
> >
> > Again, might want to add error-checking here. If the salt can't be 
> > saved for whatever reason then we don't want to destroy an existing 
> > old-style wallet by mistake.
> > 
> > It looks like it would be as simple as returning an empty QByteArray if 
> > an error is detected.

The check is done in Backend::setPassword, if the resulting salt is empty then 
we do not use the new hash.

In Backend::createAndSaveSalt I'm not sure I can add any extra check besides 
the return of QIODevice::write, but if that files you have bigger problems... 
Also from what I see in gcrypt code gcry_random_bytes can't return null, it 
doesn't seem to have any error reporting.


- Àlex


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


On Feb. 6, 2014, 3:28 p.m., Àlex Fiestas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115497/
> ---
> 
> (Updated Feb. 6, 2014, 3:28 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/CMakeLists.txt 5a5837c 
>   kwalletd/backend/backendpersisthandler.cpp bdef6ca 
>   kwalletd/backend/kwalletbackend.h 83ebf7f 
>   kwalletd/backend/kwalletbackend.cc e4d461c 
> 
> Diff: https://git.reviewboard.kde.org/r/115497/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Àlex Fiestas
> 
>

Re: 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/#review49163
---


A couple of minor things but definitely looks a lot better. Hopefully the 
kwallet integration review can be done soon by someone knowledgeable with 
kwallet but I'd run the code myself at this point. ;)


kwalletd/backend/kwalletbackend.cc


I didn't see specifics in libgcrypt's documentation but surely this memory 
needs to be freed (I'm assuming by the stdlib ::free()) after the QByteArray is 
constructed.



kwalletd/backend/kwalletbackend.cc


Again, might want to add error-checking here. If the salt can't be saved 
for whatever reason then we don't want to destroy an existing old-style wallet 
by mistake.

It looks like it would be as simple as returning an empty QByteArray if an 
error is detected.


- Michael Pyne


On Feb. 6, 2014, 3:28 p.m., Àlex Fiestas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115497/
> ---
> 
> (Updated Feb. 6, 2014, 3:28 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/CMakeLists.txt 5a5837c 
>   kwalletd/backend/backendpersisthandler.cpp bdef6ca 
>   kwalletd/backend/kwalletbackend.h 83ebf7f 
>   kwalletd/backend/kwalletbackend.cc e4d461c 
> 
> Diff: https://git.reviewboard.kde.org/r/115497/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Àlex Fiestas
> 
>

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

[Michael Pyne]

> On Feb. 5, 2014, 7:18 p.m., Michael Pyne wrote:
> > kwalletd/backend/kwalletbackend.cc, line 129
> > 
> >
> > 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.
> 
> Àlex Fiestas wrote:
> The reason why I went with PBKDF2 instead of SCRYPT is because the later 
> seems less established than the first, lacking a lot of user based feedback 
> like for example a recommended amount of iterations.
> 
> So, to what should I change it to? CRYPT with 200 iterations?
> 
> 200 on my workstation take around 500ms.

CRYPT is even worse :), at least if it's talking about UNIX crypt (which uses 
iterated DES and has very short bitlength).

There's nothing wrong with PBKDF2 (scrypt even uses it internally), the big 
difference is that scrypt tries to consume memory in addition to CPU to try to 
make it harder to bruteforce. It's not perfect at this either though (GPU is 
still the best way to crack scrypt, as the Litecoin devs found out). I'd leave 
the choice up to you as I'm not qualified to give professional crypto advice so 
if it is easier to find good guidance on how to securely use PBKDF2 I'd leave 
it at that.

I'll look at the new diff but would still want valir or an actual maintainer to 
give the "Ship It!", the crypto part only happened to catch my eye from 
kde-core-devel. ;)


- Michael


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


On Feb. 6, 2014, 3:28 p.m., Àlex Fiestas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115497/
> ---
> 
> (Updated Feb. 6, 2014, 3:28 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/CMakeLists.txt 5a5837c 
>   kwalletd/backend/backendpersisthandler.cpp bdef6ca 
>   kwalletd/backend/kwalletbackend.h 83ebf7f 
>   kwalletd/backend/kwalletbackend.cc e4d461c 
> 
> Diff: https://git.reviewboard.kde.org/r/115497/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Àlex Fiestas
> 
>

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

[Àlex Fiestas]

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

(Updated Feb. 6, 2014, 3:28 p.m.)


Review request for KDE Runtime, Teo Mrnjavac and Valentin Rusu.


Changes
---

Fixed all issues, added generated salt.

Still I haven't changed it to SCRYPT until I get some more feedback.


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 (updated)
-

  CMakeLists.txt 275a6c7 
  cmake/modules/FindLibGcrypt.cmake PRE-CREATION 
  kwalletd/backend/CMakeLists.txt 5a5837c 
  kwalletd/backend/backendpersisthandler.cpp bdef6ca 
  kwalletd/backend/kwalletbackend.h 83ebf7f 
  kwalletd/backend/kwalletbackend.cc e4d461c 

Diff: https://git.reviewboard.kde.org/r/115497/diff/


Testing
---


Thanks,

Àlex Fiestas

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

[Àlex Fiestas]

> On Feb. 5, 2014, 7:18 p.m., Michael Pyne wrote:
> > kwalletd/backend/kwalletbackend.cc, line 130
> > 
> >
> > 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.
> 
> Àlex Fiestas wrote:
> I decided to use the username because this hash will be produced from a 
> pam module as well, accessing users data from there is kind of messy and we 
> will have to generate and then chown that salt so I decided to go with a 
> shortcut for the time being.
> 
> Will implement proper salt in a future patch if that's ok for you.

Actually that will force me to update again the hash which is messy, so going 
to write that code right now.


- Àlex


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


On Feb. 5, 2014, 3:10 p.m., Àlex Fiestas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115497/
> ---
> 
> (Updated Feb. 5, 2014, 3:10 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/CMakeLists.txt 5a5837c 
>   kwalletd/backend/backendpersisthandler.cpp bdef6ca 
>   kwalletd/backend/kwalletbackend.h 83ebf7f 
>   kwalletd/backend/kwalletbackend.cc e4d461c 
> 
> Diff: https://git.reviewboard.kde.org/r/115497/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Àlex Fiestas
> 
>

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

[Àlex Fiestas]

> On Feb. 5, 2014, 7:18 p.m., Michael Pyne wrote:
> > kwalletd/backend/kwalletbackend.cc, line 635
> > 
> >
> > 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)?

Added a runtime check to decide if we shuold swap or not the hashes, also 
checking it in BlowfishPersistHandler::write.

This will fix the following usecase:
-KWallet uses SHA1 to read
-KWallet uses PBKDF2 to write, but BKDF2 hash is null

For other cases we can't add much fallback since Backend::setPassword returns 
void, and no other code using it checks for errors in anyway.


- Àlex


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


On Feb. 5, 2014, 3:10 p.m., Àlex Fiestas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115497/
> ---
> 
> (Updated Feb. 5, 2014, 3:10 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/CMakeLists.txt 5a5837c 
>   kwalletd/backend/backendpersisthandler.cpp bdef6ca 
>   kwalletd/backend/kwalletbackend.h 83ebf7f 
>   kwalletd/backend/kwalletbackend.cc e4d461c 
> 
> Diff: https://git.reviewboard.kde.org/r/115497/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Àlex Fiestas
> 
>

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

[Àlex Fiestas]

> On Feb. 5, 2014, 7:18 p.m., Michael Pyne wrote:
> > kwalletd/backend/kwalletbackend.cc, line 130
> > 
> >
> > 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.

I decided to use the username because this hash will be produced from a pam 
module as well, accessing users data from there is kind of messy and we will 
have to generate and then chown that salt so I decided to go with a shortcut 
for the time being.

Will implement proper salt in a future patch if that's ok for you.


> On Feb. 5, 2014, 7:18 p.m., Michael Pyne wrote:
> > kwalletd/backend/kwalletbackend.cc, line 129
> > 
> >
> > 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.

The reason why I went with PBKDF2 instead of SCRYPT is because the later seems 
less established than the first, lacking a lot of user based feedback like for 
example a recommended amount of iterations.

So, to what should I change it to? CRYPT with 200 iterations?

200 on my workstation take around 500ms.


- Àlex


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


On Feb. 5, 2014, 3:10 p.m., Àlex Fiestas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115497/
> ---
> 
> (Updated Feb. 5, 2014, 3:10 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/CMakeLists.txt 5a5837c 
>   kwalletd/backend/backendpersisthandler.cpp bdef6ca 
>   kwalletd/backend/kwalletbackend.h 83ebf7f 
>   kwalletd/backend/kwalletbackend.cc e4d461c 
> 
> Diff: https://git.reviewboard.kde.org/r/115497/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Àlex Fiestas
> 
>

Re: 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/#review49065
---


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


kwalletd/backend/kwalletbackend.cc


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.



kwalletd/backend/kwalletbackend.cc


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.



kwalletd/backend/kwalletbackend.cc


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


- Michael Pyne


On Feb. 5, 2014, 3:10 p.m., Àlex Fiestas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115497/
> ---
> 
> (Updated Feb. 5, 2014, 3:10 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/CMakeLists.txt 5a5837c 
>   kwalletd/backend/backendpersisthandler.cpp bdef6ca 
>   kwalletd/backend/kwalletbackend.h 83ebf7f 
>   kwalletd/backend/kwalletbackend.cc e4d461c 
> 
> Diff: https://git.reviewboard.kde.org/r/115497/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Àlex Fiestas
> 
>

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

[Richard Moore]

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



kwalletd/backend/kwalletbackend.cc


passwrod -> password



kwalletd/backend/kwalletbackend.cc


Since you seem to be returning the gpg_error_t directly later, why not use 
something like GPG_ERR_USER_1?



kwalletd/backend/kwalletbackend.cc


Consider breaking out the number of rounds and key length as constants for 
more readable code


- Richard Moore


On Feb. 5, 2014, 3:10 p.m., Àlex Fiestas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115497/
> ---
> 
> (Updated Feb. 5, 2014, 3:10 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/CMakeLists.txt 5a5837c 
>   kwalletd/backend/backendpersisthandler.cpp bdef6ca 
>   kwalletd/backend/kwalletbackend.h 83ebf7f 
>   kwalletd/backend/kwalletbackend.cc e4d461c 
> 
> Diff: https://git.reviewboard.kde.org/r/115497/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Àlex Fiestas
> 
>