Gitfed
bastien-mrq/gitfed/ Commits/ ed9440a

Enforce MR title/description/comment length server-side; write up audit

The form's maxlength attributes (200/4000) were client-side only — a direct POST bypassed them. Now enforced in handleMRCreate/handleMRComment too. Adds docs/security/AUDIT-2026-07-29b.md: the full writeup for this fix and 0.10.1's critical argument-injection fix, including the exact unauthenticated exploit that was used to confirm it (not just a theoretical read of the code), and the surfaces checked and found sound. Linked from both READMEs.

bastien-mrq 2026-07-28 23:00 commit ed9440abfd3d97c5523bf7fe7bd89658ff64213c parent f642d6c22a6f2e29b8656e68053077762024c019
5 files changed +208 −2
M CHANGELOG.md +4 −0
M README.fr.md +1 −0
M README.md +1 −0
M cmd/gitfed-web/handlers_merge_requests.go +13 −2
A docs/security/AUDIT-2026-07-29b.md +189 −0
CHANGELOG.md
diff --git a/CHANGELOG.md b/CHANGELOG.md index 884dc3b..fb7fa5b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # Changelog +## 0.10.2 + +- Merge request title/description/comment length limits are now enforced server-side, not just as HTML `maxlength` attributes a direct POST trivially bypasses. Found during the same audit as 0.10.1's fix; full writeup in `docs/security/AUDIT-2026-07-29b.md`. + ## 0.10.1 - **Security fix (critical): argument injection via commit hashes and branch names.** Every place that passed a hash or branch name as the last argument to a `git` subprocess (`ShowCommit`, `CommitDiff`, `BranchDiff`, `CheckMergeable`, `MergeBranches`, the merge-request worktree/merge machinery) trusted it as a plain revision — but git ref names can start with `-` (client-side `git branch`/`checkout -b` block it, a raw `update-ref` or push doesn't), so a crafted name like `--output=/some/path` got parsed as git's own flag instead. Confirmed exploitable **unauthenticated**, on any public repo, via `/repo-commit/{repo}?hash=--output=<path>` — no push access needed at all — to make `git` write to an arbitrary file path gitfed-server can reach, which is a path to planting a malicious git hook and getting code execution on the next push. Fixed by adding `--end-of-options` before every such argument everywhere in `internal/gitexec`, with a regression test that reproduces the exact exploit and asserts no file gets written.
README.fr.md
diff --git a/README.fr.md b/README.fr.md index bb37e09..43221a4 100644 --- a/README.fr.md +++ b/README.fr.md @@ -66,6 +66,7 @@ instance en fonctionnement. | [`docs/ARCHITECTURE.md`](docs/ARCHITECTURE.md) | Comment le code est organisé : les quatre binaires, les paquets `internal/`, pourquoi il existe un socket RPC d'administration, la topologie Kubernetes. | | [`docs/security/AUDIT.md`](docs/security/AUDIT.md) | L'audit de sécurité pré-production et ce qui en a été corrigé. | | [`docs/security/AUDIT-2026-07-29.md`](docs/security/AUDIT-2026-07-29.md) | Audit de suivi couvrant le clone HTTPS anonyme, les notifications fédérées et les dépôts épinglés ajoutés depuis. | +| [`docs/security/AUDIT-2026-07-29b.md`](docs/security/AUDIT-2026-07-29b.md) | Audit des merge requests — a trouvé et corrigé une faille critique non authentifiée (écriture de fichier arbitraire via des hash de commit / noms de branche forgés), plus deux correctifs de moindre sévérité. | | [`DESIGN.md`](DESIGN.md) | Le document de conception d'origine — le « pourquoi » derrière l'architecture, écrit avant le début de l'implémentation. | | [`CHANGELOG.md`](CHANGELOG.md) | Historique des versions (également servi sur `/changelog` dans l'interface web). | | [`THIRD_PARTY_LICENSES.md`](THIRD_PARTY_LICENSES.md) | Chaque dépendance Go et sa licence. |
README.md
diff --git a/README.md b/README.md index 53b2d5f..473e9ba 100644 --- a/README.md +++ b/README.md @@ -62,6 +62,7 @@ instance. | [`docs/ARCHITECTURE.en.md`](docs/ARCHITECTURE.en.md) | How the code is organized: the four binaries, the `internal/` packages, why there's an admin RPC socket, the Kubernetes topology. | | [`docs/security/AUDIT.md`](docs/security/AUDIT.md) *(French)* | The pre-production security audit and what was fixed as a result. | | [`docs/security/AUDIT-2026-07-29.md`](docs/security/AUDIT-2026-07-29.md) *(French)* | Follow-up audit covering the anonymous HTTPS clone, federated notifications and pinned repos added afterward. | +| [`docs/security/AUDIT-2026-07-29b.md`](docs/security/AUDIT-2026-07-29b.md) *(French)* | Audit of merge requests — found and fixed a critical, unauthenticated argument-injection bug (arbitrary file write via crafted commit hashes/branch names), plus two lower-severity fixes. | | [`DESIGN.md`](DESIGN.md) *(French)* | The original design rationale — the "why" behind the architecture, written before implementation started. | | [`CHANGELOG.md`](CHANGELOG.md) | Version history (also served at `/changelog` in the web UI). | | [`THIRD_PARTY_LICENSES.md`](THIRD_PARTY_LICENSES.md) | Every Go dependency and its license. |
cmd/gitfed-web/handlers_merge_requests.go
diff --git a/cmd/gitfed-web/handlers_merge_requests.go b/cmd/gitfed-web/handlers_merge_requests.go index 3998338..8b4b7de 100644 --- a/cmd/gitfed-web/handlers_merge_requests.go +++ b/cmd/gitfed-web/handlers_merge_requests.go @@ -13,6 +13,14 @@ import ( "gitfed/internal/store" ) +// mrTitleMaxLen/mrTextMaxLen match the form's client-side maxlength +// attributes — enforced again server-side since a direct POST bypasses +// those. +const ( + mrTitleMaxLen = 200 + mrTextMaxLen = 4000 +) + // mrView adds a display-only translated status label to a store.MergeRequest. type mrView struct { store.MergeRequest @@ -198,7 +206,10 @@ func (s *server) handleMRCreate(w http.ResponseWriter, r *http.Request) { target := strings.TrimSpace(r.FormValue("target")) newPath := "/repo-mr-new/" + name - if title == "" || source == "" || target == "" || source == target { + // The form's maxlength attributes are client-side only — a direct POST + // bypasses them, so the same limits are enforced here too. + if title == "" || source == "" || target == "" || source == target || + len(title) > mrTitleMaxLen || len(description) > mrTextMaxLen { redirectWithMsg(w, r, newPath, i18n.T(lang, "mr.msg_invalid"), true) return } @@ -475,7 +486,7 @@ func (s *server) handleMRComment(w http.ResponseWriter, r *http.Request) { back := fmt.Sprintf("/repo-mr/%s?number=%d", name, number) body := strings.TrimSpace(r.FormValue("body")) - if body == "" { + if body == "" || len(body) > mrTextMaxLen { redirectWithMsg(w, r, back, i18n.T(lang, "mr.msg_comment_empty"), true) return }
docs/security/AUDIT-2026-07-29b.md
diff --git a/docs/security/AUDIT-2026-07-29b.md b/docs/security/AUDIT-2026-07-29b.md new file mode 100644 index 0000000..6840e72 --- /dev/null +++ b/docs/security/AUDIT-2026-07-29b.md @@ -0,0 +1,189 @@ +# Audit de sécurité — merge requests (2026-07-29, suite) + +- **Périmètre** : uniquement la fonctionnalité merge requests + (`internal/store/merge_requests.go`, la plomberie git ajoutée dans + `internal/gitexec` pour les branches/fusions, `admin.Ops`/`adminrpc` + correspondants, `cmd/gitfed-web/handlers_merge_requests.go`). +- **Nature** : revue de code adversariale, **avec exploitation réelle** en + bac à sable (pas seulement une lecture de code) — chaque faille listée + ci-dessous a été effectivement déclenchée contre une instance locale + avant d'être corrigée, pas seulement supposée théorique. +- **État** : les correctifs identifiés ci-dessous sont **déjà appliqués** + dans ce commit, avec un test de régression qui reproduit l'exploit exact + pour C1 (`internal/gitexec/gitexec_test.go`) et pour L3 + (`internal/admin/admin_test.go`). + +--- + +## 1. Synthèse + +| ID | Sévérité | Titre | Statut | +|----|----------|-------|--------| +| C1 | 🔴 Critical | Injection d'argument via les hash de commit et noms de branche | ✅ corrigé | +| L3 | 🟡 Low | Fermer une MR déjà fusionnée écrase silencieusement son statut | ✅ corrigé | +| L4 | 🟡 Low | Titre/description/commentaire non plafonnés côté serveur | ✅ corrigé | + +C1 est la découverte importante de cet audit — un contournement complet +du modèle de menace habituel (aucune authentification requise, aucun accès +en écriture nécessaire) menant à une écriture de fichier arbitraire, avec +un chemin réaliste vers l'exécution de code. Le reste de la surface +merge requests (autorisation, isolation git, gestion des conflits/races) +a résisté à l'examen — voir §3. + +--- + +## 2. Détail + +### 🔴 C1 — Injection d'argument via les hash de commit et noms de branche + +**Fichiers** : `internal/gitexec/gitexec.go` (`ShowCommit`, `CommitDiff`, +`BranchDiff`, `branchFiles`, `commitFiles`, `withScratchWorktree`, +`CheckMergeable`, `MergeBranches`) + +**Le problème.** Chacune de ces fonctions passe un hash de commit ou un +nom de branche comme dernier argument d'une commande `git` : +`git show --format=... <hash>`, `git diff --no-color <target>...<source>`, +`git worktree add --detach <dir> <base>`, `git merge --no-ff -m <msg> +<source>`. Le porcelain git (`git branch`, `git checkout -b`) refuse +côté client tout nom de ref commençant par `-` (`check-ref-format`) — mais +ce n'est qu'une validation client. Un `git update-ref` direct sur le +dépôt bare, ou un push utilisant directement une refspec, crée la ref +sans broncher. Une fois cette ref créée, elle est renvoyée par +`ListBranches` exactement comme n'importe quelle autre — rien ne la +distingue dans l'interface. + +**Preuve de faisabilité (réellement exécutée, pas seulement décrite).** + +```sh +# Contourne check-ref-format en écrivant la ref directement sur le bare repo : +git --git-dir=demo.git update-ref 'refs/heads/--output=/tmp/poc.txt' <commit> +``` + +Puis, **sans authentification, sans accès en écriture, juste une requête +GET vers un dépôt public** : + +``` +GET /repo-commit/testuser/demo?hash=--output=/tmp/gitfed-commit-poc.txt +``` + +`hash` provient directement du paramètre de requête, sans aucune +validation qu'il ressemble à un hash. `git show` a interprété +`--output=/tmp/gitfed-commit-poc.txt` comme son propre indicateur +`-o`/`--output=<fichier>` ("Output to a specific file instead of +stdout.") plutôt que comme une révision — et a effectivement créé le +fichier `/tmp/gitfed-commit-poc.txt` sur le disque du serveur. + +**Impact.** Une écriture de fichier arbitraire à un chemin choisi par +l'attaquant, à l'endroit où `gitfed-server` (uid 1000, propriétaire de +tout `/data`) a le droit d'écrire — ce qui inclut `hooks/post-receive` +dans n'importe quel dépôt bare de l'instance. Un hook git s'exécute +automatiquement à chaque `git push` suivant sur ce dépôt : c'est un +chemin réaliste vers l'exécution de code arbitraire en tant que processus +`gitfed-server`, déclenchable par le tout premier visiteur anonyme d'une +instance publique, aucun compte, aucune clé SSH, aucun accès en écriture +nécessaire. La variante par branche (nécessite un accès en écriture pour +créer la ref malveillante, puis ouvrir une merge request avec comme +branche source ou cible) a été vérifiée de la même manière via +`branchFiles`/`BranchDiff`/`CheckMergeable`/`MergeBranches`. + +**Correctif.** `--end-of-options` ajouté juste avant chaque argument de ce +type — un indicateur générique reconnu par les commandes basées sur +`parse-options` de git (2.24+, largement disponible), qui force tout ce +qui suit à être traité comme un argument positionnel, jamais comme une +option. Vérifié explicitement que ça préserve le comportement correct +pour une révision légitimement préfixée par un tiret (contrairement à +`--`, qui aurait réinterprété l'argument comme un chemin de fichier au +lieu d'une révision, cassant la sémantique de plage `A...B`). Testé en +conditions réelles : + +- Un hash/nom de branche légitime continue de fonctionner exactement + comme avant (diff/merge produits correctement). +- Le même hash malveillant qu'en preuve de faisabilité ne crée plus + aucun fichier ; la page correspondante répond `200` proprement au lieu + d'un `500` (git rejette la révision, gitfed le gère normalement). +- Test de régression `TestArgumentInjectionIsBlocked` + (`internal/gitexec/gitexec_test.go`) : reproduit l'exploit exact contre + chacune des fonctions concernées et échoue si le correctif est retiré + (vérifié en désactivant temporairement `--end-of-options` sur + `CommitDiff` — le test échoue bien en détectant le fichier créé). + +--- + +### 🟡 L3 — Fermer une MR déjà fusionnée écrase silencieusement son statut + +**Fichier** : `internal/admin/admin.go` (`CloseMergeRequest`) + +**Le problème.** `CloseMergeRequest` fixait `Status = MRClosed` +inconditionnellement, sans vérifier le statut courant. Une MR déjà +fusionnée pouvait donc être "fermée" — le commit de fusion reste bien réel +dans la branche cible, mais l'enregistrement de gitfed se met à afficher +« Fermée sans fusion », un mensonge pur et simple sur l'historique. + +**Preuve de faisabilité.** MR réellement fusionnée (branche cible +avancée, vérifié par `git log`), puis `POST /repo-mr-close/{repo}` avec +son numéro : la page de détail affiche ensuite le badge « fermée » et +« Fermée sans fusion », alors que le commit de fusion est toujours +présent dans l'historique de la branche. + +**Impact.** Intégrité de la piste d'audit, pas d'élévation de privilège — +n'importe quel collaborateur en écriture peut se donner un faux +historique (« ma MR a été rejetée » alors qu'elle a bien été intégrée), +utile pour tromper une revue ou masquer qu'un changement est passé. + +**Correctif.** `CloseMergeRequest` est maintenant un no-op si le statut +n'est plus `MROpen`, à l'image de la même garde déjà présente sur +`MergeMergeRequest`. Test de régression +`TestCloseMergeRequestDoesNotOverwriteMerged` +(`internal/admin/admin_test.go`). + +--- + +### 🟡 L4 — Titre/description/commentaire non plafonnés côté serveur + +**Fichier** : `cmd/gitfed-web/handlers_merge_requests.go` + +Les attributs `maxlength` du formulaire (200 pour le titre, 4000 pour la +description/le commentaire) ne sont que côté client — un `POST` direct +les contourne trivialement, comme vérifié par exploitation réelle +(titre de 300 caractères accepté avant correctif). Contrairement au +plafond déjà en place sur les notifications fédérées (accessible à +n'importe quelle instance distante sans relation préalable), créer une MR +ou commenter exige déjà d'être authentifié et d'avoir accès en +lecture/écriture au dépôt — le risque est donc plus faible (abus par un +collaborateur déjà de confiance, pas par un tiers anonyme), ce qui justifie +un correctif proportionné : une limite de longueur serveur, sans plafond +sur le nombre de MR/commentaires par dépôt. + +**Correctif.** `mrTitleMaxLen`/`mrTextMaxLen` (200/4000, identiques aux +attributs `maxlength`) revérifiés côté serveur dans `handleMRCreate` et +`handleMRComment`. Vérifié : un titre de 300 caractères est maintenant +rejeté avec le message d'erreur habituel, un titre normal continue de +fonctionner. + +--- + +## 3. Contrôles vérifiés et jugés sains + +| Surface | Constat | +|---|---| +| Autorisation de fusion/fermeture | Passe par `CheckAccess(repo, principal, RoleWrite)`, le même contrôle qu'un `git push` par SSH — vérifié qu'un utilisateur sans accès en écriture ne voit ni bouton Fusionner ni bouton Fermer, et qu'un `POST` direct sans cet accès reçoit un 404. | +| Fuite de MR d'un dépôt privé | `canView` revérifié dans `handleMRDetail`/`handleMRList` avant tout accès, même 404 uniforme que le reste de l'app — vérifié anonyme + utilisateur sans accès. | +| XSS via titre/description/commentaire | Tout passe par `html/template` en échappement automatique, jamais de `template.HTML` sur ces champs — vérifié qu'aucun de ces trois n'est enrobé différemment des autres champs utilisateur déjà couverts par l'audit précédent. | +| CSRF sur les routes de mutation (`/repo-mr-new`, `/repo-mr-merge`, `/repo-mr-close`, `/repo-mr-comment`) | Aucune de ces routes n'est dans la liste d'exemption de `sameOriginPOST` (seule `/git-upload-pack` l'est) — protégées par défaut. | +| Isolation de la fusion elle-même | `MergeBranches` tourne dans un worktree jetable (`git worktree add --detach`), jamais sur l'état réel de la branche cible ; nettoyage (`worktree remove --force`) garanti par `defer`, vérifié qu'aucun worktree ne survit après une fusion en conflit. | +| Mise à jour du ref cible | `git update-ref refs/heads/<target> <nouveau> <ancien>` est un compare-and-swap — un push concurrent qui déplace `<target>` entre le calcul de la fusion et cet appel fait échouer la mise à jour au lieu de l'écraser silencieusement. Le commit de fusion perdant devient orphelin (récupéré par un futur `git gc`), pas de corruption de données. | +| Double clic / fusion concurrente sur la même MR | La vérification `Status == MROpen` se fait avant l'opération git (lente), mais comme la mise à jour du ref est elle-même atomique côté git, un seul des deux appels concurrents peut réellement faire avancer la branche ; le perdant reçoit une erreur serveur générique au lieu d'un message clair — une aspérité d'UX notée mais pas une faille de sécurité, la donnée stockée reste correcte. | +| Numérotation des MR (`#1`, `#2`, ...) | Lecture + incrément + écriture dans une seule transaction `bbolt.Update` — bbolt garantit une seule transaction d'écriture à la fois sur toute la base, donc pas de race possible sur l'attribution du numéro même avec des créations concurrentes. | + +--- + +## 4. Note + +C1 n'est pas une régression d'un contrôle qui existait avant et aurait été +retiré — c'est une fonctionnalité (l'affichage détaillé de commit, puis +les merge requests) ajoutée dans cette même session sans que ce chemin +d'argument-injection ait été anticipé au moment de l'écrire. L'audit du +29/07 précédent (`AUDIT-2026-07-29.md`) avait bien vérifié l'absence +d'injection d'argument sur `repo.Path` dans le clone HTTP — ce constat +reste vrai, il portait simplement sur une surface différente de celle +touchée ici, ajoutée après coup.