Fix Softnet SUID setup commands - #1273
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 81cba367a6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
81cba36 to
8142831
Compare
There was a problem hiding this comment.
Hello Minh 👋
Could you clarify the threat model this is intended to address?
The implementation appears to treat PATH as untrusted while treating paths under the Homebrew Cellar as trusted. On a typical installation, however, the Homebrew prefix is user-owned, so an actor with same-user filesystem access could potentially construct or replace a path that satisfies these checks.
The validation also prevents using a locally built softnet through PATH.
Would you be open to narrowing the PR to the useful correctness improvements that do not introduce this new trust policy?
In particular, probing the exact resolved softnet avoids checking a different binary:
-process.arguments = ["--non-interactive", "softnet", "--help"]
+process.arguments = ["--non-interactive", softnetExecutablePath, "--help"]The removal of sh -c in favor of passing arguments directly through runInteractiveSudo also seems worth keeping.
8142831 to
62b9f9d
Compare
|
I removed the Homebrew/path validation and narrowed the PR to using the resolved Softnet path for the sudo check and running chown/chmod directly instead of through sh -c. |
|
@fallintoplace thanks for the updates, I still think this PR could be simplified quite a bit. Could you please address the original question in #1273 (review)? |
64aa1dd to
62b9f9d
Compare
|
The threat I had in mind was a manipulated PATH causing Tart to resolve an unexpected Softnet binary, which could then be configured during the sudo setup if the user approved the prompt. I think that scenario is possible, but treating the user-owned Homebrew prefix as trusted does not address it correctly. I removed that validation and narrowed the PR to using the same resolved Softnet path consistently and avoiding sh -c when running chown and chmod. Let me know if further adjustments are needed. |
Summary
chownandchmodarguments directly instead of interpolating the executable path intosh -cValidation
swift buildswift test