Security question, how can this be hacked?

[phil88]

I'm learning PHP and am working on the security side of things so I can make my scripts secure, I understand about using various functions like mysql_real_escape_string() etc, but I want to be able to test stuff.


I made a (very) simple login script and was wondering, what could a user of the script do to the form or query from the browser that could;
a) Allow them to login without a password and/or username
b) Allow them to view passwords of users who aren't them

[code=php:0]
<?PHP
if ($_GET['Submit'] == FALSE){
echo "<form method='get' action='login.php'>";
echo "Username: <input type='text' name='username'>";
echo "<br>Password: <input type='password' name='password'>";
echo "<br><input type='submit' value='Submit' name='Submit'>";
echo "</form>";
}else{
mysql_connect("localhost", "user", "pass");
mysql_select_db("usershack");
$username = $_GET['username'];
$password = $_GET['password'];
$query = mysql_query(" SELECT * FROM users WHERE username='$username' AND password='$password' ");
while($row = mysql_fetch_array($query)){
echo $row['username'];
echo "<br>";
echo $row['password'];
}
}
?>[/code]

Like I said, very basic login script, I'm just working from the ground up as far as security goes.

[r-it]

an sql injection, you should encrypt your password and addslashes, use md5

[redbullmarky]

as r-it said - you're leaving yourself to SQL injections if you do not do any processing to input provided by the user - be it $_GET, $_POST or $_COOKIE (GPC). If you pop 'SQL Injection' into Google, you'll get a better idea of how it works and how to do it yourself, hence learning how to cover your tracks using 'addslashes' and other.
i have a little function i use for everything - $_POST, $_GET, the lot, which to now has worked very well for me:
[code]
<?php
function parse_input($g_p)
{
  $ret = array();
  foreach ($g_p as $key=>$value)
  {
      if (!is_array($value))
      {
        $value = strip_tags($value);
        $ret[$key] = get_magic_quotes_gpc() ? $value : addslashes($value);
      }
      else
      {
        $ret[$key] = parse_input($value);
      }
  }
  return $ret;
}
?>
[/code]
which strips out any html tags and also adds slashes depending on the PHP settings.
basically I just call it like:

[code]
<?php
$post = parse_input($_POST);
$get = parse_input($_GET);
?>
[/code]
and only refer to $_POST and $_GET values via my new $post and $get arrays.

I'm also not keen on having login scripts run on $_GET - basically, your html form presents the user with a password field (ie, starred out instead of showing the characters) but as soon as the form is submitted, the password becomes clear text in the URL. I'd recommend changing the 'action' of the form from 'get' to 'post' and also all the $_GET's to $_POST's.

cheers
Mark

[wildteen88]

Your script is very unsecure, and can lead to sql injection attacks, as r-it mentioned above. To prevent SQL Injection attacks you should consider using mysql_real_escape_string. It would be a good idea to encrypt the users password too, using md5 or sha-1

Also you should use the POST method rather than the GET method, as people will be able to see the username and password in the URL. Whereas POST sends the data in the background.

[Ninjakreborn]

mark is right having them set to get is the stupidest thing you could do.
Always use post on those kinds of things
Also if you just use mysql_real_escape_string();
and whatever else
maybe some extra safety, to make sure things don't go haywire, believe me
strtolower()
will stop all the upercase letters
you can do trim(" variable ")
which will prevent whitespaces
also validate the material
make sure it's not blank, ex cetera
I know most people won't agree, but I feel having  1 login box, not really makes things secure, but it makes it easier to run scripts on login.  I always end up having a lot of scripts, where I have to check for various things for the user and update if necessary, if there is only 1 login area, then you can do that easier, than having it go to a login.php redirrect page every time.

[phil88]

Thanks for all the help. I read up on MySQL injections and such and rewrote it, would this be secure?

[code=php:0]<?PHP

if($_POST['submit'] == FALSE){
echo "<form method='post' action='login.php'>
Username: <input type='text' name='username'><br>
Password: <input type='password' name='password'><br>
<input type='submit' value='Submit' name='submit'>
</form>";
}else{
$username = $_POST['username'];
$password = $_POST['password'];
if($username == FALSE){
echo "No username was entered.";
exit;
}elseif($password == FALSE){
echo "No password was entered.";
exit;
}else{
$username = htmlentities(mysql_real_escape_string($username));
$password = sha1($password);
mysql_connect("localhost","phil","apples");
mysql_select_db("usershack");
$query = mysql_query(" SELECT * FROM users
WHERE username='$username'
AND password='$password' ")
or die(mysql_error());
if(mysql_num_rows($query) == 0){
echo "No such user found or login details are incorrect.";
exit;
}else{
echo "Welcome back ".$username."!";
exit;
}
}
}
?>[/code]

Would there be any way to get around the login system or insert something dodgy to retrieve stuff from the database that shouldn't be retrievable? How secure are the $_POST variables? Can they be edited in anyway after the form has been submitted?


Also, once the user is logged in, what would be the most secure way of keeping track of them? Could I just create a session variable that'll be set to true when the user is logged in and false when they're logged out? How easy can $_SESSION variables be manipulated by the user?


Edit: I just read the sticky in this forum about error checking, so I'll sort out the way I've done it so it works better.

[Ninjakreborn]

No matter what you do, or how you do it, if someone wanted to do something bad enough to it, they would do something to it.  That is a fact, where there's a will there's a way, all you can do is work to make it as secure as possible, hope for the best and learn from your mistakes if you ever get hacked.
Even if you tried every possible way to make it  bullet proof, bullets could still get through, nothing you can do, but try your hardest to make it secure, while still having fun.
What you currently have looks pretty secure. but if your password is apples, then someone probably already got it, that's a shitty password, try letter's and numbers, you have to remember if someone get's the apssword they can connect from your database even from another website, unless there was a firewall behind it, and even then they sometimes could.  WHat I suggest, is if you haven't already create a good password.  Something with letters, and numbers, starting with a letter though.

[wildteen88]

Yes much better security wise. Also theres no need to use the htmlenties function with the mysql_real_escape_string.

However the layout of code could be better.

[phil88]

[quote]
What you currently have looks pretty secure. but if your password is apples, then someone probably already got it, that's a shitty password, try letter's and numbers, you have to remember if someone get's the apssword they can connect from your database even from another website, unless there was a firewall behind it, and even then they sometimes could.  WHat I suggest, is if you haven't already create a good password.  Something with letters, and numbers, starting with a letter though.[/quote]
Yeah, it's just a password on a test server I have for developing, totally disconnected from the outside world, only I have physical access to it and there's not really much you can do with it even if you do get access to it  ;D

[quote]Also theres no need to use the htmlenties function with the mysql_real_escape_string.[/quote]
Does mysql_real_escape_string escape HTML characters aswell then?

[quote]
However the layout of code could be better.[/quote]
What do you mean?

[Ninjakreborn]

THe only thing mysql_real_escape_string() doesn't do that needs to be done if magic quotes isn't on, is strip slashes for databasing.  and it may even do that.
php.net tells you everything it does on them.

[phil88]

Would using the function on php.net be a better choice than using mysql_real_escape_string?
This function;
[code=php:0]function quote_smart($value)
{
  // Stripslashes
  if (get_magic_quotes_gpc()) {
      $value = stripslashes($value);
  }
  // Quote if not a number or a numeric string
  if (!is_numeric($value)) {
      $value = "'" . mysql_real_escape_string($value) . "'";
  }
  return $value;
}[/code]

[wildteen88]

To stop html you should use strip_tags which will remove any html in a string. Yes that'll be better just incase a setting called magic quotes is enabled.

[phil88]

[quote author=wildteen88 link=topic=102152.msg405348#msg405348 date=1154115056]
To stop html you should use strip_tags which will remove any html in a string.
[/quote]
The idea wasn't so much to remove HTML, just to stop it being treated as HTML. I mean, some people would like their usernames to be <Username> for example, but if that was parsed as HTML, it would be invisible as it's between < and >.

[wildteen88]

mysql_real_escape_string includes the htmlentities function so there will be no need to use it again.

[tomfmason]

i use this. This may not be the most secure but it suits my purposes

[code=php:0]
<?php
include ('includes/db.php');
if (isset($_POST['submit'])) {
    array_pop($_POST);
    if ( get_magic_quotes_gpc() ) {
        $_POST= array_map('stripslashes', $_POST);
    }
    if ((!$username) || (!$password)) {
        echo"You did not enter the following required information";
        if (!$username) {
            echo "User name is a required field";
        }
        if (!$password) {
        echo "Password is a required field";
        }
        exit;
    }  
    $username= mysql_real_escape_string(trim($_POST['username']));
    $password= mysql_real_escape_string(trim($_POST['password']));
    $mdpwd= md5($password);

    $sql= sprintf("SELECT COUNT(*) AS login_match FROM `users` WHERE `username` = '%s' AND `password`= '%s'", $username, $mdpwd);
    $res= mysql_query($sql) or die(mysql_error());
    $login_match= mysql_result($res, 0, 'login_match');

    if ( $login_match == 1 ) {
         //loged in
    } else {
    // not logged in
}
}
//put your log in form here
?>[/code]

maybe this will give you some ideas.  For some reason the sql gets all messed up when pasting it here but this should give you a rough idea.

[HeyRay2]

What code are you using on your other php pages to ensure that users cannot bypass your login screen?

If you aren't already I would recommend using sessions. In your login code, add something like this:

[code=php:0]
if(mysql_num_rows($query) == 0){
echo "No such user found or login details are incorrect.";
exit;
}else{
                                      session_start();
                                      $_SESSION['username'] = $username;

echo "Welcome back ".$username."!";
exit;
}
[/code]

and on each page that you want to prevent unauthenticated users from accessing, add this around your page code

[code=php:0]
<?php

// initialize the session
session_start();

// Check if the user session was created
if(isset($_SESSION['username'])){

// YOUR CODE HERE
// YOUR CODE HERE
// YOUR CODE HERE

}
else{
// direct the user to the login page
Header("Location: /login.php");
}

?>
[/code]

[tomfmason]

this is a much better/ faster way of processing a login than [code=php:0]mysql_num_rows[/code]

[code=php:0]
$sql= sprintf("SELECT COUNT(*) AS login_match FROM `users` WHERE `username` = '%s' AND `password`= '%s'", $username, $mdpwd);
$res= mysql_query($sql) or die(mysql_error());
$login_match= mysql_result($res, 0, 'login_match');

if ( $login_match == 1 ) {
    session_start();
    $_SESSION['username'] = $username;
    echo "Welcome $username you are now loged in";
} else {
   echo "Your username/password do no match";
   // not logged in
}[/code]

And as far as the sessions at the top of each page I would do this.

[code=php:0]
session_start();
if (!$_SESSION['username']) {
  echo "You must log in to view this page";
  include('yourform.php");
  exit;
}[/code]

[Ninjakreborn]

it's good practice to do section even above the db calls.
the connection and everything
it should be above everything else
as far as
sprintf("
I disagree with that usage, but it's partially personal preference.

[phil88]

tomfmason:
I don't understand this line of your code;
$login_match= mysql_result($res, 0, 'login_match');

How can there be a row 0, and what should 'login_match' be if there isn't a field in the database called that?

HeyRay2:

Yes, I was going to use Sessions, but I have a question, how easily can they be modified by the user? I mean, would I need to run some sort of validation on them or could I just do a simple if(isset($_SESSION['logged_in']) to check if the user is logged in? (Assuming I set $_SESSION['logged_in'] when they log in)

[Chips]

[quote author=phil88 link=topic=102152.msg405480#msg405480 date=1154177295]
Yes, I was going to use Sessions, but I have a question, how easily can they be modified by the user? I mean, would I need to run some sort of validation on them or could I just do a simple if(isset($_SESSION['logged_in']) to check if the user is logged in? (Assuming I set $_SESSION['logged_in'] when they log in)
[/quote]

I am also interested in this piece of information as well. Apart from "other people on a shared server could see your sessions" type issues, is there any other security risk? No session id's are passed in my urls ever... I just use a $_SESSION['level'] (holds their access leve) to reveal certain parts of a site (ie special downloads, special news, admin functions etc).

Is that secure or can a user some way manipulate it. As usual, session_start() on each page, and then a check to see if they have the required level to see it. No cookies set afaik, just session variables. When browser closed, they are "logged out". Also, I didn't see a way to set a time limit on sessions like this - is there one? Or is it a php.ini alteration only? As far as I know to date it's secure, but with this topic tackling sessions and their security just now, I thought it would be a good idea to ask about it as well :)

[phil88]

[quote]Is that secure or can a user some way manipulate it. As usual, session_start() on each page, and then a check to see if they have the required level to see it. No cookies set afaik, just session variables. When browser closed, they are "logged out". Also, I didn't see a way to set a time limit on sessions like this - is there one? Or is it a php.ini alteration only? As far as I know to date it's secure, but with this topic tackling sessions and their security just now, I thought it would be a good idea to ask about it as well Smiley[/quote]
Well I guess one way to get around the time issue would be to set a cookie when the session is set, to expire after a certain amount of time. Then on each page you could call a function or something that'd check for the cookie, if the cookie is expired/isn't there, then destroy the session.

[tomfmason]

[quote author=phil88 link=topic=102152.msg405480#msg405480 date=1154177295]
tomfmason:
I don't understand this line of your code;
$login_match= mysql_result($res, 0, 'login_match');

How can there be a row 0, and what should 'login_match' be if there isn't a field in the database called that?

HeyRay2:

Yes, I was going to use Sessions, but I have a question, how easily can they be modified by the user? I mean, would I need to run some sort of validation on them or could I just do a simple if(isset($_SESSION['logged_in']) to check if the user is logged in? (Assuming I set $_SESSION['logged_in'] when they log in)
[/quote]

You do not need a field in your database called login_match. Just try the code and you will see that it works and processes the login faster then most other login scripts. See my post on the pervious page for the entire script

BusinessMan,
                why do you disagree with the usage of [code=php:0]sprintf()[/code] in the case.



By the way Business man is right about the sessions I would start the session at the very top of the script.

[phil88]

[quote]
You do not need a field in your database called login_match. Just try the code and you will see that it works and processes the login faster then most other login scripts. See my post on the pervious page for the entire script[/quote]
I'm sure it does work. I'm just curious as to know how it works and how it would be better than the standard mysql_num_rows way, is that function more resource intensive or something?

[tomfmason]

ok well this is faster manly because of the way that I choose to search the database

[code=php:0]$sql= sprintf("SELECT COUNT(*) AS login_match FROM `users` WHERE `username` = '%s' AND `password`= '%s'", $username, $mdpwd);[/code]

SELECT COUNT is a faster way of searching a database when counting how many matches you have. If you try to use [code=php:0]mysql_num_rows[/code] with SELECT COUNT then you will always return a value of 1 or a sucessful login. That is the reason I that I use mysql_result

Now I will explain [code=php:0]mysql_result[/code]

well here is what the manual says about it

string mysql_result ( resource result, int row [, mixed field] )
result
The result resource that is being evaluated. This result comes from a call to mysql_query().

row
The row number from the result that's being retrieved. Row numbers start at 0.

field
The name or offset of the field being retrieved.

It can be the field's offset, the field's name, or the field's table dot field name (tablename.fieldname). If the column name has been aliased ('select foo as bar from...'), use the alias instead of the column name. If undefined, the first field is retrieved

Good luck,
Tom

[phil88]

So with mysql_result, it'll return 1 if it finds a matching record, but if it doesn't find a matching record, it'll be false?