Das PHP-Entwicklerteam veröffentlichte vor wenigen Tagen die Version 5.3.7, nur um schon wenige Tage später vor deren Verwendung zu warnen. Der Grund dafür war ein Fehler in der Funktion crypt(), welche bei bestimmten Hash-Verfahren lediglich das Salt zurückliefert. Das kann dazu führen, dass nach einem Update auf PHP 5.3.7 keine Benutzer sich mehr auf einem Webauftritt einloggen können oder sich bei einer Passwortänderung nach einem Update auf eine spätere PHP-Version nicht mehr einloggen können. In PHP 5.3.8 wurde der Fehler wieder behoben.
Dieser Artikel ist der Versuch einer Analyse, wie es zu dem Fehler kam und warum er erst nach der Release bemerkt wurde.
Die PHP-Funktion crypt() ist in der Datei php_crypt_r.c implementiert. Folgender Codeausschnitt baut dort den Passwort-Hash zusammen:
memcpy(passwd, MD5_MAGIC, MD5_MAGIC_LEN);
strlcpy(passwd + MD5_MAGIC_LEN, sp, sl + 1);
strcat(passwd, "$");
strcat() fügt eine Zeichenkette an das Ende eines Puffers. Die Funktion gilt als unsicher, da sie nicht prüft, ob der Zielpuffer genügend Speicherplatz zur Verfügung stellt. Wird die Zeichenkette zu lang, wird der nachfolgende Speicherbereich beschädigt – ein typisches Problem bei C-Sprachen.
Aus dem Grund wurde der Aufruf durch eine sicherere Funktion ersetzt. Leider ist aber gut gemeint das Gegenteil von gut gemacht.
memcpy(passwd, MD5_MAGIC, MD5_MAGIC_LEN);
strlcpy(passwd + MD5_MAGIC_LEN, sp, sl + 1);
strlcat(passwd, "$", 1);
strlcat() stellt sicher, dass der Puffer nicht über sein Ende hinaus beschrieben wird. Dazu wird dessen Größe übergeben. Und genau hier lag das Problem, denn statt der Größe von passwd wurde anscheinend die Größe des zu kopierenden Textes übergeben, nämlich 1. Da passwd zu dem Zeitpunkt bereits deutlich mehr als ein Zeichen enthält, tut strlcat() genau das, was es tun soll, nämlich gar nichts. Das “$”-Zeichen wird nicht angehängt, das tatsächliche Ergebnis weicht damit von dem gewünschten Ergebnis ab.
Solch ein Fehler ist eigentlich ein Lehrbuchbeispiel für Unit-Tests, und der Kommentar zum Bugfix (Revision 315218) deutet auch an, dass ein solcher existiert:
Unbreak crypt() (fix bug #55439)
# If you want to remove static analyser messages, be my guest,
# but please run unit tests after
Tatsächlich gibt es einen Test, der die crypt()-Funktion mit bestimmten Werten aufruft und das Ergebnis mit einem erwarteten Ergebnis vergleicht. Dieser Test schlägt Alarm, wenn er ausgeführt wird.
Dass PHP 5.3.7 dennoch veröffentlicht wurde, lässt eigentlich nur einen Schluss zu: Der Unit-Test wurde nicht ausgeführt oder der Alarm wurde schlichtweg ignoriert. Spätestens beim Bau der finalen Version unmitelbar vor der Veröffentlichung hätte dies aber stattfinden müssen. Alles andere wäre grob fahrlässig.
Zusammengefasst hatte der Fehler also folgende Ursachen:
- die für C-Sprachen üblichen Probleme bei der sicheren Verarbeitung von Zeichenketten
- eine missverständliche oder nicht verstandene Dokumentation der Funktion
strlcat() - keine verbindlich vorgeschriebene fehlerfreie Ausführung der Unit-Tests vor der Freigabe einer Release
Insbesondere der letzte Punkt wiegt schwer und wirft ein schlechtes Licht auf die verantwortlichen PHP-Entwickler.