3 ms·

Any takers on that question?

jarito 11y ago
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.
- mc808 11y ago
  I took it to mean they should use the Linq `x.SequenceEqual(y)` instead (assuming .NET 3.5+) (and x isn't null...).
  - sdrapkin 11y ago
    Wrong.
philbarr 11y ago
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; } }
- sdrapkin 11y ago
  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?
  - ifdefdebug 11y ago
    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.
    - philbarr 11y ago
      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.
      - deleted 11y ago
        [deleted]
    - sdrapkin 11y ago
      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.
      - ifdefdebug 11y ago
        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.
- bentcorner 11y ago
  Here's Jon Skeet's writeup of why this is bad and what you should be doing: http://csharpindepth.com/Articles/General/Singleton.aspx 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.
- alkonaut 11y ago
  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.

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

philbarr 11y ago

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; } }

sdrapkin 11y ago
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?
- ifdefdebug 11y ago
  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.
  - philbarr 11y ago
    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.
    - deleted 11y ago
      [deleted]
  - sdrapkin 11y ago
    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.
    - ifdefdebug 11y ago
      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.
bentcorner 11y ago
Here's Jon Skeet's writeup of why this is bad and what you should be doing: http://csharpindepth.com/Articles/General/Singleton.aspx 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.
alkonaut 11y ago
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>.