Lock down DB config based on simpletest UA headers

Moving the discussion from #443154: Fatal errors in tests not reported as failures about security

#57
Dries - July 11, 2009 - 07:28

I wonder if this opens a security door:

+if (isset($_SERVER['HTTP_USER_AGENT']) && preg_match("/^simpletest\d+$/", $_SERVER['HTTP_USER_AGENT'])) {
+  // Log fatal errors.
+  ini_set('log_errors', 1);
+  ini_set('error_log', file_directory_path() . '/error.log');
+}

It basically enables people to write to an error.log stored in the publicly accessible file directory. This change makes me a bit nervous.

#63
boombatower - July 13, 2009 - 14:53

Something we had in SimpleTest when it went in core originally (or close to that time) was a key that was used in addition to the db prefix to confirm that SimpleTest was the one sending the prefix. The key is much easier to check than other methods (db or files), but technically isn't quite as secure. Once someone figures out the key (not easy of course) it doesn't matter if SimpleTest is doing anything. Although we can just neglect to generate the key until SimpleTest is installed...end remove the key on uninstall.

Not sure if drupal_get_private_key() would be of use here to use as the key.

Personally that the key seems like the simplest and least expensive option. One issue to consider although is that we need a way to check before invoking the db.

The need to check before setting the DB prefix is the real problem - I'm tempted to suggest that we should go back to requiring that a value be set in settings.php. Given that most people now rely on test bot, I don't think this would be a real impediment to development. Also, right now we do things like setting the DB prefix without any check that the Simpletest module is installed - that also seems a little odd.

To be more careful you could use the value in settings.php as a key to generate a simple hmac as part of the user agent header so that you are never sending the secret value. Or we could use a hash of the $databases variable as the secret key to avoid needing to set an extra value?

So the code in function curlInitialize() could look like:

    global $databases;

      if (preg_match('/simpletest\d+/', $db_prefix, $matches)) {
        $key = sha1(serialize($databases), TRUE);
        $salt = uniqid('', TRUE);
        $ua = $matches[0] . '/' . time() . '/' . $salt;
        $hmac = base64_encode(hash_hmac('sha1', $ua, $key, TRUE));
        $curl_options[CURLOPT_USERAGENT] = $ua . '/' . $hmac;
     }

This formulation would allow you to check that the request time when you receive it is within (e.g.) 2 or 5 seconds. Since you are always making these requests to yourself, clock synchronization is not an issue.

Obviously this adds a tiny overhead, but an attacker's ability to arbitrarily make such requests is now as secure as your DB pw, and their ability to sniff the header and replay it is minimized.

also fwiw, "The Hash extension requires no external libraries and is enabled by default as of PHP 5.1.2. It may be explicitly disabled by using the --disable-hash switch to configure. "