NewUnixSocketWithOpts(): reduce umask override time-window, document hack #75
Conversation
thaJeztah
force-pushed
the
move_umask
branch
3 times, most recently
from
b1278ba to
07a9292
Compare
sockets/unix_socket.go
Outdated
| umask := syscall.Umask(0777) | ||
| defer syscall.Umask(umask) |
There was a problem hiding this comment.
So, reading this again, I'm actually somewhat confused now; umask 0777 would mean "ignore all permissions"? Should this be 0000 (i.e.; don't mask anything?); is this actually needed, given that we first create the socket (umask applies to creating files), but after that we chmod (to which umask doesn't apply)?
$ docker run -it --rm alpine
/ # umask
0022
/ # umask 0777
/ # touch /mysocket && ls -la /mysocket
---------- 1 root root 0 Feb 15 11:21 /mysocket
/ # chmod 0777 /mysocket && ls -la /mysocket
-rwxrwxrwx 1 root root 0 Feb 15 11:21 /mysocketThere was a problem hiding this comment.
Digging a bit further again; moby/moby@dacae74 refactored this code to use a defer, but the original umask change was in moby/moby#5947, which only mentions "fix race condition during socket creation", and after asking for more details;
It's just a possible security race condition, no actual error.
After scratching my head a bit, I think the intent was to;
- set the umask to
0777so that socket is with0000permissions - chmod to
<desired mode>
Without the umask, the race condition is between step 1. and 2.; step 1. would create the socket with default permissions (and depend on umask), which could be "less restrictive" than "desired" (step 2.: the actual permissions)
As an alternative, the socket would have to be created with explicit 0000 permissions, but net.Listen() does not accept file permissions when creating (I know recall Simon mentioning that)
thaJeztah
force-pushed
the
move_umask
branch
from
331951c to
60fc337
Compare
thaJeztah
changed the title
|
@tiborvass @chris-crone @ndeloof ptal 🤗 |
…hack This hack was originally added in moby/moby@24c73ce, but was scarce on information, and this code was cause for some confusion. net.Listen does not allow for permissions to be set. As a result, when specifying custom permissions ("WithChmod()"), there is a short time between creating the socket and applying the permissions, during which the socket permissions are Less restrictive than desired. To work around this limitation of net.Listen(), we temporarily set the umask to 0777, which forces the socket to be created with 000 permissions (i.e.: no access for anyone). After that, WithChmod() must be used to set the desired permissions. This patch also removes the use of `defer` here, so that we can reset the umask to its original value as soon as possible. Ideally we'd be able to detect if WithChmod() was passed as an option, and skip changing umask if default permissions are used. Signed-off-by: Sebastiaan van Stijn <[email protected]>
2 participants
Relates to https://twitter.com/sferquel/status/1304071249870680066?s=20
t.b.h., still wondering what would happen if we just omitted this entirely, and used the umask as defined on the hostThis hack was originally added in moby/moby@24c73ce (moby/moby#5947), but was scarce on information, and this code was cause for some confusion.
net.Listen does not allow for permissions to be set. As a result, when specifying custom permissions ("WithChmod()"), there is a short time between creating the socket and applying the permissions, during which the socket permissions are Less restrictive than desired.
To work around this limitation of net.Listen(), we temporarily set the umask to 0777, which forces the socket to be created with 000 permissions (i.e.: no access for anyone). After that, WithChmod() must be used to set the desired permissions.
This patch also removes the use of
deferhere, so that we can reset the umask to its original value as soon as possible. Ideally we'd be able to detect if WithChmod() was passed as an option, and skip changing umask if default permissions are used.