Security is not easy

Update: After a year, both plugins are finally updated with a better random key generator.

Security is not easy. Programmers should leave things like random number and identifier generation to a library (or at least research the best way to do it). A lot of projects learned it the hard way.

Let’s talk for instance of a function I encountered about six months ago:

function generateRandomKey($len = 20)
{
  $string = '';
  $pool   = 'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789';
  for ($i = 1; $i < = $len; $i++)
  {
    $string .= substr($pool, rand(0, 61), 1);
  }
 
  return md5($string);
}

It is a real gem as it seems it was meant to concentrate everything a programmer should not do.

Flaw 1: Using rand() instead of mt_rand(). Any PHP programmer should know this.

Flaw 2: Using md5() instead of sha1(). Any PHP programmer should know this too.

Flaw 3: Generating a 20 character random string and then hashing it to a 32 character string looks a bit silly. Granted, the hash has less different characters, but still the input can have 7.04423e+35 different values while the output can have 3.40282e+38.

Flaw 4: Calling rand() multiple times. It denotes a strong ignorance on how pseudo-random number generators work. Since the seed doesn’t change, it can be easy to guess what will be the next number from the previous ones. It isn’t “more random” to call it multiple times.

Flaw 5: Using a very limited range for picking a random number. Also, why choose alphanumeric characters if you are not using them afterwards since they are passed to a hash function? This seems to be very badly designed.

Flaw 6: Feeding md5 with alphanumeric characters, present in all the rainbow tables in the world.

Flaw 7: Ever heard of chr()?

Flaw 8: Why reinvent the wheel? At least if you have a reason, leave a comment (no comments at all on this function).

Without even testing if the results are good or not, any programmer should know this function should be completely rewritten.

Let’s see how it should have been done.

function generateRandomKey()
{
 
  return base_convert(sha1(uniqid(mt_rand(), true)), 16, 36);
}

How did I come up with this? Mostly be reading the PHP manual and not creating a random number generator, but merely using one.

The use of base_convert() is not really important, it is mostly because the database field was too short (nothing is lost from the sha1()). We actually would be better off without sha1() though, it is there mostly for the eye-candy.

uniqid() will take the result of mt_rand() and append it an unique value (but in a predictable range, hence the use of both functions). This is recommended by the PHP manual and is more than enough.

Now you’re probably asking yourself where did I find this piece of code.

It is in sfGuardPlugin and sfDoctrineGuardPlugin, for the symfony framework. It’s here almost from the start, though it was even wronger. Those are by far the most used plugins, and are endorsed by the official documentation.

What does this mean? On some systems, it will be very easy to brute-force the cookie to log in as any user. Or an user could accidentally become logged as another one, since collisions are highly likely.

Come on, if this is real, someone would have spotted it! I is probably not exploitable.
Actually, it is. I have been able to reproduce it on an older Debian server, and on this server was a website where users reported multiple times getting identified as another user (hopefully this server is retired and the project now runs on the fixed function).
I also tried to reproduce it on a Windows XP virtual machine (since it was the easiest way to get a PHP with a flawed random number generator) with success. Since on these two machines I got a lot of collisions, even on better systems the flaw should be exploitable with some effort.

Why am I doing this?
Because I reported it six month ago, and it received very little attention.
I think symfony is great, but the handling of security isn’t (though my other criticisms are more on standard programming or hosting practices encouraged by symfony than on the code; more on the latter in a coming article, be sure to subscribe).

This entry was posted in PHP, Security, Symfony and tagged , , , , , . Bookmark the permalink. Post a comment or leave a trackback: Trackback URL.

8 Comments

  1. Posted 2010-02-07 at 2028 | Permalink
    Google Chrome 4.0.269.0 Google Chrome 4.0.269.0 GNU/Linux x64 GNU/Linux x64
    You obviously know a lot more about the problems associated with random generation than I do, but it seems like a no-brainer! Are there any drawbacks at all from replacing the current code with your patch?
  2. Posted 2010-02-08 at 0004 | Permalink
    Firefox 3.5.7 Firefox 3.5.7 Gentoo x64 Gentoo x64
    I don’t know much either. The main difference is that there is no algorithm in my solution; I trust the functions for doing something better than what I could ever do.

    Just using mt_rand() should be fine too, and maybe better.

  3. Bitcoder
    Posted 2010-02-10 at 1741 | Permalink
    Firefox 3.5.5 Firefox 3.5.5 Windows XP Windows XP
    You’re right man! of course nobody pay attention because its fre, only if good contributors like you put good code into the escenary our dear friend Potencier will add that to framework.
    We hope Fabien read this!

    Good job!

  4. Posted 2010-02-23 at 2211 | Permalink
    Opera 10.10 Opera 10.10 GNU/Linux GNU/Linux
    I couldn’t understand – should Fabien agree with you too, to fixed-close the ticket?
  5. Posted 2010-02-26 at 1917 | Permalink
    Firefox 3.6 Firefox 3.6 Gentoo x64 Gentoo x64
    Have you tried sending an email to security at symfony-project.com? I know that sometimes gets a quicker response.
  6. halfer
    Posted 2010-07-14 at 1208 | Permalink
    Firefox 3.6.4 Firefox 3.6.4 Windows XP Windows XP
    Hi Laurent,

    Well done; looks like you’ve done some excellent research on this one, and barring any problems with your solution, I hope it gets implemented. I agree: security is not easy!

    However, I do wonder if the general approach and tone might be somewhat dispiriting for your target audience (core team members). As a key moderator for the symfony forums, I sometimes see product feedback which is, sadly, borderline abusive, even though it is rarely intended as such. One recent thread on the quality of symfony documentation suggested that the work on the docs “sucked”, the quality is getting worse, that symfony is unsuitable for business solutions, and that their author should be ashamed of them. Not at all the kind of polite and constructive feedback I would want to hear if I was the author in question, especially if the complainants had received my work free of charge!

    I am sure the author of the sfGuard method can take criticism in their stride. But, it is easier to swallow if it is couched in +gentle+ terms. Phrases like “silly”, “strong ignorance”, “badly designed”, “every programmer should know”… are all likely to offend. Programmers, after all, are only human.

    One good approach to criticism is: “would I say this to someone in person”? If the answer is “definitely not”, then it may be wise to avoid using that phrase in print.

  7. Posted 2010-07-14 at 1348 | Permalink
    Firefox 3.6.6 Firefox 3.6.6 Gentoo x64 Gentoo x64
    The thing is that the core team doesn’t care about this issue. They don’t even seem to care enough to respond. This post was written months after reporting it.

    Meanwhile, Symfony is advertised as an enterprise-ready framework.

  8. halfer
    Posted 2010-07-14 at 1612 | Permalink
    Firefox 3.6.4 Firefox 3.6.4 Windows XP Windows XP
    Yep, I take your point there. Quite strange, given that they’ve been super-fast on security reports in the past.

Post a Comment

Your email is never published nor shared. Required fields are marked *

You may use these HTML tags and attributes: <a href="" title=""> <abbr title=""> <acronym title=""> <b> <blockquote cite=""> <cite> <code> <del datetime=""> <em> <i> <q cite=""> <s> <strike> <strong>

*
*