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
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 9 additions & 0 deletions scripts/murmur.ini
Expand Up @@ -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)
#legacyPasswordHash=false

# By default a strong amount of PBKDF2 iterations are chosen automatically. If >0 this setting
# overrides the automatic benchmark and forces a specific number of iterations.
# (Note that you should only change this value if you know what you are doing)
#kdfIterations=-1

# You can configure any of the configuration options for Ice here. We recommend
# leave the defaults as they are.
# Please note that this section has to be last in the configuration file.
Expand Down
6 changes: 6 additions & 0 deletions src/murmur/Meta.cpp
Expand Up @@ -54,6 +54,8 @@ MetaParams::MetaParams() {
iMaxUsersPerChannel = 0;
iMaxTextMessageLength = 5000;
iMaxImageMessageLength = 131072;
legacyPasswordHash = false;
kdfIterations = -1;
bAllowHTML = true;
iDefaultChan = 0;
bRememberChan = true;
Expand Down Expand Up @@ -262,6 +264,8 @@ void MetaParams::read(QString fname) {
iTimeout = typeCheckedFromSettings("timeout", iTimeout);
iMaxTextMessageLength = typeCheckedFromSettings("textmessagelength", iMaxTextMessageLength);
iMaxImageMessageLength = typeCheckedFromSettings("imagemessagelength", iMaxImageMessageLength);
legacyPasswordHash = typeCheckedFromSettings("legacypasswordhash", legacyPasswordHash);
kdfIterations = typeCheckedFromSettings("kdfiterations", -1);
bAllowHTML = typeCheckedFromSettings("allowhtml", bAllowHTML);
iMaxBandwidth = typeCheckedFromSettings("bandwidth", iMaxBandwidth);
iDefaultChan = typeCheckedFromSettings("defaultchannel", iDefaultChan);
Expand Down Expand Up @@ -458,6 +462,8 @@ void MetaParams::read(QString fname) {
qmConfig.insert(QLatin1String("port"),QString::number(usPort));
qmConfig.insert(QLatin1String("timeout"),QString::number(iTimeout));
qmConfig.insert(QLatin1String("textmessagelength"), QString::number(iMaxTextMessageLength));
qmConfig.insert(QLatin1String("legacypasswordhash"), legacyPasswordHash ? QLatin1String("true") : QLatin1String("false"));
qmConfig.insert(QLatin1String("kdfiterations"), QString::number(kdfIterations));
qmConfig.insert(QLatin1String("allowhtml"), bAllowHTML ? QLatin1String("true") : QLatin1String("false"));
qmConfig.insert(QLatin1String("bandwidth"),QString::number(iMaxBandwidth));
qmConfig.insert(QLatin1String("users"),QString::number(iMaxUsers));
Expand Down
7 changes: 7 additions & 0 deletions src/murmur/Meta.h
Expand Up @@ -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 legacyPasswordHash;
/// Contains the default number of PBKDF2 iterations to use
/// when hashing passwords. If the value loaded from config
/// is <= 0 the value is loaded from the database and if not
/// available there yet found by a benchmark.
int kdfIterations;
bool bAllowHTML;
QString qsPassword;
QString qsWelcomeText;
Expand Down
95 changes: 95 additions & 0 deletions src/murmur/PBKDF2.cpp
@@ -0,0 +1,95 @@
/* Copyright (C) 2013, tkmorris <mauricioarozi@gmail.com>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'Morris Moraes' is fine to me.

Copyright (C) 2014, Stefan Hacker <dd0t@users.sourceforge.net>

All rights reserved.

Redistribution and use in source and binary forms, with or without
modification, are permitted provided that the following conditions
are met:

- Redistributions of source code must retain the above copyright notice,
this list of conditions and the following disclaimer.
- Redistributions in binary form must reproduce the above copyright notice,
this list of conditions and the following disclaimer in the documentation
and/or other materials provided with the distribution.
- Neither the name of the Mumble Developers nor the names of its
contributors may be used to endorse or promote products derived from this
software without specific prior written permission.

THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
``AS IS'' AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE FOUNDATION OR
CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*/
#include "murmur_pch.h"

#include "PBKDF2.h"

int PBKDF2::benchmark() {
const QString pseudopass(QLatin1String("aboutAvg"));
const QString hexSalt = getSalt(); // Could tolerate not getting a salt here, will likely only make it harder.

int maxIterations = -1;

QElapsedTimer timer;
timer.start();

for (size_t i = 0; i < BENCHMARK_N; ++i) {
int iterations = BENCHMARK_MINIMUM_ITERATION_COUNT / 2;

timer.restart();
do {
iterations *= 2;

// Store return value in a volatile to prevent optimizer
// from ever removing these side-effect-free calls. I don't
// think the compiler can prove they have no side-effects but
// better safe than sorry.
volatile QString result = getHash(hexSalt, pseudopass, iterations);
Q_UNUSED(result);

} while (timer.restart() < BENCHMARK_DURATION_TARGET_IN_MS && (iterations / 2) < std::numeric_limits<int>::max());

if (iterations > maxIterations) {
maxIterations = iterations;
}
}
return maxIterations;
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

QByteArray hash(DERIVED_KEY_LENGTH, 0);

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

if (PKCS5_PBKDF2_HMAC(utf8Password.constData(), utf8Password.size(),
reinterpret_cast<const unsigned char*>(salt.constData()), salt.size(),
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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return QString();
}

return hash.toHex();
}


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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

qFatal("PBKDF2: RAND_bytes for salt failed: %s", ERR_error_string(ERR_get_error(), NULL));
return QString();
}

return salt.toHex();
}
85 changes: 85 additions & 0 deletions src/murmur/PBKDF2.h
@@ -0,0 +1,85 @@
/* Copyright (C) 2013, tkmorris <mauricioarozi@gmail.com>
Copyright (C) 2014, Stefan Hacker <dd0t@users.sourceforge.net>

All rights reserved.

Redistribution and use in source and binary forms, with or without
modification, are permitted provided that the following conditions
are met:

- Redistributions of source code must retain the above copyright notice,
this list of conditions and the following disclaimer.
- Redistributions in binary form must reproduce the above copyright notice,
this list of conditions and the following disclaimer in the documentation
and/or other materials provided with the distribution.
- Neither the name of the Mumble Developers nor the names of its
contributors may be used to endorse or promote products derived from this
software without specific prior written permission.

THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
``AS IS'' AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE FOUNDATION OR
CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*/
#ifndef MUMBLE_MURMUR_PBKDF2_H_
#define MUMBLE_MURMUR_PBKDF2_H_

#include <stddef.h>

class QString;

///
/// Fully static wrapper class for PBKF2 password hashing functionality used in Murmur.
///
/// @note Using int all over the place because OpenSSL uses them in its C interface
/// and we want to make sure not to parameterize it in unexpected ways.
/// @warning Operations in this class that experience internal failure will abort
/// program execution using qFatal.
///
class PBKDF2 {
public:
///
/// @return Upper bound on iterations possible in
/// BENCHMARK_DURATION_TARGET_IN_MS on this machine.
///
static int benchmark();

/// Performs a PBKDF2 hash operation using EVP_sha384.
///
/// @param hexSalt Hex encoded salt to use in operation.
/// @param password Password to hash.
/// @param iterationCount Number of PBKDF2 iterations to apply.
/// @return Hex encoded password hash of DERIVED_KEY_LENGTH octets.
///
static QString getHash(const QString& hexSalt,
const QString& password,
int iterationCount);

///
/// @return SALT_LENGTH octets of hex encoded random salt.
///
static QString getSalt();

/// Length of hash in octets
static const int DERIVED_KEY_LENGTH = 48;
/// Length salt in octests
static const int SALT_LENGTH = 8;

/// Duration for hash operation the benchmark function should target
static const int BENCHMARK_DURATION_TARGET_IN_MS = 10;
/// Benchmark returns highest iteration number of N benchmark attemps
static const size_t BENCHMARK_N = 40;
/// Lower bound of iteration count returned by benchmark
/// regardless of duration (should be divisible by 2)
static const int BENCHMARK_MINIMUM_ITERATION_COUNT = 1000;

};

#endif // MUMBLE_MURMUR_PBKDF2_H_