-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Replace userspace RNG with the OS's CSPRNG #2621
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an aside, I think the random mutex is not needed anymore with this change. Probably not much benefit in removing it though.
Although longer term I would refer replacing the entirety of the RNG with a well-vetted library or carefully considered framework, this appears to be an incremental improvement in terms of robustness of the existing unsourced RNG, suitable for existing uses, and less likely to cause problems with careless future uses. I reviewed the code changes and don't see any problems. Additional review welcome. |
@moneromooo-monero Regarding the mutex I would keep it since there is not real clarity on the thread safety of the Windows APIs especially across versions. The MSDN docs I found has a broken link regarding "Threading issues". So probably best to keep the mutex. (Previously this was not an issue as the Windows APIs were only used during single threaded initialization) |
Commits should be squashed |
Okay, I'll do that before finalizing. |
b2b7663
to
2cb5e5b
Compare
I would be happier to advocate something like this in place of the current implementation of + #include "sodium.h"
// ...
- /* the current implementation */
+ return randombytes_buf(result, n); |
Commenting here, though it maybe belongs in the comments on #1271 Also, #1271 has a quite misleading title. There is no evidence that the current PRNG is insecure. Use of Keccak as a CSPRNG is quite well documented http://sponge.noekeon.org/SpongePRNG.pdf Back to my first point - I'm fine with using libsodium, if only because it makes this one less thing for us to worry about maintaining ourselves. But I would still use |
You're not removing bits from the state. The current PRNG is not fork-safe. It's an additional point of failure, not a defense-in-depth. This pull request demonstrates a decrease in the likelihood of a practical attack.
I'm going to be blunt: This is stupid. If your operating system's RNG is insecure, you gain nothing by trying to improve its quality downstream. The security of your entire OS depends on the OS's CSPRNG. If that's defeated, game over. See: https://sockpuppet.org/blog/2014/02/25/safely-generate-random-numbers/ Replacing this with |
We're not using fork(). What are you talking about?
Been there. That's garbage.
Garbage.
Pretty strong reason to IGNORE the rest of this advice. |
Could you be more specific? Because no, it's not garbage. Virtually every cryptographer will agree with me on this point. |
Let me add onto this: Sysrandom does what is described on this page. To wit, sysrandom will use the following sources:
|
For starters, that page was written by someone who only knows about Linux. There are token mentions of FreeBSD and MacOSX. There's no knowledge at all about Solaris or other Unix platforms. There's no discussion of how to handle Windows at all. That page is dangerously myopic. Having OS-dependent differences in our runtime behavior makes the code vulnerable to remote fingerprinting. You haven't thought this through. |
Okay, so your proposal is to keep using a userspace RNG that uses the Keccak permutation without reseeding or removing bits because there's a left field hypothetical concern about making security through obscurity less viable if the OS-dependent behavior is different on a feature that's, by definition, designed to be noisy. (Nevermind the fact that your userspace RNG already depends on the OS-dependent generator during startup.) Whatever you think your threat model is here, I'm not interested in hearing it. @iamsmooth Yay/nay on |
You're not paying attention. If you're this bad at attention to detail, you've got no business proclaiming what's the right approach. |
See this comment, where you said:
Which is a defense of keeping things the same. Then you said:
Which is another argument in favor of a userspace RNG. Since my proposal is to use the OS's CSPRNG, your argument is against changing anything. Since we're on the topic of details, let's review some of your comments from start to finish.
If you're so intent on dismissing other peoples' technical arguments as "garbage" and attacking my character, a more appropriate venue to try would be Youtube comments, not pull requests on Github. Just because I'm focused on the problem at hand doesn't mean I'm not paying attention to your barbs. Your opinion just doesn't matter to me. Recommended reading: http://www.nonviolentcommunication.com/aboutnvc/4partprocess.htm |
Specifically, my last comment was:
but your eagle eye seems to have completely missed it. Your patch (and #2622) also shows total lack of attention to detail. Whoever convinced you that you can write security-sensitive code really did a good snowjob. |
rolls eyes You can continue to attempt to hurt my feelings, but that's not going to accomplish anything. At no point have I ever suggested that I'm a C/C++ developer. I'm a PHP developer, but the Monero devs complained that nobody sent a pull request to address this problem, so, here I am. Don't like it? Put some skin in the game. Do better. Whatever makes you feel adequate. |
My "skin is in the game" you moron. https://github.com/monero-project/monero/graphs/contributors |
Is it? I don't see a pull request solving #1271. Apparently communication in general and inferring context in particular are not your strong points. I'll try to be more precise in my word choice going forward, so as to accommodate your handicaps. |
We all seem to be in agreement on using libsodium The disagreement seems to be on using this: I presume that is a user space PRNG and you therefore consider it unsuitable. Perhaps you could provide some reference for what that function is useful for and why it exists in libsodium at all. Then we can asses whether it is suitable in this case or not. My inclination is to use the default provider from libsodium (which I guess is the kernel one) unless we have a good reason to do otherwise, on the theory that the developers of libsodium know what they are doing and provide appropriate defaults. I'm doubtful on the possibility of remote fingerprinting as I would see that as a defect of a properly functioning CSPRNG (with which it ought to be computationally indistinguishable from true random; to me this is equivalent to no fingerprinting being possible). However, it is possible I'm missing something as I'm certainly not a cryptographer. |
Certainly. This is my understanding, based on the documentation:
For people who aren't on Windows or modern Unix-based systems, I'd personally recommend asking @jedisct1 for his guidance there. |
I suspect this is a large portion of the purpose for it. It is indeed common practice to introduce a deterministic PRNG for testing. Although of course embedded platforms with no system-level CSPRNG are also plausible uses for it (with the caveat one would have to figure out how to seed and if, when, and how to reseed). For ordinary Windows and Unix-like systems, libsodium seems to recommend using the default. |
Okay, that sounds like #2622 is closer to where we want to land. Unfortunately, my background is not in C/C++ so it's currently breaking and I'm probably overlooking something obvious to C/C++ devs. Shall we move the discussion here? |
I stumbled over here out of a niche pointed interest in kCSPRNGs, like I did for many threads like this one, but I was frankly disgusted by the immediate personal attacks from someone who seems to be a core member of the community towards a new contributor. Before drawing conclusions about the Monero development community, I’d like to ask whoever represents it if they believe this behavior is acceptable and if they stand behind it. And if not, what is going to be done about it. Not knowing better who to ping, the two names at the top of the Contributors graph: @moneromooo-monero and @fluffypony. Feel free to redirect to whoever can speak for the @monero-project. |
@FiloSottile We operate as a 'big tent' with people of many different personalities. Those who do best within such a large community are generally those who accept this and do not turn every difference in style into an escalated conflict. That said, we do have a process for addressing cases where behavior is disruptive to other contributors and is not self-corrected. If you believe that to be the case then please bring it up in a developer meeting (generally biweekly on IRC) or perhaps open an issue in http://github.com/monero-project/meta. It isn't an appropriate topic for a technical issue tracker. |
@FiloSottile why don't you take your disgust somewhere else? You're not a contributor here and the internet is no place for such fragile sensibilities. |
For the record:
Lists an irrelevant weakness since there is no use of fork() in this code. Fails to make the case that the current PRNG output is predictable.
Initiates personal attack.
Proclaims behavior of code without qualifications as a coder.
The code in question is broken https://github.com/jedisct1/libsodium/blob/master/src/libsodium/randombytes/sysrandom/randombytes_sysrandom.c#L134 It polls for /dev/random to be readable and then switches to /dev/urandom. The problem is that /dev/random will be "readable" as soon as it thinks it has 64 bits of entropy. The invocations of randombytes are going to be calling for more than 64 bits at a time (typically 256 bits) and the immediate switch to /dev/urandom once this poll completes fails to check that there's actually sufficient bits in the entropy pool. I stated that @paragonie-scott wasn't paying attention and hasn't thought this through. Those are demonstrable facts. |
I just want to clarify something.
Attacking an idea ("This is stupid" not "You are stupid") is not personal. If you immediately take it personally, that sounds to me like some deep-seated personal issues that honestly don't belong in the public space. Because the idea that:
...is a stupid idea that has very little actual support even among cryptography amateurs. Whether or not this idea is stupid says nothing about you. You could be playing devil's advocate or simply misinformed for all I know. Furthermore, the old adage, "Two wrongs don't make a right," is relevant here. The appropriate reaction to a perceived slight is not to go on the offense. If the only thing you have to say in your defense is, "Well he started it!" like a young child being scolded for getting into a scrap with a classmate, maybe stop and reflect on how your actions line up with that of the sort of toxic personalities that destroy communities from the inside. |
@FiloSottile nobody can speak for the Monero project, really, and seeing as how we’re a project with nearly 300 contributors it’s not really acceptable that anyone try dictate how a contributor should behave. Regardless, there is a process as governed by our fork of the Collaborative Code Construction Contract (C4), so smooth’s suggestion applies. PS. Since you’ve blocked me on Twitter I guess we all have to deal with people being mean in one way or another. |
@paragonie-scott you're in no position to lecture here. Support your case with facts. If you're going to propose a change to the code you have to
So far you've continued to fail to demonstrate that the PRNG is generating predictable output. You've cited no references or studies showing the algorithm is weak or analyzable. You've demonstrated that you don't understand how your proposed solution works. Rather than address the facts you continue to divert into the weeds. |
Hello, Regarding the issue at hand:
As it currently stands, given that Monero has very diverse potential system-level use cases, it seems perfectly reasonable to implement a userland layer on top of
This would help address @paragonie-scott's concerns while allowing @hyc to move the project forward until a point where the actual behavior of OSRNGs is empirically, independently assessed on all of Monero's target platforms. I would recommend this approach. Regarding problems with this discussion:I frankly dislike how everyone here is behaving, and hope that I can provide a fair assessment of what went wrong. I apologize in advance for likely sounding like I'm chiding people.
Thank you for your patience with my unsolicited remarks. |
@KAepora that was an excellent, succinct comment. If GitHub supported the unicorn emoji I would’ve used that to indicate my solidarity with pretty much everything you said:) |
Alternative to monero-project#2621, uses libsodium's randombytes_buf() instead.
Http://sealedabstract.com/rants/nanomsg-postmortem-and-other-stories/
Fluffypony posted this somewhere a while back. I think it would be worthwhile for the actors in this thread to read so this project doesn't fall victim to a problem that affects so many other open source projects. Let's not work against each other, but with each other.
… On Oct 10, 2017, at 8:34 AM, Riccardo Spagni ***@***.***> wrote:
@KAepora that was an excellent, succinct comment. If GitHub supported the unicorn emoji I would’ve used that to indicate my solidarity with pretty much everything you said:)
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
Without knowing where all Monero uses randomness, there are a couple valid reasons to use a CSPRNG:
If the current CSPRNG is unable to detect and compensate for process forks, IMHO the CSPRNG should be fixed to deal with this. Perhaps the code never forks today but it's an accident waiting to happen. One way to fix it is to replace it wholesale with the OS RNG, of course, but this might be overkill? I don't mean to argue either case, but if this is the goal of the PR it would be good to be explicit about that. There are also some contexts, even in cryptography, where you do want "deterministic randomness" and you already have some secret data that can act as a seed. For example, deterministic nonce generation for signatures; deterministic key generation a la Bitcoin BIP32; randomness in rangeproof generation. For these applications it's safest and simplest to use something like RFC6979 which basically means using a single-purpose PRNG. Because the output is supposed to be deterministic there is no vulnerability to process forking, and you need your key/seed to be random anyway, so in these contexts using the freshly-seeded CSPRNG wins hands down vs making external calls. |
This is unlikely to ever happen. There is no canonical set of target platforms, only a minimum set of requirements. The primary form of delivery is source code which people can and do use on a wide variety of platforms where they are able to compile it. Adding 'properly functioning OS randomness API' to the set of requirements seems like an okay short term resolution to me. Bitcoin's approach of combining several sources of randomness including the OS adds some robustness against OS rng failure and could be added later, though even in that case the requirement for properly functioning OS randomness likely shouldn't be removed. |
It's not my intention to insert myself in the governance of a project, nor do I know anything about anyone's recurrent behavior, so I am not going to bring this to a different forum. What I wanted to check is if these comments were considered acceptable or simply overlooked. I have my answer. Since people will stumble upon this thread, allow me one last on-topic remark about this:
The code is not broken. /dev/random and /dev/urandom on Linux use (separate) pools seeded at the same time. There is no way of knowing how much entropy went into the pools, exposing applications that run extremely soon at boot to unseeded non-blocking /dev/urandom. Waiting for /dev/random to be readable before using /dev/urandom is a neat, cheap trick to avoid that and only that. Remember that there is no such a thing as "running out of entropy". For more about why that is and how the Linux sub-system works, I tried to explain it at CCC. (I am not following up on this thread, but feel free to email me if you're confused.) Finally, if I hurt anyone's feelings blocking them on Twitter, it's nothing personal nor universal. We all use the platform differently and for different purposes. |
I'm not a Monero contributor, only a cryptography beginner. My 2 cents
I've briefly reviewed the code and read this paper http://sponge.noekeon.org/SpongePRNG.pdf Actually Monero is using Shake256 ( The permutation function used in this case is Since Shake256 is used, the max lenght The paper also prove that:
In our case 2^(512/2) = 2^256 ouput block before distinguish this PRNG from an ideal one
In our case 2^c/m(A) = 2^512 So it's pretty reliable as well. The PRNG is freshly seeded from the kPRNG in the If Monero needs an userland PRNG why the current one isn't a fair choice? Edit: In case it's not clear. The monero PRNG is an implementation of SpongeRNG by the keccak authors, precisely from "Algorithm 2: Implementation of the squeezing of |
The current one would greatly benefit from two changes:
I think the premise of this question is |
@TheZ3ro IMO it isn't ideal because there are various edge cases and quality-of-implementation issues that are better addressed by a well-maintained library. For example, using getrandom, when available, rather than reading from /dev/urandom has a number of clear advantages. Reading from /dev/urandom has issues in the cold-start case which the code does not currently address. The code does not check that the initializer actually executed (credit Matthew Green for reporting this on twitter). It will on most sane platforms, but if someone compiles on some odd (perhaps embedded platform) that doesn't handle initializers properly, it will silently fail (and be unseeded). These, and others, could be fixed, of course, but there is little if any value to be added by doing so rather than just replacing it with a call to a well-vetted and well-maintained library. I will defer to others on your comments on the cryptography. To my mind (and from what I've been told) it doesn't seem to be broken but it is just better to let others who are specializing on the task handle this (and other) cryptography primitives when our usage is not in some identifiable way well outside of the norm (which I believe applies here). |
The /dev/random and /dev/urandom pools are seeded independently, at different rates. This neat cheap trick doesn't actually guarantee that /dev/urandom has been sufficiently seeded by the time /dev/random has been. It probably works, but it's depending on a specific kernel implementation and, to a large degree, luck.
The SpongePRNG paper defines the required reseed interval to be 2^32 calls. Nobody is calling often enough to run into this limit within a single program invocation.
That is only your opinion, but it is not backed by facts. "userland PRNGs must always be avoided" is a belief - it is not a fact. The fact is that OS-provided RNGs vary greatly in quality across platforms. Isolating user level from platform differences is a good thing. Uniformity is beneficial for testability, verifiability, and maintainability. Also a fact. The integrity of a system is secured by facts, not opinions or beliefs. |
The C++ spec requires support for static constructors. Any compiler capable of building this source tree will have to implement initializers. If they don't work properly, multiple pieces of this source tree will fail, noisily. This is a non-issue. That said, I think this aspect of the code base is sloppy. Use of static implicit initializers means you have no control over initialization order. It's common practice for C++ coders but IMO it's poor design.
Nonsense. Since the initializer has side effects an optimizer cannot remove it. If it does, the compiler is broken, and such a broken compiler would break a lot of other parts of the tree as well. Visibly and noisily. |
@hyc, the discussion was about robustness, not correct behavior. I've personally used platforms with broken/non-standard initializer or startup behavior. The C++ standard is irrelevant as the code in question is C, not C++, and relies on its own potentially-fragile system-dependant implementation of initializers (in src/crypto/initializer.c, and as it turns out is not used anywhere else). If nothing else, getting rid of this entire module would allow discarding this additional piece of system-dependent code which adds risk and potentially maintenance/porting complication, and does not add value (the C-based initializer implementation). "Optimized out" is kind of nonsense, in that it would never be a correct optimization (the initialized value is clearly used). Nevertheless, the result of incorrect behavior given some broken compiler (no seeding at all) could be catastrophic, and robustness (along with the low cost of being defensive) calls for a double check (which the code implements, but only in a debug build). Finally, you mention "this tree" would fail in many ways under those conditions and that is true. I've made similar arguments about how what was claimed to be a "vulnerability" is in fact not that given how the code is used. But there is simply no telling how the code might be used in the future. Such as: subsets of the tree used externally (as libraries or transplanted code), or even within the same Monero tree, by standalone utilities (previous similar examples, I think both deprecated, being connectivity_tool and simpleminer). So again, robustness calls for guarding against catastrophic failure with reduced assumptions of correct behavior (of both the code and of people) rather than making narrow arguments about how everything is perfectly okay as long as no one ever screws up or uses a slightly broken platform. |
I completely agree with you on the robustness aspect. I already agreed with you on switching to a 3rd party to remove the burden of maintenance from ourselves. (But as we've seen, the libsodium library is not provably any better in this regard.) But speaking as someone who was a gcc maintainer for 7 years, I can tell you that if a compiler suite supports C++, then the C compiler supports initialization mechanisms too. That said, the rest of your points stand. It's system-dependent and compiler-dependent and removing the dependency is a good thing. And as I also already said, the entire approach (requiring initializer support) is sloppy. Removing it is a win, no question. |
See #1271