add crandom.py, for our RNG, where we mix stuff with os.urandom - #10794
add crandom.py, for our RNG, where we mix stuff with os.urandom#10794SomberNight wants to merge 2 commits into
Conversation
Currently we rely on `os.urandom()` ~everywhere for cryptographically secure randomness. Existing code already checks at runtime that the output of `os.urandom()` looks at least somewhat sane (see if it compresses with zlib) and hard-fails if it does not. However, `os.urandom` could still be subtly "broken" (undetected by us) and produce bad quality output. That's the motivation of the new code here. We expect `os.urandom` to work well, BUT if it undetectably does not, mixing in other sources of entropy mitigates the situation somewhat. This PR introduces a new module `crandom.py` that manages the RNG, and that our other code should call. Extreme care should be taken here not to make things worse than the status quo by "rolling our own" thing. The logic is split across two modules: `crandom.py` and `crandom_env.py`. - The core sensitive logic (RNG mixing, extracting random bytes) is in `crandom.py`, which is absolutely security critical and is kept concise to ease review. - `crandom_env.py` contains secondary sources of entropy and potentially platform-specific code. It is just a companion to `crandom.py` and is only intended to be accessed from there. Even if all the entropy sources listed in `crandom_env.py` are broken, assuming `os.urandom()` produces high quality random, `crandom.py` should never produce low-quality random output. Hence `crandom_env.py` is much less critical tro review in depth. This is inspired by https://github.com/bitcoin/bitcoin/blob/67efced1fc83a0b7215cc1513e7c4754fee0f12f/src/random.h#L25, which is significantly more advanced. But I wanted to (1) lessen our dependence on `os.urandom()`, (2) while keeping it simple.
9fcec9d to
57237d7
Compare
renepickhardt
left a comment
renepickhardt
left a comment
There was a problem hiding this comment.
Disclaimer: I am no expert with randomness and security of it. So please take this feedback not as advise or proper audit. It is just some thoughts that I had while looking over this PR.
In particular not commenting on things does not mean I approve them as secure or good. It rather means I have no opinion / idea.
As a general question / comment I wonder if pulling all the data in crandom_env might not be predictable or at least bruteforcable in some scenarios.
|
|
||
| Never raises. | ||
| """ | ||
| assert nbytes >= 0, nbytes |
There was a problem hiding this comment.
also nbytes needs to by smaller equal than 32 I guess
| # we must not raise UnicodeError, hence "backslashreplace" | ||
| data = data.encode("utf-8", errors='backslashreplace') | ||
| with self.lock: | ||
| self._state = sha512(data + self._state)[0:32] |
There was a problem hiding this comment.
this also applies to other places. I am not sure about taking the first half of SHA512. Thus wouldn't it make more sense to do self._state = XOR(SHA256(data),self._state).
The - admitedly paranoid - reasoning: The hash is an additional component capable, at least in principle, of damaging a good source if the implementation or construction is defective while xor should always work?
| """ | ||
| with self.lock: | ||
| fresh_entropy = os.urandom(32) | ||
| h = sha512(fresh_entropy + self._state) |
There was a problem hiding this comment.
see comment from feed_entropy in line 82. I think xor preserves the randomness property better if at least one source was random
|
|
||
|
|
||
| # Check that os.urandom works | ||
| length = len(zlib.compress(os.urandom(1000))) |
There was a problem hiding this comment.
I am not sure about this sanity check. I think there is a non zero chance that true random output is compressable. That probability should be small however and I see what you are trying to achieve here but I am not sure if other means are more useful
|
|
||
| get_rand_bytes = _rng.get_rand_bytes | ||
| get_rand_below = _rng.get_rand_below | ||
| feed_entropy = _rng.feed_entropy |
There was a problem hiding this comment.
It absolutely makes sense to expose this API. however I am not sure if users who need this API will understand that they need it and build their application accordingly. See other comment in __init__
| # gather ghetto-entropy: | ||
| self.rand_add_refresh() # clock | ||
| crandom_env.rand_add_static_env(self.feed_entropy) | ||
| self.rand_add_refresh() # clock again |
There was a problem hiding this comment.
as far as I understand you pull OS specific values when creating the state RNGState object (for example at startup of electrum) if users do this within a local machine and clone the local machine I believe the additional source of randomness is just copied and never updated. I understand that feed_entropy could help here but I wonder if the feeding of additional runtime components should not also occasionally happen during random number generation.
| feed(getattr(os, "ctermid", lambda: "")()) | ||
| feed(os.getcwd()) | ||
| feed(str(os.get_exec_path())) | ||
| feed(str(os.getgroups())) |
There was a problem hiding this comment.
Seems to be supported only under linux? c.f. https://docs.python.org/3/library/os.html#os.getgroups
2 participants
(still WIP)
Currently we rely on
os.urandom()~everywhere for cryptographically secure randomness. Existing code already checks at runtime that the output ofos.urandom()looks at least somewhat sane (see if it compresses with zlib) and hard-fails if it does not.However,
os.urandomcould still be subtly "broken" (undetected by us) and produce bad quality output. That's the motivation of the new code here. We expectos.urandomto work well, BUT if it undetectably does not, mixing in other sources of entropy mitigates the situation somewhat.This PR introduces a new module
crandom.pythat manages the RNG, and that our other code should call. Extreme care should be taken here not to make things worse than the status quo by "rolling our own" thing.The logic is split across two modules:
crandom.pyandcrandom_env.py.crandom.py, which is absolutely security critical and is kept concise to ease review.crandom_env.pycontains secondary sources of entropy and potentially platform-specific code. It is just a companion tocrandom.pyand is only intended to be accessed from there. Even if all the entropy sources listed incrandom_env.pyare broken, assumingos.urandom()produces high quality random,crandom.pyshould never produce low-quality random output. Hencecrandom_env.pyis much less critical tro review in depth.This is inspired by https://github.com/bitcoin/bitcoin/blob/67efced1fc83a0b7215cc1513e7c4754fee0f12f/src/random.h#L25, which is significantly more advanced. But I wanted to (1) lessen our dependence on
os.urandom(), (2) while keeping it simple.