Clean Code: Wie der Apple SSL-Bug mit einfachen Mitteln zu verhindern gewesen wäre

Gerade habe ich mir ein wenig die Hintergründe zum SSL/TLS-Bug durchgelesen, der Apple diese Woche ereilt hat.

Wer grad nicht weiß, wovon ich spreche, hier ein paar Links:

Heise.de: „Ein Sicherheitsdesaster“: Hintergründe zu Apples schwerwiegendem SSL-Problem
Golem.de: iOS erhält SSL-Bugfix, OS X bald auch
Spiegel.de: Sicherheitslücke: Apples furchtbarer Fehler

Interessant dabei ist, daß dieser Bug nur auf eine einzige Code-Zeile hinausläuft. Er wäre einfach zu verhindern gewesen, hätte man die grundlegenden Regeln von „Clean Code“ befolgt.

Hier der Code-Auszug, der den Fehler beinhaltet. Ich habe ihn von Apple selbst, da der entsprechende Code unter einer OpenSource-Lizenz steht (siehe hier).

hashOut.data = hashes + SSL_MD5_DIGEST_LEN;
hashOut.length = SSL_SHA1_DIGEST_LEN;
if ((err = SSLFreeBuffer(&hashCtx)) != 0)
goto fail;

if ((err = ReadyHash(&SSLHashSHA1, &hashCtx)) != 0)
goto fail;
if ((err = SSLHashSHA1.update(&hashCtx, &clientRandom)) != 0)
goto fail;
if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0)
goto fail;
if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
goto fail;
goto fail;
if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0)
goto fail;

Warum ist nun aber genau dieser Code problematisch?

Schon gesehen?

Ich gebe zu, auch bei mir hat es ein wenig gedauert. Ich versuche es mal zu erklären:

Normalerweise ist es in der Programmiersprache C (in der dieser Code geschrieben ist) der Brauch, Anweisungen nach einem „If“-Statement in geschweifte Klammern zu setzen. In etwa so:

if (bedingung) {
Anweisung 1;
Anweisung 2;
Anweisung 3;
...
}

Es gibt jedoch die Möglichkeit, auf die geschweifte Klammer zu verzichten, wenn hinter der Bedingung nur eine einzige Anweisung folgt.

Also so:

if (bedingung)
Anweisung 1;


In diesem, zweiten Stil, sind auch alle „If“-Statements aus dem Code-Auszug von Apple geschrieben. Der Programmierer wollte sich so wohl einige Zeichen Tip-Arbeit sparen. Dummerweise hat sich aber nun ein kleiner Fehler eingeschlichen: Nach dem rot markierten If-Statement steht nicht eine, sondern 2 Anweisungen.

Was passiert nun in so einem Fall?

Nun, der Compiler wertet die Regel strikt aus: Steht hinter einem If-Statement keine Klammer, so folgt genau 1 Anweisung: Die erste Anweisung (in diesem Fall „goto fail;“) gehört also noch zum If-Statement. Die zweite Anweisung gehört für den Compiler jedoch nicht mehr zum If-Statement. Sie wird daher in jedem Fall ausgeführt. Ganz so, als sähe der Quellcode so aus:

…

if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
goto fail;

goto fail;

if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0)
goto fail;

Das zweite „goto fail;“ wird daher in jedem Fall ausgeführt, und damit die Textmarke „fail“ bereits angesprungen, bevor das letzte, blau hinterlegte if-Statement ausgeführt werden kann.

Was hat das nun aber mit Clean Code zu tun?

Aus meiner Sicht ist es kein guter Stil, in if-Statements auf die geschweifte Klammern zu verzichten. Zu welch schwerwiegenden Problemen das führen kann, sieht man z.B. genau mit diesem Beispiel hier. Besser ist es, man verwendet geschweifte Klammern, dann ist es auch eindeutig, was ausgeführt wird und was nicht. Wahrscheinlich hätte hier dann sogar der Compiler davor gewarnt, dass das zweite „goto fail;“ nicht erreicht werden kann („Dead Code“), und damit überflüssig ist.

So jedoch hatte der Compiler keine Chance. Auch die ganzen Tools zur statischen Quellcodeanalyse, die Apple sicherlich im Hintergrund anwendet, laufen hier ins Leere. Und die Wahrscheinlichkeit, daß ein anderer Programmierer das zufällig entdeckt, wenn er drüberschaut, ist ebenfalls nicht allzu hoch: Der Fehler ist gut versteckt. Man muß schon mehrmals hinsehen, um ihn zu entdecken.

Was kann man nun dagegen machen?

Die einfachste Methode ist es, sich solche „besseren“ Schreibweisen gar nicht erst anzugewöhnen, sondern beim Standard zu bleiben. Die meisten IDEs bieten dafür sogar Regeln an, die man hinterlegen kann. Dadurch werden entsprechende Code-Stellen als Warnung oder sogar als Fehler hinterlegt. Rutscht doch einmal eins durch, so wird es bei der automatischen Code-Formatierung (dessen Tastenkombination man während der Entwicklung ständig drückt) wieder in die Standard-Schreibweise aufgelöst.

Es ist eine gute Idee, einmal einen kompletten Satz solcher Code-Formatierungsregeln zu definieren, und dann anschliessend an alle Teammitglieder zu verteilen.

Man kann dies sogar so einstellen, daß es automatisch an alle verteilt wird, die das Projekt aus dem Versionsverwaltungssystem auschecken. Z.B. indem man etwa projektspezifische Regeln anlegt, und diese in ein Verzeichnis innerhalb des Projekts legt.

Sollten Sie das bisher noch nicht gemacht haben, so ist es eine gute Idee, nun damit anzufangen.

Ich kann mir vorstellen, bei Apple sind sie gerade voll dabei  😎

bfi