[PHP] Would you define this as POOR security?

[Delici0us]

Hello People,

So I’m stuck at a little cross-road at the moment, for some reason when I get into coding something I don’t stop and think about the security implications that my methods could cause. Hence why I’m asking for a second opinion on the matter.

So at the moment I’m using the following function to return data about a user to the script, however after thinking about it I’m certain it may cause problems down the road.

function UserInfo ($type)
        {
            $username = $_SESSION['USERNAME'];
            $var = $GLOBALS['DATA']->query("SELECT * FROM users WHERE username = '".$username."'");
            while ($row = $var->fetch_assoc())
                {
            switch ($type)
            {
                case "example":
                return $row['avatar'];
                break;
                case "example2":
                return $row['avatar2'];
                break;
            }
                }
        }

Is this a safe way of performing a search? Or is there another way I could perform the query better? All feedback would be appreciated.

 

[TimeBomb]

Poor security? Yes.
Exploitable security? Quite possibly. PHP sessions are server-side. If there is no way that the username session variable can contain direct user input, or characters that could result in a possible SQL inject, then you may be fine.

The answer?
The simplest way is to run mysql_real_escape_string on all variables going into a query.

The better, yet more advanced way, is to use MySQLi or PDO and take advantage of parametrized queries, i.e. binding parameters. This way, all variables are treated as variables, not part of the query, and thus, you are save from SQL injection.

 

[JaydenC]

Hello People,

So I’m stuck at a little cross-road at the moment, for some reason when I get into coding something I don’t stop and think about the security implications that my methods could cause. Hence why I’m asking for a second opinion on the matter.

So at the moment I’m using the following function to return data about a user to the script, however after thinking about it I’m certain it may cause problems down the road.

function UserInfo ($type)
        {
            $username = $_SESSION['USERNAME'];
            $var = $GLOBALS['DATA']->query("SELECT * FROM users WHERE username = '".$username."'");
            while ($row = $var->fetch_assoc())
                {
            switch ($type)
            {
                case "example":
                return $row['avatar'];
                break;
                case "example2":
                return $row['avatar2'];
                break;
            }
                }
        }

Is this a safe way of performing a search? Or is there another way I could perform the query better? All feedback would be appreciated.

I assume your approching a habbo-search tool. I would do something like so, your WAY overthinking it.

<?php
if(isset($_POST['****'];)){
$usernamesearched= $_POST['usernamesearchedfor'];
$usernamese= mysql_escape_real_string($usernamesearched);
$usernamese= stripslashes($usernamese);
$sql = "SELECT FROM users WHERE=' . $usernamese . '";
$query = mysql_query($sql);
$num = mysql_num_rows($query);
if($num=="1"){
//This is where it shall display the users information..i leave this to you.
}else{
echo "The user was not found, double check the username";
}
else{
header("Location: pages/page.php");
}
?>

My code is probably incorrect, but it works (I belive).
Just tweak it a bit.

 

[TimeBomb]

I assume your approching a habbo-search tool. I would do something like so, your WAY overthinking it.

<?php
if(isset($_POST['****'];)){
$usernamesearched= $_POST['usernamesearchedfor'];
$usernamese= mysql_escape_real_string($usernamesearched);
$usernamese= stripslashes($usernamese);
$sql = "SELECT FROM users WHERE=' . $usernamese . '";
$query = mysql_query($sql);
$num = mysql_num_rows($query);
if($num=="1"){
//This is where it shall display the users information..i leave this to you.
}else{
echo "The user was not found, double check the username";
}
else{
header("Location: pages/page.php");
}
?>

My code is probably incorrect, but it works (I belive).
Just tweak it a bit.

Replace:

$usernamesearched= $_POST['usernamesearchedfor'];
$usernamese= mysql_escape_real_string($usernamesearched);
$usernamese= stripslashes($usernamese);
$sql = "SELECT FROM users WHERE=' . $usernamese . '";

with:

$usernamesearched= mysql_escape_real_string($_POST['usernamesearchedfor']); // Can apply function to this variable. stripslashes is unnecessary.
$sql = "SELECT FROM users WHERE username ='$usernamesearch'"; // Using a period is only necessary for concatenation of variables to strings. Concatenation is completely unnecessary when using double quotes around the string. It is necessary when using single quotes. Double quotes will parse the PHP variable. Also, you forgot to put the column name (username) after WHERE.

 

[Delici0us]

Poor security? Yes.
Exploitable security? Quite possibly. PHP sessions are server-side. If there is no way that the username session variable can contain direct user input, or characters that could result in a possible SQL inject, then you may be fine.

The answer?
The simplest way is to run mysql_real_escape_string on all variables going into a query.

The better, yet more advanced way, is to use MySQLi or PDO and take advantage of parametrized queries, i.e. binding parameters. This way, all variables are treated as variables, not part of the query, and thus, you are save from SQL injection.

Thanks, you always seem to have a good reply. The Session is not set by the user, it is set by the script after the login has been verified, and I’ve already escaped the strings beforehand (so I didn’t show that in this post).

However, I am using SQLI and because I split the classes up into several files I defined the connection as a GLOBAL (However, this is most probably poor security or bad coding etiquette in itself)

I assume your approching a habbo-search tool. I would do something like so, your WAY overthinking it.

<?php
if(isset($_POST['****'];)){
$usernamesearched= $_POST['usernamesearchedfor'];
$usernamese= mysql_escape_real_string($usernamesearched);
$usernamese= stripslashes($usernamese);
$sql = "SELECT FROM users WHERE=' . $usernamese . '";
$query = mysql_query($sql);
$num = mysql_num_rows($query);
if($num=="1"){
//This is where it shall display the users information..i leave this to you.
}else{
echo "The user was not found, double check the username";
}
else{
header("Location: pages/page.php");
}
?>

My code is probably incorrect, but it works (I belive).
Just tweak it a bit.

Thanks for the reply but it has nothing to do with Habbo, just a little proof of concept design.

 

[JaydenC]

No problem
I just assumed considering you said search..and habbo..well ya' know.

 

[Delici0us]

No problem
I just assumed considering you said search..and habbo..well ya' know.

Yeah I know what you’re saying. The function was an idea of pulling the information related to the authenticated user from the database.

 

[JaydenC]

Yeah I know what you’re saying. The function was an idea of pulling the information related to the authenticated user from the database.

Why are you using a while loop btw? That is for a script that used strops to search the post' data and find anything related.
For example, There are 2 users one named delete and another delete2 if you used the strops function to search for "delete" it would output the data. ya' feel?

 

[s-p-n]

You can just do it like this,

function UserInfo ($type) 
{ 
	return mysql_fetch_assoc(
		$GLOBALS['DATA']->query("
			SELECT * 
			FROM users 
			WHERE username='{$_SESSION['username']}' 
			AND password='{$_SESSION['password']}'
		")
	);
}

Just make sure you use mysql_real_escape_string() and proper XSS protection before you put any data in the sessions.

I always check username and password.. Then if people figure out a way to set session vars, they still need to set the password or use a MySQL injection in order to get data.

 

[JaydenC]

You can just do it like this,

function UserInfo ($type) 
{ 
	return mysql_fetch_assoc(
		$GLOBALS['DATA']->query("
			SELECT * 
			FROM users 
			WHERE username='{$_SESSION['username']}' 
			AND password='{$_SESSION['password']}'
		")
	);
}

Just make sure you use mysql_real_escape_string() and proper XSS protection before you put any data in the sessions.

Why use $_SESSION? Stupid idea.. Especially if its a search. It will inerfere with other standing sessions.

 

[s-p-n]

Why use $_SESSION? Stupid idea.. Especially if its a search. It will inerfere with other standing sessions.

What standing sessions and How so?

 

[JaydenC]

What standing sessions and How so?

$_SESSION['username'] would be in use, no? Why is it session'ing something that is in a search? This is weird.. either a post or a get.

 

[s-p-n]

$_SESSION['username'] would be in use, no? Why is it session'ing something that is in a search? This is weird.. either a post or a get.

Why would you pull all of the user-data from an account using GET/POST? That would be silly. This function would get the user data for the logged-in user. Sessions should be unique to each individual user, yeah?

You should never load all of the user data to the public.

In other words, I think OP was trying to get account data for one user's session, and you think OP is trying to load an account for anyone to see.

You're right in your case- sessions wouldn't make sense. I don't think it would be good to do that, though.

 

[Delici0us]

Why would you pull all of the user-data from an account using GET/POST? That would be silly. This function would get the user data for the logged-in user. Sessions should be unique to each individual user, yeah?

You should never load all of the user data to the public.

In other words, I think OP was trying to get account data for one user's session, and you think OP is trying to load an account for anyone to see.

You're right in your case- sessions wouldn't make sense. I don't think it would be good to do that, though.

Your right. I'm getting the info for the session user. Nobody else :

 

[Chris]

Why use $_SESSION? Stupid idea.. Especially if its a search. It will inerfere with other standing sessions.

If you're grabbing the session variables to use in a query, that in no way changes, or even effects the session variables.

 

[JaydenC]

If you're grabbing the session variables to use in a query, that in no way changes, or even effects the session variables.

When he uses the $_SESSION['username'] for a search (In wich case I was led to belive that it was a search of other users) it would interfere with the search. For example, If you had a session under the usersname Chris and then you searched Mark it would appear as Chris on the found results, resulting FROM the session being started for 'chris'. But I was incorrect, but in that case..that would be the problem.

 

[Delici0us]

When he uses the $_SESSION['username'] for a search (In wich case I was led to belive that it was a search of other users) it would interfere with the search. For example, If you had a session under the usersname Chris and then you searched Mark it would appear as Chris on the found results, resulting FROM the session being started for 'chris'. But I was incorrect, but in that case..that would be the problem.

I’m not searching for other users; it’s not what you think it is. The session variable never changes once set at login, only at logout to cleanly destroy it.

$SESSION[‘USERNAME’] = ‘George’; is set once I have logged into my website, my script is not a search feature it is returning the database rows for the logged in user to view. For example nobody but the authenticated user is making use of this function.

 

[JaydenC]

I’m not searching for other users; it’s not what you think it is. The session variable never changes once set at login, only at logout to cleanly destroy it.

$SESSION[‘USERNAME’] = ‘George’; is set once I have logged into my website, my script is not a search feature it is returning the database rows for the logged in user to view. For example nobody but the authenticated user is making use of this function.

I know, I said i was wrong
As timebomb said, mysql_escape_real_strings would need to be wrapped aroung your variables, for security. Even if there is no point to it, just do it to be safe.

 

[Delici0us]

I know, I said i was wrong
As timebomb said, mysql_escape_real_strings would need to be wrapped aroung your variables, for security. Even if there is no point to it, just do it to be safe.

Like I said to him, the information is escaped before being placed into a session. I just never added that code to this little thread

 

[s-p-n]

Even if there is no point to it, just do it to be safe.

Computers don't make mistakes- you should only code things once, and code them right the first time. Um, it's good to have that mindset of security- better safe than sorry, but ideally you should only do things once- and in terms of security, as early in the script as possible- to be safe.

For example, if you did security right before you sent data to the database, you would need to repeat code everytime you write data to the database. Having security in the GET/POST API will catch everything before they can become a problem anywhere in the program's execution.