QUOTE (OneScripts @ Mo 27.02.2012, 20:01)[...] - Das der Quellcode für dich unsauber wirkt kann ich nicht nachvollziehen, er ist ordentlich und strukturiert geschrieben. Und wenn mal nicht alles so schön eingerückt ist, ist das aber kein Markel. [...]
Wenn ich in einen Quellcode folgendes zu sehen bekomme (siehe unten), ist das für mich unsauber. Gerade beim Programmieren ist sauberes Eindrücken das A & O. Von Englisch-Deutsch-Mix bei der Variablenbezeichnung wollen wir gar nicht erst groß anfangen.
Mal so als Beispiel aus der userlist.php (echo und SQL-Querys gekürzt):
CODE if(isset($_POST['submit'])){
$problem = FALSE;
if(empty($_POST['search'])){
$problem = TRUE;
echo '<p class="false">&nbsp;&nbsp;&nbsp;Geben Sie einen Suchbegriff ein!</p>';
echo '<p>&nbsp;&nbsp;&nbsp;[ <a href="userlist.php">Zur Suche</a> ]</p><br />';
}
if(!$problem){
$text = $_POST['search'];
$querys = "SELECT ...";
$results = mysql_query($querys);
if($results){
echo '...';
while ($row = mysql_fetch_array($results, MYSQL_ASSOC)){
echo '...';
}
echo '...';
}else{
echo '...';
}
Wobei hier noch eine SQL-Injektionslücke wäre, die ausgenutzt werden könnte. Und ich habe den Weg sauber zurückverfolgt bei den includes, das Ding wird ungefiltert übergeben, ok mit aktiven magic_quotes wohl nicht so das Problem, aber ohne, nur wenn das aktiv ist müssten die anderen Eingabedaten auch erst mal durch ein stripslashes, damit man sauber mit den Eingaben arbeiten kann, findet auch aber auch nicht statt:
CODE $text = $_POST['search'];
$querys = "SELECT user_id, user_name FROM users
WHERE user_name LIKE '%$text%' OR user_id LIKE '%$text%' OR user_email LIKE '%$text%'
ORDER BY user_id DESC";
$results = mysql_query($querys);
Und warum zum Teufel wird nach user_id sortiert und nicht nach user_name?
QUOTE [...] - Das mit dem BBCode kann man natürlich änder (wer ich auch machen), aber java alerts oder ähnliches kann man trotzdem nicht ausführen. Ich hab mit strip_tags dieses Problem verhindert (auch ausführlich getestet). [...]
strip_tags verhindert meinen hier aufgeführten und relativ simplen XSS nicht! Auch ist strip_tags alleine nicht wirklich berauschend um Sicherheit herzustellen, das ließ sich bisher mit etwas Aufwand trotzdem mit HTML-Tags füttern.
Wobei es unsinnig ist einen Forumsbeitrag komplett von HTML-Code zu bereinigen, sonst ist es als Forum nicht geeignet, es muss eigentlich kontrolliert und entschärft werden.
QUOTE [...]
- Zitat:
Überflüssiger Programmcode wie $msg = $msg; oder <?php echo '' . $title . ''; ?>
Warum sollte $msg überflüssig sein, wenn ich an dieser Stelle eine Nachricht ausgeben möchte bei bedarf?
und echo $title liegt daran, damit natürlich der Seitentitel auch angezeigt wird, den der User in der config.php festgelegt hat.
[...]
Weil ein
$msg = $msg bei einer Funktion die $msg als Übergabewert hat, total sinnlos ist, Du machst rein gar nichts mit der Anweisung, außer $msg $msg zuweisen...
Selbst wenn Du es ausgeben möchtest für Debug-Informationen, muss es nicht im Quellcode so stehen bleiben, oder könnte klar mit einer Debug-Flag ausgegrenzt werden, so dass klar wird, was das soll, oder es könnte auskommentiert dafür aber klar erkennbar als Ausgabe in der Zeile stehen. Aber eine Zuweisung auf sich selbst... Ganz ehrlich, das hört sich für mich gerade nur eher nach einen faulen Ausrede an.
Ich rede auch nicht vom $title sondern vom
'', so was solltest Du eigentlich selber erkennen.
QUOTE [...]
- Zitat:
seltame Programmzeilen, wie bspw. <li><a href="inbox.php?user_id=' . $_SESSION['user_id'] . '">Posteingang</a></li>
Was soll da komisch sein? Das ist ein Link zum Posteingang mit der dazuhörigen user id.
[...]
Weil die $_SESSION['user_id'] bei Übergabe der Session eh zur Verfügung stehen müsste, wozu diese also nochmal per GET-Methode übergeben? Selbst wenn die Session nicht als Cookie übergeben wird, wieso die UserID übergeben und nicht die Session?
QUOTE [...]
- Zitat:
$_POST['submit']=='Passwort ändern' & Verwendung des veralteten MySQL-Moduls
Was passt dir daran nicht? Könntest du schon genauer definieren.
[...]
Das MySQL-Module fliegt ab PHP 6 raus und wird nur noch notdürftig gepflegt, daher ist für alle neuen Projekte MySQLi Pflicht, bzw. sollte auch für alte Projekte umgestellt werden, wenn es nicht einen triftigen Grund gibt, dies nicht zu tun.
$_POST['submit'] auf 'Passwort ändern' zu prüfen kann sehr schnell zu Problemen führen, spätestens dann wenn der PHP-Code als UTF-8 vorliegt und die Website ISO-8859-1 nutzt, oder umgekehrt. Außerdem verhindert so was die Internationalisierung des Scriptes, oder führt so Problemen, wenn jemand das Design anpassen möchte.
QUOTE [...] - sha1($_POST['password']) - Du meine Güte, da hab ich halt den escape string vergessen (stellt aber keine große sicherheitslücke dar - werd ich ändern) [...]
Davon habe ich nicht gesprochen, dass es eine Sicherheitslücke ist, nur weil es bei Einen vergessen wurde zu escapen (selbst wenn es hier gar nicht notwendig wäre). Es ist prinzipiell sogar unsinnig erst zu escapen und dann eine weitere Funktion zu nutzen bevor es in die DB wandert. Unsicher ist nur die Speichermethode in der Datenbank. SHA1 ist vorbei, bei HASHs sollte minimum SHA256 genommen werden.
Nur ohne Salten und Dehnen des Hashes ist das Ding so wie so relativ wenig Wert.
Das ist insgesamt kein Beinbruch für Personen die erst seit kurzen Programmieren oder mit PHP arbeiten, aber in welcher Art Du auf dieses Feedback reagierst, ist schon etwas Frech. Du tust so als ob Du es besser wüsstest, dabei zeigst Du eigentlich nur, wie wenig Du wirklich über diese Themen weißt.
Aktuell habe ich hier folgende Fehler gefunden, die Sicherheitsrelevant sind:
- eine XSS-Lücke im BBCode (andere Felder wurden bisher nicht mal darauf geprüft)
- keine CSRF Absicherung
- SQL-Injektionlücke
- Fehlende durchgehende Kontrolle und Behandlung von magic_quotes
- Keine Überprüfung der Zeichenkodierung bei Eingabedaten
- Keine Überprüfung auf schädliche Zeichen bei Eingabedaten
- Keine Absicherung gegen Brute-Force-Angriffe auf das Login
- Keine sichere Verwahrung der Passwörter
- Preisgabe von Informationen nach außen (wie bspw. UserID)
- Verwendung von Daten in der URL, die da nicht drin stehen sollte (die UserID des angemeldeten Benutzers)
Falls Du kein Feedback zu den Scripten haben wolltest, hättest Du nicht danach Fragen sollen...
