IJHack / QtPass

QtPass is a multi-platform GUI for pass, the standard unix password manager.
https://qtpass.org/
GNU General Public License v3.0
1.02k stars 162 forks source link

Insecure Password Generation #338

Closed zx2c4 closed 6 years ago

zx2c4 commented 6 years ago

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)
zx2c4 commented 6 years ago

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 commented 6 years ago

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.

zx2c4 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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!

zx2c4 commented 6 years ago

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 commented 6 years ago

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

zx2c4 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@annejan Thanks for confirming!

Serphentas commented 6 years ago

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 commented 6 years ago

Most probably a gcc/clang version issue.

carnil commented 6 years ago

MITRE has assigned CVE-2017-18021

zx2c4 commented 6 years ago

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

noloader commented 6 years ago

@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 commented 6 years ago

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