02Sécurité5 min

Ce que j'ai appris en auditant mon propre code comme s'il était d'un autre

Une ACL vide qui renvoyait le corpus entier, une injection de CSS sans une ligne de JavaScript et une clé d'API encore valable après la résiliation de son propriétaire. Tout ça en un jour, tout ça dans du code que j'avais écrit.

Par Salva Castells

Image de couverture : Ildar Sagdejev (Specious), CC BY-SA 3.0.
Dans cet article

En une seule journée, j'ai fermé six failles de sécurité dans une API de transcription qui tournait depuis des mois. Aucune n'a été trouvée par un test. Les tests passaient au vert avant comme après. Ce qui a changé, ce n'est pas l'outil : c'est que je me suis assis pour lire mon propre code en me demandant comment je le casserais si je voulais le faire, au lieu de vérifier s'il faisait ce que j'avais prévu.

Cette différence a l'air d'être une question d'attitude. C'est une question de méthode, et ça se voit au type de faille qui sort.

Un test vérifie le chemin que tu as imaginé

Un test s'écrit depuis le cas d'usage : des données raisonnables, une sortie raisonnable. Un attaquant ne passe pas de données raisonnables. Il passe le vide, le zéro, le négatif, l'énorme. Ces valeurs sont rarement dans la suite, parce que quand tu écris le test elles ne te viennent pas à l'esprit, et quand elles te viennent tu as déjà vu le bug.

Le cas le plus net a été une ACL vide. Le moteur de recherche de récupération recevait une liste d'identifiants auxquels l'utilisateur avait accès et filtrait le corpus avec. Si la liste arrivait vide, le code l'interprétait comme « pas de filtre à appliquer » au lieu de « cet utilisateur n'a accès à rien ». Il renvoyait le corpus entier.

Ça échouait en mode ouvert : la valeur par défaut d'une absence de permissions était de tout autoriser. Aucun test du chemin heureux n'y touche jamais, parce que dans le chemin heureux la liste contient toujours quelque chose.

Depuis, je passe tous les filtres d'autorisation avec la même question : que fait ceci si la collection est vide ? Pas « que fait-il si l'utilisateur n'a pas le droit ? », qui est généralement bien couvert, mais ce qu'il fait avec le vide.

Les catégories se répètent plus qu'on ne croit

Les trouvailles se regroupent presque toujours dans les mêmes familles.

  • Des permissions qui ne sont pas exigées au bon endroit. Un endpoint d'export qui laissait sortir des données sans demander l'autorité de gestion de la structure. Une suppression en cascade d'un domaine qui vérifiait le droit une fois au début au lieu de vérifier l'autorité de suppression définitive sur chaque élément affecté. La cascade est l'endroit classique : tu autorises l'opération et ensuite la boucle interne fait des choses que personne n'a autorisées.
  • Des limites qui n'existent pas. Les listings d'administration passaient le limit reçu directement à la requête, sans plafond. Une valeur élevée depuis un script matérialisait la table entière dans la mémoire du worker. Le correctif tient en une ligne, Math.min(Math.max(limit, 1), 1000), mais il n'était pas là parce que dans l'UI le limit valait toujours dix.
  • Un état vérifié une fois et plus jamais. authenticateApiKey validait le hash, l'expiration et le flag isActive de la clé. Mais la ligne de la clé survit à l'état du compte de son propriétaire : si l'enregistrement de facturation est effacé à la résiliation, personne ne baisse ce flag et la clé continue d'authentifier. Maintenant la fonction revalide que le propriétaire a toujours un enregistrement de facturation, à un seul endroit, pour que tous les endpoints qui authentifient par clé en héritent au lieu de devoir y penser chacun de leur côté.
  • Du contenu tiers qui est rendu. C'est là que se trouve celle qui m'a le plus surpris.

Le CSS aussi est une injection

L'assainisseur de markdown autorisait les attributs style et class libres dans div, span et svg. La chaîne va de raw à sanitize puis à harden et aucune étape ne regarde à l'intérieur du CSS : la bibliothèque d'assainissement traite style comme une chaîne opaque et le durcisseur ne réécrit que les URLs dans a et img. Le style arrivait intact au rendu final.

J'ai pensé écrire un parseur CSS avec liste blanche de propriétés. Je ne l'ai pas fait : j'ai vérifié qu'aucune vue légitime n'avait besoin de style et j'ai retiré les deux attributs de la liste autorisée. Un filtre CSS maison, c'est de la surface en plus à maintenir pour couvrir un cas qui n'existait pas.

Ce qui compte dans cette trouvaille, ce n'est pas le correctif, c'est la classe de vecteur. Pas besoin de JavaScript pour attaquer un autre utilisateur : avec du CSS tu peux masquer un bouton, déplacer un texte par-dessus un autre ou changer ce qu'un message a l'air de dire. Et toute vue qui affiche du contenu d'autrui, pas seulement le sien, est un vecteur d'utilisateur à utilisateur.

Secrets dans les logs et liens qui durent trop

Deux trouvailles qui ne sont pas des bugs de logique, mais de cycle de vie.

La première : quand l'authentification par clé d'API échouait à cause d'une clé corrompue ou mal formée, le catch mettait la clé brute dans le contexte du log, et le logger sérialisait ce contexte avec JSON.stringify sans rien caviarder. Une clé valide envoyée au mauvais endpoint finissait en clair dans le log. Le caviardage que j'ai mis attaque par deux côtés, par nom de champ (/rawkey|token|authorization|password/i) et par forme de la valeur (le préfixe dk_live_ / dk_test_), et laisse visibles les huit premiers caractères suivis de ***REDACTED***, le même format avec lequel l'UI recache la clé après l'avoir montrée une fois.

La seconde : les URLs signées S3 pour le lecteur audio étaient générées avec un expiresIn de sept jours. Sept jours, c'est une éternité pour un lien qui doit juste survivre au chargement d'un <audio>. N'importe qui avec le lien — transféré, dans l'historique, mis en cache par un proxy d'entreprise — conservait l'accès à l'audio d'origine pendant toute cette fenêtre, et supprimer la transcription ne révoquait rien. J'ai baissé l'expiration à 600 secondes, avec renouvellement côté client à 480, et j'ai ajouté le DeleteObjectCommand sur l'objet lors de la suppression de la transcription, parce que jusque-là la cascade n'effaçait que la ligne.

Le coût en expérience utilisateur a été nul sauf pour un flux : transférer le lien de l'audio par mail à un collègue. Ce flux n'a jamais été une fonctionnalité supportée et n'avait aucun contrôle d'accès, et il me paraît correct qu'il ait cessé de marcher.

Maintenant je le planifie au lieu d'y penser

Le problème des audits, c'est qu'ils dépendent du fait qu'un jour ça te dise. Le mien est sorti parce que je documentais des décisions de conception et qu'à la relecture certaines choses m'ont fait tiquer.

Chaque trouvaille devient un fichier court dans docs/security/ avec le vecteur et la raison pour laquelle le correctif est celui-là et pas un autre, et de là sort la liste avec laquelle je passe le reste en revue. Quand j'ai répété l'exercice sur le boilerplate avec lequel je démarre mes projets, trente-neuf points sont sortis entre sécurité, SEO et architecture, et la plupart étaient des variantes de la même poignée de questions : que se passe-t-il avec la liste vide, qui exige le droit dans la boucle interne, quel est le plafond de cette limite, combien de temps vit ce lien.

Lire ton code pour confirmer qu'il marche et le lire pour voir comment le casser sont deux lectures différentes. La seconde ne se produit pas toute seule pendant que tu fais la première.