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

gui: remove OpenSSL PRNG seeding (Windows, Qt only) #17151

Merged
merged 1 commit into from Oct 18, 2019

Conversation

@fanquake
Copy link
Member

fanquake commented Oct 15, 2019

This removes the code introduced in #4399 that attempts to add additional entroy to the OpenSSL PRNG using RAND_event(). This is specific to bitcoin-qt running on Windows.

RAND_event() collects the entropy from Windows events such as mouse movements and other user interaction.
It should be called with the iMsg, wParam and lParam arguments of all messages sent to the window procedure.
It will estimate the entropy contained in the event message (if any), and add it to the PRNG.
The program can then process the messages as usual.

Besides BIP70, this is the last place we are directly using OpenSSL in the GUI code. All other OpenSSL usage is in random.cpp.

Note that we are still also still doing other Windows specific gathering using RandAddSeedPerfmon and RAND_screen() on top of the other generation we do.

Also note that if RAND_event returns 0 here (PRNG has NOT been seeded with enough data), we're just logging a single message and continuing, which also seems less than ideal.

This removes the code introduced in [#4399](#4399)
that attempts to add additional entroy to the OpenSSL PRNG using Windows messages.
Note that this is specific to bitcoin-qt running on Windows.

```
RAND_event() collects the entropy from Windows events such as mouse movements and other user interaction.
It should be called with the iMsg, wParam and lParam arguments of all messages sent to the window procedure.
It will estimate the entropy contained in the event message (if any), and add it to the PRNG.
The program can then process the messages as usual.
```

Besides BIP70, this is the last place we are directly using OpenSSL in the
GUI code. All other OpenSSL usage is in random.cpp.

Note that we are still also doing Windows specific entropy gathering in multiple
other places. Such as [RandAddSeedPerfmon](https://github.com/bitcoin/bitcoin/blob/master/src/random.cpp#L268)
and [RAND_screen()](https://github.com/bitcoin/bitcoin/blob/master/src/random.cpp#L600).

Also note that if RAND_event returns 0 (PRNG has NOT been seeded with enough data), we're
just logging a message and continuing on, which seems less than ideal.
@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

TheBlueMatt commented Oct 15, 2019

Can we snarf (some portion of) mouse movements via Qt directly (on all platforms)?

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Oct 15, 2019

Concept ACK on removing this, especially as it's Windows only.

But it would be nice to use UI events in general and feed them into our own RNG (perhaps batches together when there are a bunch of events accumulated).

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Oct 15, 2019

I think removing this (gui-only, windows-only) can be done independently from adding more randomness sources

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Oct 15, 2019

I think removing this (gui-only, windows-only) can be done independently from adding more randomness sources

Agreed.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Oct 16, 2019

ACK cc3b528

Maybe get rid of RAND_screen calls too. They've been deprecated a while ago.

But it would be nice to use UI events in general and feed them into our own RNG (perhaps batches together when there are a bunch of events accumulated).

I'm not sure it's worth doing, especially with the GUI and node/wallet split on the horizon. Also conceptually not sure the presence of an UI or not should affect random generation, at least manually: the OS's cryptographic randomness generation on modern OSes is supposed to take care of this. It has access to much more timings and such than a user-space application ever would.

(in any case, Marco is correct that this is a separate concern and probably should be discussed somewhere else, or at least isn't a blocker here)

@carnhofdaki

This comment has been minimized.

Copy link
Contributor

carnhofdaki commented Oct 16, 2019

the OS's randomness generation on modern OSes is supposed to take care of this. It has access to much more timings and such than a user-space application ever would.

Yes. Just for fun, no offence: Reminds me of https://flak.tedunangst.com/post/random-in-the-wild

@fanquake fanquake mentioned this pull request Oct 16, 2019
@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Oct 16, 2019

unsigned ACK cc3b528

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Oct 16, 2019

Gitian builds for commit 4cfb673 (master):

Gitian builds for commit 4093216 (master and this pull):

@theuni
theuni approved these changes Oct 18, 2019
Copy link
Member

theuni left a comment

ACK cc3b528.

Also agree with nuking the RAND_screen() as a follow-up.

fanquake added a commit that referenced this pull request Oct 18, 2019
cc3b528 gui: remove OpenSSL PRNG seeding (Windows, Qt only) (fanquake)

Pull request description:

  This removes the code introduced in [#4399](#4399) that attempts to add additional entroy to the OpenSSL PRNG using `RAND_event()`. This is specific to bitcoin-qt running on Windows.

  ```
  RAND_event() collects the entropy from Windows events such as mouse movements and other user interaction.
  It should be called with the iMsg, wParam and lParam arguments of all messages sent to the window procedure.
  It will estimate the entropy contained in the event message (if any), and add it to the PRNG.
  The program can then process the messages as usual.
  ```

  Besides BIP70, this is the last place we are directly using OpenSSL in the GUI code. All other OpenSSL usage is in [random.cpp](https://github.com/bitcoin/bitcoin/blob/master/src/random.cpp).

  Note that we are still also still doing other Windows specific gathering using [RandAddSeedPerfmon](https://github.com/bitcoin/bitcoin/blob/master/src/random.cpp#L268) and [RAND_screen()](https://github.com/bitcoin/bitcoin/blob/master/src/random.cpp#L600) on top of the other generation we do.

  Also note that if RAND_event returns `0` here (PRNG has **NOT** been seeded with enough data), we're just logging a single message and continuing, which also seems less than ideal.

ACKs for top commit:
  laanwj:
    ACK cc3b528
  MarcoFalke:
    unsigned ACK cc3b528
  theuni:
    ACK cc3b528.

Tree-SHA512: 0bb18779cf37f6670e3e5ac6a6a38e5f95199491b2684f9e56391c76f030fe1621d6df064239c2a398f228129fdf3f2220fc8cd15b2b92ecf2ea6d98a79b2175
@fanquake fanquake merged commit cc3b528 into bitcoin:master Oct 18, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fanquake added a commit to fanquake/bitcoin that referenced this pull request Oct 18, 2019
Follow up to bitcoin#17151 where
there were multiple calls to also remove our call to RAND_screen().
@fanquake fanquake deleted the fanquake:windows_qt_openssl_seeding branch Oct 18, 2019
fanquake added a commit to fanquake/bitcoin that referenced this pull request Oct 19, 2019
Follow up to bitcoin#17151 where
there were multiple calls to also remove our call to RAND_screen().
laanwj added a commit that referenced this pull request Oct 21, 2019
e892f96 random: remove call to RAND_screen() (Windows only) (fanquake)

Pull request description:

  Follow up to #17151 where there were multiple calls to also remove our call to RAND_screen().

ACKs for top commit:
  MarcoFalke:
    unsigned ACK e892f96
  laanwj:
    ACK e892f96

Tree-SHA512: 1b846016d91e8113f90466b61fcaf0574edb6b4726eba1947549e2ac28907e1318d893f7b303e756f19730c8507c79b10e08d54b97153224b585ff1e0ac1953e
sidhujag added a commit to syscoin/syscoin that referenced this pull request Oct 21, 2019
cc3b528 gui: remove OpenSSL PRNG seeding (Windows, Qt only) (fanquake)

Pull request description:

  This removes the code introduced in [bitcoin#4399](bitcoin#4399) that attempts to add additional entroy to the OpenSSL PRNG using `RAND_event()`. This is specific to bitcoin-qt running on Windows.

  ```
  RAND_event() collects the entropy from Windows events such as mouse movements and other user interaction.
  It should be called with the iMsg, wParam and lParam arguments of all messages sent to the window procedure.
  It will estimate the entropy contained in the event message (if any), and add it to the PRNG.
  The program can then process the messages as usual.
  ```

  Besides BIP70, this is the last place we are directly using OpenSSL in the GUI code. All other OpenSSL usage is in [random.cpp](https://github.com/bitcoin/bitcoin/blob/master/src/random.cpp).

  Note that we are still also still doing other Windows specific gathering using [RandAddSeedPerfmon](https://github.com/bitcoin/bitcoin/blob/master/src/random.cpp#L268) and [RAND_screen()](https://github.com/bitcoin/bitcoin/blob/master/src/random.cpp#L600) on top of the other generation we do.

  Also note that if RAND_event returns `0` here (PRNG has **NOT** been seeded with enough data), we're just logging a single message and continuing, which also seems less than ideal.

ACKs for top commit:
  laanwj:
    ACK cc3b528
  MarcoFalke:
    unsigned ACK cc3b528
  theuni:
    ACK cc3b528.

Tree-SHA512: 0bb18779cf37f6670e3e5ac6a6a38e5f95199491b2684f9e56391c76f030fe1621d6df064239c2a398f228129fdf3f2220fc8cd15b2b92ecf2ea6d98a79b2175
sidhujag added a commit to syscoin/syscoin that referenced this pull request Oct 21, 2019
Follow up to bitcoin#17151 where
there were multiple calls to also remove our call to RAND_screen().
fanquake added a commit to fanquake/bitcoin that referenced this pull request Oct 26, 2019
This should have been part of bitcoin#17151.
fanquake added a commit to fanquake/bitcoin that referenced this pull request Oct 28, 2019
This should have been part of bitcoin#17151.
sipa added a commit to sipa/bitcoin that referenced this pull request Oct 28, 2019
sipa added a commit to sipa/bitcoin that referenced this pull request Oct 29, 2019
sipa added a commit to sipa/bitcoin that referenced this pull request Nov 2, 2019
sipa added a commit to sipa/bitcoin that referenced this pull request Nov 2, 2019
fanquake added a commit to fanquake/bitcoin that referenced this pull request Nov 2, 2019
This should have been part of bitcoin#17151.
sipa added a commit to sipa/bitcoin that referenced this pull request Nov 6, 2019
sipa added a commit to sipa/bitcoin that referenced this pull request Nov 6, 2019
sipa added a commit to sipa/bitcoin that referenced this pull request Nov 7, 2019
sipa added a commit to sipa/bitcoin that referenced this pull request Nov 7, 2019
sipa added a commit to sipa/bitcoin that referenced this pull request Nov 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.