Hallo beste mensen en php-ers

Al een flinke tijd gebruik ik onderstaande login systeem, dat ik gedeeltelijk zelf, gedeeltelijk met snippets van het internet geplukt bij elkaar heb gekregen. Ik ben niet helemaal zeker of het veilig is. En ben benieuwd wat jullie er van vinden. Bvd.

Groeten Wouter

<?php 
//login.php

//cleanQuery
function cleanQuery($string)
{
  if(get_magic_quotes_gpc())  
  {
    $string = stripslashes($string);
  }
  if (phpversion() >= '4.3.0')
  {
    $string = mysql_real_escape_string($string);
  }
  else
  {
    $string = mysql_escape_string($string);
  }
  return $string;
}

//salt
function salt($pass, $salt, $str="")
{
    $pass = str_replace(" ", "", $pass);
    $pass = strrev($pass);

    $arr[0] = strlen($pass);
    $arr[1] = strlen($salt);
    
    while($arr[0] > $arr[1] + 1)
    {
        $salt = $salt.$salt;
        $arr[1] = strlen($salt);
    }

    $split_len = floor(max($arr) / (min($arr) - 1));
    
    $salt = explode(" ", chunk_split($salt, $split_len, " "));
    array_pop($salt);
    $salt = array_reverse($salt);
    
    $j = $arr[0] > count($salt) ? $arr[0] : count($salt);
    
    for($i=0; $i<$j; $i++)
    {
        if(isset($pass{$i}))    $str .= $pass{$i};
        if(isset($salt[$i]))    $str .= $salt[$i];
    }
    
    return $str;
}
 
$logingoto = "admin.php";
$validate = "validate.php";
$error = "form.php";

if (isset($_SESSION['visit'])) 
{
	$_SESSION['user'] = NULL;
	unset($_SESSION['user']);
	$_SESSION['ip'] = NULL;
	unset($_SESSION['ip']);
	$_SESSION['id'] = NULL;
	unset($_SESSION['id']);
	$_SESSION['logged'] = NULL;
	unset($_SESSION['logged']);
	
	session_destroy();
}

if (!isset($_SESSION)) 
	{  session_start(); }

require('connections/connection.php'); 

if (isset($_POST['login']))
{ 
  
  $username = cleanQuery($_POST['username']);
  $password = hash('sha256', salt($_POST['password'], 'randomnummersencijfers'));
  $ip = cleanQuery($_POST['ip']);
  $ip_check = cleanQuery($_SERVER['REMOTE_ADDR']);

  $query = mysql_query ("SELECT * FROM `users` WHERE username = '$username' ");
  $numrows = mysql_num_rows($query);
  $row = mysql_fetch_assoc($query);
  
  if($row['attempt'] % 3 == 0)  
  {
	  if (strtotime("now") < $row['attempt'])
	  {
		  echo "to soon";
		  die ;
	  }
	  else
	  {
		$outtime = strtotime("now + 2 hours");
		mysql_query("UPDATE `users` SET `outtime` = '$outtime' WHERE `id` = " . $row['id']);
		header ("Location: " . root() );
		die;
	  }
  }	 
  
  else
  { 
  	if ($row['emailcheck'] == 1 )
	{
	  if ($username && $password)
	  {	  
		  if ($numrows != 0 and $row['type'] != 0)
		  {
			  $dbusername = $row['username'];
			  $dbpassword = $row['password'];
			  $id = $row['id'];
			  
			  if ($username == $dbusername && $password == $dbpassword)
			  {
				  $session_id = rand(100000000,999999999);
	
				  $_SESSION['login'] = TRUE ;
				  $_SESSION['user'] = $row['id'];
				  $_SESSION['id'] = $session_id;
				  
				  $insert_session_id = mysql_query("UPDATE `users` SET `session` = '" . $session_id . "' , `attempt` = 1 WHERE `id` = " . $row['id'] );
				  
				  if (isset($_POST['verify']))
				  {
					  $verify = hash('sha256', salt(cleanQuery($_POST['verify']), 'randomnummersencijfers'));			
										  
					  $check_query = mysql_query("SELECT * FROM `users` WHERE `id` = " . $id . " AND `verify` = '$verify' ");
					  $numrows_q = mysql_num_rows($check_query);
					  
					  if (isset($numrows_q) and @$numrows_q != 0 and $ip == $ip_check)
					  {
						  $insert_ip = mysql_query("INSERT INTO `userip` (`uid`, `ip`) VALUES (".$id.",'$ip')");					
					  }
					  
					  else
					  {
						  header("Location: $error");
						  die;
					  }
				  }
				  
				  if ($ip == $ip_check)
				  {
					  $ip_query = mysql_query("SELECT * FROM `userip` WHERE `uid` = " . $row['id'] . " AND `ip` = '$ip' ");
					  @$ip_row = mysql_fetch_assoc($ip_query);
					  @$numrowsip = mysql_num_rows($ip_query);
				  
					  if (isset($numrowsip) and @$numrowsip != 0 and $ip_row['logged'] != 1)
					  {
						  //mysql_query("UPDATE `userip` SET `logged` = " . 1 . " WHERE `ip` = '$ip' AND `uid` = " . $id );
						  mysql_query("UPDATE `users` SET `logged` = '$ip', `attempt` = 1 WHERE `id` = " . $id );
						  $_SESSION['ip'] = $ip ;
						  header("Location: $logingoto");
						  die ;
					  }
					  
					  elseif ($ip_row['logged'] == 1)
					  {
						  header ("Location: $error");
						  die ;
					  }
					  
					  else 
					  {
						  include('admin/layout/head.php');
						  include('admin/validate.php');
						  die ;
					  }
				  }				
			  }
	  
			  else 
				{ 
					$max = maximum('attempt', 'users', 'id', $id);
					@mysql_query("UPDATE `users` SET `attempt` = " . $max . " WHERE `id` = " . $id );
					header("Location: $error"); die ; 
				}	
		  }
	  
		  else 
		  { 
			$max = maximum('attempt', 'users', 'id', $id) ;
			@mysql_query("UPDATE `users` SET `attempt` = " . $max . " WHERE `id` = " . $id );
			header("Location: $error"); die ;
		  }
	  }
	  
	  else
	  {
		  $max = maximum('attempt', 'users', 'id', $id); 
		  @mysql_query("UPDATE `users` SET `attempt` = " . $max . " WHERE `id` = " . $id );
		  header("Location: $error"); die ; 
	  }	
	}
	
	elseif (isset($_POST['email']))
	{ 
		$email_ver = mysql_query("UPDATE `users` SET `emailcheck` = 1 WHERE `id` = " . $row['id'] ); 
		if (isset($email_ver)) { header("location: " . root()."verify"); die; }
		else echo mysql_error();
	}
	else	
	{ 
		header("Location: $error"); die ; 
	}
  }  
}

?>
het formulier:
        <form action="login.php" method="POST">
            <p>Naam:<br /><input type="text" name="username" class="login"></p>
            <p>Wachtwoord:<br /> <input type="password" name="password" class="login" ></p>
            <input type="submit" name="login" class="logbutton" value="Login">
            <input type="hidden" name="ip" value="<?php echo $_SERVER['REMOTE_ADDR'];?>" />
        </form>

- die() is niet netjes
- Fouten onderdrukken met @ is onnodig
- Gebruik liever de functies van mysqli i.p.v. mysql
- Waarom controleer je boven nog op PHP 4.3.0 of lager? Zo goed als niemand draait deze meer.
Hallo Wouter H,

Goed dat je een code online plaatst om gerated te worden. Onderstaand een aantal tips, hopelijk heb je er wat aan

Om te beginnen zou ik een inlogsysteem maken gebaseerd op cookies. Hiermee heb je iets meer mogelijkheden wat mij betreft. Het is bijvoorbeeld makkelijker om een bepaalde tijd aan te geven hoelang de persoon is ingelogd. Bovendien sla je er erg veel in op. Een id is meer dan voldoende

$_SESSION['user'] = NULL;
unset($_SESSION['user']);
$_SESSION['ip'] = NULL;
unset($_SESSION['ip']);
$_SESSION['id'] = NULL;
unset($_SESSION['id']);
$_SESSION['logged'] = NULL;
unset($_SESSION['logged']);


Ten tweede, hoewel ik de code niet helemaal technisch heb gelezen lijkt het mij erg makkelijk om in jouw login systeem te komen. Heb je wel eens gelezen over SQL injection? Dit is een zeer makkelijke methode om op vele sites in te loggen. Er zijn tegenwoordig nog belabberd veel websites waarop het mogelijk is. Probeer er eens wat informatie over te zoeken via google met 'php sql injection'.
'
En ten derde voor nu: Probeer af te komen van die oneindige if constructies. Een if in een if in een if in een if in een if. Reden: je code is nogal lastig om te lezen. ELke keer weer kijken waarvoor die else okal weer is! Probeer alles zo te beperken tot max. 2 if's in een if.

Je hebt bijvoorbeeld: <?php if (isset($_POST['login'])) { ... ?>
Waarin een hele hoop zooi staat. Hoewel ik betwijfel of je niet gewoon kunt controlleren of username en password er zijn, had je het volgende beter kunnen doen (in geval post['login'] nodig is):

<?php
if( ! isset($_POST['login'])){
Error::render("De variable login is er niet."); //of maak er een echo van :P
}
//Content wat er in de if stond kan nu er onder worden gezet.
//Dit scheelt toch weer overal een TAB en je hebt direct overzicht.
//Je weet namelijk direct dat deze if GEEN else bevat.
//Bij jouw is dit overal weer een raadsel.
//Je mag overigens best wel een DIE in je code behouden maar je hebt er wel erg veel. Beperk dit tot max 2.
?>

Hopelijk is de uitleg duidelijk, aangezien ik niet al te veel tijd voor heb kunnen nemen.1111111
@aar, dankjewel voor de reactie. Ook de code tags zal ik gebruiken.

@phpnuke r, dankjewel. Ik dacht dat door de functie cleanQuery, mysql injectie al werd voorkomen eigenlijk. Verder ga ik er mee aan de slag.

1) Welke PHP versie draai je?

2) Gebruik ALTIJD prepared statements, dan heb je die baggere (sorry, het is de waarheid) cleanQuery() ook niet meer nodig.
Dit betekend trouwens wel dat je MySQLi/PDO moet gebruiken, niet helemaal waar, maar dit is een goed iets.

3) Gebruik aub de familie van password_* functies. Dus password_hash() voor het hashen van een wachtwoord, password_verify() voor het controleren of een password en hash bij elkaar horen. Het genereert automatisch salts, veel veiliger dan dat jij het doet. De controlle van passwords is beter bestand tegen timing-attacks. Het algemene 'don't roll your own crypto, you are not an expert' verhaal dus...

Dankjewel!

En de versie php is: 5.3
Dan is die hele cleanQuery() functie dus niet meer nodig. Verder kan ik zeker aanraden om naar password_hash() en password_verify() te kijken.

Hoewel deze pas vanaf PHP 5.5 werken, is er al wel een compatibiliteitspakket beschikbaar die vanaf 5.3.7 werkt, ervanuitgaande dat je hoger dan deze PHP-versie zit:
https://github.com/ircmaxell/password_compat


Reageren