Up for assessing my script?

[Zola]

Hi,

 

I am relatively new to PHP.

This a login script for a restricted area on a website I am making. There is a database which collects info, such as who downloads what material, time stamps etc.

 

This is log.php, a file which is required and called in index.php (within the restricted directory).

 

This is a script I downloaded and modified for my own needs.

 

I am hopeful someone would take a quick look at it and tell me if you see anything wrong, bugs, potential errors etc?

 

Everything works fine, I am just interested in best practices.

 

 

<?php session_start(); ?>


<?php

$hostname = "..";
$username = "..";
$password = "..";
$database = "..";

$link = MYSQL_CONNECT($hostname,$username,$password);

mysql_select_db($database); 
?>

<?php

if($_GET['action'] == "login") {
$conn = mysql_connect("..","..","..");
$db = mysql_select_db(".."); //Your database name goes in this field.
$name = $_POST['user'];
$ip=$_SERVER['REMOTE_ADDR'];
$var = mysql_real_escape_string($var);
$country = file_get_contents('http://stonito.com/script/geoip/?ip='.$ip);
$q_user = mysql_query("SELECT * FROM customer WHERE username='$name'");

?>

<?php
               $insert_query = ("INSERT INTO login(username, ip, country) VALUES ('$name','$ip','$country');");
               mysql_query($insert_query) or die('Error, insert query failed');

?>

<?php
if(mysql_num_rows($q_user) == 1) {

$query = mysql_query("SELECT * FROM customer WHERE username='$name'");
$data = mysql_fetch_array($query);
if($_POST['pwd'] == $data['password']) {
$_SESSION["name"];
header("Location: http://myurl.com/restricted.php"); // This is the page that you want to open if the user successfully logs in to your website.
exit;
} else {
header("Location: http://myurl.com/register.html");
exit;
}
} else {
header("Location: http://myurl.com/");
exit;
}
}

// if the session is not registered
if(!isset($_SESSION['name'])) {
header("Location: http://www.myurl.com");
}
?>

 

 

 

Thanks for any input. 

[nine7ySix]

There doesn't seem to be anything that looks wrong, but make sure you mysql_real_escape_string $name.

 

Also $var doesn't seem have much use.

 

Also might want to have $_SESSION["name"] equal something, if not, I would suggest just making a variable, instead of a $_SESSION, and you should try to use single quotes for these, instead of $_SESSION["name"], use $_SESSION['name'], it's best to try and be consistent.

 

Also, you may get headers already sent errors because of your

 

?>

 

<?php

 

appearing everywhere.

 

It would also be a good idea to check how many MySQL connections you're making.

[Christian F.]

As nine7ySix already have pointed out, you will have problems with the above code due to the liberally distributed PHP-tags. Not because of the tags themselves, but because of the whitespace in between. This gets sent to the browser, which in turns causes the web server to send out all headers.

If you take a look at the PHP manual you'll quickly see why that's a Bad Thing.

 

Secondly, why are you running the same query twice in a row?

$q_user = mysql_query("SELECT * FROM customer WHERE username='$name'");
/* SNIP */
if(mysql_num_rows($q_user) == 1) {
    $query = mysql_query("SELECT * FROM customer WHERE username='$name'");

This is a complete waste of time and resources, as you have the results available already in the $q_user variable.

 

The third point I'd like to mention is that you seem to store your password in clear text, which is a very Bad Thing.

Always(!) hash your passwords, with individual salts. That is, hash not encrypt?/url].

 

For my fourth point I'd like to reiterate what nine7ySix already said: Always escape output when sending it to a different parser/system. That means "htmlspecialchars ()" for (non-HTML) output to browsers, "rawurlencode ()" for URLs, "mysql_real_escape_string ()" or Prepared Statements for MySQL and so forth.

Combined with proper input validation this will drastically reduce the number of weak spots in your application, might even remove them completely depending upon the complexity of your code.

 

And lastly, something I don't think a lot of people are aware over: $_SERVER['REMOTE_ADDR'] can be spoofed.

That means that it is quite trivial for an attack to add whatever s/he likes to the URL request, and make it appear as if it's your server that's attacking your GeoIP provider instead. That's why you always validate input from the user, and never trust client-provided data. (Granted, in this case it might be difficult to actually know that we're talking about client-provided data.)

 

PS: I know this thread is a little old,  but seeing as most of my comments are security related, and might be of use for someone else, I decided to reply anyone.

[Nyphrex]

I think the archaic mysql_connect could (and should) be supplanted by PDO - http://net.tutsplus.com/tutorials/php/why-you-should-be-using-phps-pdo-for-database-access/