fix(hub): alle bevindingen uit de M34 security-review #160
No reviewers
Labels
No labels
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
janpeter/Scrum4Me!160
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/hub-decision-signature"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Uit de security-review van M34. Dit sluit de enige bevinding die ik als "vóór rollout oplossen" bestempelde.
Het probleem
De telefoon ondertekent zijn antwoord met ECDSA richting de server, maar op het traject server → hook zat daar niets van, en
HUB_URLwerd nergens op scheme gecontroleerd.Een probe met een neppe hub op plain
http, die niets anders doet dan{"status":"answered","answer":"approve_always"}terugsturen:Geen telefoon, geen tik. Wie het netwerkpad, de DNS of de
HUB_URLbeheerste, keurde elke permission-prompt op elke fleet-host goed. M31 had dit al voor een eenmaligeallow; M34 verhoogde de inzet naar een permanente regel.Maatregel 1 — https vereist
isSecureHubUrlweigert alles behalvehttps, met een uitzondering voor loopback (localhost,127.0.0.0/8,[::1]) — het runbook gebruikt bewusthttp://localhost:3000voor de rooktest, en daar bestaat geen af te luisteren pad. Bij een onveilige URL raadpleegt de hook de hub niet en meldt dat op stderr; de prompt valt terug naar de terminal.Maatregel 2 — het besluit is ondertekend
Beide wait-routes sturen een
sigmee: HMAC-SHA256 met het ingest-secret overJSON.stringify([approvalId, status, answer, note]). De hook verifieert die vóór hij handelt. Dat bindt het besluit aan dit verzoek én aan het exacte antwoord — een handtekening van een andere approval, of over een ander antwoord, past niet.Een JSON-array en geen
\n-join:answerennotezijn vrije tekst en mogen newlines bevatten. Bij een join zouden (answer"x\ny", note"") en (answer"x", note"y") dezelfde handtekening delen; daar is een test voor.signedDecisionBodybouwt body én handtekening in één keer, zodat de ondertekende velden per constructie de verzonden velden zijn.Wat dit niet afdekt: het secret is symmetrisch, dus een gecompromitteerde hub-server kan wél tekenen. De maatregel sluit het transport af, niet de server. Dat staat zo in het runbook.
Drift
De canonieke vorm staat noodgedwongen twee keer — de hook is bewust dependency-vrij en kan de lib niet importeren.
__tests__/lib/hub/decision-signature.test.tslaadt beide kanten en faalt zodra ze uiteenlopen. Dat is de enige bescherming die hier werkt.Geverifieerd
De oorspronkelijke exploit-probe levert nu geen besluit, geen settings en geen register meer op.
RED-controle op beide maatregelen, niet alleen groen achteraf:
Nieuwe end-to-end tests spawnen het echte hookproces tegen een neppe hub. Drie varianten worden genegeerd — ongetekend, getekend met het verkeerde secret, en getekend over een ander antwoord — met assertie op zowel de lege stdout als het uitblijven van
settings.local.json. De bestaande "onvoorwaardelijke allow"-test tekent nu zoals de echte route, en is daarmee het bewijs dat een geldige handtekening het gespawnde proces wél passeert.npm run verify: 2005 tests groen (+19). Volledige gate in een wegwerp-clone op de gepushte SHA — uitkomst onderaan.Uitrolvolgorde
Eerst de server, dan de hooks op de hosts. Een nieuwe hook tegen een oude server krijgt geen handtekening, weigert elk besluit en meldt dat op stderr: fail-closed en veilig, maar je bent de hub kwijt tot de server bij is. Andersom werkt ongestoord — een oude hook negeert het veld.
De overige bevindingen (tweede commit)
Afkapping was onzichtbaar. Een commando van 655 tekens werd een samenvatting van 500, middenin afgekapt, zonder enig teken dat er iets ontbrak — je keurde een prefix goed terwijl de staart meedraaide.
summarizezet er nu[… N tekens afgekapt en dus NIET zichtbaar]achter en knipt nooit midden in een surrogate pair. Een afgekapte input blijft niet-persisteerbaar, dus dit raakt alleen de eenmalige goedkeuring — maar juist die werd blind gegeven.De aanwijzing bij "weiger" komt van de telefoon en landt in
permissionDecisionReason, dus in de context van de agent.userNoteReasonciteert hem nu expliciet als gebruikerstekst en niet als systeeminstructie. Bewust los vanhookOutput: die wordt ook met interne redenen aangeroepen (S4M Hub allowlist) en die zijn geen citaat.resolveWaitablewas niet user-scoped. Het ingest-secret zegt "een fleet-host", niet "wélke gebruiker"; elke id leverde status, antwoord én aanwijzing van een willekeurige approval. Nu dezelfde scope alsanswerApproval. Vandaag single-tenant en dus onopvallend, bij een tweede gebruiker een horizontaal gat.De commandotekst gaat ook langs Apple. APNs-alert-bodies zijn voor Apple leesbaar; end-to-end-versleuteling bestaat daar niet voor. Dit is de enige bevinding die hier niet met code is opgelost — hij staat als bekende eigenschap in het runbook, mét de consequentie: een secret in een commando is buiten je eigen infrastructuur geweest, en roteren is dan het enige echte antwoord. Bewust géén regex-redactie: die mist gevallen en levert vooral vals vertrouwen op.
Mijn eerste formulering hierbij ("valt niet te fixen zonder M34's kern weg te nemen") was te stellig en is gecorrigeerd in een aparte commit. Het is wél verkleinbaar: een push met alleen een id plus een onschuldige kop, waarna de NSE de volledige tekst ophaalt en de body invult. Dat raakt het payload-contract, de app en de e2e-gate, dus het is vastgelegd als IDEA-181 in plaats van er hier bij gebouwd.
RED-controle ook hier op alle drie de codewijzigingen: markering weghalen maakt 2 tests rood, de note kaal teruggeven 1, de user-scope weghalen 1.
Buiten scope
scripts/hub-ask.mjsverifieert bewust niet: dat is een generieke vraag-client, geen autorisatiepad. Het extra veld negeert hij.Let op bij mergen
De commit-messages beschrijven het lek expliciet en die tekst mirrort mee naar GitHub. De fix zit in dezelfde commits, maar de fleet-hosts draaien de oude hook tot je ze bijwerkt — werk de hosts dus bij vóór of tegelijk met de mirror-sync, niet erna.
APPROVED
geen gekoppeld plan gevonden — beoordeeld op codekwaliteit + product-standaarden.
Findings
Review-notities
approvalId,status,answerennotesamen via HMAC, waardoor replay/tampering tussen hub en hook fail-closed wordt.decideFromStatusof persistente allow-regels in beeld komen.HUB_URLis gedocumenteerd en getest, met expliciete loopback-uitzondering voor lokale rooktests.De resterende drie bevindingen uit de security-review van M34, plus de vierde die alleen documentatie kon zijn. **Afkapping was onzichtbaar.** Een commando van 655 tekens werd een samenvatting van 500, middenin afgekapt, zonder enig teken dat er iets ontbrak — je keurde een prefix goed terwijl de staart meedraaide. `summarize` zet er nu `[… N tekens afgekapt en dus NIET zichtbaar]` achter en knipt nooit midden in een surrogate pair. Een afgekapte input blijft niet-persisteerbaar, dus dit raakt alleen de eenmalige goedkeuring — maar juist die werd blind gegeven. **De aanwijzing bij "weiger" komt van de telefoon** en landt in `permissionDecisionReason`, dus in de context van de agent. `userNoteReason` citeert hem nu expliciet als gebruikerstekst en niet als systeeminstructie. Los gehouden van `hookOutput`, want die wordt ook aangeroepen met interne redenen ('S4M Hub allowlist') die geen citaat zijn. **`resolveWaitable` was niet user-scoped.** Het ingest-secret zegt "een fleet-host", niet "wélke gebruiker"; elke id leverde status, antwoord én aanwijzing van een willekeurige approval. Nu dezelfde scope als `answerApproval`. Vandaag single-tenant en dus onopvallend, bij een tweede gebruiker een horizontaal gat. **De commandotekst gaat ook langs Apple** — APNs-alert-bodies zijn voor Apple leesbaar. Dat valt niet te fixen zonder M34's kern weg te nemen (kiezen op de push vereist leesbare tekst), dus het staat nu als bekende eigenschap in het runbook, mét de consequentie: een secret in een commando is buiten je eigen infrastructuur geweest en roteren is het enige echte antwoord. RED-controle op alle drie de codewijzigingen: markering weghalen maakt 2 tests rood, de note kaal teruggeven 1, de user-scope weghalen 1. npm run verify groen: 2012 tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Mijn eerdere formulering ("valt niet te fixen zonder M34's kern weg te nemen") was te stellig. Een push met alleen een id plus een onschuldige kop, waarna de NSE de volledige tekst ophaalt en de body invult, houdt de commandotekst bij Apple weg zonder het kiezen-op-de-push op te geven. De prijs is de tijdslimiet van de NSE: wordt die overgeslagen, dan resteert de kop. Dat is een wijziging aan het payload-contract, geen hygiëne-maatregel — opgeschreven als follow-up. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Verdict: APPROVED
Geen gekoppeld plan gevonden — beoordeeld op codekwaliteit + product-standaarden.
Findings
De diff volgt de hub permission-hook runbook-richting: onveilige non-loopback
httpwordt vóór netwerkgebruik geweigerd, wait-besluiten worden met het ingest-secret aanapprovalId/status/answer/notegebonden, en de hook faalt gesloten bij ontbrekende of ongeldige signatures. De extra user-note quoting en zichtbare truncation-markering sluiten logisch aan op de bestaande permission-hook risico's.Tests zijn passend uitgebreid voor server-signing, hook-verificatie, replay/forgery, user-scope op
resolveWaitable, insecureHUB_URL, en de end-to-end writer-flow.fix(hub): bind het besluit aan het verzoek en eis https voor HUB_URLto fix(hub): alle bevindingen uit de M34 security-reviewVerdict: APPROVED
Geen gekoppeld plan gevonden — beoordeeld op codekwaliteit + product-standaarden.
Findings
De diff is consistent met de hub-permission-hook runbookrichting en de relevante route/securitypatronen. De security-fixes zijn server- en hook-zijdig afgedekt met gerichte regressietests voor unsigned/gewijzigde decisions, replay over een andere approvalId, verkeerde secrets, onveilige HUB_URLs, zichtbare truncatie en geciteerde user-notes.