Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

Merged
ethanndickson merged 1 commit intomainfromethan/validate-slim-binary-version
Aug 6, 2025

Conversation

@ethanndickson
Copy link
Member

@ethanndicksonethanndickson commentedJul 31, 2025•
edited
Loading

Relates to#201.

After we've validated the binary signature, we execcoder version --output=json to 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, whenposix_spawning the slim binary from an unsandboxed LaunchDaemon, is this check absolutely necessary.

@ethanndicksonGraphite App
Copy link
MemberAuthor

ethanndickson commentedJul 31, 2025•
edited
Loading

@ethanndicksonethanndickson marked this pull request as ready for reviewJuly 31, 2025 07:27
@matifali
Copy link
Member

Not directly related but@jdomeracki-coder should we also implement binary verification in Coderd Desktop?

cc:@deansheather

@ethanndickson
Copy link
MemberAuthor

ethanndickson commentedJul 31, 2025•
edited
Loading

@matifali We already check the code signature for bothmac and Windows, it was part of the RFC.
It's even more important for Coder Desktop because the process runs as root.

matifali reacted with thumbs up emoji

@ethanndicksonethanndickson self-assigned thisJul 31, 2025
@ethanndicksonethanndicksonforce-pushed theethan/validate-slim-binary-version branch from929c831 to262c4d1CompareJuly 31, 2025 08:07

letversion:String
do{
letversionOutput=tryawaitSubprocess.data(for:[binaryPath.path,"version","--output=json"])
Copy link
Member

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...

Copy link
MemberAuthor

@ethanndicksonethanndicksonAug 4, 2025•
edited
Loading

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/whoami

One 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 permitted

does 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.

Copy link
Member

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

@ethanndicksonethanndicksonforce-pushed theethan/validate-slim-binary-version branch from262c4d1 toa7b16eaCompareAugust 4, 2025 02:59
@ethanndicksonethanndicksonforce-pushed theethan/validate-slim-binary-version branch froma7b16ea to9695afeCompareAugust 4, 2025 03:00
@ethanndicksonethanndicksonforce-pushed theethan/validate-slim-binary-version branch 2 times, most recently fromc229789 toa723a9aCompareAugust 4, 2025 07:58
@ethanndicksonethanndicksonforce-pushed theethan/validate-slim-binary-version branch froma723a9a toffe7d2eCompareAugust 4, 2025 08:05
@ethanndicksonethanndicksonforce-pushed theethan/validate-slim-binary-version branch fromffe7d2e toab9ca27CompareAugust 4, 2025 08:07
@ethanndicksonethanndicksonforce-pushed theethan/validate-slim-binary-version branch fromab9ca27 to50e4a63CompareAugust 4, 2025 09:42
@ethanndicksonethanndicksonforce-pushed theethan/validate-slim-binary-version branch from50e4a63 to8fb9382CompareAugust 4, 2025 12:53
@ethanndicksonethanndicksonforce-pushed theethan/validate-slim-binary-version branch from8fb9382 to431149fCompareAugust 5, 2025 04:51
Copy link

CopilotAI left a 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 executingcoder 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.

FileDescription
Coder-Desktop/VPNLib/Validate.swiftRefactors validation logic, adds version validation function, and enhances certificate chain requirements
Coder-Desktop/Coder-DesktopHelper/Manager.swiftUpdates to use new two-step validation process (signature then version)

@ethanndicksonethanndicksonforce-pushed theethan/validate-slim-binary-version branch from8fceeab to62c3d0aCompareAugust 6, 2025 03:38
@ethanndicksonGraphite App
Copy link
MemberAuthor

ethanndickson commentedAug 6, 2025•
edited
Loading

Merge activity

  • Aug 6, 3:39 AM UTC: A user started a stack merge that includes this pull request viaGraphite.
  • Aug 6, 3:52 AM UTC:Graphite rebased this pull request as part of a merge.
  • Aug 6, 3:53 AM UTC:@ethanndickson merged this pull request withGraphite.

@ethanndicksonethanndickson changed the base branch fromethan/slim-over-dylib tographite-base/211August 6, 2025 03:48
@ethanndicksonethanndickson changed the base branch fromgraphite-base/211 tomainAugust 6, 2025 03:50
@ethanndicksonethanndicksonforce-pushed theethan/validate-slim-binary-version branch from62c3d0a to7c6b9a7CompareAugust 6, 2025 03:51
@ethanndicksonethanndickson merged commitb74def3 intomainAug 6, 2025
4 checks passed
@ethanndicksonethanndickson deleted the ethan/validate-slim-binary-version branchAugust 6, 2025 03:53
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

Copilot code reviewCopilotCopilot left review comments

@deansheatherdeansheatherdeansheather approved these changes

Assignees

@ethanndicksonethanndickson

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ethanndickson@matifali@deansheather
[8]ページ先頭
©2009-2025 Movatter.jp