Login script

Overzicht Reageren

Sponsored by: Vacatures door Monsterboard

Jorn Reed

Jorn Reed

24/09/2019 03:59:23
Quote Anchor link
Hoi, ik ben bezig met een login script. Wat ik heb werkt wel prima, maar ik vroeg me af, of dit wel veilig is en of ik misschien iets belangrijks mis.

Code (php)
PHP script in nieuw venster Selecteer het PHP script
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
<?php
    require_once 'db.php';

    $errors = [];

    if(!isset($_POST['username'])){
        $errors[] = 'Username is required.';
    }

    $username = htmlspecialchars(trim($_POST['username']));
    if(!isset($_POST['password'])){
        $errors[] = 'Password is required.';
    }

    $password = htmlspecialchars(trim($_POST['password']));

    $query=$db->prepare("SELECT * FROM `users` WHERE `username` = :username");
    $query->execute([':username' => $username]);
    
    if($query->rowCount() == 1){
        $result = $query->fetch(PDO::FETCH_ASSOC);

        $dbpassword = $result['password'];

        $user_id = $result['id'];

        if(password_verify($password, $dbpassword))
        {

            $msg = "Login succesful";
                
            session_start();

            $_SESSION['id'] = $user_id;
        }

        else
        {
            $msg = "Wrong password";
        }
    }

?>
Gewijzigd op 24/09/2019 03:59:47 door Jorn Reed
 
PHP hulp

PHP hulp

09/07/2020 09:20:56
 
- Ariën -
Beheerder

- Ariën -

24/09/2019 08:49:40
Quote Anchor link
Controleer je niet of de gebruiker bestaat?
Sowieso hoor je voor de veiligheid nooit te vermelden of een wachtwoord incorrect is. Op die manier kan een aanvaller eenvoudig zien of een gebruikersnaam al bestaat, zodat die verder kan hameren om een wachtwoord te raden. Geef gewoon aan dat "de gebruikersnaam en/of het wachtwoord fout zijn".

En waarom zou je htmlspecialchars() en trim() op een password en username gooien? Een bewerking als htmlspecialchars() hoort in de output te gebeuren. Verder zou je aan de hand van een reguliere expressie kunnen bepalen waaraan een gebruikersnaam moet voldoen. Logischerwijs zijn dit voornamelijk normale karakters van A tot Z en, cijfers en een hoog of laag streepje.

session_start(); kan je ook gewoon aan het begin van je site plaatsen, dan kan je overal je sessies gebruiken.

Verder zou je session-fixation nog kunnen voorkomen daar een nieuwe SessieID te maken na de inlog met session_regenerate_id().
Gewijzigd op 24/09/2019 08:51:35 door - Ariën -
 
Wijnand de Ridder

Wijnand de Ridder

24/09/2019 11:44:11
Quote Anchor link
En daarnaast: je controleert met isSet, ik zou dat met empty doen, want een lege waarde is ook gezet.
 
Thomas van den Heuvel

Thomas van den Heuvel

24/09/2019 15:56:22
Quote Anchor link
Al het bovenstaande, en dan ook:
- hangt van db.php af :p; is dit PDO, een aangepaste variant van MySQLi? Iets anders? Want als die op een of andere manier lek gaat maakt het niet uit wat je verder doet
- het enige wat je nodig hebt uit de users tabel zijn het id en het wachtwoord, dit zijn dus ook de enige kolommen die je op zou moeten halen; wat natuurlijk ook niet echt handig is is dat je kolomnaam "password" een gereserveerd woord is in MySQL (er vanuitgaande dat je een MySQL database gebruikt)
- gebruik je ook HTTPS? Want als je op een onveilig netwerk zit dan halen dit soort (serverside) voorzieningen anders niet zoveel uit
- het aanpassen van user-credentials en/of sessie-gegevens is een state change, je zou dan ook direct na afloop de pagina moeten verversen of van locatie moeten veranderen met behulp van een header('Location: ...') in combinatie met een exit; statement, en zoals @Ariën aangeeft, session_regenerate_id() is dan geen overbodige luxe
- er is geen scheiding tussen validatie en verdere verwerking, het een vloeit gewoon over in het ander; het heeft helemaal geen zin om een query uit te voeren als een of beide velden leeg zijn of ontbreken, dus dan zou ik daar de loginpoging al afbreken eerlijk gezegd; zoals in elk formulier: pas als de validatie succesvol is ga je verder

Wat je ook zou kunnen overwegen:
- loginpogingen loggen, zowel succesvol als niet succesvol
- een limiet aan het aantal inlogpogingen

En natuurlijk is het ook interessant wat je hierna allemaal kunt doen. Zitten er ook rechten/rollen/privileges in het systeem? Het kan zijn dat dat op dit moment nog hard coded is op userniveau. In principe is daar niets mis mee maar op het moment dat je straks meer functionaliteit hebt is dit mogelijk een dingetje wat je verder uit zult moeten breiden.

En dan, hoe refereer je aan de gegevens van een geauthenticeerd persoon? Stop je alles in $_SESSION en raadpleeg je rechtstreeks $_SESSION of bouw je elk request een soort User object ofzo?

En tot slot: simpelweg omdat je alles aan de poort eenmalig controleert wil niet zeggen dat alles daarna ineens veilig is :p. Zelfs als je login veilig is zul je moeten blijven waken voor veiligheid. Als op een andere plek SQL-injectie mogelijk is dan heb je nog steeds een heel groot probleem en maakt het niet uit hoe veilig de loginprocedure is. Iets met kettingen en zwakste schakels.
Gewijzigd op 24/09/2019 16:00:50 door Thomas van den Heuvel
 
Rob Doemaarwat

Rob Doemaarwat

24/09/2019 20:36:43
Quote Anchor link
Nog eentje: Je doet nu
Code (php)
PHP script in nieuw venster Selecteer het PHP script
1
2
3
4
    if(!isset($_POST['username'])){
        $errors[] = 'Username is required.';
    }
    $username = htmlspecialchars(trim($_POST['username']));

Eerst controleer je dus of $_POST['username'] wel/niet bestaat, maar vervolgens ga je er toch altijd mee aan de slag. PHP rammelt wel door, maar in je log zul je vermoedelijk "Undefined index" terug vinden ('username' zit niet in de $_POST). Het is altijd fijn als je log helemaal "schoon" is. _Als_ er dan een keer wat in komt te staan weet je ook meteen dat je een probleem(pje) op moet lossen (anders zie je door de bomen het bos niet meer, en zie je nooit of je een fout hebt gemaakt).

Ik zou zelf iets doen ala:
Code (php)
PHP script in nieuw venster Selecteer het PHP script
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
    $errors = [];
    $username = $password = null;

    if(isset($_POST['username'])) $username = $_POST['username'];
    else $errors[] = 'Username is required.';
 
    if(isset($_POST['password'])) $password = $_POST['password'];
    else $errors[] = 'Password is required.';

    if(!$errors){
        $query=$db->prepare("SELECT * FROM `users` WHERE `username` = :username");
        $query->execute([':username' => $username]);
    
        if($query->rowCount() == 1){
//enz
 
Frank Nietbelangrijk

Frank Nietbelangrijk

24/09/2019 21:10:21
Quote Anchor link
Wijnand de Ridder op 24/09/2019 11:44:11:
En daarnaast: je controleert met isSet, ik zou dat met empty doen, want een lege waarde is ook gezet.


Het probleem van empty is dat een string '0' ook als leeg gezien wordt.
Code (php)
PHP script in nieuw venster Selecteer het PHP script
1
2
3
4
5
6
7
8
9
<?php

$wachtwoord
= '0';

if(empty($wachtwoord)) {
    echo 'wachtwoord is leeg!';
}


?>

Beter alternatief:
Code (php)
PHP script in nieuw venster Selecteer het PHP script
1
2
3
4
5
6
<?php
if(strlen($password) == 0)
{

    // geen wachtwoord ingevuld.
}
?>

Hoewel dit topic over het inloggen gaat is het belangrijker dat bij het aanmaken van een gebruiker (registratie) en bij het wijzigen van wachtwoorden er op wordt gevalideerd dat er een wachtwoord van een bepaalde minimale lengte is ingeklopt.
Gewijzigd op 24/09/2019 21:11:06 door Frank Nietbelangrijk
 



Overzicht Reageren

 
 

Om de gebruiksvriendelijkheid van onze website en diensten te optimaliseren maken wij gebruik van cookies. Deze cookies gebruiken wij voor functionaliteiten, analytische gegevens en marketing doeleinden. U vindt meer informatie in onze privacy statement.