Hacker News Reader

Comment by sdrapkin | Hacker News Reader

Any takers on that question?

Well, it's not thread safe but they might not think that's an issue. It looks like this:

		private static CryptoRandom m_pInstance = null;
		public static CryptoRandom Instance
		{
			get
			{
				if(m_pInstance != null) return m_pInstance;

				m_pInstance = new CryptoRandom();
				return m_pInstance;
			}
		}

Philbarr is correct. The pattern they are using is fundamentally supposed to provide a thread-safe Singleton, and it fails to do that. Is that a security problem in this specific context? No.

But it's a "No" because the authors are lucky in this case - not because they are competent.

Now, that's just one instance of poor skill. There are many more. Are you sure none of them have security implications?

That's quite harsh. I guess you verified that there are actually different threads able to access this code before making such a statement? For instance, if I make a single-threaded application in the first place then I don't care about thread-safety at all. Because I am not going to need it.

Later on in the class I notice they have this:

		public ulong GeneratedBytesCount
		{
			get
			{
				ulong u;
				lock(m_oSyncRoot) { u = m_uGeneratedBytesCount; }
				return u;
			}
		}

...so if you care about thread safety at some point in your class, then you should care about it during it's initialisation.

I have verified that the CryptoRandom class is part of a standalone library with (1) should be thread-safe since it cannot dictate how it will be used by callers; (2) the authors clearly intended this library to be thread-safe (based on "thread-safe" comments in its source code).

And in all likelihood it is thread-safe - but that's due to being lucky - not competent.

The larger issue is that we have a widely-used crypto software which is clearly (1) not designed well; (2) not implemented well.

How much trust one is willing to place into current and future versions by the same author(s) is up to you.

Well, "(1) should be thread-safe since it cannot dictate how it will be used by callers" doesn't hold. For instance most of .NET classes are not thread-safe because thread-safety has a cost. So it's a question of documentation (it's well documented in .NET).

But "(2) the authors clearly intended this library to be thread-safe" means that piece of code is bad. So you have a point here.

To be a little more specific, the class itself is not marked as thread-safe. Only the AddEntropy() and GetRandomBytes() methods are so marked. So it would technically be permissible for any other part of the class to not be thread-safe.

But since it's a singleton, having a non-thread-safe initializer is cutting that distinction a little fine.

Given the discussion expressed in this chain and the fact that this is a reasonably small open source project. How many times could a solution have been submitted to the project for this and other noticed issues in the time it was discussed?

I'm sure the author would be very happy to see a sudden influx of contributions to the project, and we'd all have a better product in the end too.

Seems odd the spirit of open source in this respect tends to be more about pointing out the failures of the author than to collectively improve the actual product.

That's a fair point. If this was on Github I might take that to heart and submit a PR. Since it's hosted on Sourceforge, which I don't have an account on and don't want to given their recent behavior, I'm not going to.

The odds of an singleton being called for the first time simultaneously is abysmal. Usually you even call a singleton first in your own initialization code making errors impossible.

I do agree that technically you are correct and you should wear belt and suspenders, especially if it's a library for third-party consumption and labeled as thread-safe... but still... its pretty esoteric, and only used by the author (I assume) who knows its not thread-safe. Locking isn't exactly without its performance implications either and even though that is neglible it feels unecessary if a race condition is de facto near impossible.

Here's Jon Skeet's writeup of why this is bad and what you should be doing:

http://csharpindepth.com/Articles/General/Singleton.aspx

Note that using the last example isn't necessarily "the best", it really depends on your requirements.

Nonetheless, a very interesting read.

That should be a perfectly usable Singleton, if you just stay on one thread? Is the application using multiple threads? Should it be?

Most UI applications sooner or later need at least some basic background processing but if I was writing a simple password manager, I'd most likely just do everything blocking on the UI thread.

That said: for simple patterns like singleton, there is really no reason not to use the builtin and recommended way which is the Lazy<T>.

I assume that they are implying that the code is not constant time. In this snippet, the code bails as soon as a deviation is detected. This can, in theory, allow an attacker to determine the desired value by measuring the time taken to reject incorrect options. I haven't reviewed the code to see if this is actually a problem, but that's my guess for why it was highlighted.

I took it to mean they should use the Linq `x.SequenceEqual(y)` instead (assuming .NET 3.5+) (and x isn't null...).

Wrong.

Reply on news.ycombinator.com