2

I've been using an OAuthBase class found HERE in my SSIS 2008 C# Script Components (.NET 3.5).

It's been working fine, but recently I've ran into the problem where if I execute multiple script components in the same Data Flow Task, using the GenerateNonce method in the above OAuthBase class, I end up with the same nonce (random number).

Here's an excerpt from the OAuthBase class that generates the nonce:

using System;
using System.Security.Cryptography;
using System.Collections.Generic;
using System.Text;
using System.Web;

namespace OAuth {
 public class OAuthBase {

    ....snip......

    protected Random random = new Random();

    public virtual string GenerateNonce() {
        // Just a simple implementation of a random number between 123400 and 9999999
        return random.Next(123400, 9999999).ToString();
    }
  }
}

In each script component I'm using this C# code to initiate the class and generate a nonce:

        OAuthBase oAuth = new OAuthBase();
        string nonce = oAuth.GenerateNonce();

From my searching around I think this is related to it not being thread safe? I'm not totally sure.

I'm only able to run .NET 3.5 in SSIS 2008, so I know some of the newer stuff introduced in .NET 4.0 I can't use.

Any ideas on how I can either modify the OAuthBase class and/or my C# script component code?

jared
  • 1,344
  • 4
  • 20
  • 38
  • This nonce has very few possible values. Even the threading bug aside it is not unique by any means. – usr Feb 12 '13 at 23:33
  • @usr - thanks. I was wondering about that. Maybe I just do something like `return Guid.NewGuid.ToString();`? I'm not totally sure how long my nonce can be (hitting Magento REST API). – jared Feb 12 '13 at 23:41

2 Answers2

7

If you create multiple instance of OAuthBase at the same time, it is entirely possible that the individual instance will have a Random instance with the same seed, which by default the seed is the current tick count. So this means the individual instances of Random possibly have been created with the same seed. Try making the Random instance static. But since Random is not thread safe. You would need to protect access to it.

private static readonly Random random = new Random();
private static readonly object randLock = new object();

public virtual string GenerateNonce()
{
    lock (randLock)
    {
        // Just a simple implementation of a random number between 123400 and 9999999
        return random.Next(123400, 9999999).ToString();
    }
}

// since you had protected access on random, I'm assuming sub classes want to use it
// so you'll need to provide them with access to it
protected int NextRandom(...)
{
    lock (randLock)
    {
         random.Next(...);
    }
}

But as others have suggested since you don't have a cryptographically strong source of randomness, you may want to look at other ways to generate your value.

// RNGCryptoServiceProvider is thread safe in .NET 3.5 and above
// .NET 3.0 and below will need locking to protect access
private static readonly RNGCryptoServiceProvider random =
    new RNGCryptoServiceProvider();

public /*virtual*/ byte[] GenerateNonce(int length)
{
    // a default length could be specified instead of being parameterized
    var data = new byte[length];
    random.GetNonZeroBytes(data);
    return data;
}
// or
public /*virtual*/ string GenerateNonce(int length)
{
    var data = new byte[length];
    random.GetNonZeroBytes(data);        
    return Convert.ToBase64String(data);
}
Community
  • 1
  • 1
JG in SD
  • 5,427
  • 3
  • 34
  • 46
  • Combine this with replacing Random with [RNGCryptoServiceProvider](http://msdn.microsoft.com/en-us/library/system.security.cryptography.rngcryptoserviceprovider.aspx) like [usr said](http://stackoverflow.com/a/14843988/80274) and you will have the perfect answer. – Scott Chamberlain Feb 12 '13 at 23:54
  • @ScottChamberlain updated to include an example using RNGCryptoServiceProvider – JG in SD Feb 13 '13 at 00:00
  • 2
    Just a fyi to everyone else in case they did not know, [RNGCryptoServiceProvider is thread safe only for versions 3.5 and newer of .NET](http://msdn.microsoft.com/en-us/library/system.security.cryptography.rngcryptoserviceprovider%28v=vs.85%29.aspx#threadSafetyToggle), so if you are targeting 3.0 or older you still need to use a lock. – Scott Chamberlain Feb 13 '13 at 00:04
  • Just a note on how Base64 works, the length of the string generated from the last method will be 4/3 the length of the byte array passed in (`length`) (Remainders are rounded up and padded with `=` at the end) – Scott Chamberlain Feb 13 '13 at 00:30
  • Thanks for the help and explanation on this. One minor thing is the `random.GetNonZeroByte` should be `random.GetNonZeroBytes` (I tried to edit the answer but it wouldn't let me just add 2 's' to each method). – jared Feb 13 '13 at 07:17
  • @JGinSD - if I create 2 'OAuthBase` objects at the same time from different threads, will each object have the same `random` object/value since it's being created at the time the `OAuthBase oAuth = new OAuthBase();` is being called? `random` is only used in the `GenerateNonce` method, so would it be better if I just put that directly in the method? (sorry if I'm not making sense or using the wrong syntax...I'm a bit of a n00b still). – jared Feb 13 '13 at 19:21
  • @jared If you marked the `random` member instance as **static** there will only ever be one random number generator, the number of threads creating an `OAuthBase` at the time won't matter. If you created the instance of `Random` within the `GenerateNonce` method you still have the potential for the same problem, as then individual `Random` instances could possibly be created at the same time. – JG in SD Feb 13 '13 at 19:24
3

This nonce has very few possible values. Even the threading bug aside it is not unique by any means. Use a cryptographically strong source of randomness (http://msdn.microsoft.com/en-us/library/system.security.cryptography.rngcryptoserviceprovider.aspx), output 16 bytes and base64 them. This takes care of all of that.

usr
  • 168,620
  • 35
  • 240
  • 369