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

Be Sociable, Share!

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

  1. rd (Dienstag, der 25. Februar 2014)

    Sieht nach einem Flüchtigkeitsfehler aus (versehentlich zweimal „einfügen“) gedrückt. Kommt davon, wenn man schneller schreibt (programmiert) als denkt. Kenne ich gut, gerade in „c“.

  2. Chris Wood (Mittwoch, der 26. Februar 2014)

    Dear Bernhard, rather than depend on good programming style, I would advise testing!
    For an important program like this, every version should go through an automatic „regression“ test. Such testing facilities should be designed into the system from the start. The total complexity of the test material should rival the program itself, since it should include practically all facilities and options. It must be maintained in parallel with the program itself, especially when the hooks used to automate it change.
    Good programmers should produce these tests, (although runtime speed is less important).

  3. at (Mittwoch, der 26. Februar 2014)

    Zum Thema „beim Standard bleiben“:
    Dummerweise sind if-statements ohne geschweifte Klammern ja eben Teil des C-Standards. Wenn man es im Standard unterbinden würde, könnte man sanity checks ifür alten Code immer noch durch ein Compiler Flag oder Einführung eines #THIS_FILE_IS_STUPID pragmas abschalten 😉

    — Andreas

  4. Chris Wood (Donnerstag, der 27. Februar 2014)

    On second thought, there is a serious bug elsewhere in this program. How can a superfluous „goto fail“ produce such nasty results? It is wrong to carry on blithely, or try to correct things in a „fail“ case. („Correcting“ things usually destroys diagnostic data and often makes things worse). The programmer either did not understand English, or had a strange sense of humour.
    It is a pity that good experienced programmers get pushed into management or retirement. (Concerning „goto“, I agree with Knuth).

  5. Hans Bonfigt (Freitag, der 28. Februar 2014)

    Hello Chris,

    „rather than depend on good programming style, I would advise testing!“

    This was also my first thougt – and it cannot be stressed enough !

Kommentar verfassen

*