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

PRNG seeding is done totally wrong #238

Closed
tezeb opened this Issue Nov 24, 2016 · 9 comments

Comments

Projects
3 participants
@tezeb
Contributor

tezeb commented Nov 24, 2016

I think this requires a standalone issue(not only pull request) as I belive that it has huge impact on users security.
Fix from commit 961530c is, well, almost pointless. It can be used as a textbook example of why non-cryptographers should not implement crypto-related algorithms.

What happens now(setup with no pwgen and git enabled):

  1. Start application
  2. qsrand(time in seconds)
  3. work for some time
  4. generate password and store it in file(with timestamp in seconds) and git(with yet another timestamp!)
  5. or maybe even push it somewhere, it's encrypted right?

On the bright side, (afaik) it won't tell passwords straight away with offline attack on password-store(thx to GPG). There is also no way to discover method used for password generation. But it is possible to create all passwords that were created(right now around 28M seeds) by all users of QtPass that were not using pwgen.

Please see pwgen source for reference on PRNG seeding:
https://github.com/jbernard/pwgen/blob/master/randnum.c
as for the c++ using might be a solution.
http://www.cplusplus.com/reference/random/

@jounathaen

This comment has been minimized.

Member

jounathaen commented Nov 24, 2016

Pass itself uses pwgen as password generator. Why don't we simply drop the other password generation option?
Isn't the dependency already given?

@tezeb could you short explain, why are there only 28M possible Passwords? I don't fully understand the problem from your post.

@annejan

This comment has been minimized.

Member

annejan commented Nov 24, 2016

Main reason is Windows users don't have pwgen same reason we have "fake pass" fallback . .

@annejan

This comment has been minimized.

Member

annejan commented Nov 24, 2016

As far as I can tell step 2 seeds the pool . .
When you call the qrand in step 4 it takes it's random input from that pool.

The timestamps saved in step 4 and 5 have no relation to the time the pool was seeded in step 2.
There should be no way to correlate the generated password from those!
http://doc.qt.io/qt-5/qtglobal.html#qsrand

@tezeb

This comment has been minimized.

Contributor

tezeb commented Nov 24, 2016

It's not 28M passwords, but 28M PRNG seeds(although it's really close in that case).

PRNG use seed value as a starting point to generate sequence of numbers. As PRNG's are reproducible, every time they are seeded with the same value, they produce the same sequence of numbers. So if you know the seed, you know exactly all numbers that were generated, thus you know the sequence of characters that were generated for password. As we seed with a timedate value with a seconds resolution we got:
~330(days since the commit, on 31 of December, 2015) * 24(hours) * 3600 (seconds) = 28.5M.
For each of this seeds one can generate character string, which is further divided into potential passwords. There is a bit of added complexity, because attacker does not know the lenght of password(but it defaults to 8), or how many times user regenerated it before storing, but overall it significantly reduces complexity of password guessing. As an example:
8 digit only letters(upper&lower case) password: (2*26)^8 = 53e12
10 cases(say lenght 8-18) for known-seed: 28e6 * 10 = 28e7

This will be more complicated to fix than it seems, as support for random_devices across systems/compilers is random at best ie. MinGW for Windows, does not provide it and fallbacks to 0-preseeded Marsene Twister.

@tezeb

This comment has been minimized.

Contributor

tezeb commented Nov 24, 2016

Actually they do. They give you upper bound for searching the seed used. Decrement from there. No one is running their Windows longer than 12h? 24h? a week maybe?...

@annejan

This comment has been minimized.

Member

annejan commented Nov 24, 2016

I have to agree with @tezeb you can assume the time between storing your password and starting QtPass is somewhere in the hours range, limiting the number of possible passwords for any given entry (with default settings) to thousands . .

@tezeb

This comment has been minimized.

Contributor

tezeb commented Nov 24, 2016

The least that can be done is to use miliseconds resolution for qsrand.

@annejan

This comment has been minimized.

Member

annejan commented Nov 24, 2016

Yes, the milliseconds I noticed too @tezeb
The common way in Qt seems to be qsrand(static_cast<uint>(QTime::currentTime().msec()));

.. best options still seems to be to switch to the pwgen method or the <random> method as mentioned in your first post . .

I'll have some people with crypto background weigh in on this too before committing to a solution for the upcoming release.

annejan added a commit that referenced this issue Nov 24, 2016

@annejan annejan self-assigned this Nov 24, 2016

@annejan annejan added the bug label Nov 25, 2016

annejan added a commit that referenced this issue Dec 2, 2016

@annejan

This comment has been minimized.

Member

annejan commented Jan 16, 2017

Related #276

@annejan annejan closed this Jun 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment