Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use PBKDF2 for user password hashing (2) #1422

Merged
merged 6 commits into from Dec 22, 2014

Conversation

@hacst
Copy link
Member

commented Oct 2, 2014

This pull request is a squashed version of tkmorris's work in #169 as well as a substantial review and refactoring commit hopefully resolving the remaining issues to get this merged. As this patchset changes highly critical authentication as well as database code please review and test carefully.

At this point I only did very rudimentary testing. Namely database creation, update from last version, PBKDF2 benchmark and iteration updates (with SuperUser only and no other users). I also verified that what ends up in the database matches the PBKDF2 hash returned by hashlib in python. This patch will require testing with actual real-life databases.

tkmorris and others added 2 commits Jul 29, 2013
Review and refactor of PBKDF2 support patch.
* Adjusted to coding guidelines
* Pulled out PBKDF2 functionality into own class
* Make benchmark a best of N approach with guaranteed minimum
* Fixed broken database migration code. Don't try to alter
  tables and instead rely on them being re-created with the
  new fields.
* Fixed some typos in ini. Also move to the setting to the
  end so ppl. don't get the idea they have to change this.
* Chose a scarier name for the plain hash function
* Use int instead of size_t for iteration counts as it is
  the datatype used in the OpenSSL API. Otherwise we just
  have to much pain with constantly converting and might
  expose ourselves to size issues in the future.
* Moved new UserInfo enum entry to the end as to preserve
  the order

@hacst hacst added the murmur label Oct 15, 2014

@hacst hacst added this to the 1.3.0 milestone Oct 15, 2014

@@ -63,6 +63,13 @@ class MetaParams {
int iMaxImageMessageLength;
int iOpusThreshold;
int iChannelNestingLimit;
/// If true the old SHA1 password hashing is used instead of PBKDF2
bool bPlainPasswordHash;

This comment has been minimized.

Copy link
@mkrautz

mkrautz Oct 19, 2014

Member

bLegacyPasswordHash to match the .ini file?

This comment has been minimized.

Copy link
@hacst

hacst Oct 19, 2014

Author Member

Good call. No idea why I didn't do that.

/// @param password Password to hash.
/// @param iterationCount Number of PBKDF2 iterations to apply.
/// @return Hex encoded password hash of DERIVED_KEY_LENGTH octets.
/// Null string if hashing failed.

This comment has been minimized.

Copy link
@mkrautz

mkrautz Oct 19, 2014

Member

Aborts the program if hashing failed?

Edit: What I mean is:

Null string if hashing failed -> Aborts the program if hashing failed


///
/// @return SALT_LENGTH octets of hex encoded random salt.
/// Null string if not enough entropy was available.

This comment has been minimized.

Copy link
@mkrautz

mkrautz Oct 19, 2014

Member

I'd hate to have this detail leak through our API (see http://www.2uo.de/myths-about-urandom/, etc.).

But it's also incorrect: we don't return a null QString. We call qFatal, which shuts down the process.

iterationCount,
EVP_sha384(),
DERIVED_KEY_LENGTH, reinterpret_cast<unsigned char*>(hash.data())) == 0 ) {
qFatal("PBKDF2: PKCS5_PBKDF2_HMAC failed: %s", ERR_error_string(ERR_get_error(), NULL));

This comment has been minimized.

Copy link
@mkrautz

mkrautz Oct 19, 2014

Member

I am fine with this being a qFatal as long as it's not something that can be triggered by a potential attacker. (That is., qFatal should only be triggered if we've made a programmer error.)

That seems to be the case according to https://www.openssl.org/docs/crypto/PKCS5_PBKDF2_HMAC.html

The source also seems to agree, I think. (There are a lot of failure cases for HMAC_CTX_copy and the like, but I think those fall under the above criteria...)

do {
iterations *= 2;

if(getHash(hexSalt, pseudopass, iterations).isNull()) {

This comment has been minimized.

Copy link
@mkrautz

mkrautz Oct 19, 2014

Member

If we qFatal in getHash on failure, this failure case shouldn't exist.

This comment has been minimized.

Copy link
@hacst

hacst Oct 19, 2014

Author Member

agreed

@@ -61,6 +61,7 @@ class ServerDB {
static QVariant getConf(int server_id, const QString &key, QVariant def = QVariant());
static void setConf(int server_id, const QString &key, const QVariant &value = QVariant());
static QList<LogRecord> getLog(int server_id, unsigned int offs_min, unsigned int offs_max);
static QString getLegacySHA1Hash(const QString pw);

This comment has been minimized.

Copy link
@mkrautz

mkrautz Oct 19, 2014

Member

const QString &pw?

query.addBindValue(srvnum);
query.addBindValue(0);
SQLEXEC();
}

QString ServerDB::getLegacySHA1Hash(const QString pw) {

This comment has been minimized.

Copy link
@mkrautz

mkrautz Oct 19, 2014

Member

const QString &pw?

@@ -240,7 +273,7 @@ ServerDB::ServerDB() {
SQLDO("CREATE UNIQUE INDEX `%1channel_info_id` ON `%1channel_info`(`server_id`, `channel_id`, `key`)");
SQLDO("CREATE TRIGGER `%1channel_info_del_channel` AFTER DELETE on `%1channels` FOR EACH ROW BEGIN DELETE FROM `%1channel_info` WHERE `channel_id` = old.`channel_id` AND `server_id` = old.`server_id`; END;");

SQLDO("CREATE TABLE `%1users` (`server_id` INTEGER NOT NULL, `user_id` INTEGER NOT NULL, `name` TEXT NOT NULL, `pw` TEXT, `lastchannel` INTEGER, `texture` BLOB, `last_active` DATE)");
SQLDO("CREATE TABLE `%1users` (`server_id` INTEGER NOT NULL, `user_id` INTEGER NOT NULL, `name` TEXT NOT NULL, `pw` TEXT, `salt` TEXT, `kdfmeter` INTEGER, `lastchannel` INTEGER, `texture` BLOB, `last_active` DATE)");

This comment has been minimized.

Copy link
@mkrautz

mkrautz Oct 19, 2014

Member

Shouldn't kdfmeter just be called kdfiter? That'd make it fit better in with our C++ naming.

(Also, s/kdfmeter/kdfiter/g?)

return maxIterations;
}

QString PBKDF2::getHash(const QString& hexSalt, const QString& password, int iterationCount) {

This comment has been minimized.

Copy link
@mkrautz

mkrautz Oct 19, 2014

Member

Coding style: Don't we still put the & where the * would be?

This comment has been minimized.

Copy link
@hacst

hacst Oct 21, 2014

Author Member

I don't think so. Do we? ;)

const QByteArray utf8Password = password.toUtf8();
const QByteArray salt = QByteArray::fromHex(hexSalt.toLatin1());

if( PKCS5_PBKDF2_HMAC(utf8Password.constData(), utf8Password.size(),

This comment has been minimized.

Copy link
@mkrautz

mkrautz Oct 19, 2014

Member

Coding style:

Missing space between if and first (.

Too many spaces inside the if statement? (Opening paren and closing paren should not have spaces after/before them).

if(! Meta::mp.bPlainPasswordHash) {
saltHash = PBKDF2::getSalt();
pwHash = PBKDF2::getHash(saltHash, pw, Meta::mp.kdfIterations);
}

This comment has been minimized.

Copy link
@mkrautz

mkrautz Oct 19, 2014

Member

Coding style.

} else {
kdfIterations = Meta::mp.kdfIterations;
if (info.contains(ServerDB::User_KDFIterations)) {
const int targetIterations = info.value(ServerDB::User_KDFIterations).toInt();

This comment has been minimized.

Copy link
@mkrautz

mkrautz Oct 19, 2014

Member

Drop 'const'?

This comment has been minimized.

Copy link
@hacst

hacst Oct 19, 2014

Author Member

Hm. Why? I don't plan on changing that variable --> const

QString passwordHash, salt;
int kdfIterations = -1;

if(Meta::mp.bPlainPasswordHash) {

This comment has been minimized.

Copy link
@mkrautz

mkrautz Oct 19, 2014

Member

Coding style.

query.addBindValue(iServerNum);
query.addBindValue(name);
SQLEXEC();
if (query.next()) {
const int userId = query.value(0).toInt();

This comment has been minimized.

Copy link
@mkrautz

mkrautz Oct 19, 2014

Member

Drop const?

This comment has been minimized.

Copy link
@hacst

hacst Oct 21, 2014

Author Member

Nope ;) I don't change it -> const

const int userId = query.value(0).toInt();
QString storedPasswordHash = query.value(2).toString();
QString storedSalt = query.value(3).toString();
const int storedKdfIterations = query.value(4).toInt();

This comment has been minimized.

Copy link
@mkrautz

mkrautz Oct 19, 2014

Member

Drop const?

This comment has been minimized.

Copy link
@hacst

hacst Oct 21, 2014

Author Member

Actually I'll add more const ;) Could even do const T& but I won't go there ;)

if (!Meta::mp.bPlainPasswordHash) {
if (Meta::mp.kdfIterations <= 0) {
// Configuration doesn't specify an override, load from db

This comment has been minimized.

Copy link
@mkrautz

mkrautz Oct 19, 2014

Member

Drop empty line?

This comment has been minimized.

Copy link
@hacst

hacst Oct 21, 2014

Author Member

But I like that empty line :( (basically because syntax highlight has the same color for comments and strings for me ;) I can remove it if it actually bothers you).

// A user has password authentication enabled if there is a password hash.

if (storedKdfIterations <= 0) {
// If storedmeter is <=0 this means this is an old-style SHA1 hash

This comment has been minimized.

Copy link
@mkrautz

mkrautz Oct 19, 2014

Member

Reference to 'storedmeter'. Should be storedKdfIterations, I guess!

@@ -172,6 +172,15 @@ users=100
# system.
#sendversion=True

# This sets password hash storage to legacy mode (1.2.4 and before)
# (Note that setting this to true is insecure and should not be used unless absolutely necessary)
#legacyPasswordwHash=false

This comment has been minimized.

Copy link
@hacst

hacst Oct 21, 2014

Author Member

Juck. I typoed. C&P'd to other places too.

@hacst

This comment has been minimized.

Copy link
Member Author

commented Oct 21, 2014

@mkrautz I tried to address everything you said. Only with the "const" I'm sticking to my "if I don't change it should better be const" mantra unless you can convince me otherwise ;) Also found a nasty typo and expanded some abbreviations.

QString PBKDF2::getSalt() {
QByteArray salt(SALT_LENGTH, 0);

if (RAND_bytes(reinterpret_cast<unsigned char*>(salt.data()), salt.size()) == 0) {

This comment has been minimized.

Copy link
@mkrautz

mkrautz Oct 21, 2014

Member

if ([...] <= 0) { or != 1

https://www.openssl.org/docs/crypto/RAND_bytes.html
RAND_bytes() returns 1 on success, 0 otherwise. The error code can be obtained by ERR_get_error(3). RAND_pseudo_bytes() returns 1 if the bytes generated are cryptographically strong, 0 otherwise. Both functions return -1 if they are not supported by the current RAND method.

This comment has been minimized.

Copy link
@hacst

hacst Oct 21, 2014

Author Member

And this is why I don't like C APIs...also I should've paid more attention to the documentation....

if(Meta::mp.legacyPasswordHash) {
// Downgrade the password to the legacy hash
QMap<int, QString> info;
info.insert(ServerDB::User_Password, password);

This comment has been minimized.

Copy link
@mkrautz

mkrautz Oct 21, 2014

Member

Doesn't this store the password in plain text?

Edit: Guess not, since it's used in the PBKDF2 part below as well. I'll keep trying to grok this.

Edit 2: I see. The setInfo() function wasn't visible to me while looking through the code.

Fix RAND_bytes return value checking.
That function can actually return -1 if it isn't
supported.
@mkrautz

This comment has been minimized.

Copy link
Member

commented Oct 21, 2014

This looks good to me now.

There are a few style issues still, though:

  • void hello(const QString &str); vs. void hello(const QString& str); -- this doesn't matter much to me (but we should be consistent).
  • } else { -- some places in the patch have a wrong style here. (else { part on a new line`)
  • if([...]) { should be if ([...]) {
@hacst

This comment has been minimized.

Copy link
Member Author

commented Oct 26, 2014

Any comments on the current revision?

@@ -0,0 +1,95 @@
/* Copyright (C) 2013, tkmorris <mauricioarozi@gmail.com>

This comment has been minimized.

Copy link
@mkrautz

mkrautz Oct 26, 2014

Member

Use his real name instead? I'm not a fan of having nicknames in the copyright lines or LICENSE file. (So I'm not a fan of having "Kissaki" or "snares" in there, instead of their real names either...)

[Removed. See tkmorris's comment below]

@tkmorris Is that OK with you?

(Should be fixed for PBKDF2.h as well)

This comment has been minimized.

Copy link
@tkmorris

tkmorris Oct 26, 2014

Contributor

'Morris Moraes' is fine to me.

@mkrautz

This comment has been minimized.

Copy link
Member

commented Oct 26, 2014

@hacst Took another look. The copyright line is my only gripe remaining. Once fixed, feel free to move to master.

@hacst

This comment has been minimized.

Copy link
Member Author

commented Oct 26, 2014

@mkrautz I would like to get some out-of-master testing in first.
@Natenom / @fwaggle : Anyone up for some testing? Basically up/downgrade with real(istic) user databases as well as checking it doesn't catch fire with MySQL. Ideally the test server would have some traffic as joining/leaving users upgrade/downgrade their passwords. This does change the database so backups are a good idea.

@hacst

This comment has been minimized.

Copy link
Member Author

commented Dec 6, 2014

Anyone opposed to merging this?

@mkrautz

This comment has been minimized.

Copy link
Member

commented Dec 6, 2014

I'm fine with merging it.

hacst added a commit that referenced this pull request Dec 22, 2014
Merge pull request #1422 from hacst/auth
Use PBKDF2 for user password hashing (2)

@hacst hacst merged commit 88cf21d into mumble-voip:master Dec 22, 2014

fwaggle added a commit to fwaggle/mumble that referenced this pull request Jul 8, 2015
unascribed added a commit to unascribed/mumble that referenced this pull request Apr 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.