loginscript (veilig?)

Door Wouter OOP op 07-05-2009 19:09

1.533 views

Hallo allemaal, ik ben nieuw met php en mysql en ben een maand of 2 geleden begonnen met het leren ervan.
Nou heb ik een tutorial gevolgd om een loginscript te maken, nou vroeg ik me af of het misschien nog beter beveiligd moet worden.

Het zijn de volgende files:

login_check.php:

<?php

$inlognaam = $_POST['inlognaam'];
$wachtwoord = $_POST['wachtwoord'];

if ($inlognaam&&$wachtwoord)
{
$connect = mysql_connect ("localhost","root","") or die ("Kan niet verbinden");
mysql_select_db ("test") or die ("Kan db niet vinden");

$inlognaam = stripslashes($inlognaam);
$wachtwoord = stripslashes($wachtwoord);
$inlognaam = mysql_real_escape_string($inlognaam);
$wachtwoord = mysql_real_escape_string($wachtwoord);

$query = mysql_query ("SELECT * FROM leden WHERE inlognaam='$inlognaam' AND wachtwoord='$wachtwoord'");

$numrows = mysql_num_rows ($query);

if ($numrows !=0)
{
session_register("inlognaam");
session_register("wachtwoord");
header("location:login_success.php");
}
else
die ("Inlognaam / Wachtwoord klopt niet!");
}
else
echo "Vul inlognaam en wachtwoord in!";

?>

login_succes.php:

<?php

session_start();

if(!session_is_registered("inlognaam")){
header("location:login_form.php");
}

?>

<html>

<body>
Ingelogd!<br/>
<a href=logout.php>Log out!</a>
</body>

</html>

Bedankt,

Wouter

Mark PHP

07-05-2009 19:21

Een aantal punten die me direct opvallen:
- wachtwoord ongecodeerd in database, gebruik hash(),
- onnodige stripslashes,
- foutafhandeling ontbreekt,
- session_register en session_is_registered zijn verouderd, gebruik $_SESSION,
- de laatste regels van login_check.php kloppen niet: dubbele else.

Emmanuel Delay

07-05-2009 19:21

Nee, dit is een slecht idee. (EDIT: ik spreek uiteraard niet over de post boven mij.)

Eerst en vooral: je slaat je wachtwoorden zomaar op, niet gecodeerd. Leuk. Zo kan je de paswoorden van iedereen zomaar in de db zien.

paswoorden zet je ook nooit in een sessie.
Het paswoord heb je enkel nodig op het moment van het inloggen. Daarna laat je dat paswoord gerust.

Afra ca

07-05-2009 19:40

Agirre heeft gedeeltelijk gelijk over de dubbele else.

Als je goed zou inspringen zul je zien dat de eerste else nog van een if statement in de if statement komt. Desondanks klopt hij niet omdat je bij de 1-na-laatste else de brackets ( "{" en "}" ) vergeet!

edit:

* onnodig variabelen gaan definieren, je kan prima werken met post variabelen, en de stripslashes kun je tegelijk in de query toepassen.

* variabelen buiten quotes halen (bijv. in query)

* Mocht hash() je als beginner wat lastig lijken, kan je ook prima werken met [php]md5[/php]($_POST['wachtwoord']) , alleen wees je ervan bewust dat er tegenwoordig databases worden bijgehouden met bekende md5 wachtwoorden, dus overweeg [php]sha1[/php] (iets minder bekend, maar ook daar als DB's voor), of dus gewoon wel de [php]hash[/php]() functie

edit 2:

* Je moet zelf even wat onderzoek doen naar de goede combinatie van [php]stripslashes[/php] & [php]mysql_real_escape_string[/php] (ivm. magic quotes die soms aan en soms uit staan)

Mr.Ark

07-05-2009 20:02

Even snel je code herschreven, Zoals eerder gezegd, Sla je wachtwoord als MD5 op. Zorg ook dat je sql injection tegen gaat, En dat je goeie checks uitvoert.

<?PHP
/* Inlognaam check */
if(isset($_POST['inlognaam'] && !empty($_POST['inlognaam']))
{
/* Wachtwoord check */
if(isset($_POST['wachtwoord'] && !empty($_POST['wachtwoord']))
{
/* Query uitvoeren */
$Query =
"
SELECT
ID
FROM
tabel
WHERE
inlognaam = '".mysql_real_escape_string($_POST['inlognaam'])."'
AND
wachtwoord = '".md5($_POST['wachtwoord'])."'
";

/* Result */
$Result = mysql_query($Query);

/* Kijken of query is gelukt. */
if(!$Result)
{
echo 'Fout opgetreden met query, '.$Query.'';
}
else
{
/* Kijken of er een result is. */
if(mysql_num_rows($Result) !== 0)
{
/* Sessies aanmaken. */
$Rij = mysql_fetch_assoc($Result);

$_SESSION['USER_ID'] = $Rij['ID'];
$_SESSION['USER_IP'] = $_SERVER['REMOTE_ADDR'];

/* Doorsturen */
header('location: jouwpagina.php');
}
else
{
/* Geen result. */
echo 'Geen resultaat gevonden in database';
}
}
}
else
{
/* Geen wachtwoord ingevuld. */
echo 'Geen wachtwoord ingevuld.';
}
}
else
{
/* Geen inlognaam ingevuld. */
echo 'Geen inlognaam ingevuld.';
}
?>

Mark PHP

07-05-2009 21:18

@Ark
Ziet er al beter uit, alleen waarom zou je het IP vanuit de database selecteren? Dat wil je niet. Als je het IP in een sessie wilt hebben (wat soms handig is), gebruik dan $_SERVER['REMOTE_ADDR'].

@Afra
Je kan md5 ook prima met hash() voor elkaar krijgen.

Mr.Ark

07-05-2009 21:22

@ Agirre

Je hebt gelijk, uit snelheid had ik daar niet bij nagedacht.
Zal het even verbeteren.

Afra ca

07-05-2009 21:48

@ Agirre,

zeker, aangezien je bij hash als eerste parameter gewoon het algoritme kan aangeven. Maar denk dat

<?php hash(md5,$tekst); ?>

iets lastiger is dan

<?php md5($tekst); ?>

voor een beginner.

Mark PHP

07-05-2009 21:50

Klopt, voor een beginner is dat inderdaad makkelijker. Alleen hash() is meer flexibel en biedt meer mogelijkheden, dus vandaar dat ik deze altijd gebruik (meestal sha256).

Afra ca

07-05-2009 22:05

Stem ik zeker mee in, en ik raad dan ook eigenlijk het gebruik van hash() aan. En met 35 algoritmes is de keuze reuze :)

Begrijp wel, de encryptie (ok, hash is officieel gezien geen encryptie) kan nog zo goed zijn, het gaat om de manier waarop het gebruikt wordt in een systeem. (evt. as easy as SQL INJECTION)

Wouter OOP

07-05-2009 23:37

Bedankt voor alle reacties.
Ik vond het al heel wat dat ik begreep wat ik heb geschreven :), kennelijk komt er nog heel wat meer bij kijken.

@ark

Ook jij bedankt voor het herschrijven van het script, ik ga dit zeker gebruiken om te bestuderen.

Nogmaals bedankt,

Wouter

Reageren

Inloggen om te reageren