Spot the Bug(s) - Encryption Problem

For such a small amount of code it is surprisingly buggy... See if you spot all the bugs; it does compile.

class randomNumber
{
 Seeder seeder;
 Boolean created = false;
 int seed = 0;
 
 public randomNumber(int seed)
 {
  this.seed = seed; 
 }

 public randomNumber()
 {
  if (created == true)
   seed = DateTime.Now.Second / seeder.getSeed();
  else
  {
   created = true;
   Random number = new Random();
   seeder = new Seeder(number.Next(0, 9999));
   seed = DateTime.Now.Second / seeder.getSeed(); 
  }
 }

 public KeyPair GenerateKeyPair()
 {
  if (seed == 0)
   randomNumber(); 
  return KeyGenerator.CreateKeyPair(seed); 
 }
}

First bug is ... it's not in VB.Net !



VB rules !

blowdart wrote:

Well it's not thread safe for a start (don't mind me, I'm on a thread safety jag at the moment!)

Correct, and an intentional bug.

blowdart wrote:

in the parameterised constructor created never gets set to true.

That isn't a bug as Seeder is never used in that case.

blowdart wrote:

in GenerateKeyPair the constructor is called again. What the hell?

It compiles; is there anything wrong with calling it again? Plus do you see any other problems in that general area?

Yeah this encryption method sucks, Use Rijandel.. I can help you out with that if you want.

- Steve

Not using the System.Security.Cryptography namespace. The use of a pseudo-random number generator is not good enough for generating encryption keys. A key generator should use a true random source of input.

Even if not used for security purposes, you should not constantly reseed the random number generator. That causes very poor randomness.

I'm guessing that seeder.getSeed() could return 0, in which case you'll get a division-by-zero exception. The range of values for seed in the computation is very small - between 0 and 59, and because presumably the value of seeder.getSeed() will normally be larger than 59 (99.4% of the time), seed will usually be 0. This very small range means that most of the time, GenerateKeyPair() will generate the exact same key pair.

Mike Dimmick wrote:

Not using the System.Security.Cryptography namespace. The use of a pseudo-random number generator is not good enough for generating encryption keys. A key generator should use a true random source of input.

100% correct and an intentional problem.

Mike Dimmick wrote:

I'm guessing that seeder.getSeed() could return 0, in which case you'll get a division-by-zero exception.

 
Which is the second 'major' bug in the code (The other being object not set to instance of object if another thread runs while created is set but the object hasn't been initialised).  Good job.


There is one more small error checking implementation bug in the code, see if you can spot it.

 

Manip wrote:

There is one more small error checking implementation bug in the code, see if you can spot it.

 

There is no need for using the ‘created’ variable since it is used only in the constructor of the class, and when creating a new instance of the randomNumber class would have its own ‘created’ variable, thus ‘created’ will never be true when creating an instance of this class (only after; and then it is just plain useless in the current implementation)

mot256

I would venture that one problem is relying upon the current time to be a seed. I seem to remember some research somewhere where numbers can be predicted if a seed is known. And since reflector can be used here, it can be reversed engineered quite interestingly. I'd do something more random.

if (seed == 0)
   randomNumber(); 

What is wrong with that?

If memory serves, Random() is a Pseduo-Random number generator, which has (potentially) some issues. Granted, it's theorical issues, but I just wanted to bring it up.

From the Wikipedia:
http://en.wikipedia.org/wiki/Pseudo-random_number_generator

Or, just read Knuth..

seeder = new Seeder(number.Next(0, 9999));
In this case it returns 0 as the number, try

seeder = new Seeder(number.Next(1, 9999));

- Steve

LarryOsterman wrote:

Manip wrote: Mike Dimmick wrote: Not using the System.Security.Cryptography namespace. The use of a pseudo-random number generator is not good enough for generating encryption keys. A key generator should use a true random source of input. 100% correct and an intentional problem.

I'm going to call you on that - you said that this was an encryption problem, and thus a poor choice of crypto algorithm is absolutely a bug, you can't claim it was intentional unless you explicitly state that the poor choice of random number generators was intentional.

The poor choose of random number generator for seeding the encryption algorithm (which wasn't posted above) *was* an intentional issue that I wanted people to think about.

I was far more interested in if people would spot the concurrency issue and threw the divide by zero and bad seed check in for kicks.

The naming of the thread was only to help me remember which problem it was later, and not to be an accurate description of the problem.

Plus this code is completely fictional and created for the sole purpose of spotting bugs, it doesn't exist in any code base I'm involved with.