Do password hashing properly

[Chocobo1]

Some notes:

• openssl >= 1.0 is now a required dependency.
• This PR makes stored password hash very much harder to crack compared to previously used method.
  This basically means even if bad actor acquired your password hash, he won't be able to easily compute your real password (assuming the real password is good enough for brute force attack).
• UI changes: GUI, WebUI
• PBKDF2 is chosen because qbt already uses openssl which saves us from adding another library, and also it is the only method openssl implemented AFAIK.
• PBKDF2 parameters are chosen to be in line with 1password settings: https://support.1password.com/pbkdf2/
• I've not provide mitigation code yet, that is, user need to change the password from the default one after upgrading, is this really required?

[glassez]

@Chocobo1, why don't you use QCryptographicHash just with more complex algorithm (SHA512)?

[Chocobo1]

@Chocobo1, why don't you use QCryptographicHash just with more complex algorithm (SHA512)?

It is too weak for this purpose, you may look up "Key stretching".

[Chocobo1]

PR updated: resolved all comments.

[glassez]

LGTM. Untested though.

[sledgehammer999]

IMO, PR scope looks good. But I wonder if we should do more for the nox users. I assume they have set it up to run as a daemon and may not realize the daemon doesn't run due to this change.

Or maybe target this for 4.2.0?

Any thoughts?

[Chocobo1]

I assume they have set it up to run as a daemon and may not realize the daemon doesn't run due to this change.

The mitigation action will look like this:

// pseudo code

void AuthController::loginAction()
{
  const bool passwordEqual = Utils::Password::PBKDF2::verify(secret, passwordFromWeb);
  if (!passwordEqual) {
    const bool oldPasswordEqual = // load previous webui password and verify it using previous method
    if (oldPasswordEqual)
      // update webui password using new method
    else
      // authentication failed
  }
}

While it is convenient for WebAPI users, we probably need to keep this mitigation code forever...
Or we can put out a warning in the News section? Stating that we are hardening this part and users needs to update their password from the default one.

Or maybe target this for 4.2.0?

Should be better than breaking WebAPI users in a qbt minor release, this PR is still eligible for merging into master right?

[sledgehammer999]

I would have no problem if this goes into master. I just hope it won't make backporting other commits harder.

[Chocobo1]

PR updated: resolved conflicts in configure file.

[LordNyriox]

@Chocobo1:

openssl >= 1.0 is now a required dependency.

Can you please add the OpenSSL version to the About –> Libraries GUI?

It has bugged me forever that I can never tell what OpenSSL version qBittorrent was built on (without asking someone).

[sledgehammer999]

@Chocobo1 I don't know the level of paranoid we should be about this but just in case:

1. Variable number of iterations: Have the base iteration number (100.000) and add a random number between 1 and 20.000. Of course save this info along with the salt too
2. Use scrypt/argon2 instead of PBKDF2

Just suggestions. They are not required for merging this.

[Chocobo1]

Can you please add the OpenSSL version to the About –> Libraries GUI?

OK, but will do it in another PR.

Variable number of iterations: Have the base iteration number (100.000) and add a random number between 1 and 20.000. Of course save this info along with the salt too

I don't think this is the correct way (or any better), since we already use a lengthy salt that is infeasible for the hacker to pre-compute, and besides (in the extreme case) the hacker can just use 100.000 as a start and iterate all the way up to 120.000.

Use scrypt/argon2 instead of PBKDF2

We can but we need to find & include another library, using openssl is the cheapest in this matter :P

[Chocobo1]

Thanks for the review!

[WolfganP]

While compiling from master, I'm seeing a linking error now:

linking qbittorrent-nox

/usr/bin/ld: password.o: undefined reference to symbol 'PKCS5_PBKDF2_HMAC@@OPENSSL_1_1_0'

//usr/lib/arm-linux-gnueabihf/libcrypto.so.1.1: error adding symbols: DSO missing from command line

collect2: error: ld returned 1 exit status

Makefile:576: recipe for target 'qbittorrent-nox' failed

make[1]: *** [qbittorrent-nox] Error 1

make[1]: Leaving directory '/mnt/usb_12/wTools/qBittorrent/src'

Makefile:42: recipe for target 'sub-src-make_first' failed

make: *** [sub-src-make_first] Error 2

[thalieht]

A ./configure solved it for me.

[WolfganP]

A ./configure solved it for me.

@thalieht Thx, it worked for me as well.

[Piccirello]

Could you update the WebAPI documentation for get/set preferences with this change? It still talks about md5 hashes.

[Chocobo1]

Could you update the WebAPI documentation for get/set preferences with this change? It still talks about md5 hashes.

Done, I presume it will start taking effect in API v.2.3.0.

[sledgehammer999]

API v.2.3.0.

Whatever the API will be in v4.2.0. This wasn't backported to the v4.1.x series.