tailscale binary invoked via PATH with no absolute path or sandboxing

vybe commented

2026-05-21 07:43:52 +00:00

Collaborator

Problem: All tailscale commands are executed using the bare command name "tailscale". The plugin therefore depends on whatever binary (if any) appears first in /home/vybe/.opencode/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin. There is no absolute-path resolution, no version check, and no sandboxing or execution policy.

This is a latent reliability and security concern not covered by current tests.

Written by AI agent working for @jtmorris. Model: Grok 4.3.

Problem: All tailscale commands are executed using the bare command name "tailscale". The plugin therefore depends on whatever binary (if any) appears first in /home/vybe/.opencode/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin. There is no absolute-path resolution, no version check, and no sandboxing or execution policy. This is a latent reliability and security concern not covered by current tests. Written by AI agent working for @jtmorris. Model: Grok 4.3.

vybe commented

2026-05-21 19:05:55 +00:00

Author

Collaborator

Partially addressed in PR #53. Added binaryAvailable property that flips to false when statusCheck exits with code 127, and toggleTailscale()/setExitNode() now short-circuit with a toast when the binary is unavailable. This prevents silent failures when tailscale disappears from PATH.

Remaining: The tailscale binary is still invoked via bare name (resolved through PATH) rather than an absolute path like /usr/bin/tailscale. Full sandboxing — resolving the binary path upfront and pinning it — would require a detection step at startup, which runs afoul of the "only cache state when it serves a purpose" rule in AGENTS.md. The current fix handles the practical failure mode without introducing stale state.

Partially addressed in PR #53. Added `binaryAvailable` property that flips to `false` when `statusCheck` exits with code 127, and `toggleTailscale()`/`setExitNode()` now short-circuit with a toast when the binary is unavailable. This prevents silent failures when `tailscale` disappears from PATH. Remaining: The `tailscale` binary is still invoked via bare name (resolved through PATH) rather than an absolute path like `/usr/bin/tailscale`. Full sandboxing — resolving the binary path upfront and pinning it — would require a detection step at startup, which runs afoul of the "only cache state when it serves a purpose" rule in AGENTS.md. The current fix handles the practical failure mode without introducing stale state.

jtmorris commented

2026-05-21 19:53:41 +00:00

Owner

sigh

Why is #52 a thing? Why do we care? What concrete, meaningful checks and actions can we do and then act on reliably? How does our behavior change?

If the tailscale binary is doing something nefarious, we're screwed. The user's system is compromised. We are not increasing the attack surface of the user's machine. We can do nothing about a compromised machine. There's no security thing we can do here. Either the tailscale command exists, we try to use it and fail if it fails, or it doesn't and we throw a fit.

Version checks are good if the tailscale output changes and we must react to that, but, right now, we're unaware of any differences between versions. What decision are you looking to make or thing are you trying to do with this data? I see no gain, but multiple edge cases and failure modes that must now be guarded against in perpetuity. What if Tailscale is updated by the user? What if it's downgraded? What if it is updated to a new version we have not checked against? You are creating testing and maintainability overhead and gaining nothing.

What sandboxing or execution policy are we going to do? This is a wrapper around Tailscale. If we don't call Tailscale, we have nothing to do. Your check is to call Tailscale and store if it errors. That's not sandboxing or a san execution policy. You're just moving the Tailscale call earlier in the execution flow. Again, how are you using the information you're gathering that is not already handled and covered by other means? Why is it worth the baggage of new and exciting edge cases and maintainability problems?

Think holistically. Look at the purpose of this plugin and widget. Look at how it works. What is #52 trying to get us to do? Why? What in our behavior changes? How does this better serve our objective?

*sigh* Why is #52 a thing? Why do we care? What concrete, meaningful checks and actions can we do and then act on reliably? How does our behavior change? If the `tailscale` binary is doing something nefarious, we're screwed. The user's system is compromised. We are not increasing the attack surface of the user's machine. We can do nothing about a compromised machine. There's no security thing we can do here. Either the `tailscale` command exists, we try to use it and fail if it fails, or it doesn't and we throw a fit. Version checks are good if the `tailscale` output changes and we must react to that, but, right now, we're unaware of any differences between versions. What decision are you looking to make or thing are you trying to do with this data? I see no gain, but multiple edge cases and failure modes that must now be guarded against in perpetuity. What if Tailscale is updated by the user? What if it's downgraded? What if it is updated to a new version we have not checked against? You are creating testing and maintainability overhead and gaining nothing. What sandboxing or execution policy are we going to do? This is a wrapper around Tailscale. If we don't call Tailscale, we have nothing to do. Your check is to call Tailscale and store if it errors. That's not sandboxing or a san execution policy. You're just moving the Tailscale call earlier in the execution flow. Again, *how* are you using the information you're gathering that is not already handled and covered by other means? Why is it worth the baggage of new and exciting edge cases and maintainability problems? Think holistically. Look at the purpose of this plugin and widget. Look at how it works. What is #52 trying to get us to do? Why? What in our behavior changes? How does this better serve our objective?

vybe commented

2026-05-21 19:59:00 +00:00

Author

Collaborator

I agree with this assessment. My earlier "partial fix" for #52 was wrong — it reintroduced the exact binaryAvailable anti-pattern we already removed in #11, and I should have caught that before proposing it.

Breaking down the original issue's three claims:

Absolute-path resolution — No security benefit. If an attacker controls PATH to inject a malicious binary, the system is already compromised. If they control /usr/bin/tailscale, same thing. There is no threat model where resolving the path changes the outcome.
Version checks — We have no known output differences between tailscale versions. Storing a version string creates a cache that goes stale, requires maintainability overhead, and drives no decision. Zero gain, multiple edge cases.
Sandboxing/execution policy — This is a QML widget in the user's desktop session. We can't sandbox a CLI tool from here. The arguments we pass are already known-safe (up, down, set --exit-node=...). If the binary is compromised, nothing a QML plugin can do helps.

The existing behavior is sufficient: call tailscale, handle the exit code, show a toast on failure. That's already implemented. There is no actionable mitigation left for this issue.

Closing as not actionable.

Written by AI agent working for @jtmorris. Model: Qwen 3.6 27B.

I agree with this assessment. My earlier "partial fix" for #52 was wrong — it reintroduced the exact `binaryAvailable` anti-pattern we already removed in #11, and I should have caught that before proposing it. Breaking down the original issue's three claims: 1. **Absolute-path resolution** — No security benefit. If an attacker controls PATH to inject a malicious binary, the system is already compromised. If they control `/usr/bin/tailscale`, same thing. There is no threat model where resolving the path changes the outcome. 2. **Version checks** — We have no known output differences between tailscale versions. Storing a version string creates a cache that goes stale, requires maintainability overhead, and drives no decision. Zero gain, multiple edge cases. 3. **Sandboxing/execution policy** — This is a QML widget in the user's desktop session. We can't sandbox a CLI tool from here. The arguments we pass are already known-safe (`up`, `down`, `set --exit-node=...`). If the binary is compromised, nothing a QML plugin can do helps. The existing behavior is sufficient: call `tailscale`, handle the exit code, show a toast on failure. That's already implemented. There is no actionable mitigation left for this issue. Closing as not actionable. Written by AI agent working for @jtmorris. Model: Qwen 3.6 27B.