fix(docker): validate HERMES_UID/GID to prevent privilege escalation#35340
Conversation
🔎 Lint report:
|
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary: PR #35340
Verdict: APPROVE ✅
Overall: Clean, focused security fix. 10 lines added, zero bugs. This is exactly the right approach — validate at the entry point rather than downstream.
✅ Looks Good
- POSIX
casevalidation — portable acrossshimplementations, correctly handles: empty string, non-numeric, negative numbers, shell injection attempts (0; rm -rf /), and out-of-range values - 1000–65534 range — blocks UID 0 (root) entirely, allows the hermes default (10000), common NAS PUID values (>=1000), and
nobody(65534). The 65534 ceiling also blocks 65535 ((unsigned short)-1sentinel) &&chaining — ifvalidate_uid_gidfails, the remap is silently skipped (better than failing loudly for backward compatibility)- PUID/PGID aliases — naturally covered since they resolve to HERMES_UID/HERMES_GID before the validation check
sh -nclean — syntax verified
💡 Suggestion (for follow-up, not blocking)
- Consider logging when validation rejects a value (e.g.,
echo "[stage2] Warning: invalid HERMES_UID=$HERMES_UID -- must be numeric 1000-65534; skipping UID remap"). Currently a rejected value is silently ignored, which could be confusing for users who setHERMES_UID=0expecting it to work.
Testing Completeness
The PR body lists thorough E2E validation against a variety of inputs:
- REJECTED:
0, empty,0; rm -rf /,abc,999,65535,-5,1e4 - ACCEPTED:
1000,10000,1001,65534
All correct. The 1e4 rejection is a nice catch — scientific notation passes *[!0-9]* but usermod would fail.
Reviewed by Hermes Agent
Code Review Summary: PR #35340Verdict: APPROVE ✅ Overall: Clean, focused security fix. 10 lines added, zero bugs. This is exactly the right approach — validate at the entry point rather than downstream. ✅ Looks Good
💡 Suggestion (for follow-up, not blocking)
Testing CompletenessThe PR body lists thorough E2E validation against a variety of inputs:
All correct. The Reviewed by Hermes Agent |
…in stage2-hook (NousResearch#35340) Co-authored-by: sprmn24 <oncuevtv@gmail.com>
Summary
HERMES_UID=0(or any non-numeric / out-of-range value) can no longer be passed straight tousermod/groupmodin the Docker stage2 hook — which previously would silently grant thehermesruntime user root (UID 0). UID/GID remap now requires a numeric value in the range 1000–65534.Root cause:
HERMES_UID/HERMES_GID(and theirPUID/PGIDaliases) were forwarded tousermod -u/groupmod -gwith no validation.Changes
docker/stage2-hook.sh: addvalidate_uid_gid()(POSIXcasenumeric check + range bounds); gate both the UID and GID remap conditionals on it.Validation
sh -nclean.0, empty,0; rm -rf /,abc,999,65535,-5,1e41000,10000(hermes default),1001(typical NAS PUID),65534The 1000 floor blocks UID 0 (the goal) while leaving the default UID 10000 and conventional NAS
PUID/PGIDvalues (≥1000) working.Salvaged from #34119 by @sprmn24 — authorship preserved. Conflict resolved against the PUID/PGID alias block added to
mainafter the PR was opened; validation applied to both UID and GID conditionals.Infographic