Xz : un seul caractère qui a neutralisé Landlock sous Linux

 (git.tukaani.org)
1 points par GN⁺ 2024-03-31 | 1 commentaires | Partager sur WhatsApp
  • La modification de CMakeLists.txt dans xz.git, qui faisait évoluer la détection du sandbox Landlock d’une simple vérification de présence d’en-tête vers un véritable test de compilation, a introduit un caractère . isolé dans le code de test
  • Cette nouvelle vérification a été ajoutée pour écarter les systèmes où linux/landlock.h existe mais où les définitions de syscall requises sont absentes
  • Le test de compilation inclut <linux/landlock.h>, <sys/syscall.h>, <sys/prctl.h> et référence prctl, SYS_landlock_create_ruleset, SYS_landlock_restrict_self, LANDLOCK_CREATE_RULESET_VERSION
  • Le critère existant HAVE_LINUX_LANDLOCK_H devient HAVE_LINUX_LANDLOCK, et les réglages de SANDBOX_COMPILE_DEFINITION ainsi que de SANDBOX_FOUND suivent ce résultat
  • Le commit corrige le test de fonctionnalité Linux Landlock des builds Autotools et CMake, et le diff fourni montre les changements côté CMake

Changement de la méthode de détection de Landlock dans CMake

  • Le fichier concerné est CMakeLists.txt dans xz.git, et la modification se situe dans le bloc # Sandboxing: Landlock
  • L’ancienne logique CMake se contentait, quand ENABLE_SANDBOX valait ON ou landlock et que SANDBOX_FOUND était faux, de vérifier la présence de l’en-tête avec check_include_file(linux/landlock.h HAVE_LINUX_LANDLOCK_H)
  • Après modification, check_c_source_compiles est utilisé pour compiler réellement un petit code C afin de vérifier si Landlock est utilisable
    • Certains systèmes peuvent disposer de linux/landlock.h sans pour autant fournir les définitions de syscall nécessaires à Linux Landlock
    • Le code de test inclut <linux/landlock.h>, <sys/syscall.h>, <sys/prctl.h>
    • Dans my_sandbox(), il référence prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0), SYS_landlock_create_ruleset, SYS_landlock_restrict_self, LANDLOCK_CREATE_RULESET_VERSION

Caractère isolé et changement de nom de variable

  • Une ligne contenant uniquement . a été insérée juste après le bloc d’include du code de test de compilation
  • La variable de résultat passe de HAVE_LINUX_LANDLOCK_H à HAVE_LINUX_LANDLOCK
  • La condition est elle aussi modifiée de if(HAVE_LINUX_LANDLOCK_H) à if(HAVE_LINUX_LANDLOCK)
  • La valeur de SANDBOX_COMPILE_DEFINITION utilise "HAVE_LINUX_LANDLOCK" au lieu de "HAVE_LINUX_LANDLOCK_H"
  • Le réglage ultérieur de SANDBOX_FOUND ON est conservé

À lire aussi

1 commentaires

 
GN⁺ 2024-03-31
Avis de Hacker News

  • Réponse : https://git.tukaani.org/?p=xz.git;a=commitdiff;h=f9cf4c05edd...
    Si vous ne connaissez pas bien le système de contrôle d’accès Landlock de Linux, l’explication est ici : https://docs.kernel.org/userspace-api/landlock.html
    Page officielle (probablement...) de réponse à l’incident xz : https://tukaani.org/xz-backdoor/

    • Donc cette fonction vérifiait si le code C qui suivait compilait, et n’activait Landlock que dans ce cas ?
      Mais à cause d’un minuscule point difficile à repérer, collé près du bord gauche du diff, le code C devenait toujours invalide, et donc Landlock restait toujours désactivé ?
      Impressionnant de sournoiserie, et sournois de manière impressionnante. Je ne l’ai pas vu non plus à la première lecture
    • Il y a ici une possibilité de déni plausible
      Pour cacher ça, il existe de meilleures méthodes consistant à remplacer par des caractères Unicode ressemblants. Certains paraissent identiques au pixel près selon la police
      J’ai peut-être raté le fil, mais l’intentionnalité est-elle établie ici ?
    • C’est tellement vicieux que même si c’était repéré en plein milieu d’une PR, on pourrait s’en tirer avec un « ah, mon IDE a autoformaté ça comme ça »
    • Lasse Collin est réapparu, et il a probablement l’air furieux
    • J’ai l’impression que mon système de gestion de versions met mieux en évidence les caractères modifiés que ces gros blocs de chaînes vertes/rouges

  • En parcourant prudemment, j’ai vu un point à un endroit où il ne devait clairement pas être, et je me suis dit : « ce n’est pas si difficile que ça »
    J’ai tapoté l’écran, et le point a bougé
    Fichue poussière sur l’écran

  • Je n’arrive pas à croire que la sécurité du système repose sur une chaîne d’exactitude aussi fragile. Il y avait quantité d’endroits où cela aurait pu être empêché
    + # A compile check is done here because some systems have
    + # linux/landlock.h, but do not have the syscalls defined
    + # in order to actually use Linux Landlock.
    Il faudrait corriger les en-têtes de ces systèmes pour les exclure explicitement. À quoi sert un en-tête s’il ne déclare pas ses propres fonctionnalités ?
    Et je ne comprends pas non plus pourquoi, une fois le blob binaire créé (même compilé depuis de l’open source), il n’y a aucun test vérifiant que la sécurité est intacte
    On ne déploierait pas non plus une fonctionnalité de paiement sur un site web sans test de bout en bout de la fonctionnalité principale. On dirait qu’il y a un désalignement des priorités, où l’ajout de fonctionnalités passe avant la fiabilité
    J’espère que le correctif ne se contente pas de supprimer le .. Je viens justement de voir un autre post sur HN montrant la suppression du .

    • Il a introduit un tout nouveau framework de test, puis a lentement implanté la backdoor sur une période de deux ans
    • Ce que vous citez vient de Jia Tan [1]. C’est un commentaire écrit par cet acteur malveillant alors qu’il sabotait délibérément le contrôle dès le départ
      Corriger les en-têtes ou ajouter des tests n’aurait pas empêché cela. D’abord parce qu’il n’y avait aucun signe que l’en-tête était cassé, et ensuite parce que des tests supplémentaires auraient pu être compromis autrement ou ignorés dans le tarball de release
      [1] https://git.tukaani.org/?p=xz.git;a=commit;h=328c52da8a2bbb8...
    • C’est un peu hors sujet, mais GCC 9.4.0 a déjà distribué un en-tête arm_acle.h cassé [https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100985] — le code qui incluait cet en-tête échouait toujours à la compilation
      Comme un utilisateur pouvait vouloir compiler avec GCC 9.4.0, et que la fonctionnalité dépendant de cet en-tête n’était pas essentielle à l’application, si le build détectait que l’en-tête était cassé, il désactivait simplement cette fonctionnalité et émettait un avertissement
      Pour revenir à xz, si la sécurité n’est pas la priorité absolue, ignorer l’échec d’une fonctionnalité optionnelle et continuer paraît raisonnable. Bien sûr, après coup, il est facile de pointer du doigt, mais hors de ce contexte, ce n’est pas une décision totalement absurde
    • En tant qu’utilisateur d’open source, je ne m’y connais pas assez pour faire ce genre de proposition ; donc si vous pouviez la développer vous-même et contribuer, ce serait apprécié
    • Savez-vous si le code du commentaire que vous citez est exact ?

  • Oups, son dernier commit aggrave le signalement de sécurité : https://git.tukaani.org/?p=xz.git;a=commitdiff;h=af071ef7702...

    • Pourquoi est-ce que ça a été accepté ? Je suis sérieusement curieux

  • Landlock était censé être utilisé où et comment ?
    J’ai l’impression que ce serait difficile à utiliser en pratique dans une bibliothèque généraliste, comme pour la compression/décompression. Une bibliothèque ne peut pas savoir ce que le programme doit faire ni ce qu’il faut limiter.
    Pour un programme, ce serait plus clair.
    L’attaque contre sshd utilisait liblzma comme bibliothèque. Dans ce cas, désactiver Landlock semble sans rapport, non ? Est-ce le signe qu’il y avait encore du mauvais code non découvert, ou qu’ils prévoyaient d’en ajouter plus tard ?

    • Il est probable que sshd lui-même l’utilisait pour verrouiller ses propres privilèges après un certain point de l’exécution.
      Si on retire Landlock, le démon ne se verrouille pas lui-même, ce qui facilite l’exécution de la charge utile une fois l’étape d’exploitation atteinte.
      Je ne pense pas que les deux soient sans rapport. Ils avaient déjà la charge utile en tête, et ont manifestement compris que ce serait un obstacle.
    • C’est étrange. Faire deux travaux qui ressemblent à des backdoors sans rapport entre eux dans le même projet, c’est vraiment bâclé à mon avis. Le gain est faible, mais le risque d’être repéré augmente beaucoup.
      Cette affaire est très confuse. Certains aspects sont d’une sophistication presque incroyable, tandis que d’autres paraissent assez négligés.
    • C’est utilisable et c’est une bonne idée, mais c’est plutôt destiné à empêcher l’exploitation de vulnérabilités qu’à contrer une backdoor intentionnelle.
      L’idée est que la bibliothèque lance une tâche complexe dans un thread séparé, puis que le code utilisateur attende ce thread dans l’API qu’il a appelée. Le thread s’applique Landlock à lui-même avant de commencer le travail. Si quelque chose tourne mal, le code est isolé.
      Bien sûr, dans ce cas précis, le bac à sable est contrôlé par l’attaquant, donc cela ne fonctionne pas : il peut simplement le désactiver ou le rendre plus faible qu’à l’origine. Cela dit, on peut aussi faire autrement.

  • Mais qu’est-ce qu’ils cherchaient à faire ici, au juste ? Ajouter plus tard davantage de backdoors, plus plausiblement déniables ? À mon avis, oss-fuzz comme cette modification n’auraient pas réellement trouvé la backdoor qui a été découverte.
    Alors pourquoi mettre tous les œufs « backdoor » dans le même panier, c’est-à-dire dans une seule bibliothèque ?

    • Cette bibliothèque est suffisamment profondément intégrée, suffisamment digne de confiance, et son mainteneur principal s’était absenté longtemps d’Internet pour des raisons de santé mentale.
      En plus, il ne s’agit pas de mettre une backdoor dans la bibliothèque elle-même, mais de viser les outils qui l’utilisent. C’est dans l’esprit de « Reflections on trusting trust ».
      Jusqu’à l’échec, ça ressemble à un plan parfait.
    • Mais qui peut dire que c’était la seule bibliothèque ?

  • Je me demande en quoi empêcher l’activation de Landlock dans xz était utile. Comptaient-ils injecter du contenu malveillant dans des archives xz à une étape ultérieure ? Si oui, pourquoi ne pas simplement avoir ajouté un comportement malveillant dans xz lui-même ?

    • Parce qu’une vulnérabilité intentionnelle est beaucoup plus facile à dissimuler que du véritable contenu malveillant.
      Introduire discrètement un dépassement de tampon ou une utilisation après libération dans un projet C que l’on maintient peut passer inaperçu. Distribuer un vrai cheval de Troie est bien plus difficile, et la backdoor xz-vers-sshd l’a montré.
    • Il pourrait aussi y avoir une backdoor plus subtile visant xz lui-même, en plus de celle contre ssh. L’objectif était clairement la discrétion.

  • C’est étonnamment beaucoup trop visible. La technique pour désactiver la fonctionnalité est intelligente et le commit assez plausible. Mais pourquoi avoir choisi une erreur de syntaxe évidente au lieu d’une simple faute de frappe ? Par exemple, si l’erreur avait été PR_SET_NO_NEW_PRIV, l’aurait-on remarquée ?
    Cela aurait aussi été plus facile à nier.

  • À part ça, cette équipe de malware a fourni un excellent jeu de données pour entraîner une IA à identifier des problèmes de sécurité. Chaque commit contient un problème de sécurité, et la communauté open source va les passer en revue un par un pour les trouver.
    Merci aux mainteneurs qui font le travail de nettoyage. Ce n’est sûrement pas amusant.

    • Je ne pense pas que cela se généralise très bien. Au mieux, ce n’est qu’une course aux armements.
    • C’est l’un des usages de l’IA les plus intéressants que j’aie vus.
    • C’est la première fois que j’en entends parler ; tu pourrais partager des infos à ce sujet ?

  • C’est pour ça que je n’aime vraiment pas les systèmes de build de style configure qui activent et désactivent automatiquement des fonctionnalités. Si je veux quelque chose, je préfère l’activer explicitement. Si une fonctionnalité a une bonne raison d’être activée par défaut, il faut plutôt permettre de la désactiver explicitement.

    • Pour le packaging, c’est une énorme douleur. On configure le paquet, on ajoute des dépendances jusqu’à ce qu’il compile, puis on pense avoir terminé. Mais la fonctionnalité X manque. Quoi ? Ah, libY n’était pas là, donc elle a été désactivée silencieusement. On revient en arrière et on l’ajoute. Puis un utilisateur signale que la fonctionnalité Z manque.
      Il suffit de définir un ensemble de fonctionnalités par défaut et d’autoriser --enable-a --disable-b selon les besoins.
      Le fait d’avaler silencieusement les bugs du processus de vérification est encore un autre problème.