Extremely Poor Style of Objects

by Mike Willbanks on August 18th, 2005

Our business deals alot with implementing features into peoples old applications that other companies online developed for them. While doing this we run into a ton of poorly coded applications.

This current one I am working on attempts to use the variable $this as you would in an object when assigning a class wide variable. Take a peak at this example code:

$this->c=mysql_query("select * from users where uid='$myuid'");

Now this is extremely wrong. First of all this file is not an object. While mysql_fetch_object creates an object that would be correct to simply use a variable called $d. But yet again your variables should be descriptive…. Also the usage of the variable being inputted to mysql to login has no variable checking at all. This user id is part of the session and could easily be manipulated from the login… Here is the source of this:

$username = $_POST['username'];
$password = $_POST['password'];

$query = "SELECT * FROM users WHERE username='$username' AND password='$password'";

As you can see this could quite easily be manipulated if magic_quotes are off. There is also no checking of the values which can flag notices if nothing is inserted and/or no checking on the values themselves. In this case it is not but still there should be no assumptions with this type of data.
Assuming magic_quotes are off you could simply put use the username as ‘ or 1=1 and the same thing for password. Now you will be authenticated to move on to the next section. This is much easier if you know a username because then you can access authenticated users.

A better way to write those two code snipplets is as follows:

$myuid = "'".mysql_real_escape_string($myuid)."'"; //add the quotes in this step around the data.
$query = mysql_query('select * from users where uid='.$myuid);

//I would add more complex checking with regular expressions in this data but just a quick example.
if (empty($_POST['username']) || empty($_POST['password'])) {
exit('No input for username and/or password!');

$username = "'".mysql_real_escape_string($_POST['username'])."'";
$password = "'".mysql_real_escape_string($_POST['password'])."'";
//add in a limit to only select one. I would also only select the data you need!
$query = 'SELECT * FROM users WHERE username='.$username.' AND password='.$password.' LIMIT 1';

This should help for a quick example. Remember although to do a better job of filtering :)

From PHP

Leave a Reply

Note: XHTML is allowed. Your email address will never be published.

Subscribe to this comment feed via RSS