Comment GitHub a découvert et corrigé une rare race condition dans la gestion des sessions

Le 8 mars dernier, GitHub a déconnecté tous les utilisateurs de GitHub.com en raison d’une faille de sécurité.

• Le 2 mars, un utilisateur a signalé qu’après s’être connecté, il avait été authentifié en tant qu’un autre utilisateur. Il s’est immédiatement déconnecté, puis a signalé le problème, et l’enquête a commencé aussitôt. Quelques heures plus tard, un autre utilisateur a signalé un problème similaire.

• L’enquête initiale a révélé que la session d’un utilisateur avait été partagée entre deux adresses IP au moment où le signalement a été fait.

• En examinant les changements récents de l’infrastructure, l’équipe a constaté une mise à niveau récente du load balancer et du routage, avec une modification des HTTP keepalives. Cela semblait lié au problème, mais l’enquête a finalement montré que ce n’était pas le cas.

• Cependant, au cours de l’examen de l’infrastructure, ils ont découvert que les requêtes ayant reçu la mauvaise session avaient été traitées exactement sur la même machine et par le même processus.

• En analysant les logs, ils ont constaté que le corps de réponse était correct et que seuls les cookies envoyés étaient erronés. Le cookie d’un autre utilisateur, traité dans le même processus, avait été renvoyé par erreur. Dans l’un des cas signalés, les deux requêtes étaient consécutives, et dans l’autre, deux autres requêtes s’étaient intercalées entre elles.

• Ils ont alors formulé l’hypothèse d’une fuite d’état entre des requêtes traitées par le même processus Ruby, et se sont demandé comment cela pouvait être possible.

• En passant en revue les changements récents, ils ont découvert qu’afin d’améliorer les performances, la logique qui vérifie les fonctionnalités activées pour un utilisateur ne s’exécutait plus pendant le traitement de la requête, mais dans un thread de fond qui mettait les données à jour à intervalles réguliers. L’enquête s’est alors concentrée sur la sûreté de ce fonctionnement multi-thread.

• L’application principale de GitHub.com est écrite en Ruby on Rails, et comporte de nombreux composants qui n’ont pas été conçus pour fonctionner en multi-thread.

• Des threads étaient déjà utilisés dans l’application, mais ce nouveau thread de fond a provoqué un comportement inattendu dans la routine de gestion des exceptions.

• Lorsqu’une exception se produisait dans le thread de fond, le log d’erreur contenait à la fois les informations du thread de fond et celles de la requête en cours d’exécution.

• Au début, ils pensaient que le fait que des données de requêtes sans rapport apparaissent dans les logs du thread de fond n’était qu’une incohérence du système interne de reporting.

• Comme Rails crée un nouvel objet contrôleur pour chaque requête, ils pensaient que cela garantissait l’isolation.

• Le problème restait donc difficile à expliquer.

• Une avancée est apparue lorsqu’ils ont découvert que Unicorn, utilisé comme serveur HTTP Rack pour l’application Rails, ne crée pas un nouvel objet env distinct pour chaque requête.

• À la place, Unicorn alloue une table de hachage Ruby pour chaque requête, puis la vide (clear).

• Ils ont ainsi compris que les logs du thread de fond ne révélaient pas une incohérence de reporting, mais un véritable partage de données de requête.

• Ils ont tenté de reproduire cette race condition en environnement de développement, et ont découvert que la situation devait commencer par une requête anonyme.

1. Une requête anonyme (requête n°1) arrive, et une callback est enregistrée dans la bibliothèque de rapport d’erreur. Cette callback contient une référence à l’objet contrôleur Rails qui accède à l’objet d’environnement de requête Rack fourni par Unicorn.

2. Une exception se produit dans le processus de fond. Pour la signaler, le contexte complet est copié, y compris la callback.

3. Une nouvelle requête authentifiée commence sur le thread principal. (requête n°2)

4. Dans le thread de fond, le système de rapport d’erreur traite la callback de contexte. Il tente de lire l’identifiant de session utilisateur, mais comme il n’existe pas, il envoie via le contrôleur Rails de la requête n°1 une demande au système d’authentification. Comme Rack réutilise le même objet pour toutes les requêtes, le contrôleur récupère alors le cookie de session de la requête n°2.

5. Sur le thread principal, la requête n°2 se termine.

6. Une autre requête authentifiée arrive. (requête n°3) L’authentification est déjà terminée.

7. Dans le thread de fond, le contrôleur écrit le cookie de session dans le cookie jar présent dans l’environnement Rack afin de finaliser l’authentification. À ce moment-là, il s’agit du cookie jar de la requête n°3.

8. L’utilisateur reçoit la réponse de la requête n°3, mais comme le cookie de session de la requête n°2 a été écrit dans le cookie jar, il est authentifié en tant qu’utilisateur de la requête n°2.

En résumé, si une exception survient et que le traitement des requêtes s’enchaîne exactement dans cet ordre, la session d’une réponse est remplacée par celle d’une réponse précédente. Cela ne se produisait que dans l’en-tête des cookies ; les réponses elles-mêmes, comme le HTML, contenaient bien les données de l’utilisateur initialement authentifié.

Ce bug ne pouvait se produire que lorsque toutes ces conditions complexes étaient réunies.

• Pour corriger le problème, GitHub a supprimé le thread de fond nouvellement introduit et a déployé ce changement en production le 5 mars.

• Ensuite, l’équipe a créé un patch pour Unicorn afin d’empêcher le partage de l’environnement, puis l’a déployé le 8 mars.

• Après analyse des logs, ils ont constaté que le problème s’était produit rarement, mais ont invalidé les sessions de tous les utilisateurs afin d’éliminer tout risque potentiel.

• Une fois le problème résolu, ils ont travaillé avec les mainteneurs de Unicorn pour intégrer la correction en amont.

[kunggom]

Le traitement parallèle est clairement complexe. Moi aussi, ce week-end, en essayant de faire exécuter en parallèle, à titre d’étude personnelle, un code que j’avais récemment écrit sur autant de threads CPU que possible, j’ai longtemps tâtonné. J’y suis finalement parvenu, mais je reste encore un peu dubitatif quant au fait que ce soit vraiment correct.