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

small band aid fix for password generation on windows #276

Merged
merged 6 commits into from Jan 16, 2017

Conversation

Projects
None yet
2 participants
@treat1
Contributor

treat1 commented Jan 16, 2017

Uses BCryptGenRandom win32 api function but still falls back to qrand. A proper fix would require more changes further up the stack maybe a dialog with retry and cancel button if password generation should fail. Could also be extended to use other apis on other platforms e.g. /dev/random on linux. Does this sound worthwhile or is it being work on elsewhere?

treat1 added some commits Jan 16, 2017

use win32 crypt api for password generation on windows
creates a new function rand in the util class that calls bcryptgenrandom
from the win32 api when running on windows and falls back to qrand on
other platforms
fallback to qrand if BCryptGenRandom fails
Don't know if this function can fail for other reasons then incorrect
parameters (not enough entropy maybe?). Falling back to qrand is not
very useful a proper fix will require more changes further up the stack.
@annejan

This comment has been minimized.

Member

annejan commented Jan 16, 2017

Aparently bcrypt is not available on Windows as-per standard.

fatal error: bcrypt.h: No such file or directory
 #include <bcrypt.h>

Any idea how to install it from within .appveyor.yml ?

@treat1

This comment has been minimized.

Contributor

treat1 commented Jan 16, 2017

It's an issue with mingw in appveyor then. Bcrypt is included with windows since vista. I have the latest mingw from qt installer installed and it has the bcrypt.h/lib included.

@annejan

This comment has been minimized.

Member

annejan commented Jan 16, 2017

I'm downloading a Windows VM from modern.ie to check this one out :)

Would like to have a working appveyor (since it does the windows builds so I don't have to) before merging.

@treat1

This comment has been minimized.

Contributor

treat1 commented Jan 16, 2017

The header file doesn't come bundled with windows only the dll. The header file comes from the mingw package.

@treat1

This comment has been minimized.

Contributor

treat1 commented Jan 16, 2017

On my computer It's found in C:\Qt\Tools\mingw530_32\i686-w64-mingw32\include\bcrypt.h

@treat1

This comment has been minimized.

Contributor

treat1 commented Jan 16, 2017

If you can't get it working a work around would be to use LoadLibrary/GetProcAddress to load the library instead.

@treat1

This comment has been minimized.

Contributor

treat1 commented Jan 16, 2017

Possible fix. The path on your appveyor thing should include C:\Qt\Tools\mingw530_32\bin not C:\MinGW\bin From line 30 in the log. I'm pretty sure this is it now. It's what Qt Creator does aswell.

@annejan

This comment has been minimized.

Member

annejan commented Jan 16, 2017

That did the trick!
Thanks for this update @treat1, I'll look into the /dev/random and other implements later on :)

@annejan annejan merged commit ec02de0 into IJHack:master Jan 16, 2017

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
linthub Linthub has cleared the pull request, no code style suggestions found.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment