La récente faille de sécurité trouvée dans iOS comme OS X met en avant des défauts dans les directives de style de code, de tests unitaires, de politiques de revue de code, de stratégies de gestion d'erreurs, et de déploiement d'outils.
Sur ZDNet, Larry Seltzer décrit ce bug comme "un bug choquant et embarrassant" et remarque que le fait qu'Apple ait aussi livré un patch pour iOS 6 démontre son importance : "Je suis sûr qu'Apple ne veut pas faire quoi que ce soit pour aider les utilisateurs à rester sur iOS 6, mais ils l'ont tout de même patché. Cela montre à quel point c'est sérieux".
Au moment de la rédaction de cet article, Apple a déjà livré des mises à jour logiciel à la fois pour iOS et OS X pour résoudre le problème : une vulnérabilité dans les communications encryptées qui permettait à un attaquant d'intercepter, de lire ou de modifier des données encryptées. Malgré cela, on peut tirer quelques leçons de cet épisode.
Adam Langley, de Google, a expliqué que le bug est situé dans le framework SecureTransport, une implémentation des protocoles SSL/TLS livrée en Open Source par Apple, et est causé par du code qui devient inatteignable. Si vous jetez un oeil au code ci-dessous :
static OSStatus
SSLVerifySignedServerKeyExchange(SSLContext *ctx, bool isRsa, SSLBuffer signedParams,
uint8_t *signature, UInt16 signatureLen)
{
OSStatus err;
...
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;
...
fail:
SSLFreeBuffer(&signedHashes);
SSLFreeBuffer(&hashCtx);
return err;
}
Vous remarquerez les deux lignes goto fail à la suite. Le deuxième goto fail, à cause des accolades manquantes autour du bloc if, produira toujours un saut vers le label fail, ignorant donc les vérifications suivantes. Le tout combiné au fait que, au moment du saut, la variable err ne contient aucune erreur et engendrera un retour de la méthode sans erreur. Adam Langley continue pour clarifier :
Cette vérification de signature vérifie la signature dans un message ServerKeyExchange. Cela est utilisé dans les suites de Cipher DHE et ECDHE pour communiquer des clés éphémères pour la connexion. Le serveur dit : "voici la clé éphémère et voici la signature, provenant de mon certificat, pour que tu saches que cela vient de moi". Maintenant, si le lien entre la clé éphémère et le certificat est brisé, alors tout s'effondre. Il est possible d'envoyer un lien de certificat correct au client, mais de signer le contrat avec la mauvaise clé privée, ou ne pas le signer du tout ! Il n'y a aucune preuve que le serveur possède la clé privée correspondant à la clé publique dans son certificat.
D'après Larry Seltzer, nous ne savons pas comment le bug a été découvert, parce qu'Apple n'a pas donné beaucoup de détails, mais cet épisode amène à se poser des questions sur les pratiques de revue de code chez Apple. Il note aussi que, même si l'erreur est facile à reconnaître lorsque vous la regardez de près, cela peut être compliqué à voir quand vous regardez le fichier entier, qui fait 1970 lignes.
De nombreux intervenants sur le sujet #gotofail de Twitter ont identifié un coupable flagrant dans l'utilisation du goto, célèbrement décrit comme "nocif" dans un article de Dijkstra. Bien que ce soit indéniable, Arie van Deursen, Professeur d'Ingénierie Logicielle à la Delft University of Technology, aux Pays-Bas, explique l'utilisation du goto dans le but d'implémenter un mécanisme de type exception en C à travers l'utilisation de l'expression return-error-code, ce qui fait de cette expression le véritable coupable.
En effet, van Deursen écrit que cette expression return-error-code est omniprésente dans le fichier contenant le bug, et que cela apporte ses propres problèmes. Une des découvertes clés d'un article de 2005 de van Deursen, co-écrit avec Mgiel Bruntink et Tom Tourwe est "une densité d'anomalies de 2.1 provenant de l'expression return-error-code pour 1000 lignes de code", résultant de l'inspection d'une importante base de code. Une si grande densité d'anomalies était dûe à des appels non vérifiés, des codes d'erreur mal propagés, et des conditions d'erreur mal gérées, ce qui montre que "l'expression est particulièrement source d'erreurs".
Kevin Marks, un ancien employé d'Apple, mentionne dans un commentaire au post de van Deursen qu'il y a des façons moins dangereuses d'utiliser l'expression return-error-code en se servant des macros du pré-processeur. On trouve des exemples de telles approches dans BailOSErr et dans l'objectif de l'implémentation des exceptions en C.
En parlant de la gestion des codes d'erreur, Chris Leishman remarque, dans un commentaire du post de van Deursen, que l'utilisation d'un code d'erreur initialisé à succès est un facteur-clé pour que le double goto crée des failles. Le système aurait démontré un comportement plus sécuritaire si l'erreur était initialisée à OSStatus err = OSUnknownError.
D'un autre côté, Landon Fuller, ingénieur logiciel chez Plausible Labs, fournit une analyse de testabilité du code affecté par le bug et démontre que SLLVerifySignedServerKeyExchange est une isolation testable unitairement. Ceci, d'après C. Keith Ray, marque un point pour le TDD : "vous ne pouvez pas écrire une condition if tant que vous n'avez pas un test qui la nécessite. Vous finirez avec un test pour le cas où la condition if est vraie et un pour celui où elle est fausse".
Arie van Deursen met aussi le doigt sur des aspects plus controversés de l'histoire.
Il remarque d'abord que le fichier qui contient le bug "n'est pas systématiquement formaté automatiquement : Il y a beaucoup d'espaces inutiles, de tabulations, et de code en commentaire", alors que "corriger l'indentation montre immédiatement quand quelque chose de louche se passe" et aurait permis de remarquer le bug plus facilement. Au cours de ces lignes, il en arrive au point de suggérer que "la formatation du code est une fonctionnalité de sécurité" et que "les espaces représentent une question de sécurité".
Langley écrit dans son blog qu'il pense que les revues de code pourraient être efficaces pour éviter ce type de problèmes. Arie van Deursen souligne cependant que, dans une précédente étude sur l'application des revues de code chez Microsoft, il s'est avéré que :
La revue ne résulte pas en l'identification des anomalies aussi souvent que les membres du projet le souhaiteraient et détecte encore plus rarement les problèmes profonds, subtiles ou au niveau "macro". Se fier aux revues de code dans cette optique d'assurance de qualité peut être risqué.
Enfin, les outils n'aident pas non plus dans ce cas, puisque l'option -Wall de Clang ne remarquera ni la double ligne goto ni le code inatteignale qui s'ensuit, comme Langley le fait remarquer. Clang propose un flag -Weverything qui aurait débusqué l'erreur, d'après Simon Nicolussi, tandis que GCC la laisse passer silencieusement. C'est aussi confirmé par Peter Nelson, qui souligne aussi l'existence d'une option spécifique -Wunreachable-code. Van Deursen explique que le principal problème avec le code inatteignable est que sa détection est un problème fondamentalement indécidable, qui mène à un compromis entre l'exhaustivité et les faux-positifs, ce qui pourrait donc expliquer pourquoi il n'est pas inclus par défaut.