Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create admin socket synchronously before privdrop #1201

Merged
merged 1 commit into from
Nov 11, 2024

Conversation

klemensn
Copy link
Contributor

@klemensn klemensn commented Nov 3, 2024

Creating UNIX sockets the listen() goroutine that races against the main
one dropping to an unprivileged user may cause startup failure when
privdrop happens before privileged filesystem access.

Setup or fail in New() and only do listen(2) in listen() to avoid this.

# yggdrasil -autoconf -user nobody
2024/11/03 21:15:27 Build name: yggdrasil-go
2024/11/03 21:15:27 Build version: 0.5.9
...
2024/11/03 21:15:27 Admin socket failed to listen: listen unix /var/run/yggdrasil.sock: bind: permission denied

Rerun, now the order is flipped:

# yggdrasil -autoconf -user nobody
2024/11/03 21:15:34 Build name: yggdrasil-go
2024/11/03 21:15:34 Build version: 0.5.9
[...]
2024/11/03 21:15:34 UNIX admin socket listening on /var/run/yggdrasil.sock
[...]

Fixes #927.

@klemensn
Copy link
Contributor Author

klemensn commented Nov 3, 2024

In a single-core VM the timing is different enough for above mentioned -autoconf -user nobody to never trigger, but disabling TUN and multicast, i.e. without extra work in between, it always fails:

# uname -a
Linux alpine 6.6.59-0-virt #1-Alpine SMP PREEMPT_DYNAMIC 2024-11-01 11:02:13 x86_64 Linux
# nproc
1
# yggdrasil -version
Build name: yggdrasil
Build version: 0.5.9
# yggdrasil -genconf -json | jq '.MulticastInterfaces = [] | .IfName = "none"' | yggdrasil -useconf -user nobody
2024/11/03 22:46:54 Build name: yggdrasil
2024/11/03 22:46:54 Build version: 0.5.9
2024/11/03 22:46:54 Your public key is 52f2d9aa58335096fcefc78440230d6d51ac84a14df5064f1b5f9ab293e58265
2024/11/03 22:46:54 Your IPv6 address is 201:b434:9956:9f32:bda4:c40:e1ee:ff73
2024/11/03 22:46:54 Your IPv6 subnet is 301:b434:9956:9f32::/64
2024/11/03 22:46:54 Admin socket failed to listen: listen unix /var/run/yggdrasil.sock: bind: permission denied

@klemensn
Copy link
Contributor Author

Anyone?

Introducing this synchronisation point allows me to also reliably drop privileges further via pledge(2) on OpenBSD, since it is then guaranteed that the socket is set up already, see #1193 (comment)

@neilalexander
Copy link
Member

Not ignoring this, I just haven't had the time for the last week to review PRs due to work and other things. I'll try to take a look later today.

Copy link
Member

@neilalexander neilalexander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if the better approach might just be to move some of the initialisation logic out of the listen() goroutine and into New(), such that by the time the function returns, we know if it succeeded or failed and the listener would already be created?

Creating UNIX sockets the listen() goroutine that races against the main
one dropping to an unprivileged user may cause startup failure when
privdrop happens before privileged filesystem access.

Setup or fail in New() and only do listen(2) in listen() to avoid this.
@klemensn klemensn changed the title Wait for admin goroutine to create control socket before changing user Create admin socket synchronously before privdrop Nov 11, 2024
@klemensn
Copy link
Contributor Author

I'm wondering if the better approach might just be to move some of the initialisation logic out of the listen() goroutine and into New(), such that by the time the function returns, we know if it succeeded or failed and the listener would already be created?

That also works, thanks; I've updated the PR.

Copy link
Member

@neilalexander neilalexander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

@neilalexander neilalexander merged commit 8346800 into yggdrasil-network:develop Nov 11, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants