Authentication Class

Door Kwastie , 14 jaar geleden, 4.715x bekeken

Ik ben sinds een paar maand me gaan verdiepen in OOP.

Het probleem waar ik eigelijk tegen aanliep is: 'Doe ik het nou eigelijk wel goed?'
Kan iemand mij vertellen over aan mijn class nog ogen of haken zitten?

Verder zijn alle opmerkingen/verbeteringen uiteraard welkom.

Bugfix:
- 2x htmlspecialchars vervangen door mysql_real_escape_string

Update 1.01:
- Login methode verranderd, er word nu 1 saltkey aangemaakt voor alle gebruikers. Letop chmod werkt niet (volledig?) onder windows. Dus onder windows is het mogelijk om de salt.key te lezen. Als je gebruik maakt van een linux server: Dan niet.
- verifyPassword verwijderd. Er word pas data opgehaald als wachtwoord+gebruikersnaam correct is.

Update Versie 1.10:
- Nieuwe functie "setRequiredAuth" hiermee kun je een bepaald gebruiker niveau van de gebruiker eisen. Op dit moment geeft hij een een 'hard' exit. Met userlevels eventueel zelf uit database halen. Voorbeeld gegeven.
- Exceptions ipv eigen error handler
- PDO ipv mysql_error;
- Code optimalisatie

Update 1.11:
- ipadress veranderd naar ip_adress (typo)
- Groter ipv Kleiner hakje
- ($this->m_aUserdata = $aSql[0];) => ($this->m_aUserdata = $aSql;) Deze fout zat er in omdat ik iets te snel ben wisselt naar PDO.
[met dank aan Ron voor het vinden van deze fouten]

Todo:
- Goede oplossing vinden voor saltkeys onder windows. (chmod werkt niet namelijk, dus het bestand is gewoon te opnenen.
- setRequiredAuth opties toevoegen (redirecten naar login pagina bijvoorbeeld)

Gesponsorde koppelingen

PHP script bestanden

  1. authentication-class

 

Er zijn 22 reacties op 'Authentication class'

PHP hulp
PHP hulp
0 seconden vanaf nu
 

Gesponsorde koppelingen
Klystr
klystr
14 jaar geleden
 
0 +1 -0 -1
Allereerst moet ik zeggen dat je netjes programmeert, code ziet er echt uit zoals het hoort in mijn ogen.

Dan heb ik nog wel een vraagje, en da's niet ten nadele van jouw code, maar eigenlijk een heel concept dat ik niet begrijp.

Je hebt het dus over OOP, maar ik mis in PHP toch echt de hele OOP-aanpak. Ja, je kunt objecten van klassen aanmaken, maar in mijn ogen is de bedoeling van OOP dat je juist met meerdere objecten gaat werken (eigenlijk maak je in PHP steeds 1 object van de klasse), of...zie ik dit helemaal verkeerd en heb heb jij meerdere Authentication objecten naast elkaar lopen...

Verbeter me als ik ernaast zit hoor...maar in Java is OOP toch eigenlijk een hele andere insteek, of zie ik dat verkeerd?
Frank -
Frank -
14 jaar geleden
 
0 +1 -0 -1
Code (php)
PHP script in nieuw venster Selecteer het PHP script
1
2
3
4
<?
$p_sUsername
= htmlspecialchars($p_sUsername);
$p_sPassword = htmlspecialchars($p_sPassword);            
?>

htmlspecialchars() gebruik je om data op de juiste manier in de browser weer te geven, ik bespeur geen browser binnen jouw classe of database. Deze functie is dus niet op zijn plaats.
Code (php)
PHP script in nieuw venster Selecteer het PHP script
1
2
3
<?
sha1(sha1($p_sPassword).$this->m_aUserdata['saltkey'])
?>

Een dubbele sha1 is onveiliger dan een enkele en voegt dus helemaal niets toe aan de beveiliging. Het feit dat jij het wachtwoord van een user _uit_ de database haalt en dan pas gaat vergelijken, maakt de boel nog weer onveiliger. Dat de salt per user in de database wordt opgeslagen, maakt het toevoegen van salt volkomen zinloos, dit is nu voor iedereen toegankelijk die weet in te breken op de database. Dan is het een koud kunstje om de hashes te kraken.

Daarnaast roep jij dat bij een mislukte inlogpoging bv. het de user onbekend is of dat het wachtwoord fout is, dit soort informatie hoor je nooit te geven. Hiermee geef je de hacker al 50% van de informatie die hij nodig heeft, nog even doorzetten en de resterende informatie heeft hij ook te pakken.

Maak gewoon 1 query die een vergelijking doet op zowel de user als het wachtwoord en die niets doet met salt. De salt regel je in je PHP en sla je op buiten de webroot.
Code (php)
PHP script in nieuw venster Selecteer het PHP script
1
2
3
4
5
6
7
8
9
10
11
12
13
<?
$salt
= 'boe'; // opslaan in een extern bestand, buiten de webroot
$query = "
SELECT
  id
FROM
  gebruikers
WHERE
  gebruikersnaam = LOWER('"
.mysql_real_escape_string($_POST['gebruikersnaam']."')
AND
  userpassword = '"
.sha1($salt.$_POST['pw'])."'
"
;
?>


Netjes gescript, maar de gebruikte techniek zul je moeten aanpassen, de veiligheidsniveau is onnodig laag. Met een paar kleine en eenvoudige aanpassingen wordt het als snel 10x zo veilig.

Ps. Maak een VIEW aan in je database om de gegevens op te vragen en blokkeer vervolgens alle users op de tabel, maakt het nog weer veiliger.
Kwastie
Kwastie
14 jaar geleden
 
0 +1 -0 -1
@pgFrank
Dat 'htmlspecialchars' was een foutje, dat had htmlentities moeten zijn.

Over de Saltkey ben ik niet met je eens.
Ik ga er namelijk vanuit dat elke gebruiken een eigen Saltkey heeft. En dan is dit is de enige goede manier om het te gebruiken. Het hele idee achter een Saltkey is trouwens dat het moeilijker is om een wachtwoord te decoden, dit zal je dus alleen gebeuren als je database gehacked is of iemand teveel rechten heeft.

Over de error berichten ben ik het helemaal mee eens, ik moet niet 'teveel' informatie weggeven. Maar sommige berichten krijg je eigelijk nooit tezien als normale gebruiker. (mist je het goed in de applicatie verwerkt is natuurlijk)
Frank -
Frank -
14 jaar geleden
 
0 +1 -0 -1
Quote:
Dat 'htmlspecialchars' was een foutje, dat had htmlentities moeten zijn.
En dan is jouw database ineens een browser geworden? Je hebt geen html-functies nodig om met een database te werken, een database doet niets met html. html-functies gebruik je voor html-output naar de browser of evt. een html-email. Nooit en te nimmer voor data in een database.

En die salt, daar sla jij de plank goed mis. Het is inderdaad de bedoeling dat daarmee het "kraken" van de hash moeilijker wordt, maar wanneer jij zowel de hash als de key buiten de database bekend maakt (zie jouw SELECT-query), dan geef je de hacker alle informatie die hij/zij nodig heeft. Daarmee wordt het toevoegen van een snufje salt volkomen zinloos.

Daarnaast hoort de hash niet buiten de database te komen, dat mag gewoon niet gebeuren. Doe je dat wel, tja, dan vraag je om overbodige problemen. De salt sla je ergens op waar iemand er nooit bij kan komen, dus buiten de webroot en niet in je database. Wanneer nu iemand jouw database weet binnen te komen, heeft hij geen salt en wanneer iemand de database niet in komt heeft hij geen hash. Jij vertrekt echter alle informatie...

Kortom, jouw methode is zinloos en niet veiliger dan alleen een sha1-hash. De dubbele sha1 met een salt is schijnveiligheid, jij vertrekt alle benodigde informatie om de boel te hacken.
Kwastie
Kwastie
14 jaar geleden
 
0 +1 -0 -1
Ik slaap, htmlentities had add_slashes moeten zijn. Zoals jij al zegt: htmlentities is voor input van gebruikers. Htmlentities is bedoelt voor user-input.

Ja ik begrijp het, maar waar zou jij die externe value opslaan(saltkey dus) ?

Trouwens in jou 'hack' scenario ga je er vanuit dat er een fout zit in de afhandeling van 'input' van de gebruiker, mijn scenario ging ervanuit dat de server werd gehacked. Ik ga het dus verbeteren :)

(ik heb deze saltkey manier trouwens afgekeken van mybboard (een forum systeem))
Frank -
Frank -
14 jaar geleden
 
0 +1 -0 -1
Nee, ook niet add_slashes() gebruiken, die verknalt de data met slashes, zoals de naam al zegt. Je gaat op een slimme manier verboden tekens escapen, in jouw geval met mysql_real_escape_string(). Dan hoef je ook nooit meer met stripslashes() de rommel van addslashes() op te ruimen.

Mijn "hack-scenario" zorgt er voor dat er nooit een wachtwoordhash buiten de database terecht komt, er kan dus ook veel minder fout gaan. Je stopt alleen een hash ter vergelijking in de query, binnen de database wordt dan e.e.a. vergeleken. Er mag nooit en te nimmer een hash buiten de database komen, dan loop je namelijk al op 2 plaatsen risico dat een hacker er mee vandoor gaat. Dan zul je dus ook al op 2 plaatsen de boel moeten dichtspijkeren, wat een stuk moeilijker is.

De salt zet je in een php-bestandje buiten de webroot, is via de browser nooit te achterhalen, die je gebruikt om de hashes in je php-code aan te maken. Dit resultaat stop je in je query en de database zoekt het dan lekker zelf uit.

Uiteraard gebruik je een VIEW en spijker je de tabel dicht. Dan kan alleen de database administrator nog bij de hashes komen, voor de rest van de wereld is het over en sluiten. Dat gaat je met PHP niet lukken.
Mark PHP
Mark PHP
14 jaar geleden
 
0 +1 -0 -1
In dit artikel (punt 6) wordt een salt&pepper methode voorgesteld. Voor elke user houd je een unieke salt in de database bij en daarnaast nog een constante salt in je PHP-script (buiten je webroot uiteraard).
Zo heb je voor elke user gegarandeerd een verschillende hash, ook als twee users eenzelfde paswoord hebben.
Michiel Prank
Michiel Prank
14 jaar geleden
 
0 +1 -0 -1
Quote:
Mijn "hack-scenario" zorgt er voor dat er nooit een wachtwoordhash buiten de database terecht komt, er kan dus ook veel minder fout gaan. Je stopt alleen een hash ter vergelijking in de query, binnen de database wordt dan e.e.a. vergeleken. Er mag nooit en te nimmer een hash buiten de database komen, dan loop je namelijk al op 2 plaatsen risico dat een hacker er mee vandoor gaat. Dan zul je dus ook al op 2 plaatsen de boel moeten dichtspijkeren, wat een stuk moeilijker is.


Waarom niet? Als het in de PHP code terecht is gekomen, hoe wil je het dan als gebruiker eruit halen? De webserver hacken en een programma installeren waarmee je het geheugen kan inzien?
De waarde van die variabele kan je niet meer uithalen, tenzij er een paar groffe andere fouten in de PHP code zitten (slecht beveligde eval() of gwn dom gescript).

Verder ben ik het wel met je eens mbt real_escape_string, aangeven of de username of wachtwoord fout is, en etc.

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
<?php
   private function verifySession()
    {

        if($this->m_LoggedIn)
        {

            if($_SESSION['user_agent'] != $_SERVER['HTTP_USER_AGENT'])
            { }

            elseif($_SESSION['ip_adress'] != $_SERVER['REMOTE_ADDR'])
            { }

            elseif(empty($_SESSION['server_generated']))
            { }

            else
            {
                return true;
            }

            $this->setError('Sessie informatie incorrect, je bent uitgelogd');
            $this->Logout();
            return false;
        }

        return false;
    }

?>


Dat kan beter..
Jan geen
Jan geen
14 jaar geleden
 
0 +1 -0 -1
Misschien excepties gooien ipv setError()
Thijs X
Thijs X
14 jaar geleden
 
0 +1 -0 -1
Netjes gescript, ik zou idd Exceptions gebruiken voor je foutmeldingen. En waarom gebruik je in verifySession zon lange if else constructie als het ook met 1 kan?

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
<?php
   private function verifySession()
    {

        if($this->m_LoggedIn)
        {

            if( ($_SESSION['user_agent'] == $_SERVER['HTTP_USER_AGENT']) && ($_SESSION['ip_adress'] == $_SERVER['REMOTE_ADDR']) && !empty($_SESSION['server_generated']))
            {

                return true;
            }

            else {
                $this->setError('Sessie informatie incorrect, je bent uitgelogd');
                $this->Logout();
                return false;
            }
        }

        return false;
    }

?>
Kwastie
Kwastie
14 jaar geleden
 
0 +1 -0 -1
excepties vind ik niet erg fijn werken. Mischien in een later stadium voeg ik het nog wel toe.
Veder heb ik verifyPassword verwijderd en word er pas data uit de database gehaald als zowel het wachtwoord als gebruikers naam kloppen.

Als ik nu nogmaals een salt zou willen gebruiken, zoals Agirre beschrijft, zou ik de data eerst uit de database moeten halen en dan pas kunnen vergelijken, wat zoals pgFrank ook zegt: Fout is.
PHP erik
PHP erik
14 jaar geleden
 
0 +1 -0 -1
Voor de liefhebbers: bekijk Zend_Auth eens. Werkt echt perfect. Minimale code, maximaal resultaat. In ieder geval handig ter vergelijking.
Jacco Engel
Jacco Engel
14 jaar geleden
 
0 +1 -0 -1
Mischien dom idee voor salt maar :

Bij het genereren van een pass kijk je naar de elengte van de pass. Dit doe je min 1. Vervolgens doe je een rand tussen 0 en de lengte van je pass -1. Vervolgens sla je dit random nummer op. Je salt is dan elke keer :

substr(userpass,randomnr,1);

Zo weet jij alleen DAT er een salt is maar je hebt geen idee wat het is omdat je alleen de positie van de salt binnen het WW van je gebruiker weet. Verder is het bij gelijke wachtwoorden vaak verschillend.

Dat is waarschijnlijk hoe ik een salt zou maken.
Rudie dirkx
rudie dirkx
14 jaar geleden
 
0 +1 -0 -1
Ik sla wachtwoorden altijd op in combinatie met user id, dus dan zijn de hashes ook praktisch altijd verschillend.
Ik snap ook de functie van de salt niet in de db. Dit is niet waar ie voor is 'uitgevonden'.

pgFrank, wil je dit eens uitleggen:
Quote:
1. Een dubbele sha1 is onveiliger dan een enkele en voegt dus helemaal niets toe aan de beveiliging.
2. Het feit dat jij het wachtwoord van een user _uit_ de database haalt en dan pas gaat vergelijken, maakt de boel nog weer onveiliger.

Ik ben het met beide opmerkingen oneens: 1. een dubbele of driedubbele hash is volgens mij precies even veilig, alleen iets langzamer dan een enkele. 2. niemand ziet wat je ophaalt, dus waarom niet ophalen? Omdat het dan in het geheugen komt? SQL gooit ook spulletjes in het geheugen.
Ben erg benieuwd... Als je nou een goede uitleg hebt, moet ik veel gaan veranderen.
Jurgen assaasas
Jurgen assaasas
14 jaar geleden
 
0 +1 -0 -1
Ik vind het opzich een net script niets mis mee. Maar ik zou als ik jou was gewoon de Exception class gebruiken die zit ingebakken in PHP en werkt heel simpel.


Code (php)
PHP script in nieuw venster Selecteer het PHP script
1
2
3
4
5
6
7
8
9
10
11
12
<?
try
{
$someclass = new iets();
$someclass->somemethod();
}

catch(Exception $e);
{

echo $e->getMessage();
}


?>


In je klasse doe je dat dan zo:

Code (php)
PHP script in nieuw venster Selecteer het PHP script
1
2
3
4
5
6
7
8
9
10
11
<?php

public function method()
{

    if($i == 10)
    {

        throw new Exception('Hier je foutmelding.');
    }
}


?>


Misschien is het ook wel leuk om eens naar PDO te kijken om je applicatie makkelijker migreerbaar te maken.

Misschien nog een extra performance stukje, door de database versie van SHA1 te gebruiken. Dit is volgens mij ietsjes sneller maar niet noodzakelijk.


@t vercetti

Het is nergens voor nodig om een wachtwoord uit de database te halen. een simpele controle om te kijken of er een match is, is in principe genoeg.
Rudie dirkx
rudie dirkx
14 jaar geleden
 
0 +1 -0 -1
@Jurgen: dat weet ik wel, maar waarom is het onveilig om het 'uit de database te halen'?? Of PHP nou de match maakt, of de inner workings van SQL... Hoe is het ene veiliger of onveiliger dan het ander? Dat was mijn vraag.
Verder is het een beetje raar (misschien ligt dat aan mij) om een exception te gooien in een IF... Als je een 'error' kan afvangen met een IF, lijkt me een exception een beetje vergezocht, toch!?
Frank -
Frank -
14 jaar geleden
 
0 +1 -0 -1
@vercetti: Zowel de webserver (PHP) als de databaseserver kunnen worden gehackt. Wanneer jij er voor zorgt dat een hack op jouw webserver nog steeds geen data in de database bloot geeft, ben je dus veiliger. Dan hou je één enkel punt over waar je de data vrij geeft, de database. En laat de database nu prachtige tools hebben om de beveiliging te regelen: GRANT en REVOKE doen wonderen.

Een wachtwoord heb je nooit nodig buiten de database, dus geef je hem ook nooit vrij. Doe je dat wel, dan vraag je om, problemen. Via een hack op de webserver kan de hacker dan het wachtwoord opvragen uit de database.

Voor de onveilige dubbele hash mag je even de wiskunde en encryptietechnieken induiken, ik heb de achtergrond hier niet voorhanden. Het is me eens uitgelegd bij een cursus. Hexadecimaal kent te weinig variaties, dat was een belangrijk punt.

Maar goed, jouw hashes mogen nooit buiten de database komen, dan speelt dit probleem ook helemaal niet.
Kwastie
Kwastie
14 jaar geleden
 
0 +1 -0 -1
Update Versie 1.10:
- Nieuwe functie "setRequiredAuth" hiermee kun je een bepaald gebruiker niveau van de gebruiker eisen. Op dit moment geeft hij een een 'hard' exit. Met userlevels eventueel zelf uit database halen. Voorbeeld gegeven.
- Exceptions ipv eigen error handler
- PDO ipv mysql_query
- Code optimalisatie
Jan geen
Jan geen
14 jaar geleden
 
0 +1 -0 -1
Stukken beter, maar waarom haal je de constanten niet binnen je klasse?
Kwastie
Kwastie
14 jaar geleden
 
0 +1 -0 -1
Als de constanten buiten de classe zitten kan je ze eventueel nog uit een database halen anders niet.
Ron -
Ron -
14 jaar geleden
 
0 +1 -0 -1
je script klopt nog niet helemaal

je handeld de exceptions niet af, ze worden niet gevangen zodat je foutmeldingen krijgt

ook controleer je niet alles, bijv of de verbinding met de db wel wordt gemaakt, en of de querys kloppen

op mijn server werkt de functie getUserdata niet, de data die wordt opgehaald met de query, kan niet op jouw manier in een array worden gezet ($this->m_aUserdata = $aSql[0];)

ook staan er nog kleine typfouten in (ipadress ipv ip_adress)

voor de rest klasse script, ik begin pas net met klasses, dit is een goed voorbeeld, hopelijk kun je wat met mijn opmerkingen

dit:
Code (php)
PHP script in nieuw venster Selecteer het PHP script
1
2
3
4
5
6
<?php
if($_SESSION['auth_lvl'] > $p_nRequired)
{

        echo 'Je hebt niet genoeg rechten om hier te mogen komen';
}

?>

moet natuurlijk dit zijn:
Code (php)
PHP script in nieuw venster Selecteer het PHP script
1
2
3
4
5
6
<?php
if($_SESSION['auth_lvl'] < $p_nRequired)
{

        echo 'Je hebt niet genoeg rechten om hier te mogen komen';
}

?>
PHP hulp
PHP hulp
0 seconden vanaf nu
 

Gesponsorde koppelingen
Kwastie
Kwastie
14 jaar geleden
 
0 +1 -0 -1
Bedankt Ron, heb zelf deze fouten er ook al uitgehaald alleen ik heb hethet script hier niet geüpdate. Het script is namelijk sterk veranderd omdat hij gebruik maak van een andere database en userlevels uit de database haalt en iets anders gebruikt. Verder zou dit script nog gewoon moeten werken

Om te reageren heb je een account nodig en je moet ingelogd zijn.

Inhoudsopgave

  1. authentication-class

Labels

  • Geen tags toegevoegd.

Navigatie

 
 

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.