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

Insecure Password Generation #338

Closed
zx2c4 opened this issue Dec 22, 2017 · 23 comments
Closed

Insecure Password Generation #338

zx2c4 opened this issue Dec 22, 2017 · 23 comments
Assignees
Labels

Comments

@zx2c4
Copy link

zx2c4 commented Dec 22, 2017

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:

      for (int i = 0; i < length; ++i) {
        int index = Util::rand() % charset.length();
        QChar nextChar = charset.at(index);
        passwd.append(nextChar);
      }

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:

...
  qsrand(static_cast<uint>(QTime::currentTime().msec()));
...
int Util::rand() {
#ifdef Q_OS_WIN
  quint32 ret = 0;
  if (FAILED(BCryptGenRandom(NULL, (PUCHAR)&ret, sizeof(ret),
                             BCRYPT_USE_SYSTEM_PREFERRED_RNG)))
    return qrand();
  return ret % RAND_MAX;
#else
  return qrand();
#endif
}

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 with currentTime()->msec() is even more dangerous. Not only is the current time a guessable/bruteforcable parameter, but the documentation for QTime::msec actually 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/urandom on Mac/Linux and RtlGenRandom on Windows.


The password generator inside pass(1) itself is implemented like this:

read -r -n $length pass < <(LC_ALL=C tr -dc "$characters" < /dev/urandom)
  • uses /dev/urandom
  • discards characters that aren't part of the set
@zx2c4
Copy link
Author

zx2c4 commented Dec 23, 2017

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.

@annejan
Copy link
Member

annejan commented Dec 27, 2017

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.

@annejan annejan self-assigned this Dec 27, 2017
@annejan annejan added the bug label Dec 27, 2017
@zx2c4
Copy link
Author

zx2c4 commented Dec 27, 2017

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.

@annejan
Copy link
Member

annejan commented Dec 27, 2017

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.
Since raising to 5.10 might hinder adoption.

@zx2c4
Copy link
Author

zx2c4 commented Dec 27, 2017

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 RtlGenRandom, but you can probably also just control the Qt version there, so don't bother.

@annejan
Copy link
Member

annejan commented Dec 27, 2017

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.
Good thing is I don't know of any (or at-least many) people doing their own windows builds, other than getting a VM up and installing there should be no more work than issuing a build time error, which might even get us a Windows maintainer if complaints come in ;-)

The linux solution will probably work on macos as-well.

Currently playing with it in some VMs . .

@zx2c4
Copy link
Author

zx2c4 commented Jan 2, 2018

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.

annejan added a commit that referenced this issue Jan 2, 2018
@annejan
Copy link
Member

annejan commented Jan 2, 2018

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.

@annejan
Copy link
Member

annejan commented Jan 2, 2018

The version currently in master should work on everything but QT 5.9 and older on Windows.

Note, this has only been tested marginally.

@annejan
Copy link
Member

annejan commented Jan 4, 2018

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

@annejan
Copy link
Member

annejan commented Jan 4, 2018

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!

@annejan annejan closed this as completed Jan 4, 2018
@zx2c4
Copy link
Author

zx2c4 commented Jan 4, 2018

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.

@annejan
Copy link
Member

annejan commented Jan 4, 2018

Surely will, wouldn't want to have the rest of ecosystem be soiled by our crappy quality controll indeed!

@zx2c4
Copy link
Author

zx2c4 commented Jan 4, 2018

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.

@innir
Copy link
Contributor

innir commented Jan 4, 2018

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.

@annejan
Copy link
Member

annejan commented Jan 4, 2018

I can confirm that when pwgen is used, which is the default on linux, the fallback generator was not used.
When pass is available, which is default on Debian and most other Linux distribution, the fallback generator was not used.

However pwgen has it's own downsides too, but that us for a different thread.

@innir
Copy link
Contributor

innir commented Jan 4, 2018

@annejan Thanks for confirming!

@Serphentas
Copy link

Serphentas commented Jan 5, 2018

There is a problem with src/simpletransaction.h. During make, I get the following error:

cd src/ && ( test -e Makefile || /usr/lib/x86_64-linux-gnu/qt5/bin/qmake /tmp/QtPass-1.2.1/src/src.pro -o Makefile ) && make -f Makefile 
make[1]: Entering directory '/tmp/QtPass-1.2.1/src'
rm -f libqtpass.a
ar cqs libqtpass.a mainwindow.o configdialog.o storemodel.o util.o usersdialog.o keygendialog.o trayicon.o passworddialog.o qprogressindicator.o qpushbuttonwithclipboard.o qtpasssettings.o settingsconstants.o pass.o realpass.o imitatepass.o executor.o simpletransaction.o singleapplication.o moc_mainwindow.o moc_configdialog.o moc_storemodel.o moc_usersdialog.o moc_keygendialog.o moc_trayicon.o moc_passworddialog.o moc_qprogressindicator.o moc_deselectabletreeview.o moc_qpushbuttonwithclipboard.o moc_pass.o moc_imitatepass.o moc_executor.o moc_singleapplication.o
make[1]: Leaving directory ''/tmp/QtPass-1.2.1/src'
cd tests/ && ( test -e Makefile || /usr/lib/x86_64-linux-gnu/qt5/bin/qmake '/tmp/QtPass-1.2.1/tests/tests.pro -o Makefile ) && make -f Makefile 
make[1]: Entering directory ''/tmp/QtPass-1.2.1/tests'
cd auto/ && ( test -e Makefile || /usr/lib/x86_64-linux-gnu/qt5/bin/qmake '/tmp/QtPass-1.2.1/tests/auto/auto.pro -o Makefile ) && make -f Makefile 
make[2]: Entering directory ''/tmp/QtPass-1.2.1/tests/auto'
cd util/ && ( test -e Makefile || /usr/lib/x86_64-linux-gnu/qt5/bin/qmake '/tmp/QtPass-1.2.1/tests/auto/util/util.pro -o Makefile ) && make -f Makefile 
make[3]: Entering directory ''/tmp/QtPass-1.2.1/tests/auto/util'
g++ -c -m64 -pipe -DSINGLE_APP=1 -O2 -Wall -W -Wno-unknown-pragmas -D_REENTRANT -fPIC -DVERSION="\"1.2.1\"" -DQT_NO_DEBUG -DQT_WIDGETS_LIB -DQT_GUI_LIB -DQT_NETWORK_LIB -DQT_TESTLIB_LIB -DQT_CORE_LIB -DQT_TESTCASE_BUILDDIR='"'/tmp/QtPass-1.2.1/tests/auto/util"' -I. -I../../../src -isystem /usr/include/x86_64-linux-gnu/qt5 -isystem /usr/include/x86_64-linux-gnu/qt5/QtWidgets -isystem /usr/include/x86_64-linux-gnu/qt5/QtGui -isystem /usr/include/x86_64-linux-gnu/qt5/QtNetwork -isystem /usr/include/x86_64-linux-gnu/qt5/QtTest -isystem /usr/include/x86_64-linux-gnu/qt5/QtCore -I. -I/usr/lib/x86_64-linux-gnu/qt5/mkspecs/linux-g++-64 -o tst_util.o tst_util.cpp
g++ -c -m64 -pipe -DSINGLE_APP=1 -O2 -Wall -W -Wno-unknown-pragmas -D_REENTRANT -fPIC -DVERSION="\"1.2.1\"" -DQT_NO_DEBUG -DQT_WIDGETS_LIB -DQT_GUI_LIB -DQT_NETWORK_LIB -DQT_TESTLIB_LIB -DQT_CORE_LIB -DQT_TESTCASE_BUILDDIR='"'/tmp/QtPass-1.2.1/tests/auto/util"' -I. -I../../../src -isystem /usr/include/x86_64-linux-gnu/qt5 -isystem /usr/include/x86_64-linux-gnu/qt5/QtWidgets -isystem /usr/include/x86_64-linux-gnu/qt5/QtGui -isystem /usr/include/x86_64-linux-gnu/qt5/QtNetwork -isystem /usr/include/x86_64-linux-gnu/qt5/QtTest -isystem /usr/include/x86_64-linux-gnu/qt5/QtCore -I. -I/usr/lib/x86_64-linux-gnu/qt5/mkspecs/linux-g++-64 -o moc_imitatepass.o moc_imitatepass.cpp
In file included from ../../../src/imitatepass.h:5:0,
                 from moc_imitatepass.cpp:9:
../../../src/simpletransaction.h:11:54: error: ‘>>’ should be ‘> >’ within a nested template argument list
   std::queue<std::pair<Enums::PROCESS, Enums::PROCESS>> transactionQueue;
                                                      ^
Makefile:411: recipe for target 'moc_imitatepass.o' failed
make[3]: *** [moc_imitatepass.o] Error 1
make[3]: *** Waiting for unfinished jobs....
In file included from ../../../src/imitatepass.h:5:0,
                 from ../../../src/qtpasssettings.h:5,
                 from ../../../src/storemodel.h:10,
                 from ../../../src/util.h:4,
                 from tst_util.cpp:1:
../../../src/simpletransaction.h:11:54: error: ‘>>’ should be ‘> >’ within a nested template argument list
   std::queue<std::pair<Enums::PROCESS, Enums::PROCESS>> transactionQueue;
                                                      ^
Makefile:405: recipe for target 'tst_util.o' failed
make[3]: *** [tst_util.o] Error 1
make[3]: Leaving directory ''/tmp/QtPass-1.2.1/tests/auto/util'
Makefile:42: recipe for target 'sub-util-make_first' failed
make[2]: *** [sub-util-make_first] Error 2
make[2]: Leaving directory ''/tmp/QtPass-1.2.1/tests/auto'
Makefile:42: recipe for target 'sub-auto-make_first' failed
make[1]: *** [sub-auto-make_first] Error 2
make[1]: Leaving directory ''/tmp/QtPass-1.2.1/tests'
Makefile:69: recipe for target 'sub-tests-make_first' failed
make: *** [sub-tests-make_first] Error 2

If I change the file as stated, i.e. std::queue<std::pair<Enums::PROCESS, Enums::PROCESS>> transactionQueue; becomes std::queue<std::pair<Enums::PROCESS, Enums::PROCESS> > transactionQueue;, then it compiles just fine.

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.

@annejan
Copy link
Member

annejan commented Jan 5, 2018

Most probably a gcc/clang version issue.

@carnil
Copy link

carnil commented Jan 5, 2018

MITRE has assigned CVE-2017-18021

@zx2c4
Copy link
Author

zx2c4 commented Jan 5, 2018

Posted to oss-security: http://www.openwall.com/lists/oss-security/2018/01/05/5

@noloader
Copy link

noloader commented Jan 10, 2018

@Serphentas,

If I change the file as stated, i.e. std::queue<std::pair<Enums::PROCESS, Enums::PROCESS>> transactionQueue; becomes std::queue<std::pair<Enums::PROCESS, Enums::PROCESS> > transactionQueue;, then it compiles just fine.

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.

std::queue<std::pair<Enums::PROCESS, Enums::PROCESS>>

The trailing >> is taken as the extraction operator in C++, and not part of the template syntax. The operator is inherited from std::istream::operator>>.

Cross platform software should always add the extra space as you found:

std::queue<std::pair<Enums::PROCESS, Enums::PROCESS> >

We have a library full of the fix because we support C++03 through C++17.

@annejan
Copy link
Member

annejan commented Jan 10, 2018

That has been fixed in #346 thank you for the explanation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants