SQL, XSS Injection
ik ben bezig met een registratie script voor m'n website. Ik heb me voorgenomen de beveiliging zo goed als ik kan te maken vanaf het begin.
Is dit script veilig? En waarom geeft mysql_real_escape_string niets terug?
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
if($_SERVER['REQUEST_METHOD'] == 'POST') {
$fname = @$_POST['fname'];
$lname = @$_POST['lname'];
$uname = @$_POST['uname'];
$email = @$_POST['email'];
$password = @$_POST['password'];
$checkbox = @$_POST['checkbox'];
$errors = array();
if(trim($fname)=='') {
$errors[] = 'U moet uw voornaam invullen';
}
if(trim($lname)=='') {
$errors[] = 'U moet uw achternaam invullen';
}
if(trim($uname)=='') {
$errors[] = 'U moet uw gebruikersnaam invullen';
}
if(trim($email)=='') {
$errors[] = 'U moet uw email invullen';
}
if (filter_var($email, FILTER_VALIDATE_EMAIL)) {
} else {
$errors[] = 'U moet een geldig emailadres invullen';
}
if(trim($password)=='') {
$errors[] = 'U moet uw wachtwoord invullen';
} else {
$password = password_hash($password, PASSWORD_DEFAULT);
}
if(!isset($checkbox)) {
$errors[] = 'U moet de richtlijnen accepteren';
}
if ($errors==true) {
$err = true;
} else {
$sql = "INSERT INTO users (
id, fname, lname, uname, email, password
) VALUES (
'', '" . mysqli_real_escape_string($connection, $fname) . "', '" . mysqli_real_escape_string($connection, $lname) . "', '" . mysqli_real_escape_string($connection, $uname) . "', '" . mysqli_real_escape_string($connection, $email) . "', '" . mysqli_real_escape_string($connection, $password) . "'
)";
if(mysqli_query($connection, $sql)) {
echo 'Succes!';
} else {
echo "Error: " . $sql . "<br>" . mysqli_error($connection);
}
}
}
?>
if($_SERVER['REQUEST_METHOD'] == 'POST') {
$fname = @$_POST['fname'];
$lname = @$_POST['lname'];
$uname = @$_POST['uname'];
$email = @$_POST['email'];
$password = @$_POST['password'];
$checkbox = @$_POST['checkbox'];
$errors = array();
if(trim($fname)=='') {
$errors[] = 'U moet uw voornaam invullen';
}
if(trim($lname)=='') {
$errors[] = 'U moet uw achternaam invullen';
}
if(trim($uname)=='') {
$errors[] = 'U moet uw gebruikersnaam invullen';
}
if(trim($email)=='') {
$errors[] = 'U moet uw email invullen';
}
if (filter_var($email, FILTER_VALIDATE_EMAIL)) {
} else {
$errors[] = 'U moet een geldig emailadres invullen';
}
if(trim($password)=='') {
$errors[] = 'U moet uw wachtwoord invullen';
} else {
$password = password_hash($password, PASSWORD_DEFAULT);
}
if(!isset($checkbox)) {
$errors[] = 'U moet de richtlijnen accepteren';
}
if ($errors==true) {
$err = true;
} else {
$sql = "INSERT INTO users (
id, fname, lname, uname, email, password
) VALUES (
'', '" . mysqli_real_escape_string($connection, $fname) . "', '" . mysqli_real_escape_string($connection, $lname) . "', '" . mysqli_real_escape_string($connection, $uname) . "', '" . mysqli_real_escape_string($connection, $email) . "', '" . mysqli_real_escape_string($connection, $password) . "'
)";
if(mysqli_query($connection, $sql)) {
echo 'Succes!';
} else {
echo "Error: " . $sql . "<br>" . mysqli_error($connection);
}
}
}
?>
Alvast bedankt!!!
Mvg
Gewijzigd op 05/09/2015 18:38:54 door John De Zon
Zo moet het wel:
Code (php)
1
2
3
2
3
<?php
mysqli_real_escape_string($con, $fname);
?>
mysqli_real_escape_string($con, $fname);
?>
Verder raad ik aan om even per veld te kijken hoe je het valideert. Want ik heb het idee dat je lukraak gewoon wat regels aan het kopiëren bent. Want je kan prima kijken naar de juiste waarde die er nodig is.
Voor mailadressen is er filter_var('[email protected]', FILTER_VALIDATE_EMAIL);
Voor je checkbox zullen mogelijk vaste waardes zijn, zoals 1 of 0
En voor passwoorden hoe je niet op XSS te controleren. Die worden toch altijd gehashed.
Gewijzigd op 04/09/2015 15:00:35 door - Ariën -
Is deze code veilig? Welke extra beveiliging moet ik er nog aan toevoegen om echt veilig te zijn?
Gewijzigd op 04/09/2015 15:04:03 door - Ariën -
En empty() is overigens ook een vreemde controle die meer toestaan dan je hoopt.
Ik gebruik altijd:
if(trim($var)=='') {
Escaping (htmlspecialchars, mysqli_real_escape_string) werkt alleen goed wanneer je met de juiste character encoderingen aan de gang gaat:
(- in de opslag van je code-documenten, maar dit zorgt niet vaak voor problemen)
- in je HTML document
- in je database-connectie
- in je database tabellen
Verder:
Je bent daar INPUT aan het escapen middels (strip_tags, htmlspecialchars).
Waarom doe je dit? Dit wordt niet als een goede bezigheid beschouwd - je past daarmee al op voorhand je rauwe input aan. Dat is nergens voor nodig. Daarnaast escape je deze voor de HTML-context, terwijl je deze data daarna invoegt (en opnieuw escaped) in een SQL-context (met behulp van _real_escape_string).
Nog veel belangrijker dan wat je doet is dat je begrijpt wat je doet.
De algemene stelregel is nog steeds filter input, escape output.
Wat in het bovenstaande script ontbreekt is fatsoenlijke input filtering. Er vanuitgaande dat dit een soort van gebruikers registratie systeem is zou je aan de volgende input filtering kunnen denken:
- als alle invoer verplicht is, kijk of de getrimde variant van de invoer niet leeg is
- als je beperkingen wilt opleggen aan het uiterlijk van een gebruikersnaam, maak dan gebruik van een whitelist (geef aan wat is toegestaan) in plaats van een blacklist (door tags te strippen) en dan maar te hopen dat het resultaat iets is wat niet voor problemen zorgt; het grote nadeel van een blacklist is dat als je een geval vergeet dit geval wordt doorgelaten; daarom is een whitelist meestal beter: je definieert precies wat is toegestaan
- gebruik filter_var voor het e-mailadres en check voor een MX record, of nog beter, stuur gewoon een activatiemail naar het bewuste adres
Haal ook die @ weg en ontwikkel met error_reporting en display_errors aan.
Maak een database-wrapper voor mysqli-functies, zodat je in plaats van mysqli_real_escape_string($connection, $field) een kortere variant kunt gebruiken ($db->escape($field) ofzo) en ook andere zaken makkelijk kunt automatiseren.
Gewijzigd op 04/09/2015 15:17:21 door Thomas van den Heuvel
Thomas van den Heuvel op 04/09/2015 15:15:16:
- als alle invoer verplicht is, kijk of de getrimde variant van de invoer niet leeg is
Trimmen is niet slim op een wachtwoord veld. Het is een extra karakter die mensen kunnen gebruiken in hun wachtwoord.
Thomas van den Heuvel op 04/09/2015 15:15:16:
- als je beperkingen wilt opleggen aan het uiterlijk van een gebruikersnaam, maak dan gebruik van een whitelist (geef aan wat is toegestaan) in plaats van een blacklist (door tags te strippen) en dan maar te hopen dat het resultaat iets is wat niet voor problemen zorgt; het grote nadeel van een blacklist is dat als je een geval vergeet dit geval wordt doorgelaten; daarom is een whitelist meestal beter: je definieert precies wat is toegestaan
???
Een simpele regex is voldoende -> http://bfy.tw/1daY
Thomas van den Heuvel op 04/09/2015 15:15:16:
Maak een database-wrapper voor mysqli-functies, zodat je in plaats van mysqli_real_escape_string($connection, $field) een kortere variant kunt gebruiken ($db->escape($field) ofzo) en ook andere zaken makkelijk kunt automatiseren.
Als er een classe word gemaakt zou het "escapen" eigenlijk automatisch moeten gaan.
Gewijzigd op 04/09/2015 18:51:54 door Johan K
- Aar - op 04/09/2015 18:49:18:
Een reguliere expressie van tekens die mogen.
?? En hoe maak ik dat? Ik heb geen idee hoe ik daaraan zou moeten beginnen....
Alvast bedankt voor de snelle reacties!
Een voorbeeld is:
"[A-Za-z_0-9]"
Met preg_match() kan je controleren of een waarde aan deze voorwaarde voldoet.
Gewijzigd op 04/09/2015 19:07:24 door - Ariën -
Johan K op 04/09/2015 18:48:46:
Trimmen is niet slim op een wachtwoord veld. Het is een extra karakter die mensen kunnen gebruiken in hun wachtwoord.
Ik zeg nergens dat dat de enige controle zou moeten zijn of dat je dit altijd en overal blind zou moeten toepassen. Je moet per veld bepalen wat nodig is. Als een veld verplicht is, is het controleren of deze "leeg" is een minimale controle. Ik zeg dat je de getrimde variant moet vergelijken met een lege string, NIET dat je de invoer moet AANPASSEN naar de getrimde variant.
Johan K op 04/09/2015 18:48:46:
Een simpele regex is voldoende
Hoe is een regex geen whitelist?
Johan K op 04/09/2015 18:48:46:
Als er een classe word gemaakt zou het "escapen" eigenlijk automatisch moeten gaan.
Nee, niet automatisch nee. Tenzij je een prepared statements laag aan het bouwen bent, in welk geval je beter van PDO gebruik kunt maken (die van MySQLi is brak). Naar mijn mening moet je dit soort dingen niet automatiseren omdat er dan geen bewustzijn meer is.
Daarnaast wil je soms nog steeds de keuze hebben om op bepaalde plaatsen rauwe data te gebruiken, je wilt dan niet hebben dat deze data automatisch ge-escaped wordt. Je wilt niet dat je klasse je op voorhand beperkt in je handelen.
@Aar, je wilt de hele string controleren dit doe je met ^...expressie...$. Daarnaast heb je de de /i modifier die een case-insensitive match doet. Hierbij moet je ook niet vergeten dat $ ook één newline karakter (\n) accepteert wat vaak vergeten wordt en dat de return-waarde van preg_match gelijk moet zijn aan 1.
Een match voor een (niet-lege) string die enkel uit alfanumerieke karakters zou bestaan wordt dus zoiets:
Code (php)
1
2
3
4
5
6
7
8
9
2
3
4
5
6
7
8
9
<?php
$input = 'lala123';
if (preg_match('#^[a-z0-9]+$#i', $input) == 1) {
echo 'input bestaat enkel uit alfanumerieke karakters';
// en voor de goede orde zou je $input hier nog moeten trimmen!
} else {
echo 'input bestaat NIET enkel uit alfanumerieke karakters of is leeg';
}
?>
$input = 'lala123';
if (preg_match('#^[a-z0-9]+$#i', $input) == 1) {
echo 'input bestaat enkel uit alfanumerieke karakters';
// en voor de goede orde zou je $input hier nog moeten trimmen!
} else {
echo 'input bestaat NIET enkel uit alfanumerieke karakters of is leeg';
}
?>
Zelf ook ff testen natuurlijk...
Met accolades zou je nog een minimale/maximale lengte aan kunnen geven.
{exacte_lengte}
{minimale_lengte,}
{mininmale_lengte, maximale_lengte}
Gewijzigd op 04/09/2015 23:45:53 door Thomas van den Heuvel
Maar dit staat wel toe dat ik bv. Jan-" ingeef.
Je had dit ook met enige moeite al af kunnen leiden uit mijn vorige post:
Hele string matchen is ^...expressie...$
Gewijzigd op 04/09/2015 23:49:26 door Thomas van den Heuvel
Thomas van den Heuvel op 04/09/2015 19:45:32:
Ik zeg nergens dat dat de enige controle zou moeten zijn of dat je dit altijd en overal blind zou moeten toepassen. Je moet per veld bepalen wat nodig is. Als een veld verplicht is, is het controleren of deze "leeg" is een minimale controle. Ik zeg dat je de getrimde variant moet vergelijken met een lege string, NIET dat je de invoer moet AANPASSEN naar de getrimde variant.
Ik vergelijk niets met een lege string, hou het gewoon lekker simpel en voer direct de regex uit op velden waar nodig is zoals een username & password. Op andere velden zoals checkbox of optionele velden voldoet empty().
Thomas van den Heuvel op 04/09/2015 19:45:32:
Hoe is een regex geen whitelist?
Is het ook, maar je maakt het moeilijker dan dat het is.
Thomas van den Heuvel op 04/09/2015 19:45:32:
Nee, niet automatisch nee. Tenzij je een prepared statements laag aan het bouwen bent, in welk geval je beter van PDO gebruik kunt maken (die van MySQLi is brak). Naar mijn mening moet je dit soort dingen niet automatiseren omdat er dan geen bewustzijn meer is.
Daarnaast wil je soms nog steeds de keuze hebben om op bepaalde plaatsen rauwe data te gebruiken, je wilt dan niet hebben dat deze data automatisch ge-escaped wordt. Je wilt niet dat je klasse je op voorhand beperkt in je handelen.
Daarnaast wil je soms nog steeds de keuze hebben om op bepaalde plaatsen rauwe data te gebruiken, je wilt dan niet hebben dat deze data automatisch ge-escaped wordt. Je wilt niet dat je klasse je op voorhand beperkt in je handelen.
PDO bindParam / bindValue escaped alles als string tenzij anders is doorgegeven of je wilt je zelf open stellen om user input direct in je sql te gebruiken.
Voor mijn username validatie gebruik ik deze:
Code (php)
1
2
3
4
2
3
4
function validateUsername($name){
return preg_match('/^[A-z0-9_-]{3,16}$/', $name);
}
echo validateUsername('FooBar_09') ? 'true' : 'false'; // true;
return preg_match('/^[A-z0-9_-]{3,16}$/', $name);
}
echo validateUsername('FooBar_09') ? 'true' : 'false'; // true;
Dit houd in dat alfabetische, numerieke en _ en - karakters mogen, de rest niet.
Code (php)
Nadeel hieraan is dat je de gebruiker niet laat weten, als ie:
- zijn naam te kort heeft ingevuld
- zijn naam te lang heeft ingevuld
- zijn naam illegale karakters bevat
Gewijzigd op 05/09/2015 11:41:05 door DavY -
Johan K op 05/09/2015 11:18:56:
Ik vergelijk niets met een lege string, hou het gewoon lekker simpel en voer direct de regex uit op velden waar nodig is zoals een username & password. Op andere velden zoals checkbox of optionele velden voldoet empty().
Ik heb het ook niet over wat jij doet, ik voelde mij genoodzaakt op opnieuw/beter toe te lichten wat ik bedoelde, omdat het antwoord wat jij gaf mij deed denken dat ik invoer aan het aanpassen (trimmen) was...
Johan K op 05/09/2015 11:18:56:
Is het ook, maar je maakt het moeilijker dan dat het is.
Daarom zeg ik ook dat je per veld moet bepalen wat nodig is...
Johan K op 05/09/2015 11:18:56:
PDO bindParam / bindValue escaped alles als string tenzij anders is doorgegeven
Oh really, zet querylogging eens aan en kijk wat PDO daadwerkelijk doet. 9 van de 10 keer negeert deze gewoon typehints en zet om alle DATA quotes heen (in de uiteindelijke query). But hey, don't take my word for it.
Johan K op 05/09/2015 11:18:56:
of je wilt je zelf open stellen om user input direct in je sql te gebruiken.
Dat lijkt mij duidelijk niet de bedoeling. Maar goed, het kan zijn dat jij nooit rauwe SQL passages nodig hebt gehad in je query en hier dus nog nooit tegenaan bent gelopen.
Johan K op 05/09/2015 11:18:56:
Voor mijn username validatie gebruik ik deze:
Dit houd in dat alfabetische, numerieke en _ en - karakters mogen, de rest niet.
Code (php)
1
2
3
4
2
3
4
function validateUsername($name){
return preg_match('/^[A-z0-9_-]{3,16}$/', $name);
}
echo validateUsername('FooBar_09') ? 'true' : 'false'; // true;
return preg_match('/^[A-z0-9_-]{3,16}$/', $name);
}
echo validateUsername('FooBar_09') ? 'true' : 'false'; // true;
Dit houd in dat alfabetische, numerieke en _ en - karakters mogen, de rest niet.
Lees de handleiding van preg_match() eens, hier staat:
Quote:
preg_match() returns 1 if the pattern matches given subject, 0 if it does not, or FALSE if an error occurred.
Als je je return-value van je functie als boolean wilt behandelen/gebruiken, vergelijk dan met het cijfer 1.
Daarnaast accepteert jouw functie ook de volgende waarde:
"1234567890123456\n"
Yup. 17 karakters. En een linebreak. De volgende keer dat jij een csv-export van je users maakt dan breekt deze misschien wel.
Lukraak reageren op wat iemand zegt is makkelijk.
Proberen te begrijpen en te doorgronden wat iemand bedoelt is aanzienlijk moeilijker.
Waarom niet gewoon zo?
Code (php)
1
2
3
4
5
6
7
2
3
4
5
6
7
<?php
$input = 'abc123';
if (ctype_alnum($input)) {
echo 'oke';
} else {
echo 'niet oke of leeg';
}
$input = 'abc123';
if (ctype_alnum($input)) {
echo 'oke';
} else {
echo 'niet oke of leeg';
}
Dat van die whitelist ga ik niet meer doen omdat ik het nut er niet meer kan inzien. Dank voor het uitleggen van de Whitelist ik zal dit zeker meenemen.
Is het script veilig? Zijn er eventueel nog dingen die beter moeten?
Een rtrim($iets) zou echt handig zijn.
Ook bij emailadressen: mijn emailadres staat in mijn woordenboek (Swiftkey), dus als ik maar 4 letters type, dan vult hij het aan. Met een spatie. Nu moet ik die altijd verwijderen (heel vervelend) omdat het anders verkeerd kan gaan.
Trouwens: facebook bijvoorbeeld niet. Als je daar je emailadres invult bij het inloggen MET een spatie, dan haalt ie die spatie weg.
Waarschijnlijk kijkt hij eerst of je rauwe input bestaat. Zo niet: is een rtrim() anders dan de rauwe input? Zo ja: probeer het eens met rtrim() erover..