Gevaarlijk script

Waar ik mee zou beginnen is één aanpak kiezen voor het aanspreken van je database.

In bovenstaande code gebruik je zowel de rechtstreekse manier om queries op te bouwen (op een onveilige manier) en vervolgens gebruik je prepared statements voor het loggen van fouten (zij het op een verkeerde manier, die ook onveilig is).

Persoonlijk vind ik prepared statements in MySQLi nogal bagger. Als je daar fan van bent kun je mogelijk beter PDO gebruiken. Maar ook dan geldt dat een onjuist gebruik onveilig is.

En dan moet de code in bovenstaand fragment nodig gerefactored worden. Deze kan vele malen korter. Hebben al die errorcodes echt nut en betekenis? Ik hoop dat je niet terugkoppelt aan een gebruiker wat er precies niet goed was aan de inlog, want dit maakt het inbreken in je site makkelijker.

De uitkomt van een inlogpoging is ofwel GOED ofwel FOUT, en verder niets.

Thomas had het over broncode. Terwijl die dat niet nodig is voor het schrijven van een inlogscript.

Ik heb het script nu zo aangepast, ik heb het nog niet gechecked, dus er kunnen nog wat foutjes inzitten :

<?php
$password = $_POST['mw_pass'];
$mw_user = $_POST['mw_user'];
//check personal salt
$saltqry = $connection->query("
								SELECT
										salt,
										mw_gegevens_groep										
								FROM 
										mw_gegevens
								WHERE 	
										mw_gegevens_persnr=?
								");
		$statement = $connection->prepare($saltqry);
		if($qry === false)
		{
		echo "Query error:.". $connection->error();
		}
		else
		{ 	
			$statement->bind_param('ii', $mw_user, $userpassword);   
			$statement->execute();
			$result = $statement->get_result();	
			$satement->close;
			$saltuitkomt = $result->fetch_assoc();
		}
	($erroruitkomst = $saltqry->fetch_assoc());
	if($saltqry->num_rows == 0)
	{
		$salt='';
	}
	else
	{
		$salt = $saltuitkomst['salt'];
	}
	
	include_once('inlog/passcrypt.php'); 

	$userpassword = $Nieuw_ww; 
	

	$mw_gegevens_qry = $connection->query("
								SELECT
										* 
								FROM 
										mw_gegevens
								WHERE 	
										mw_gegevens_persnr=?
								AND 	
										mw_gegevens_pass=?
								AND
										mw_gegevens_pass!='' 
								");
		$statement = $connection->prepare($mw_gegevens_qry);
		if($qry === false)
		{
		echo "Query error:.". $connection->error();
		}
		else
		{ 	
			$statement->bind_param('ii', $mw_user, $userpassword);   
			$statement->execute();
			$result = $statement->get_result();	
			$satement->close;
			$mwgegevens = $result->fetch_assoc();
		}
?>

Toevoeging op 28/05/2016 17:06:44:

Thomas van den Heuvel op 28/05/2016 17:03:59

Waar ik mee zou beginnen is één aanpak kiezen voor het aanspreken van je database.

In bovenstaande code gebruik je zowel de rechtstreekse manier om queries op te bouwen (op een onveilige manier) en vervolgens gebruik je prepared statements voor het loggen van fouten (zij het op een verkeerde manier, die ook onveilig is).

Persoonlijk vind ik prepared statements in MySQLi nogal bagger. Als je daar fan van bent kun je mogelijk beter PDO gebruiken. Maar ook dan geldt dat een onjuist gebruik onveilig is.

En dan moet de code in bovenstaand fragment nodig gerefactored worden. Deze kan vele malen korter. Hebben al die errorcodes echt nut en betekenis? Ik hoop dat je niet terugkoppelt aan een gebruiker wat er precies niet goed was aan de inlog, want dit maakt het inbreken in je site makkelijker.

De uitkomt van een inlogpoging is ofwel GOED ofwel FOUT, en verder niets.

Ik koppel terug dat een invulveld leeg is, of dat een account geblokkeerd is.

of dat een account geblokkeerd is.

Dat is het ontsluiten van informatie over de toestand van een account wat mij niet verstandig lijkt.

Je zou eerder een algemene foutmelding kunnen geven als een loginpoging strandt. Hierbij zou je niet eens onderscheid te hoeven maken of iemand iets invult of niet, maar je zou in de afhandeling dan verdere controles geheel kunnen skippen (als een gebruikersnaam of wachtwoord leeg is hoef je niets te controleren omdat dat toch nooit wat oplevert).

Je zou in de algemene foutmelding kunnen opnemen dat als een inlogpoging herhaaldelijk mislukt dat iemand kan proberen zijn/haar wachtwoord op te vragen. Vervolgens zou je dan iemand persoonlijk vervolgstappen kunnen berichten. Hierin zou je ook kunnen aangeven dat iemand zijn account is geblokkeerd en wat deze persoon vervolgens kan doen.

Goed idee.

Ik kom dan op zoiets uit.
Het betreft hier onze medewerkers website. Als een medewerker niet meer in dienst is, dan wordt het account gebokkeerd.

Wat is precies het gevaar van die info delen? Ik zie dat zelf altijd wel als goede service.

<?php
if(isset($_POST['login']))
{
	if($_POST['mw_pass']=='' && $_POST['mw_user']=='')
	{
		$aErrors= 87;
		$user = '0';
	}
	elseif($_POST['mw_user']=='')
	{
		$aErrors=71;
		$user = '0';
	}
	elseif($_POST['mw_pass']=='')
	{
		$aErrors=8;
		$user = $_POST['mw_user'];
	}
	else
	{
		$password = $_POST['mw_pass'];
		$user = $_POST['mw_user'];
		//check personal salt
		$saltqry = $connection->query("
										SELECT
												salt,
												mw_gegevens_groep										
										FROM 
												mw_gegevens
										WHERE 	
												mw_gegevens_persnr=?
										");
				$statement = $connection->prepare($saltqry);
				if($qry === false)
				{
				echo "Query error:.". $connection->error();
				}
				else
				{ 	
					$statement->bind_param('ii', $user, $userpassword);   
					$statement->execute();
					$result = $statement->get_result();	
					$satement->close;
					$saltuitkomt = $result->fetch_assoc();
				}
			($erroruitkomst = $saltqry->fetch_assoc());
			if($saltqry->num_rows == 0)
			{
				$salt='';
			}
			else
			{
				$salt = $saltuitkomst['salt'];
			}
			
			include_once('inlog/passcrypt.php'); 

			$userpassword = $Nieuw_ww; 
	

	$mw_gegevens_qry = $connection->query("
								SELECT
										* 
								FROM 
										mw_gegevens
								WHERE 	
										mw_gegevens_persnr=?
								AND 	
										mw_gegevens_pass=?
								AND
										mw_gegevens_pass!='' 
								");
		$statement = $connection->prepare($mw_gegevens_qry);
		if($qry === false)
		{
		echo "Query error:.". $connection->error();
		}
		else
		{ 	
			$statement->bind_param('ii', $user, $userpassword);   
			$statement->execute();
			$result = $statement->get_result();	
			$satement->close;
			$mwgegevens = $result->fetch_assoc();
		}
		if($mw_gegevens_qry->num_rows == 0)
		{
			$aErrors= 91;
		}
		elseif($saltqry->num_rows != 0 && $erroruitkomst['mw_gegevens_groep']==7)
		{
			$aErrors= 11;
		}
		elseif($saltqry->num_rows == 1)
		{
			$aErrors=0;
		}
		else
		{
			$aErrors=9999;
		}
	}
?>

Wat is precies het gevaar van die info delen?

In dit concrete geval:
- een aanvaller weet dan dat dat een bestaand (zij het een gedeactiveerd) account betreft, waarmee deze een informatie-bestand kan opbouwen van accounts die mogelijk toegepast kan worden bij andere "ingangen" in je applicatie
- de aanvaller weet dan ook dat het op dat moment niet zinnig is om met dat account verdere loginpogingen te ondernemen

Ik zou eigenlijk alleen intern inlogpogingen bijhouden, en daarbij enkel het onderscheid maken tussen het slagen of falen hiervan en niet zozeer wat hier precies mis aan was, tenzij dit om een of andere reden heel belangrijk is. Anders is dit toch een beetje verspilde moeite.

OK daar heb je wel een goed punt.

Ik zal dan de fouten wel blijven opslaan, maar niet terugkoppelen naar de website.
Op de website maak ik dan wel een melding dat het inloggen niet gelukt is.