Code Penguin (Laurent Bachelier)

Security is not easy

By Laurent | Published: 2010-02-07

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).

Share:

Posted in PHP, Symfony | Tagged entropy, full disclosure, PHP, random numbers, security, Symfony | 2 Comments

Extract from Doctrine_Record

By Laurent | Published: 2009-07-03

 
    /**
     * returns an array of modified fields and associated values
     * @return array
     * @todo What about a better name? getModifiedFields?
     */
    public function getModified()
    {
        $a = array();
 
        foreach ($this->_modified as $k => $v) {
            $a[$v] = $this->_data[$v];
        }
        return $a;
    }
 
    /**
     * REDUNDANT?
     */
    public function modifiedFields()
    {
        $a = array();
 
        foreach ($this->_modified as $k => $v) {
            $a[$v] = $this->_data[$v];
        }
        return $a;
    }

Share:

Posted in PHP, Symfony | Tagged doctrine, funny, Symfony | Leave a comment

Extending plugins in PHP and Symfony

By Laurent | Published: 2009-06-22

Plugins are great but they are never what you exactly wanted. When they are designed properly, the best way to customize them is to extend them instead of directly editing them.

Now, imagine I have:¹

# Penguin.class.php
class Penguin
{
  public function __construct()
  {
    echo "Windows is bad\n";
  }
}

# Herd.class.php
class Herd
{
  public function __construct()
  {
    while ($i++ < 42) new Penguin();
  }
}
 
# test
new Herd();

I want to change Penguin's behavior to something a bit more positive. However I don't want to extend Herd especially if it's tied to Penguin everywhere... I'm stuck with the Penguin class and can't use another one.

With some languages you can alter classes dynamically, and it can be referred as monkeypatching. While it is actually possible in PHP, it is ugly at best; you end up typing code in character strings, losing proper syntax highlighting and the opcode caching. Unless there is a very good language support for these methods it's best to avoid them.

A handful of plugins for Symfony, like sfGuardPlugin (one of the most popular plugins) already have some kind of solution. They come with two classes, a dummy one and a real one:

# Penguin.class.php
class Penguin extends pluginPenguin
{
}

# pluginPenguin.class.php
class pluginPenguin
{
  public function __construct()
  {
    echo "Windows is bad\n";
  }
}

Which means you just have to edit the (almost) blank Penguin class.
This is a clean, object-oriented way to solve the problem.

However, there are practicals problems that will arise.

You still edit an existing file; your modifications would be erased by upgrading to a newer version of the plugin
It will confuse version control systems if you use one to retrieve the plugin
Your custom code is in the plugin directory, which is just illogical
You could chose no to ship any of the blank files, but the user would have to create all of them

There is a very simple solution to overcome all that: the Symfony autoloader will pick classes from local folders (like the lib folder of your project) first.
Which means you can just copy the Penguin.class.php file and customize it:

# Penguin.class.php your project's lib/ folder
class Penguin extends pluginPenguin
{
  public function __construct()
  {
    echo "Linux is good\n";
  }
}

If you don't use Symfony and/or a similar autoloader, there is another solution:

# Penguin.class.php in the sfHerdPlugin directory
require dirname(__FILE__).'/plugin/plugin'.basename(__FILE__);
 
if (is_readable(dirname(__FILE__).'/../sfHerdPluginCustom/'.basename(__FILE__)):
  require dirname(__FILE__).'/../sfHerdPluginCustom/'.basename(__FILE__);
else:
  class Penguin extends pluginPenguin
  {
  }
endif;

# Penguin.class.php in the sfHerdPluginCustom directory (optional)
class Penguin extends pluginPenguin
{
  public function __construct()
  {
    echo "Linux is good\n";
  }
}

The cool aspect is that if you don't create any corresponding files in the sfHerdPluginCustom directory, it will still work perfectly.

The Herd and Penguin classes are references to GOTO++, a funny programming language. [↩]

Share:

Posted in PHP, Symfony | Tagged autoloading, inheritance, monkeypatching, PHP, plugins, Symfony | Leave a comment

Musings on software, internet and technology by Laurent Bachelier

Security is not easy

Extract from Doctrine_Record

Extending plugins in PHP and Symfony

Categories

Blogroll

Cool stuff

Me elsewhere

Feeds

Archives

Meta

Recent Posts

Recent Comments