Tijden veranderen
Beveiligingscontrole loginsysteem
Momenteel ben ik bezig om een veilig loginsysteem te bouwen voor een bekende van me. Nu luidt de vraag; is mijn beveiligingssysteem (redelijk) veilig?
- De gebruiker logt in met de gegevens op de loginpagina. Indien alles correct is zal het systeem de sessies aanmaken voor de gebruiker. Één sessie voor het gebruikersid, één voor het ip-adres en één voor de sleutel.
- Op een 'beveiligde pagina' wordt de gebruiker gechecked door middel van de functie die hieronder staat.
Note 1: de inlogpagina is volgens mij wel veilig, maar kan ik op verzoek ook wel even plaatsen.
Note 2: de functie werkt gewoon dus ik test hem wél.
Kortom; is deze functie veilig genoeg?
Code (php)
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
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
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
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
<?php
function getLevel()
{
if (isset($_SESSION['gebruikersid']) AND !empty($_SESSION['gebruikersid']) AND ctype_digit($_SESSION['gebruikersid']))
{
if (isset($_SESSION['ipadres']) AND !empty($_SESSION['ipadres']) AND $_SESSION['ipadres'] == $_SERVER['REMOTE_ADDR'])
{
if (isset($_SESSION['sleutel']) AND !empty($_SESSION['sleutel']) AND strlen($_SESSION['sleutel']) == 40)
{
$qSelect = mysql_query("SELECT gebruiker_actief
FROM gebruikrs
WHERE gebruiker_id = '". veiligeInvoer($_SESSION['gebruikersid']) ."'
AND inlog_ipadres = '". veiligeInvoer($_SESSION['ipadres']) ."'
AND inlog_sleutel = '". veiligeInvoer($_SESSION['sleutel']) ."'
");
if (mysql_num_rows($qSelect) == 1)
{
$qFetch = mysql_fetch_assoc($qSelect);
if ($qFetch['gebruiker_actief'] == 1)
{
return 1;
}
else
{
verwijderSessies();
return 0;
}
}
else
{
verwijderSessies();
return 0;
}
}
else
{
verwijderSessies();
return 0;
}
}
else
{
verwijderSessies();
return 0;
}
}
else
{
verwijderSessies();
return 0;
}
}
?>
function getLevel()
{
if (isset($_SESSION['gebruikersid']) AND !empty($_SESSION['gebruikersid']) AND ctype_digit($_SESSION['gebruikersid']))
{
if (isset($_SESSION['ipadres']) AND !empty($_SESSION['ipadres']) AND $_SESSION['ipadres'] == $_SERVER['REMOTE_ADDR'])
{
if (isset($_SESSION['sleutel']) AND !empty($_SESSION['sleutel']) AND strlen($_SESSION['sleutel']) == 40)
{
$qSelect = mysql_query("SELECT gebruiker_actief
FROM gebruikrs
WHERE gebruiker_id = '". veiligeInvoer($_SESSION['gebruikersid']) ."'
AND inlog_ipadres = '". veiligeInvoer($_SESSION['ipadres']) ."'
AND inlog_sleutel = '". veiligeInvoer($_SESSION['sleutel']) ."'
");
if (mysql_num_rows($qSelect) == 1)
{
$qFetch = mysql_fetch_assoc($qSelect);
if ($qFetch['gebruiker_actief'] == 1)
{
return 1;
}
else
{
verwijderSessies();
return 0;
}
}
else
{
verwijderSessies();
return 0;
}
}
else
{
verwijderSessies();
return 0;
}
}
else
{
verwijderSessies();
return 0;
}
}
else
{
verwijderSessies();
return 0;
}
}
?>
Alvast bedankt.
Met vriendelijke groet,
Kevin de Groot
Gewijzigd op 15/12/2010 17:14:09 door Kevin de Groot
Code (php)
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
39
40
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
39
40
<?php
function getLevel()
{
if (isset($_SESSION['gebruikersid']) AND !empty($_SESSION['gebruikersid']) AND ctype_digit($_SESSION['gebruikersid']))
{
$sessie = false;
if (isset($_SESSION['ipadres']) AND !empty($_SESSION['ipadres']) AND $_SESSION['ipadres'] == $_SERVER['REMOTE_ADDR'])
{
$sessie = false;
if (isset($_SESSION['sleutel']) AND !empty($_SESSION['sleutel']) AND strlen($_SESSION['sleutel']) == 40)
{
$sessie = false;
$qSelect = mysql_query("SELECT gebruiker_actief
FROM gebruikrs
WHERE gebruiker_id = '". veiligeInvoer($_SESSION['gebruikersid']) ."'
AND inlog_ipadres = '". veiligeInvoer($_SESSION['ipadres']) ."'
AND inlog_sleutel = '". veiligeInvoer($_SESSION['sleutel']) ."'
");
if (mysql_num_rows($qSelect) == 1)
{
$qFetch = mysql_fetch_assoc($qSelect);
if ($qFetch['gebruiker_actief'] == 1)
{
return 1;
}
else if ( $sessie = false )
{
verwijderSessie();
}
}
}
}
}
}
[/code]
function getLevel()
{
if (isset($_SESSION['gebruikersid']) AND !empty($_SESSION['gebruikersid']) AND ctype_digit($_SESSION['gebruikersid']))
{
$sessie = false;
if (isset($_SESSION['ipadres']) AND !empty($_SESSION['ipadres']) AND $_SESSION['ipadres'] == $_SERVER['REMOTE_ADDR'])
{
$sessie = false;
if (isset($_SESSION['sleutel']) AND !empty($_SESSION['sleutel']) AND strlen($_SESSION['sleutel']) == 40)
{
$sessie = false;
$qSelect = mysql_query("SELECT gebruiker_actief
FROM gebruikrs
WHERE gebruiker_id = '". veiligeInvoer($_SESSION['gebruikersid']) ."'
AND inlog_ipadres = '". veiligeInvoer($_SESSION['ipadres']) ."'
AND inlog_sleutel = '". veiligeInvoer($_SESSION['sleutel']) ."'
");
if (mysql_num_rows($qSelect) == 1)
{
$qFetch = mysql_fetch_assoc($qSelect);
if ($qFetch['gebruiker_actief'] == 1)
{
return 1;
}
else if ( $sessie = false )
{
verwijderSessie();
}
}
}
}
}
}
[/code]
heb je hem al getest? zoja ? en werkt hij ook
Kevin de Groot op 15/12/2010 16:47:15:
Code (php)
1
2
3
4
2
3
4
<?php
if (isset($_SESSION['gebruikersid']) AND !empty($_SESSION['gebruikersid']) AND ctype_digit($_SESSION['gebruikersid']))
?>
if (isset($_SESSION['gebruikersid']) AND !empty($_SESSION['gebruikersid']) AND ctype_digit($_SESSION['gebruikersid']))
?>
Waarom die !empty controle; je controleert toch met isset al of session bestaat en met ctype controleer je of het een getal is?
Kevin de Groot op 15/12/2010 16:47:15:
Hiervoor hetzelfde; waarom die !empty controle?
Kevin de Groot op 15/12/2010 16:47:15:
Hiervoor hetzelfde; waarom die !empty controle?
En waarom in zoveel regels? Afgaande op de inhoud van de diverse else, kan het volgens mij gewoon in 1x.
Zorg voor foutafhandeling van je query.
Waar komt veiligeInvoer en verwijderSessie vandaan en wat doet die?
Daarnaast: er is verschil tussen AND en &&.
Gewijzigd op 15/12/2010 17:00:45 door Obelix Idefix
De functie 'veiligeInvoer' ziet er als volgt uit:
Code (php)
1
2
3
4
5
6
7
8
9
10
2
3
4
5
6
7
8
9
10
<?php
function veiligeInvoer($sInput)
{
$sOutput = (function_exists('mysql_real_escape_string')) ? mysql_real_escape_string($sInput) : addslashes($sInput);
return htmlentities($sOutput, ENT_QUOTES);
}
?>
function veiligeInvoer($sInput)
{
$sOutput = (function_exists('mysql_real_escape_string')) ? mysql_real_escape_string($sInput) : addslashes($sInput);
return htmlentities($sOutput, ENT_QUOTES);
}
?>
De functie 'verwijderSessies' ziet er als volgt uit:
Code (php)
1
2
3
4
5
6
7
8
9
10
11
12
2
3
4
5
6
7
8
9
10
11
12
<?php
function verwijderSessies()
{
if (isset($_SESSION['gebruikersid']))
{
$_SESSION = array();
session_destroy();
}
}
?>
function verwijderSessies()
{
if (isset($_SESSION['gebruikersid']))
{
$_SESSION = array();
session_destroy();
}
}
?>
Jordi kroon op 15/12/2010 17:09:02:
zonder session_start(); werkt session_destroy(); ook niet
Het bestand waar deze functie in staat wordt d.m.v. een include 'toegevoegd' aan de index.php. In de index.php staat session_start();.
Obelix en Idefix op 15/12/2010 17:00:01:
... Waarom die !empty controle; ...
Kevin de Groot op 15/12/2010 17:04:33:
Ik controleer eerst of de sessie geset is, vervolgens of hij niet leeg is (want het is mogelijk dat een sessie geen waarde heeft)
De functie empty doet zelf al een isset check. Je mag dus empty() gebruiken zonder dat je eerst moet checken op isset().
Kris Peeters op 15/12/2010 17:14:36:
De functie empty doet zelf al een isset check. Je mag dus empty() gebruiken zonder dat je eerst moet checken op isset().
Obelix en Idefix op 15/12/2010 17:00:01:
... Waarom die !empty controle; ...
Kevin de Groot op 15/12/2010 17:04:33:
Ik controleer eerst of de sessie geset is, vervolgens of hij niet leeg is (want het is mogelijk dat een sessie geen waarde heeft)
De functie empty doet zelf al een isset check. Je mag dus empty() gebruiken zonder dat je eerst moet checken op isset().
Dit is nieuw voor mij, ik las zoiets dus net ook al op PHP.net. Ik zal het er meteen in verwerken. Dankje. Maar kunnen mensen (lees: hackers) ook eenvoudig inbreken?
Kwestie dat mensen geen actieve html kunnen posten (post bv. dit: "</div></div></div>" en de kans is vrij groot dat je hele lay out naar de knoppen is).
htmlentities gebruik je dan ook bij het tonen van gegevens naar de client. De functie dient niet om je database te beschermen.
Ik zie dus niet goed wat dat daar komt te staan.
Ik zal zelf ook eens testen.
Nog iets: als je een integer verwacht, cast die dan naar int. Meer bescherming heb je niet nodig.
bv. WHERE gebruiker_id = '". (int) $_SESSION['gebruikersid'] ."'
----------------------------------
PS.
@ PHP Scripter:
else if ( $sessie = false ) {
Dit slaat nergens op.
Ten eerste hoort daar een dubbele =
Ten tweede, ik zie nergens $sessie = true; staan.
Gewijzigd op 17/12/2010 13:52:34 door Kris Peeters
Kris Peeters op 17/12/2010 13:48:36:
htmlentities is een functie die je beschermt tegen (al dan niet kwalijke) html.
Kwestie dat mensen geen actieve html kunnen posten (post bv. dit: "</div></div></div>" en de kans is vrij groot dat je hele lay out naar de knoppen is).
htmlentities gebruik je dan ook bij het tonen van gegevens naar de client. De functie dient niet om je database te beschermen.
Ik zie dus niet goed wat dat daar komt te staan.
Ik zal zelf ook eens testen.
----------------------------------
PS.
@ PHP Scripter:
else if ( $sessie = false ) {
Dit slaat nergens op.
Ten eerste hoort daar een dubbele =
Ten tweede, ik zie nergens $sessie = true; staan.
Kwestie dat mensen geen actieve html kunnen posten (post bv. dit: "</div></div></div>" en de kans is vrij groot dat je hele lay out naar de knoppen is).
htmlentities gebruik je dan ook bij het tonen van gegevens naar de client. De functie dient niet om je database te beschermen.
Ik zie dus niet goed wat dat daar komt te staan.
Ik zal zelf ook eens testen.
----------------------------------
PS.
@ PHP Scripter:
else if ( $sessie = false ) {
Dit slaat nergens op.
Ten eerste hoort daar een dubbele =
Ten tweede, ik zie nergens $sessie = true; staan.
Dat heb ik gedaan voor overige invoer (voor bijvoorbeeld berichten) en dacht dat het geen kwaad zou doen om deze functie ook hier te gebruiken. Vandaar die htmlentities.
Edit: die htmlentities is voor TniyMCE.
Gewijzigd op 17/12/2010 13:53:11 door Kevin de Groot
Dan nog ...
Het is beter om data van gebruikers zo puur mogelijk op te slaan. Juist beschermen tegen injection.
Je moet dan wel telkens bij het tonen van de data elk veld controleren.
Een argument: stel dat een gebruiker zijn post wil aanpassen, dan zal hij niet snappen dat daar van alles staat dat hij niet zelf heeft gepost.
Gewijzigd op 17/12/2010 13:57:47 door Kris Peeters
Kris Peeters op 17/12/2010 13:56:17:
(gepost vóór de laaste aanpassing vorige post)
Dan nog ...
Het is beter om data van gebruikers zo puur mogelijk op te slaan. Juist beschermen tegen injection.
Je moet dan wel telkens bij het tonen van de data elk veld controleren.
Een argument: stel dat een gebruiker zijn post wil aanpassen, dan zal hij niet snappen dat daar van alles staat dat hij niet zelf heeft gepost.
Dan nog ...
Het is beter om data van gebruikers zo puur mogelijk op te slaan. Juist beschermen tegen injection.
Je moet dan wel telkens bij het tonen van de data elk veld controleren.
Een argument: stel dat een gebruiker zijn post wil aanpassen, dan zal hij niet snappen dat daar van alles staat dat hij niet zelf heeft gepost.
Oké, ik kreeg het namelijk niet voor elkaar zonder die htmlentities. Maar beveiliging voor de database (injecties) kan dus gewoon beter met addslashes of mysql_real_escape_string, toch?
De else doet al alles; die $sessie doet niets.
Gewijzigd op 17/12/2010 14:04:16 door Kris Peeters
Note: ik heb de else-jes laten staan. Dit vind ik sowieso wat overzichtelijker.
Code (php)
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
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
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
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
<?php
function mijnGebruikerslevel()
{
if (!empty($_SESSION['gebruikersid']) AND ctype_digit($_SESSION['gebruikersid']))
{
if (!empty($_SESSION['ipadres']) AND $_SESSION['ipadres'] == $_SERVER['REMOTE_ADDR'])
{
if (!empty($_SESSION['sleutel']) AND strlen($_SESSION['sleutel']) == 40)
{
$gSelect = mysql_query("SELECT COUNT(gebruikersid) AS aantal_resultaten
FROM gebruikers
WHERE gebruiker_id = '". (int)$_SESSION['gebruikersid'] ."'
AND gebruiker_actief = '1'
AND login_ipadres = '". $_SESSION['ipadres'] ."'
AND login_sleutel = '". $_SESSION['sleutel'] ."'
ORDER BY gebruiker_id
LIMIT 0,1
");
$gFetch = mysql_fetch_assoc($gSelect);
if ($gFetch['aantal_resultaten'] == 1)
{
return true;
}
else
{
verwijderSessies();
return false;
}
}
else
{
verwijderSessies();
return false;
}
}
else
{
verwijderSessies();
return false;
}
}
else
{
verwijderSessies();
return false;
}
}
function verwijderSessies()
{
if (isset($_SESSION['gebruikersid']))
{
$_SESSION = array();
session_destroy();
}
}
?>
function mijnGebruikerslevel()
{
if (!empty($_SESSION['gebruikersid']) AND ctype_digit($_SESSION['gebruikersid']))
{
if (!empty($_SESSION['ipadres']) AND $_SESSION['ipadres'] == $_SERVER['REMOTE_ADDR'])
{
if (!empty($_SESSION['sleutel']) AND strlen($_SESSION['sleutel']) == 40)
{
$gSelect = mysql_query("SELECT COUNT(gebruikersid) AS aantal_resultaten
FROM gebruikers
WHERE gebruiker_id = '". (int)$_SESSION['gebruikersid'] ."'
AND gebruiker_actief = '1'
AND login_ipadres = '". $_SESSION['ipadres'] ."'
AND login_sleutel = '". $_SESSION['sleutel'] ."'
ORDER BY gebruiker_id
LIMIT 0,1
");
$gFetch = mysql_fetch_assoc($gSelect);
if ($gFetch['aantal_resultaten'] == 1)
{
return true;
}
else
{
verwijderSessies();
return false;
}
}
else
{
verwijderSessies();
return false;
}
}
else
{
verwijderSessies();
return false;
}
}
else
{
verwijderSessies();
return false;
}
}
function verwijderSessies()
{
if (isset($_SESSION['gebruikersid']))
{
$_SESSION = array();
session_destroy();
}
}
?>
ik kan het mishebben maar is dat niet overbodig