Page 1 of 2

PHP Security Question

Posted: Sat Dec 13, 2008 5:44 pm
by cavemaneca
I have this log in page for a admin console on a business website I have made. Since it's important, and I want to know if this is secure or not. I have been in programming for awhile, but I'm new to php, so I don't know the specifics.
Is there a more secure way of sending information from a form? And, is encrypting with crypt() and using the previously encrypted password as the salt sufficient?
Note: rmv_sp_chars remove all of the following from both the password and name, and additionally I remove all spaces from the password

Code: Select all

`~!@#$%^&*()_-+=|\"':;?/>.<,

Code: Select all

<html>
<head></head>
<body>
<div align="center">
<?php 
        session_start();
if ($_SESSION['auth'] == 1) { 
    // check if authentication was performed 
    echo 'You Are Already Logged In!';
} 
else {
if (isset($_POST['name']) || isset($_POST['pass'])) { 
    // form submitted 
    // check for required values 
    if (empty($_POST['name'])) { 
        die ("ERROR: Please Enter Username!"); 
    } 
    if (empty($_POST['pass'])) { 
        die ("ERROR: Please Enter Password!"); 
    } 
 
   
    define('IN_SCRIPT',1);
    require_once('db_settings.inc.php');
    require_once('settings.inc.php');
    require_once('input.inc.php');
 
    $name = sanitize($_POST['name'], true);
    $pass = sanitize($_POST['pass']));
 
    $query = "SELECT * FROM users WHERE user = '" . $name . "'";     
    $result = mysql_query($query) or die ("Error in query: $query. " . mysql_error()); 
    
        if (mysql_num_rows($result) < 1) {
        die('ERROR: Incorrect Username!');
        }
        else {    
    $row = mysql_fetch_row($result);
        $salt = $row[1];
        }
         
    // create query 
    $query = "SELECT * FROM users WHERE user = '" . $name . "' AND pass = '".crypt($pass, $salt)."'";
     
    // execute query 
    $result = mysql_query($query) or die ("Error in query: $query. " . mysql_error()); 
     
    // see if any rows were returned 
    if (mysql_num_rows($result) == 1) { 
        // if a row was returned 
        // authentication was successful 
        // create session and set cookie with username  
        $_SESSION['auth'] = 1; 
        setcookie("username", $name, time()+(84600*30)); 
        echo "Access Granted!"; 
    } 
    else { 
        // no result 
        // authentication failed 
        echo "ERROR: Incorrect Password!"; 
    } 
     
    // free result set memory 
    mysql_free_result($result); 
     
    // close connection 
    mysql_close($connection); 
} 
else { 
    // no submission 
    // display login form 
?> 
    <html> 
    <head></head> 
    <body> 
    <center> 
    <form method="post" action="<?php echo $_SERVER['PHP_SELF']; ?>"> 
    Username: <input type="text" name="name" value="<?php echo $_COOKIE['username']; ?>"> 
    <p /> 
    Password: <input type="password" name="pass"> 
    <p /> 
    <input type="submit" name="submit" value="Log In"> 
    </center> 
    </body> 
    </html> 
<?php 
} 
}
?>
 
</body>
</html>

Re: PHP Security Question

Posted: Sat Dec 13, 2008 5:50 pm
by jaoudestudios
You can only improve form security with SSL (not entirely true because javascript can help, but then again the user can just turn it off!).

However your sql queries are not secure!

Re: PHP Security Question

Posted: Sat Dec 13, 2008 9:51 pm
by cavemaneca
what is wrong with the queries and where can I learn how to fix them? As well, is the encryption method I used for passwords sufficient?

Re: PHP Security Question

Posted: Sun Dec 14, 2008 5:08 am
by jaoudestudios
Go to php.net they have loads of info on mysql queries and security.

No, sorry, crypt is not really good enough.

Re: PHP Security Question

Posted: Mon Dec 15, 2008 6:37 am
by kaisellgren
What you have made is an extra hassle, not an improvement of security. Search for SQL Injection attacks on Google and this forum, and use SSL to secure your form.

Re: PHP Security Question

Posted: Mon Dec 15, 2008 7:22 pm
by cavemaneca
I'm not exactly sure how to use SSL, but I'm working on it. Also, I tried every type of sql injection I could think of or find on google and none of them ended up in the query, but I am still working to improve the function I built to sanatize the input.

Also, what makes crypt a less secure method, and what would be the best way of doing it?

EDIT: Also, certain parts of the site use different users with specific permissions. i.e. the login page can only use SELECT because it is the only one needed.

EDIT2: How about this function for the passwords?(pulled from sha1 at php.net)

Code: Select all

<?php
function obscure ($hString, $hDecode = NULL, $hSLength = 10, $keepLength = NULL, $minhPass = 10, $hMethod = sha1)
{
    if ($hDecode == NULL)
    {
        for ($i = 0; $i<16; $i++)
        {
            
            $hSalt = rand(33, 255);
            $hRandomSalt .= chr($hSalt);
        }
        $hRandomSalt = hash($hMethod, $hRandomSalt);
    }
    else
    {
        $hRandomSalt = $hDecode;
    }
 
    if ($keepLength != NULL)
    {
        
        if ($hSLength > (strlen($hRandomSalt) - $minhPass))
        {
            $hSLength = (strlen($hRandomSalt) - $minhPass);
        }
    }
    else if ($hSLength < 0)
    {
        $hSLength = 0;
    }
 
    $hLPosition = strlen($hString);
 
    while ($hLPosition > $hSLength)
    {
        $hNumber = substr($hLPosition, -1);
        
        $hLPosition = $hLPosition * ($hNumber/10);
    }
 
    $hLPosition = (integer)$hLPosition;
    $hRPosition = $hSLength - $hLPosition;
 
    $hFSalt = substr($hRandomSalt, 0, $hLPosition);
    $hLSalt = substr($hRandomSalt, -$hRPosition, $hRPosition);
 
    $hPassHash = hash($hMethod, ($hLSalt . $hString . $hFSalt));
 
    if ($keepLength != NULL)
    {
        if ($hSLength != 0)
        {
            $hPassHash = substr($hPassHash, $hLPosition, -$hRPosition);
        }
    }
 
    return $hFSalt . $hPassHash . $hLSalt;
}
?>
EDIT: since I couldn't find a better way, I made this long and boring function.
It is pretty bulky, but I haven't noticed any lag from using it. It just looks ugly.

Code: Select all

function sanitize($string, $allow_space = false, $allow_special = false) {
  $length = strlen($string);
  for ($i = 0; $i <= $length; $i++) {
    $length = strlen($string);
    $char = substr($string, $i, 1);/*
        echo $string;
        echo '<br>';*/
        switch ($char) {
          case 'A':
          case 'B':
          case 'C':
          case 'D':
          case 'E':
          case 'F':
          case 'G':
          case 'H':
          case 'I':
          case 'J':
          case 'K':
          case 'L':
          case 'M':
          case 'N':
          case 'O':
          case 'P':
          case 'Q':
          case 'R':
          case 'S':
          case 'T':
          case 'U':
          case 'V':
          case 'W':
          case 'X':
          case 'Y':
          case 'Z':
          case 'a':
          case 'b':
          case 'c':
          case 'd':
          case 'e':
          case 'f':
          case 'g':
          case 'h':
          case 'i':
          case 'j':
          case 'k':
          case 'l':
          case 'm':
          case 'n':
          case 'o':
          case 'p':
          case 'q':
          case 'r':
          case 's':
          case 't':
          case 'u':
          case 'v':
          case 'w':
          case 'x':
          case 'y':
          case 'z':
          case '0':
          case '1':
          case '2':
          case '3':
          case '4':
          case '5':
          case '6':
          case '7':
          case '8':
          case '9':
              break;
            default:
              if ($allow_special) {
                  if ($char == '@' || $char == '-' || $char == '_' || $char == '.' || $char == ',' || $char == '\'') {
                  if ($char == '\'') {
                          $string = substr_replace($string, '\\'.$char, $i, 1);
                          $i++;
                        }
                        break;
                    }
                    else {
                  $string = substr_replace($string, '', $i, 1);
                      $i--;
                        break;
                    }
                }
              if ($allow_space && $char == ' ') {
                break;
                }
                if (!$allow_special || !$allow_space) {
                $string = substr_replace($string, '', $i, 1);
                    $i--;
                }
                break;
        }
    }
    return $string;     
}

Re: PHP Security Question

Posted: Tue Dec 16, 2008 6:32 am
by kaisellgren
Your sanitize function is aweful, it's pointless, useless yet bad written. The script you posted in the first post is vulnerable to even the most basic SQL injections.

If I say your source code is not revealed to the public, am I right? Is it closed source? Otherwise encryptions can be always reversed if the key is known.

When you use a wireless connection for instance, then anyone could potentially read the packets that you computer transmits to the website. Regardless of your attempts, the attacker may read them and hack into your site. What you should encrypt, is the packet being sent over the net. Use SSL to do this. Actually using SSL is simple, the very basic way to make your forms being sent encrypted is making sure the site loads with https:// and you of course need to setup SSL to function on the server and buy a certificate. Google "Trial InstantSSL" for a 3 months trial certificate, you need a working domain name for it, too.

After you have the SSL functioning, please read some articles about security..

Re: PHP Security Question

Posted: Tue Dec 16, 2008 2:22 pm
by cavemaneca
This site still isn't even running yet, and it won't until I've finished EVERYTHING. Also, Where do you get off? Did you actually test my script, or just look at it and say b.s.? It might be ugly but in default circumstances it removes EVERYTHING besides [A-z] and [0-9]. I told you I have tested it, and none of those characters work. And with the allow_special true it properly escapes the single quotes so why don't you test something before you say it won't work? Unless you can type a character that reads like a single quote but looks like a letter and compares like a letter it won't work. Every other function I have seen for it is too complicated and this actually runs very fast, at least for me, even on the development server. As, most of the one step or even three step built in functions seemed to not do enough, mainly because they only removed certain characters, and they didn't check for different encoding for it. I wrote this code because it is a list of allowed characters, and the built in regex functions didn't allow a list of safe characters(as far as I could tell), only what it should match and change. I agree with you on SSL, and the whole admin side of it will only allow SSL. However, the client side at most can still only read the database, only has an account that reads the database, and uses a different database for the clients than the admins, so the only thing that can get out on that side is data that is not sensitive anyway. What I'm trying to say is this, I've read articles about security(mostly about SQL Injection), I'm working on it, and don't comment about my code if you won't even try it.

Re: PHP Security Question

Posted: Tue Dec 16, 2008 2:29 pm
by jaoudestudios
You could probably simplify your function a lot by using regular expressions.

Re: PHP Security Question

Posted: Tue Dec 16, 2008 2:46 pm
by kaisellgren
cavemaneca wrote:This site still isn't even running yet, and it won't until I've finished EVERYTHING. Also, Where do you get off? Did you actually test my script, or just look at it and say b.s.? It might be ugly but in default circumstances it removes EVERYTHING besides [A-z] and [0-9]. I told you I have tested it, and none of those characters work. And with the allow_special true it properly escapes the single quotes so why don't you test something before you say it won't work? Unless you can type a character that reads like a single quote but looks like a letter and compares like a letter it won't work. Every other function I have seen for it is too complicated and this actually runs very fast, at least for me, even on the development server. As, most of the one step or even three step built in functions seemed to not do enough, mainly because they only removed certain characters, and they didn't check for different encoding for it. I wrote this code because it is a list of allowed characters, and the built in regex functions didn't allow a list of safe characters(as far as I could tell), only what it should match and change. I agree with you on SSL, and the whole admin side of it will only allow SSL. However, the client side at most can still only read the database, only has an account that reads the database, and uses a different database for the clients than the admins, so the only thing that can get out on that side is data that is not sensitive anyway. What I'm trying to say is this, I've read articles about security(mostly about SQL Injection), I'm working on it, and don't comment about my code if you won't even try it.
I never said "it does not work". I just mentioned how inefficient it is and it *should* be rewritten in my opinion. And yeah I did test it, the first thing I noticed was an error because you typed ''' instead of "'" or something else in line 74. From looking at the script, what you want is to filter out non A-z,0-9 plus a space where ever needed in addition to possible "special" characters.

After I tried:
echo sanitize('test " \'',0,'"');
It did not leave the double quote, but instead it left the single quote? I am not following... is this the purpose or a failure of code?

Here's my function, which is still not the fastest way to do it, but rather more "clever". Feel free to experiment with it.

Code: Select all

function filterout($string,$space = false,$chrs = false)
 {
  $space = ($space) ? ' ' : '';
  $chrs = preg_quote($chrs,'#');
  $regex = "#[^A-Za-z0-9$space$chrs]#";
  return preg_replace($regex,'',$string);
 }

Re: PHP Security Question

Posted: Tue Dec 16, 2008 2:46 pm
by John Cartwright

Code: Select all

$sample = 'Foobar '. PHP_EOL;
echo sanitize($sample, true, true);
Where is my line break going? Your escaping function should not remove data, only escape it.

I have to ask, why are you rewritting proven escape functions and encryption algorythms. People spend many, many years developing these techniques through public scrutiny. I think I'll stick with those.

As per the mysql_real_escape_string() manual page, you should also be escaping \x00, \n, \r, \, ', " and \x1a.

Remember, filtering != escaping.

Re: PHP Security Question

Posted: Tue Dec 16, 2008 3:07 pm
by cavemaneca
kaisellgren wrote: After I tried:
echo sanitize('test " \'',0,'"');
It did not leave the double quote, but instead it left the single quote? I am not following... is this the purpose or a failure of code?
The code was not written that way. there is an escape there for the single quote, it just does not show up when using the code tags. And I've been trying to learn preg tags and regex but until then I put this together, it is definitely not the best way, and I should still use mysql_escape_string().
And to Jcart, What is wrong with simply removing things I don't want? What if they accidently brush the single quote while typing in their password? Removing it mean's the can't find a way to unexcape it, because it's not there, and I provide options to make sure it will allow other characters when I need them.

Re: PHP Security Question

Posted: Tue Dec 16, 2008 3:15 pm
by kaisellgren
cavemaneca wrote:
kaisellgren wrote: After I tried:
echo sanitize('test " \'',0,'"');
It did not leave the double quote, but instead it left the single quote? I am not following... is this the purpose or a failure of code?
The code was not written that way. there is an escape there for the single quote, it just does not show up when using the code tags. And I've been trying to learn preg tags and regex but until then I put this together, it is definitely not the best way, and I should still use mysql_escape_string()
And to Jcart, What is wrong with simply removing things I don't want? What if they accidently brush the single quote while typing in their password? Removing it mean's the can't find a way to unexcape it, because it's not there, and I provide options to make sure it will allow other characters when I need them.
I did not understand your point that code was not written that way? Of course I put the backslash \' into it because it's used inside single quoted string :|

No you should not use mysql_escape_string(), but mysql_real_escape_string() instead.

The removal of characters like ' is definetly bad user experience. What happens when O'reilly or O'hara wants to use his REAL name? He can't! Really really bad... and even if no one has ¤ in his name, why can't I use it in my password? What if I want to have a secure password ¤%¤#%mypass"¤)(= isntead of just typing a simple short password like "table"?

Escaping exists so that you can prevent any unauthorized personnel to mess up your code by exploiting injections within SQL. Do not try to remove characters to stop SQL injections, use Escaping, not filtering/sanitizing/wtvr else you were going to use.

There's also a thing called Defense in Depth.

Re: PHP Security Question

Posted: Tue Dec 16, 2008 3:37 pm
by cavemaneca
If you also notice, with special characters NOT removed, it does escape the single quote. And Like I said before, The data available to the standard user interface does not need very good security, and standard access does not even allow you to change any database, or even read the database that administrative accounts are stored in. All of the passwords and usernames will be provided to the client when they create an account, so I shouldn't have to worry about bob o'reilly anyway, because he will never have an account on my website. This is a business site, not some stupid forum or the like. The clients that work with us have a simple login to view details we have about them and information they need from us, so that they don't need to call us every five minutes. They only reason anything needs to be secure is I don't want some idiot going in and screwing up the data. The worst thing that could happen in that case anyway is we get confused. We are not storing any sensitive data on the webserver anyway. Whatever way you look at it, removing anything I don't want should not be a problem if no one can have a username or password that is that customized. If i needed to i could escape those characters.

Anyway, I am glad for all the feedback you have given. And I haven't used it yet so i forgot that it was mysql_real_escape_string().

Re: PHP Security Question

Posted: Tue Dec 16, 2008 4:52 pm
by kaisellgren
cavemaneca wrote:If you also notice, with special characters NOT removed, it does escape the single quote. And Like I said before, The data available to the standard user interface does not need very good security, and standard access does not even allow you to change any database, or even read the database that administrative accounts are stored in. All of the passwords and usernames will be provided to the client when they create an account, so I shouldn't have to worry about bob o'reilly anyway, because he will never have an account on my website. This is a business site, not some stupid forum or the like. The clients that work with us have a simple login to view details we have about them and information they need from us, so that they don't need to call us every five minutes. They only reason anything needs to be secure is I don't want some idiot going in and screwing up the data. The worst thing that could happen in that case anyway is we get confused. We are not storing any sensitive data on the webserver anyway. Whatever way you look at it, removing anything I don't want should not be a problem if no one can have a username or password that is that customized. If i needed to i could escape those characters.

Anyway, I am glad for all the feedback you have given. And I haven't used it yet so i forgot that it was mysql_real_escape_string().
So in order to not be unfriendly to your users, you have to list all characters you think your users might want to use? wow... do you know what letters I as a Finn would like to use in my username? How about Indians or Koreans or ... you really are going to list those in the special character parameter? Did you know that ' is not the only character that must be escaped?

Are you saying you have a separate database for just the users? So that the read-only database account can only Read the database which contains only the users? It's still very bad practise, because any attacker can read the users' passwords and username and other details - which is not just bad for you or for your users, but also AN IDENTITY LOST for some people. Not everyone in this world want to share their information with everybody. Especially if this is all about business, and not a hobbyist site.

I'm just wondering, why on earth overcomplicate things when all you need to do is to escape the user submitted details? What's the big benefit of reinventing the wheel that the coders of the SQL languages made?