Insecure Password Generation #338
Comments
|
This is probably what you want: /* Copyright (C) 2017 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved. */
#include <QString>
#include <QRandomGenerator>
quint32 boundedRandom(quint32 bound)
{
if (bound < 2)
return 0;
quint32 randval;
const quint32 max_mod_bound = (1 + ~bound) % bound;
do
randval = QRandomGenerator::system()->generate();
while (randval < max_mod_bound);
return randval % bound;
}
QString generateRandomPassword(const QString &charset, unsigned int length)
{
QString out;
for (unsigned int i = 0; i < length; ++i)
out.append(charset.at(boundedRandom(charset.length())));
return out;
}You may use this code under the current license of the project at the time of this comment, provided you retain the copyright header. |
|
Thank you for bringing this to our attention, and providing a proposed fix. This code looks completely reasonable, and indeed a lot more cryptographically sound. I'll look into integrating this tomorrow and writing a brief advisory on this issue to be included in the changelog and on the site. |
|
Glad to hear. One thing to keep in mind is that this raises the minimum Qt version to 5.10, which could make deployment a bit tricky for a little while. |
|
Yes, this makes it very hard in upstream linux distributions where in some cases they are having a hard time even switching from Qt 4.8 That's why I would love to include some fallback behaviour for QT 5 < 5.10 as-well. |
|
On Linux you can do something like this, though you may need to be careful in the case of threads: quint32 boundedRandom(quint32 bound)
{
static int fd = -1;
if (bound < 2)
return 0;
if (fd == -1)
assert((fd = open("/dev/urandom", O_RDONLY)) >= 0);
quint32 randval;
const quint32 max_mod_bound = (1 + ~bound) % bound;
do
assert(read(fd, &randval, sizeof(randval)) == sizeof(randval));
while (randval < max_mod_bound);
return randval % bound;
}On Windows you can use |
|
Windows is indeed easiest, but since I use appveyor for that, and it doesn't support Qt5.10 yet (opened a request) I'll have to build release exec and installer on a VM myself. The linux solution will probably work on macos as-well. Currently playing with it in some VMs . . |
|
This was reported 11 days ago. Code snippets are readily available in this report. As the maintainer of the parent project (pass), I'm going to start advising people stop using QtPass if we don't start to see some patches/PRs in the next few days. This is a serious vulnerability, and it's not responsible to simply let it sit here. |
|
I understand you frustration @zx2c4 IMHO the most important to do is place a PSA on https://QtPass.org which I have done now, also mentioned on twitter, will post something on the password store mailing list as-well and mention it in the changelog as soon as we have a version that works on most distro's that have a package. The reason for no progress so far is that unfortunately I haven't had any time to work on support for Qt5.9 and older which is needed for all the linux distro packages etc. I have ran a big amount of Angel shifts at 34c3, haven't touched my PC in over a week . . Working on including your proposed patches in the https://github.com/IJHack/QtPass/tree/insecure_password_generation branch. |
|
The version currently in Note, this has only been tested marginally. |
|
Tested on Linux (Debian, CentOS + Arch) with Qt 5.5, Qt 5.9 and Qt 5.10 Tested on MacOS (Qt 5.10) Testing on Windows now . . |
|
This has now been resolved https://github.com/IJHack/QtPass/releases/tag/v1.2.1 Updating site and sending out tweets warning people to update their passwords! |
|
Glad to hear. Please make it clear in your communications that this applies to the QtPass GUI only --- which is a separate project --- and not to pass. I'll be fairly upset if pass gets a bad name because of this incompetence here. |
|
Surely will, wouldn't want to have the rest of ecosystem be soiled by our crappy quality controll indeed! |
|
I just sent out a PSA to my pass list: https://lists.zx2c4.com/pipermail/password-store/2018-January/003165.html I put QtPass in quotation marks and consistently referred to it as third-party software, not to be a snarky jerk, but just to drive home that it's different software from pass, to prevent confusion. |
|
Could you please confirm that this doesn't affect cases where 'pwgen' - the default on linux I think - was used to generate the passwords. Thanks. |
|
I can confirm that when However |
|
@annejan Thanks for confirming! |
|
There is a problem with If I change the file as stated, i.e. Not sure if this is related to my setup or a general bug. EDIT: I tested this with the source files attached to the 1.2.1 release. |
|
Most probably a gcc/clang version issue. |
|
MITRE has assigned CVE-2017-18021 |
|
Posted to oss-security: http://www.openwall.com/lists/oss-security/2018/01/05/5 |
Yes, this is a known parsing problem in C++. It is present in C++98 and C++03. I think it was fixed in C++11. The trailing Cross platform software should always add the extra space as you found: We have a library full of the fix because we support C++03 through C++17. |
|
That has been fixed in #346 thank you for the explanation. |
The current way of generating passwords is insecure. All passwords that have been generated with QtPass in the past must be regenerated and changed.
Here is the current password generation function:
The problem here is that modulo will not uniformly distribute that set. The proper way to do things is to just throw away values that are out of bounds. You could try to do the calculation correctly to uniformly stretch or compress, but it's hard to get right, so it's best to just discard numbers outside the set and try again.
Secondly, and more critically, here is the implementation of
Util::rand:Unfortunately, using a non-cryptographically secure random number generator like libc's
rand()is problematic -- future outputs can be derived from knowing only a handful of past outputs -- and seeding that deterministic rng withcurrentTime()->msec()is even more dangerous. Not only is the current time a guessable/bruteforcable parameter, but the documentation forQTime::msecactually indicates that this is merely the "the millisecond part (0 to 999) of the time", which means there are only 1000 possibilities of generated sequences of passwords.This is as bad as it gets, in terms of password manager password generation.
The proper fix is to use Qt 5.10's
QRandomGenerator::system(). If 5.10 is not available to you, use/dev/urandomon Mac/Linux andRtlGenRandomon Windows.The password generator inside pass(1) itself is implemented like this:
The text was updated successfully, but these errors were encountered: