Herald commented on pull request qpismont/steep#7 2026-09-20 16:40:32 +02:00
1.2: Sandboxing

Review Feedback

Herald commented on pull request qpismont/steep#7 2026-09-20 16:40:32 +02:00
1.2: Sandboxing

[performance] La collecte de la sortie d'un exec accumule toute la sortie dans deux String sans limite. La limitation à 32 KiB dans l'agent intervient seulement après le retour de cette fonction ; un read_file sur un gros fichier ou un grep très bavard peut donc consommer une quantité arbitraire de mémoire avant d'être tronqué. Il faut borner la sortie pendant la lecture, ou interrompre l'exec dès que la limite est atteinte.

Herald commented on pull request qpismont/steep#7 2026-09-20 16:40:32 +02:00
1.2: Sandboxing

[security] La vérification normalized.starts_with(workspace) compare des chaînes et ne respecte pas les frontières de composants. Avec un workspace /workspaces/repo, un chemin comme ../repo-secrets devient /workspaces/repo-secrets et passe ce test, ce qui permet aux outils de lire un répertoire voisin dans le container. Utiliser une comparaison de chemins basée sur les composants, par exemple strip_prefix(workspace).is_ok() après normalisation.

Herald commented on pull request qpismont/steep#7 2026-09-20 16:40:32 +02:00
1.2: Sandboxing

[performance] Si remove_container échoue, le ? quitte immédiatement la méthode et le réseau ainsi que l'image ne sont jamais supprimés. Après un daemon indisponible, un container déjà supprimé ou une erreur transitoire, chaque sandbox peut donc laisser des ressources persistantes. Effectuer le nettoyage du réseau et de l'image même lorsque la suppression du container échoue, puis retourner l'erreur.

Herald commented on pull request qpismont/steep#7 2026-09-20 16:27:44 +02:00
1.2: Sandboxing

Review Feedback

Herald commented on pull request qpismont/steep#7 2026-09-20 16:27:44 +02:00
1.2: Sandboxing

Le nettoyage s'arrête immédiatement si remove_container échoue, et les suppressions du réseau et de l'image ne sont alors jamais tentées. Une course, un conteneur déjà supprimé ou une erreur transitoire peut donc laisser systématiquement le réseau et l'image derrière lui. Il faut tenter les trois nettoyages indépendamment, puis agréger ou retourner l'erreur pertinente.

Herald commented on pull request qpismont/steep#7 2026-09-20 16:27:44 +02:00
1.2: Sandboxing

La vérification ne protège pas réellement la frontière du dépôt lorsque workspaceFolder vient du dépôt non fiable. Une valeur comme / ou /tmp passe la normalisation et permet aux outils de lire n'importe quel chemin du conteneur, contrairement au contrat annoncé (« confined to the repository workspace »). Il faut imposer un workspace absolu dédié et vérifier qu'il s'agit bien du répertoire de dépôt, plutôt que de faire confiance à workspaceFolder fourni par la PR.

Herald commented on pull request qpismont/steep#7 2026-09-20 16:27:44 +02:00
1.2: Sandboxing

Le flux de build n'est considéré en échec que lorsque error_detail.message est renseigné. L'API Docker peut aussi fournir l'erreur dans le champ error de BuildInfo; dans ce cas la méthode retourne Ok(()) alors que l'image n'a pas été construite. Il faut traiter les deux champs et préserver le message d'erreur le plus utile.

Herald commented on pull request qpismont/steep#7 2026-09-20 16:27:44 +02:00
1.2: Sandboxing

La valeur de DOCKER_HOST est seulement stockée dans endpoint pour les logs : Docker::connect_with_defaults() ne l'utilise pas. Ainsi, la configuration documentée pour Podman ou pour un socket Docker non standard est ignorée et le runtime tente toujours sa connexion par défaut. Il faut construire le client Bollard avec l'endpoint lu dans l'environnement, ou supprimer cette configuration trompeuse.

Herald commented on pull request qpismont/steep#7 2026-09-20 16:27:44 +02:00
1.2: Sandboxing

Le schéma ne prend en charge que dockerfile et args, puis DevContainer::build utilise systématiquement le répertoire du Dockerfile comme contexte. La spécification devcontainer.json permet pourtant de définir build.context (souvent la racine du dépôt) ; les Dockerfile qui font COPY . ... ou qui utilisent un contexte distinct échoueront ou ne verront pas les fichiers attendus. Il faut modéliser et résoudre le contexte de build, avec une validation empêchant qu'il sorte du dépôt.

Herald commented on pull request qpismont/steep#7 2026-09-20 16:27:44 +02:00
1.2: Sandboxing

La suppression de .dockerignore fait que le contexte envoyé lors de buildah bud contient désormais notamment .git, les répertoires de build et le fichier .env local. Même si le Containerfile ne le copie pas explicitement, ce contexte est transmis au moteur de build et peut contenir des secrets ou devenir inutilement volumineux. Il faut conserver un .dockerignore excluant au minimum .git, .env*, target et les fichiers locaux.

Herald commented on pull request qpismont/steep#7 2026-09-20 16:09:04 +02:00
1.2: Sandboxing

Review Feedback

Herald commented on pull request qpismont/steep#7 2026-09-20 16:09:04 +02:00
1.2: Sandboxing

Le chemin build.dockerfile fourni par une pull request est concaténé sans normalisation ni vérification qu'il reste dans le dépôt. Une valeur comme ../../... peut faire choisir un Dockerfile situé hors du clone ; comme son parent devient ensuite le contexte envoyé au daemon, cela peut empaqueter des fichiers d'autres sandboxes ou du système dans le contexte de build. Résolvez le chemin puis imposez qu'il soit situé sous la racine du dépôt avant de l'utiliser.

Herald commented on pull request qpismont/steep#7 2026-09-20 16:09:04 +02:00
1.2: Sandboxing

endpoint lit DOCKER_HOST, mais le client Bollard est créé avec connect_with_defaults() sans utiliser cette valeur. Ainsi, le daemon configuré via DOCKER_HOST (notamment le socket Podman rootless documenté) risque d'être ignoré, tandis que les logs indiquent un endpoint différent de celui réellement utilisé. Il faut construire le client à partir de l'endpoint résolu, ou vérifier que l'API utilisée prend effectivement en charge DOCKER_HOST.

Herald commented on pull request qpismont/steep#7 2026-09-20 16:09:04 +02:00
1.2: Sandboxing

La vérification de confinement dépend entièrement de workspace, qui est contrôlé par le devcontainer.json de la pull request. Une PR peut définir workspaceFolder à /, après quoi starts_with(workspace) autorise pratiquement tout le système de fichiers du container et les outils peuvent lire des fichiers hors dépôt. Le workspace doit être validé et forcé sous un répertoire dédié au dépôt, indépendamment de la configuration non fiable.

Herald commented on pull request qpismont/steep#7 2026-09-20 14:58:14 +02:00
1.2: Sandboxing

Review Feedback

Herald commented on pull request qpismont/steep#7 2026-09-20 14:58:14 +02:00
1.2: Sandboxing

La méthode quitte immédiatement si remove_container échoue, alors que sa documentation indique que le réseau et l'image doivent également être supprimés en best-effort. Dans ce cas, l'image et le réseau restent orphelins, et Sandbox::cleanup ne peut pas récupérer ces ressources. Il faut tenter les suppressions même après l'échec du container, puis retourner une erreur agrégée ou la première erreur rencontrée.

Herald commented on pull request qpismont/steep#7 2026-09-20 14:58:14 +02:00
1.2: Sandboxing

label=disable désactive explicitement le confinement SELinux du container. Pour une sandbox qui exécute du code provenant d'une PR non fiable, cela élargit fortement l'impact d'une évasion ou d'une vulnérabilité du runtime. Il faudrait privilégier un montage correctement relabellisé (:Z/équivalent) ou rendre cette option explicite et désactivée par défaut, avec une justification claire du niveau de risque.

Herald commented on pull request qpismont/steep#7 2026-09-20 14:58:14 +02:00
1.2: Sandboxing

La normalisation est uniquement lexicale et ne résout pas les liens symboliques. Un dépôt contrôlé par l'auteur de la PR peut donc contenir un lien tel que secret -> /proc/self/environ ou secret -> /etc/...; read_file transmettra ensuite ce chemin à cat et permettra de lire des fichiers hors du workspace. Il faut résoudre et valider la cible réelle dans le container, ou refuser les liens symboliques avant toute lecture.

Herald commented on pull request qpismont/steep#7 2026-09-18 15:38:01 +02:00
1.2: Sandboxing