CodeSOD: Ten Times as Unique

James works with a financial services company. As part of their security model, they send out verification codes for certain account operations, and these have to be unique.

So you know what happens. Someone wrote their own random string generator, then wrapped it up into a for loop and calls it until they get a random string which is unique:

private string GetUniqueVerificationCode(){ // Generate a new code up to 10 times and check for uniqueness - if it's unique jump out // IRL this should only hit once, it;s a random 25 char string ffs but you can never be too careful :) for(var tries = 0; tries < 10; tries++) { var code = RandomStringGenerator.GetRandomAlphanumericString(50); if(!this.userVerificationCodeRepository.CodeExists(code)) { return code; } } throw new Exception("Unable to generate unique verification code.");}

It's the details, here. According to the comment, we expect 25 characters, but according to the call, it looks like it's actually 50- GetRandomAlphanumericString(50). If, after ten tries, there isn't a unique and random code, give up and chuck an exception- an untyped exception, making it essentially impossible to catch and respond to in a useful way.

As the comment points out- the odds of a collision are exceedingly small- at least depending on how the "random alphanumeric string" is generated. Even with case insensitive "alphanumerics", there are quadrillions of possible strings at twenty five characters. If it's actually fifty, well, it's a lot.

Now, sure, maybe there's a bias in the random generation, making collisions more likely, but that's why we try and design our applications to avoid generating the random numbers ourselves.

James pointed out that this was silly, but the original developer misunderstood, and thought the for loop was the silly part, so now the code looks like this:

private string GetUniqueVerificationCode(){ var code = RandomStringGenerator.GetRandomAlphanumericString(50); while (this.userVerificationCodeRepository.CodeExists(code)) { code = RandomStringGenerator.GetRandomAlphanumericString(50); } return code;}

Might as well have gone all the way to a do...while. The best part is that regardless of which version of the code you use, since it's part of a multiuser web application, there's a race condition- the same code could be generated twice before being logged as an existing code in the database. That's arguably more likely, depending on how the random generation is implemented.

Based on a little googling, I suspect that the GetRandomAlphanumericString was copy-pasted from StackOverflow, and I'm gonna bet it wasn't one of the solutions that used a cryptographic source.

[Advertisement] Forget logs. Next time you're struggling to replicate error, crash and performance issues in your apps - Think Raygun! Installs in minutes. Learn more.