Is it worth fixing an admin system which had REGISTER GLOBALS on?

Question

I have an admin site that I have copied over to a new server to test for bugs and put live, along with some other sites.

The admin appears to have REGISTER GLOBALS on and is using it for most of the 300 php files.

Based on the fact that you have to login to this system anyway is it worth the weeks of work to re code all the variables?

Or be happy that I would fix each page as I add any new feature to it in the future?

Does Register Globals leave problems in code that has been cleaned, if we don't fix all at once? I'm guessing it could as $user_id can be set by any global.

Answer 1

Register_Globals is insecure and shouldn't be used. If I were you I would rewrite the code, or the application itself from scratch. However if this is an admin system, and no one knows its URL and therefore only the admin himself can access it, then you should be fine with not changing it (just make sure its URL remains a secret)

Answer 2

It can lead to some very serious security issues, it kinda depends on how it's coded.

For example:

<?php
// $loggedin comes from $_SESSION['loggedin']
if($loggedin)
{
    echo 'Loggedin!';
}
else
{
    echo 'Please login';
}
?>

The main problem here is, is that the script doesn't check where $loggedin comes from. So if I would do script.php?loggedin=1, I would be logged in. Seeing as there are ~300 PHP files, it would be hard to check everything.

So keeping it is a (very) bad idea. Even it you'd use .htaccess to block access, it would probably lead to problems in the future (IE web hoster setting REGISTER_GLOBALS off) and confusion.

Answer 3

Don't re-write the entire system. If the system works and you'll casually upgrade as you go you don't need to start from scratch.

I would weigh the importance of addressing the Register Globals based on the sensitivity of the information.

If it's a well built system you should be able to see what variables are used throughout the site and simply make them available at the top of the pages. Be wary of any functions that pull in their data through global $this, $that;

My vote, if the data is important to protect, is to do the work.

Answer 4

This app could be littered with many other shoddy programming practices as well. (How large is the app to warrant 300 php files?). If that's the case, it might be a good idea to leave the app as it is and code a new version from scratch on top of a decent framework if maintenance has already become too troublesome.

Answer 5

This depends on your quite a lo t of things. Just to name a few:

1. Company's security policy
2. Cost to rewrite
3. Importance of the application
4. Impact on other parts of the application.
5. etc

Answer 6

I would disable register_globals from the php.ini, and put a code block at the top of each script that extracts the variables from the $_REQUEST, $_GET or $_POST, something like:

$nVars = extract($_GET, EXTR_SKIP);

The above code will register variables by the same name as the key in the passed array. It is useful for quickly refactoring old REGISTER_GLOBALS enabled code, but you must be careful. Read the following excerpt from the PHP extract() documentation:

Do not use extract() on untrusted data, like user-input ($_GET, ...). If you do, for example, if you want to run old code that relies on register_globals temporarily, make sure you use one of the non-overwriting extract_type values such as EXTR_SKIP and be aware that you should extract in the same order that's defined in variables_order within the php.ini.