Testing SQL-Injection

Unregistriert

Hallo

Ich möchte wissen ob ich mit diesem Script vor SQL-Injections sicher bin.

PHP

<?php
error_reporting(E_ALL);
$username = (isset($_GET['username']) and !empty($_GET['username'])) ? $_GET['username'] : '';
$password = (isset($_GET['password']) and !empty($_GET['password'])) ? $_GET['password'] : '';
if ($username and $password) {
	$db = @mysql_connect('xxxx', 'xxxx', 'xxxx') or die("No Access");
	$select = @mysql_select_db("xxxx", $db) or die("NO Access");
	if (get_magic_quotes_gpc) {
		$username = stripslashes($username);
		$password = stripslashes($password);
	}
	$username = mysql_real_escape_string($username); 
	$password = mysql_real_escape_string($password); 
	$result = mysql_query("SELECT `username`, `password` FROM `test` WHERE `username` ='$username' and `password` ='$password'");
	if (!$result) {
		echo "Connection failed";
		exit;
	}
	else {
		$row = mysql_fetch_assoc($result);
		if ($row) {
			echo '<span style="color:red;">' . date("m:d:Y") .', '. date("h:i:s A") .' </span><br \>';
			echo  '<h3 style="display: inline;">Success</h3>: Your are inside!';
		}
		else {
			echo "<h2>No Access!</h2>";
		}
	} 
}
else {
	echo 'Please get <a href="test.php">back</a> and fill in all fields.';
}
?>

Alles anzeigen

Bitte testen und mir mitteilen ob ich eine Lücke offen habe: http://www.reza24.com/test.php

The User

Ich kann da SQLMap empfehlen. (siehe Injection)
Dein Script ist aber so weit was SQL-Injection angeht in Ordnung.
Aber:
-Die Überprüfung mit isset, empy etc. ist absolut unnötig und somit zeitraubend.
-Lass das mit der magic-quotes-Überprüfung. Wenn du sicher gehen willst, dass keine nervigen Backslashes da sind, dann überprüf das ganz am Anfang des Scripts und entferne sie in allen $_POST, $_GET und $_COOKIE Elementen. Dann sparst du dir die ständige Abfrage.
-Die Ansage: "Connection Failed" ist Unsinn, vielmehr ist das Passwort falsch.
-Du musst deine Passwörter hashen! Ein winzige Sicherheitslücke, und ein Angreifer würde alle Passwörter auslesen können. Da musst du dich absichern.

Viele liebe Grüße
The User

Afrael

Zitat von The User

-Die Überprüfung mit isset, empy etc. ist absolut unnötig und somit zeitraubend.

Ich glaub, da werden wir beide uns nie einig Fakt ist, dass in diesem Fall die Prüfung auf !empty tatsächlich überflüssig ist. Du setzt $username ja im Zweifelsfalle sowieso auf leer, da ist es egal, ob vielleicht schon leer ist.
Fakt ist außerdem, dass man eine E_NOTICE kriegt, wenn man isset nicht verwendet. Und ich mag meine Variablen lieber initialisiert.

Zitat

-Die Ansage: "Connection Failed" ist Unsinn, vielmehr ist das Passwort falsch.

Äh... die mysql_query scheitert ja schon, nicht die Ergebnisse. Das mit Connection Failed dürfte dann stimmen.
@Threadersteller
Das muss <br /> statt <br \> heißen
Ach, und zur Verbesserung der Performance solltest du in

PHP

SELECT `username`, `password` FROM `test` WHERE `username` ='$username' and `password` ='$password'

ein LIMIT 1 hinzufügen, du hast ja nur einen Benutzer mit diesem Benutzernamen.
Ansonsten seh ich auch keine Möglichkeit einer Injection.

The User

@Koko
Hast Recht, bei dem Connection-Fail hatte ich mich verlesen. Ich dachte da wäre schon fetch.

Ich habs ausprobiert:
Du hast Recht: Mit Prüfung ist es schneller, scheiß Notice, absolut sinnlos.

@Threadstarter
Wonach hast du gesucht, um das Forum zu finden?

Unregistriert

Also was das 'Passwörter hashen' angeht, muss ich mich erst noch informieren. Kenne diese Funktion noch nicht. Jedoch kenne ich hash von anderen Programmiersprachen, bin mir aber nicht sicher ob auch das damit gemeint ist. Naja, ich werd's schon sehen.

Was die E_NOTICE angeht, finde ich ein bisschen komisch, denn ich bekomme keinerlei Notices oder irgendwelche Art von Fehlermeldungen angezeigt.

Was die isset() und empty() angeht, habt ihr beide natürlich recht. Ich habe versucht den Code ein wenig sinnvoller zu gestalten. So sieht er nun aus:

PHP

$username = (isset($_GET['username']) and !empty($_GET['username'])) ? $_GET['username'] : 'false';
$password = (isset($_GET['password']) and !empty($_GET['password'])) ? $_GET['password'] : 'false';
if ($username and $password != 'false') {

Ist diese Methode nun besser oder habt ihr einen besseren Vorschlag? Ich arbeite deshalb mit den Ternary Operator, da ich nicht grad ein Fan von langen Scripts bin.

The User: Ich kenne dieses Forum schon seit über einem Jahr. Wahrscheinlich habe ich damals einfach nur nach 'HTML' gesucht

Afrael

Zitat von Unregistriert

Was die E_NOTICE angeht, finde ich ein bisschen komisch, denn ich bekomme keinerlei Notices oder irgendwelche Art von Fehlermeldungen angezeigt.

Jap, ist richtig, weil du sauber codest Würdest du einfach $username=$_GET['username']; schreiben, hättest du eine E_NOTICE, wenn $_GET['username'] nicht initialisiert ist;

Zitat
Was die isset() und empty() angeht, habt ihr beide natürlich recht. Ich habe versucht den Code ein wenig sinnvoller zu gestalten. So sieht er nun aus:
PHP
$username = (isset($_GET['username']) and !empty($_GET['username'])) ? $_GET['username'] : 'false';
$password = (isset($_GET['password']) and !empty($_GET['password'])) ? $_GET['password'] : 'false';
if ($username and $password != 'false') {
Ist diese Methode nun besser oder habt ihr einen besseren Vorschlag?

Ich fand das, was du da hattest, schon richtig so, bis auf die Sache mit dem empty, die ist wie gesagt überflüssig.

PHP

$username = isset($_GET['username']) ? $_GET['username'] : '';
$password = isset($_GET['password'])  ? $_GET['password'] : '';

So würd ichs machen.

Unregistriert

Ahso, tut mir leid, dann habe ich dich wohl falsch verstanden. Na dann ist ja alles in Ordnung.

Danke für die Aufklärung!

Tags