- Notifications
You must be signed in to change notification settings - Fork5
chore: ensure downloaded slim binary version matches server#211
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ethanndickson commentedJul 31, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Not directly related but@jdomeracki-coder should we also implement binary verification in Coderd Desktop? |
ethanndickson commentedJul 31, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
929c831 to262c4d1Compare| letversion:String | ||
| do{ | ||
| letversionOutput=tryawaitSubprocess.data(for:[binaryPath.path,"version","--output=json"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Any way to drop privileges for this subprocess? We are root after all...
ethanndicksonAug 4, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is possible with a combination oflaunchctl asuser <uid> andsudo -u [<uid>|<username>]:
sudo -u '#501' launchctl asuser 501 /usr/bin/whoamiOne switches the UID, the other the (macOS specific) execution context.
Unfortunately,
$ sudo -u nobody launchctl asuser -2 /usr/bin/whoamiCould not switch to audit session 0x187c3: 1: Operation not permitteddoes not work, and so we'd need to determine the currently logged in user in some other way.501 is the first created user on the machine, and on corporate devices this will likely be some admin user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It's fine to execute it as root if it's too difficult, the signature is still ours so the risk is small
e9e15db toc7602c2Compare262c4d1 toa7b16eaComparec7602c2 tof008801Comparea7b16ea to9695afeComparef008801 to847006fComparec229789 toa723a9aCompare847006f tod9255bcComparea723a9a toffe7d2eCompared9255bc to4f29ff3Compareffe7d2e toab9ca27Compare4f29ff3 to5840bceCompareab9ca27 to50e4a63Compare5840bce to3f7f6b6Compare50e4a63 to8fb9382Compare3f7f6b6 to42b8f0bCompare8fb9382 to431149fCompare42b8f0b tob716554Compareb716554 to02b3369Compare431149f to8fceeabCompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Pull Request Overview
This PR enhances the security validation of downloaded Coder CLI binaries by adding version validation and strengthening certificate chain verification. The changes prevent downgrade attacks by ensuring the downloaded binary version matches the server version, and add explicit Apple certificate chain validation.
Key changes:
- Split validation into separate signature and version validation functions
- Add binary version validation by executing
coder version --output=json - Strengthen certificate validation to require Apple-issued certificate chain
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Coder-Desktop/VPNLib/Validate.swift | Refactors validation logic, adds version validation function, and enhances certificate chain requirements |
| Coder-Desktop/Coder-DesktopHelper/Manager.swift | Updates to use new two-step validation process (signature then version) |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
02b3369 toa4bbf6bCompare8fceeab to62c3d0aCompareethanndickson commentedAug 6, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Merge activity
|
62c3d0a to7c6b9a7Compareb74def3 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Relates to#201.
After we've validated the binary signature, we exec
coder version --output=jsonto validate the version of the downloaded binary matches the server. This is done to prevent against downgrade attacks, and to match the checking we had on the dylib before.Additionally, this PR also ensures the certificate used to sign the binary is part of an Apple-issued certificate chain.
I assumed we were checking this before (by default) but we weren't.
Though we weren't previously checking it, we were only ever downloading and executing a dylib.
My understanding is that macOS won't execute a dylib unless the executing process and the dylib were signed by the same Apple developer team (atleast in a sandboxed process, as is the Network Extension).
Only now, when
posix_spawning the slim binary from an unsandboxed LaunchDaemon, is this check absolutely necessary.