Kurioser oder richtig dreckiger Quellcode? Immer her damit!

Ich möchte den folgenden C-Code für den Wettbewerb einreichen. Diesen Code habe ich vor über 10 Jahren im Programmcode eines namhaften Software- und Beratungshauses ‘gefunden’. Wahnsinn, dass ich das immer noch aus dem Gedächtnis hinkriege. Es handelte sich eigentlich um ein C++-Programm, das aber hier C-Funktionen verwendet (Damals gab es noch keine Std.-Stringklasse etc.). Die Deklaration der Variablen habe ich weggelassen, da sie zum Verständnis nicht benötigt werden.

// ———————————————
size_t len = strlen(source);
char dest[len];
char * dest = strncpy(dest, source, len);
dest[len] = ‘\0′; // zur Sicherheit
// ———————————————

Dieser Code ist nicht nur genauso dumm wie der Kommentar ‘zur Sicherheit’, sondern auch noch falsch:

1. strncpy füllt automatisch bis zum Ende mit ‘\0′ auf, solange dest größer als source ist.
2. In diesem Beispiel ist source genauso groß wie dest und man überschreibt mit der unnötigen Zuweisung von ‘\0′, das erste Byte, das auf dest folgt.

Schönes Beispiel. Ich hoffe mal, dass ich das richtig verstanden habe.

Der Programmierer wusste anscheinend, dass es einen Unterschied zwischen der Methode ‘strcpy’ und ‘strncpy’ gibt (‘strncpy’ hängt automatisch den null-character an), er war jedoch einfach zu faul in der Doku nachzuschauen. … Und ‘zur Sicherheit’ überschreibt er das nächste Byte im Speicher.

… Typischer Fall von: “Zweimal abgeschnitten und immer noch zu kurz!”

Mein Beitrag aus aktuellem Anlass: Das Sicherheitsleck in Debians OpenSSL-Bibliothek.

Es geschah im Mai 2006, als ein Debian-Maintainer den Quellcode von OpenSSL durch einen automatischen Code-Checker laufen lies. Dieser meldete ihm an mehreren Stellen mögliche Probleme durch nicht korrekt initialisierte Variablen.

Eine dieser Variablen betraf die Initialisierung des Zufallszahlengenerators bei der Erzeugung von neuen Schlüsseln / Zertifikaten:

/*
* Don’t add uninitialised data.
MD_Update(&m,buf,j);
*/
MD_Update(&m,(unsigned char *)&(md_c[0]),sizeof(md_c));
MD_Final(&m,local_md);
[...]
MD_Update(&m,local_md,MD_DIGEST_LENGTH);
MD_Update(&m,(unsigned char *)&(md_c[0]),sizeof(md_c));
#ifndef PURIFY
/*
* Don’t add uninitialised data.
MD_Update(&m,buf,j); /* purify complains */
*/
#endif

Nachdem er die von Purify (dem Code-Checker) monierten Stellen aus dem Code entfernt hatte, funktionierte alles wie bisher und Purify beschwerte sich nicht mehr über unsauberen Code.

Doch zu früh gefreut:
Die entfernten Stellen tragen maßgeblich zur Initialisierung des Zufallszahlengenerators bei. Ohne diese Code-Zeilen wird dieser nur mit der aktuellen Prozess-ID initialisiert. Di
ese kann einen Wert zwischen 1 und 32768 annehmen. Damit ist die Initialisierung des Generators sehr gut vorhersagbar und das berechnen von 32768 Schlüsseln stellt auf aktuellen
Systemen keine Herausforderung mehr dar.
Ein potentieller Angreifer muss alle möglichen Kombinationen in der Hoffnung ausprobieren, dass eine die richtige ist. Da es nun aber nur noch 32768 verschiedene gibt und von diesen garantiert eine korrekt ist, stellt dieses Angriffsszenario eine Bedrohung dar.

Was lernen wir daraus?

Wenn man Änderungen in anderer Leute Quellcode vornimmt, muss man anschließend den geänderten Code sehr genau testen. Insebesondere sollte man verstehen
1. was der Code tat, bevor man ihn ändert
2. welche Konsequenzen sich aus der Änderung ergeben.

Wenn man Codezeilen eines fremden Programms entfernt, obwohl diese den internen Zustand verändern und der Code dennoch fehlerfrei läuft, muss man misstrauisch werden.

Danke Markus.
Ich habe den Ausführungen nur Eines hinzuzufügen:
Wenn ich mir den Code so anschaue, dann kann ich mir vorstellen, das sich der Entwickler vieleicht sogar die Mühe gemacht hat den Code zu verstehen. Dies ist aber bei diesem Quellcode nur sehr schwer möglich, z.B.:

MD_Update(&m,(unsigned char *)&(md_c[0]),sizeof(md_c));

Was macht diese Zeile Code? Was enthalten die Variablen m und md_c? Warum zur Hölle schreibt jemand solchen Quellcode?

Mich persönlich wundert es nicht, dass bei diesem Quellcode solche weitreichenden Fehler entstehen. Das ist ein sehr gutes Beispiel für dreckigen Quellcode und man kann sehr schön sehen was daraus entstehen kann.