Fragen? Antworten! Siehe auch: Alternativlos
Damit war nicht OpenSSL gemeint, sondern sowas wie X, perl, gcc. Aber OpenSSL fiel auch darunter, in dem Sinne, dass ich da eh noch andere Vorbehalte für einen Audit hatte. Und diese Policy hat dann dazu geführt, dass ich da auch nicht mal oberflächlich reingucken wollte.
Da ich angesichts aktueller Entwicklungen eh nicht gut schlafen kann mit OpenSSL gerade, habe ich mich entschieden, doch mal kurz einen Blick zu werfen.
Ich bin, gelinde gesagt, entsetzt. Schon die OpenBSD-LibreSSL-CVS-Checkin-Kommentare haben ja tief blicken lassen.
Es gibt so ein paar Kriterien, an denen ich bei C-Code meinen ersten Eindruck festmache. Das erste Kriterium ist, dass Längen und Offsets immer unsigned sein müssen, und vom Typ size_t. Früher hätte ich gesagt: unsigned long ist auch OK. Aber es gibt Plattformen, auf denen long 32-bit aber size_t 64-bit ist. 64-bit Windows z.B. Daher muss es size_t sein, nicht unsigned long. Und schon gar nicht long. Hier ist, was OpenSSL macht:
static int asn1_get_length(const unsigned char **pp, int *inf, long *rl, int max)Hier fallen ja schonmal als erstes die intuitiven Variablennamen auf. Da weiß man doch sofort, was gemeint ist! Ich kläre mal auf: Diese Routine soll einen ASN.1 DER Längenwert parsen. Das Encoding davon ist: Wenn das erste Byte das höchste Bit gesetzt hat, dann sind die unteren 7 Bits die Länge in Bytes für die Länge, die dahinter in big endian folgt. Wenn das erste Byte das höchste Bit nicht gesetzt hat, dann sind die unteren 7 Bits der Wert. Weil X.509 DER benutzt, gibt es zusätzlich noch die Regel, dass alle Längen minimal encoded sein müssen. Beispiele:
05 - Wert 5Meinem aktuellen Verständnis nach kann indefinite length bei X.509 nicht vorkommen, und ich unterstütze den Fall in meinen ASN.1-Routinen im Moment auch nicht sondern liefere einen Fehlerwert zurück. OpenSSL supported das. Möglicherweise übersehe ich da was. Wo ich aber nichts übersehe: OpenSSL prüft an keiner Stelle, dass das Encoding minimal ist. Der Effekt ist, dass man das identische Zertifikat auf mehrere Arten kodieren kann, und damit möglicherweise Angriffsfläche auf Krypto-Verfahren schaffen kann. Überhaupt sind Unterschiede zwischen Parsern immer doof, Differentiale will man an solchen Stellen vermeiden. Das ist jetzt kein "OMG RUN FOR THE HILLS"-Moment, aber wenn man schon eine zentrale Library macht für sowas, dann doch um da penibel solche Sachen abzufangen, damit die Leute das nicht alle von Hand machen und vergessen.
7f - Wert 127
81 01 - Ungültig, da nicht minimal encoded; wäre sonst 1
82 00 23 - Ungültig, da nicht minimal encoded; wäre sonst 35
82 12 34 - Wert 4660 (0x1234)
89 11 11 11 11 11 11 11 11 11 - Ungültig, da der 9-Byte-Wert nicht in einen Integer passt
0x80 - Sonderfall, "indefinite length".
Aber unabhängig davon. pp ist der Quell-Zeiger (das const ist ein gutes Zeichen, das wäre Kriterium 2 für das Erkennen von schlechtem Code gewesen). inf wird auf 1 gesetzt, wenn indefinite length encoding reinkommt. Ich finde, das hätte man direkt zurückweisen sollen. Indefinite length encoding funktioniert so, dass man am Anfang sagt, man weiß nicht, wie groß die Daten werden, die jetzt kommen, und dann schickt man halt so viele Daten wie halt kommen und dann zwei Null-Bytes. Ganz schlechte Idee, und habe ich in der Praxis auch noch nie im Einsatz beobachten können. Das ist ja gerade der Grund, wieso man ASN.1 DER einsetzt, damit man vorher weiß, wieviele Daten jetzt kommen werden. Und im Übrigen siehe oben zu den Parser-Differenzen. Das will man vermeiden.
Aber der eigentliche Punkt, auf den ich die ganze Zeit hinauswill: die Länge ist ein long. Das ist ein ganz schlechtes Zeichen. Die Routine versucht, das Schlimmste zu verhindern, indem sie als unsigned parsed und dann einen Fehler meldet, wenn der Wert größer als LONG_MAX ist. Und in der Tat, wenn sie das nicht gemacht hätte, hätte es im String-Parsing direkt einen schönen Buffer Overflow gegeben.
Ich habe jetzt jedenfalls ein schlechtes Gefühl bei OpenSSL und werde glaube ich erstmal die ASN.1-Routinen von PolarSSL auditieren, damit ich einen Fallback habe, wenn ich die ganzen Atommüllablagerungen in den Fracking-Schächten bei OpenSSL finde.
Update: Oh Graus, ich erfahre gerade, dass ich X.509 die ganze Zeit falsch verstanden hatte. Ich hatte das so verstanden, dass X.509 immer DER Encoding nimmt. Denn, mit Verlaub, alles andere ergibt auch gar keinen Sinn. Man will das ja in digitalen Signaturen verwenden. DER ist eine Teilmenge von BER, die genau den Zweck und die Daseinsberechtigung hat, dass man alles nur auf genau eine wohldefinierte Art kodieren kann. Begründung: Das braucht man so für digitale Signaturen. Und jetzt erfahre ich gerade, dass X.509 gar nicht DER sondern BER benutzt! Man soll das als Implementation anscheinend als BER parsen, dann soll man das als DER neu kodieren und dann die Signaturberechnungen machen!? Das kann ja wohl nicht wahr sein! Dann hätte OpenSSL Recht mit ihrem Code. Heilige Scheiße, wer denkt sich denn solche Standards aus!? In der Praxis habe ich noch nie was anderes als DER-Encoding gesehen. In meiner SSL-Library werde ich jedenfalls gleich beim parsen alles rejecten, das nicht DER ist.
Update: Interessanterweise macht OpenSSL es auch falsch, wenn BER Absicht ist.
173 if (i > sizeof(long))
174 return 0;