| Message ID | 1534449309-21642-1-git-send-email-twilson@redhat.com |
|---|---|
| State | Deferred |
| Headers | show |
| Series | [ovs-dev] Fix socket permissions on Linux | expand |
On Thu, Aug 16, 2018 at 07:55:09PM +0000, Terry Wilson wrote: > Unix sockets were not being created with the permission 0770, > instead using the current umask value. The manpage for fchmod() > states that that if filedes refers to a socket, the behavior is > undefined. Insetad, use the same code as *BSD to ensure the 0770 > permission is set on unix sockets. > > Signed-off-by: Terry Wilson <twilson@redhat.com> It's extraordinarily expensive to fork() to make a single system call. As far as I can tell, the existing code actually works on Linux, in the same way as the third 'mode' parameter to open(2). Surely there's a better way to do this.
On Thu, Aug 16, 2018 at 4:57 PM, Ben Pfaff <blp@ovn.org> wrote: > On Thu, Aug 16, 2018 at 07:55:09PM +0000, Terry Wilson wrote: >> Unix sockets were not being created with the permission 0770, >> instead using the current umask value. The manpage for fchmod() >> states that that if filedes refers to a socket, the behavior is >> undefined. Insetad, use the same code as *BSD to ensure the 0770 >> permission is set on unix sockets. >> >> Signed-off-by: Terry Wilson <twilson@redhat.com> > > It's extraordinarily expensive to fork() to make a single system call. I agree it is ridiculously ugly, though it isn't like this is something that is done in a tight loop anywhere either. > As far as I can tell, the existing code actually works on Linux, in the > same way as the third 'mode' parameter to open(2). It doesn't (and never has) on my Centos 7 machine. I ran into this a couple of years ago and ended up just working around it. As an example after make rpm-fedora and installing: [centos@test x86_64]$ ls -al /var/run/openvswitch/db.sock srwxr-x---. 1 openvswitch openvswitch 0 Aug 16 22:09 db.sock So we've got 0750 and not 0770 like the hardcoded value in the source. > Surely there's a better way to do this. I *hope* so. I mean it certainly seems like something one would want to be able to do, but I remember looking for a couple of days 2 years ago and giving up. umask seemed like the only reliable option. Whatever the solution is, fchmod is *not* it since it is specifically undefined behavior to use it on a socket. I'll try with ubuntu and see what happens there, but wouldn't imagine it to be different. From man 3 fchmod: DESCRIPTION ... If fildes refers to a socket, the behavior of fchmod() is unspecified. ... Terry
>> Surely there's a better way to do this. > > I *hope* so. I mean it certainly seems like something one would want > to be able to do, but I remember looking for a couple of days 2 years > ago and giving up. umask seemed like the only reliable option. > Whatever the solution is, fchmod is *not* it since it is specifically > undefined behavior to use it on a socket. I'll try with ubuntu and see > what happens there, but wouldn't imagine it to be different. BTW, I should mention "reliable" above is assuming you want to set permissions before the file shows up on the filesystem. using chmod on the bind_path works, it's just that theoretically the file could be moved, etc. Terry
Terry Wilson <twilson@redhat.com> writes: > On Thu, Aug 16, 2018 at 4:57 PM, Ben Pfaff <blp@ovn.org> wrote: >> On Thu, Aug 16, 2018 at 07:55:09PM +0000, Terry Wilson wrote: >>> Unix sockets were not being created with the permission 0770, >>> instead using the current umask value. The manpage for fchmod() >>> states that that if filedes refers to a socket, the behavior is >>> undefined. Insetad, use the same code as *BSD to ensure the 0770 >>> permission is set on unix sockets. >>> >>> Signed-off-by: Terry Wilson <twilson@redhat.com> >> >> It's extraordinarily expensive to fork() to make a single system call. > > I agree it is ridiculously ugly, though it isn't like this is > something that is done in a tight loop anywhere either. > >> As far as I can tell, the existing code actually works on Linux, in the >> same way as the third 'mode' parameter to open(2). > > It doesn't (and never has) on my Centos 7 machine. I ran into this a > couple of years ago and ended up just working around it. As an example > after make rpm-fedora and installing: > [centos@test x86_64]$ ls -al /var/run/openvswitch/db.sock > srwxr-x---. 1 openvswitch openvswitch 0 Aug 16 22:09 db.sock > > So we've got 0750 and not 0770 like the hardcoded value in the source. > >> Surely there's a better way to do this. > > I *hope* so. I mean it certainly seems like something one would want > to be able to do, but I remember looking for a couple of days 2 years > ago and giving up. umask seemed like the only reliable option. > Whatever the solution is, fchmod is *not* it since it is specifically > undefined behavior to use it on a socket. I'll try with ubuntu and see > what happens there, but wouldn't imagine it to be different. So... Gather 'round folks, and let me tell you the tale of a series long ago posted: https://mail.openvswitch.org/pipermail/ovs-dev/2016-August/321866.html Something... something ... black magic... I think the fchmod needs to happen after the bind for the permissions to actually be changed. That's how the unit tests in that series are coded. > From man 3 fchmod: > DESCRIPTION > ... > If fildes refers to a socket, the behavior of fchmod() is unspecified. > ... I think that's because some unixes don't even honor permissions on sockets, and some don't allow any changing of those permissions. > Terry
On Thu, Aug 16, 2018 at 05:21:28PM -0500, Terry Wilson wrote: > On Thu, Aug 16, 2018 at 4:57 PM, Ben Pfaff <blp@ovn.org> wrote: > > On Thu, Aug 16, 2018 at 07:55:09PM +0000, Terry Wilson wrote: > >> Unix sockets were not being created with the permission 0770, > >> instead using the current umask value. The manpage for fchmod() > >> states that that if filedes refers to a socket, the behavior is > >> undefined. Insetad, use the same code as *BSD to ensure the 0770 > >> permission is set on unix sockets. > >> > >> Signed-off-by: Terry Wilson <twilson@redhat.com> > > > > It's extraordinarily expensive to fork() to make a single system call. > > I agree it is ridiculously ugly, though it isn't like this is > something that is done in a tight loop anywhere either. > > > As far as I can tell, the existing code actually works on Linux, in the > > same way as the third 'mode' parameter to open(2). > > It doesn't (and never has) on my Centos 7 machine. I ran into this a > couple of years ago and ended up just working around it. As an example > after make rpm-fedora and installing: > [centos@test x86_64]$ ls -al /var/run/openvswitch/db.sock > srwxr-x---. 1 openvswitch openvswitch 0 Aug 16 22:09 db.sock > > So we've got 0750 and not 0770 like the hardcoded value in the source. Presumably, that's 0770 modulo the umask, which is the same thing you get out of the third argument to open(). I guess your umask is 0022. If we need exactly 0770 then it's harder and we probably have to > > Surely there's a better way to do this. > > I *hope* so. I mean it certainly seems like something one would want > to be able to do, but I remember looking for a couple of days 2 years > ago and giving up. umask seemed like the only reliable option. > Whatever the solution is, fchmod is *not* it since it is specifically > undefined behavior to use it on a socket. I'll try with ubuntu and see > what happens there, but wouldn't imagine it to be different. > > From man 3 fchmod: > DESCRIPTION > ... > If fildes refers to a socket, the behavior of fchmod() is unspecified. That's what POSIX says, at http://pubs.opengroup.org/onlinepubs/9699919799/functions/fchmod.html. This particular behavior has been in Linux forever as far as I can tell.
On Thu, Aug 16, 2018 at 06:58:54PM -0400, Aaron Conole wrote: > Terry Wilson <twilson@redhat.com> writes: > > > On Thu, Aug 16, 2018 at 4:57 PM, Ben Pfaff <blp@ovn.org> wrote: > >> On Thu, Aug 16, 2018 at 07:55:09PM +0000, Terry Wilson wrote: > >>> Unix sockets were not being created with the permission 0770, > >>> instead using the current umask value. The manpage for fchmod() > >>> states that that if filedes refers to a socket, the behavior is > >>> undefined. Insetad, use the same code as *BSD to ensure the 0770 > >>> permission is set on unix sockets. > >>> > >>> Signed-off-by: Terry Wilson <twilson@redhat.com> > >> > >> It's extraordinarily expensive to fork() to make a single system call. > > > > I agree it is ridiculously ugly, though it isn't like this is > > something that is done in a tight loop anywhere either. > > > >> As far as I can tell, the existing code actually works on Linux, in the > >> same way as the third 'mode' parameter to open(2). > > > > It doesn't (and never has) on my Centos 7 machine. I ran into this a > > couple of years ago and ended up just working around it. As an example > > after make rpm-fedora and installing: > > [centos@test x86_64]$ ls -al /var/run/openvswitch/db.sock > > srwxr-x---. 1 openvswitch openvswitch 0 Aug 16 22:09 db.sock > > > > So we've got 0750 and not 0770 like the hardcoded value in the source. > > > >> Surely there's a better way to do this. > > > > I *hope* so. I mean it certainly seems like something one would want > > to be able to do, but I remember looking for a couple of days 2 years > > ago and giving up. umask seemed like the only reliable option. > > Whatever the solution is, fchmod is *not* it since it is specifically > > undefined behavior to use it on a socket. I'll try with ubuntu and see > > what happens there, but wouldn't imagine it to be different. > > So... > > Gather 'round folks, and let me tell you the tale of a series long > ago posted: > > https://mail.openvswitch.org/pipermail/ovs-dev/2016-August/321866.html Wow, I don't remember reading that patch at all. That's unusual, for me.
> Gather 'round folks, and let me tell you the tale of a series long > ago posted: > > https://mail.openvswitch.org/pipermail/ovs-dev/2016-August/321866.html > > Something... something ... black magic... > I think the fchmod needs to happen after the bind for the permissions > to actually be changed. That's how the unit tests in that series are > coded. If you move fchmod after the bind, you get 0755 and not 0750 on my system. It seems weird fchmod interacting with umask since it seems like the whole purpose of chmod/fchmod are to change permissions ignoring umask. You certainly aren't going to get the same behavior with fchmod on a regular file anyway. It seemed like the patch that specifically changed 0700 -> 0770 intended group permissions to always be there. I can see arguments both ways, though. The problem with modifying umask before calling is that you are changing the permissions for every created file (log files, etc.). Maybe that isn't a problem, I just know that it was something that came up in our internal discussions hacking around things with umask. Terry
>> It doesn't (and never has) on my Centos 7 machine. I ran into this a >> couple of years ago and ended up just working around it. As an example >> after make rpm-fedora and installing: >> [centos@test x86_64]$ ls -al /var/run/openvswitch/db.sock >> srwxr-x---. 1 openvswitch openvswitch 0 Aug 16 22:09 db.sock >> >> So we've got 0750 and not 0770 like the hardcoded value in the source. > > Presumably, that's 0770 modulo the umask, which is the same thing you > get out of the third argument to open(). I guess your umask is 0022. Ok, so fchmod() followed by bind() is similar to 3-arg open(). That makes sense to me. Still a little confused about fchmod() after bind() not doing anything when chmod does and they both eventually call into chmod_common(), with fchmod() just grabbing the file path to pass in to chmod_common() > If we need exactly 0770 then it's harder and we probably have to To me this seems very beneficial to have since setting umask before calling ovsdb-server means that log files etc. also get created with different permissions. In any case, the behavior is different between the Linux and *BSD implementations of bind_unix_socket() since Linux takes does mode&~umask and the*BSD version does umask = mode. Thanks for the explanation so far, anyway. This is surprisingly confusing. Terry
On Thu, Aug 16, 2018 at 10:38:01PM -0500, Terry Wilson wrote: > >> It doesn't (and never has) on my Centos 7 machine. I ran into this a > >> couple of years ago and ended up just working around it. As an example > >> after make rpm-fedora and installing: > >> [centos@test x86_64]$ ls -al /var/run/openvswitch/db.sock > >> srwxr-x---. 1 openvswitch openvswitch 0 Aug 16 22:09 db.sock > >> > >> So we've got 0750 and not 0770 like the hardcoded value in the source. > > > > Presumably, that's 0770 modulo the umask, which is the same thing you > > get out of the third argument to open(). I guess your umask is 0022. > > Ok, so fchmod() followed by bind() is similar to 3-arg open(). That > makes sense to me. Still a little confused about fchmod() after bind() > not doing anything when chmod does and they both eventually call into > chmod_common(), with fchmod() just grabbing the file path to pass in > to chmod_common() I get the impression that there are two different entities, which are the socket and a file that references the socket. The socket has a mode that can be set with fchmod(), and bind() takes that mode and applies the umask to it and uses that as the initial mode for the file that it creates. The socket's mode is otherwise ignored, so that you can fchmod() the socket as much as you want afterward and there's no visible behavioral change. But chmod() gets to the file and changes its mode and that's what actually influences behavior. > > If we need exactly 0770 then it's harder and we probably have to > > To me this seems very beneficial to have since setting umask before > calling ovsdb-server means that log files etc. also get created with > different permissions. In any case, the behavior is different between > the Linux and *BSD implementations of bind_unix_socket() since Linux > takes does mode&~umask and the*BSD version does umask = mode. > > Thanks for the explanation so far, anyway. This is surprisingly confusing. It's bizarre.
Terry Wilson <twilson@redhat.com> writes: >> Gather 'round folks, and let me tell you the tale of a series long >> ago posted: >> >> https://mail.openvswitch.org/pipermail/ovs-dev/2016-August/321866.html >> >> Something... something ... black magic... >> I think the fchmod needs to happen after the bind for the permissions >> to actually be changed. That's how the unit tests in that series are >> coded. > > If you move fchmod after the bind, you get 0755 and not 0750 on my > system. It seems weird fchmod interacting with umask since it seems > like the whole purpose of chmod/fchmod are to change permissions > ignoring umask. You certainly aren't going to get the same behavior > with fchmod on a regular file anyway. It seemed like the patch that > specifically changed 0700 -> 0770 intended group permissions to always > be there. I can see arguments both ways, though. The problem with > modifying umask before calling is that you are changing the > permissions for every created file (log files, etc.). Maybe that isn't > a problem, I just know that it was something that came up in our > internal discussions hacking around things with umask. Strange. I've tested on regular files, and find that umask is *correctly* ignored. The test program I'm using: --- 8< --- #include <fcntl.h> #include <stdio.h> #include <sys/stat.h> #include <unistd.h> int main(int argc, char *argv[]) { /* Change the mode */ if (argc < 2) { printf("pass a filename\n"); return -1; } int fd = open(argv[1], O_RDWR); if (fd < 0) { perror("open"); return -1; } fchmod(fd, 0770); close(fd); return 0; } --- >8 --- output ----------- 10:42:20 aconole /tmp$ umask 0022 10:42:22 aconole /tmp$ touch tmp.foo 10:42:25 aconole /tmp$ ls -lah | grep tmp.foo -rw-r--r--. 1 aconole aconole 0 Aug 20 10:42 tmp.foo 10:42:29 aconole /tmp$ ./test tmp.foo 10:42:36 aconole /tmp$ ls -lah | grep tmp.foo -rwxrwx---. 1 aconole aconole 0 Aug 20 10:42 tmp.foo ----------- of course, /tmp has some other sticky bits set, but I tried in my own home directory, with the same results. Then, I played a bit with unix sockets. What I found was interesting. If I use fchmod on the actual socket, it seems not possible to adjust the permissions after creation time. That means post bind, the permissions are locked for fd, at least as far as fchmod goes. HOWEVER, if I independently call 'chmod(path, mode)' the mode bits are changed without factoring the umask (as I expect). There might need to be some additional work to get this to do the right thing under linux, but I think it's possible without going through the clone path. Looking at the old patches I had, I did a traversal and eventually fchmodat(), which probably does the right thing (but I haven't dusted those patches off to actually try them out). Ben, Terry, how would you feel if I did some minor necromancy on that code? Any objections?
On Mon, Aug 20, 2018 at 11:08:36AM -0400, Aaron Conole wrote: > Terry Wilson <twilson@redhat.com> writes: > > >> Gather 'round folks, and let me tell you the tale of a series long > >> ago posted: > >> > >> https://mail.openvswitch.org/pipermail/ovs-dev/2016-August/321866.html > >> > >> Something... something ... black magic... > >> I think the fchmod needs to happen after the bind for the permissions > >> to actually be changed. That's how the unit tests in that series are > >> coded. > > > > If you move fchmod after the bind, you get 0755 and not 0750 on my > > system. It seems weird fchmod interacting with umask since it seems > > like the whole purpose of chmod/fchmod are to change permissions > > ignoring umask. You certainly aren't going to get the same behavior > > with fchmod on a regular file anyway. It seemed like the patch that > > specifically changed 0700 -> 0770 intended group permissions to always > > be there. I can see arguments both ways, though. The problem with > > modifying umask before calling is that you are changing the > > permissions for every created file (log files, etc.). Maybe that isn't > > a problem, I just know that it was something that came up in our > > internal discussions hacking around things with umask. > > Strange. I've tested on regular files, and find that umask is *correctly* > ignored. The test program I'm using: > > --- 8< --- > #include <fcntl.h> > #include <stdio.h> > #include <sys/stat.h> > #include <unistd.h> > > int main(int argc, char *argv[]) > { > /* Change the mode */ > if (argc < 2) { > printf("pass a filename\n"); > return -1; > } > > int fd = open(argv[1], O_RDWR); > if (fd < 0) { > perror("open"); > return -1; > } > > fchmod(fd, 0770); > close(fd); > return 0; > } > --- >8 --- > > output > ----------- > > 10:42:20 aconole /tmp$ umask > 0022 > 10:42:22 aconole /tmp$ touch tmp.foo > 10:42:25 aconole /tmp$ ls -lah | grep tmp.foo > -rw-r--r--. 1 aconole aconole 0 Aug 20 10:42 tmp.foo > 10:42:29 aconole /tmp$ ./test tmp.foo > 10:42:36 aconole /tmp$ ls -lah | grep tmp.foo > -rwxrwx---. 1 aconole aconole 0 Aug 20 10:42 tmp.foo > > ----------- > > of course, /tmp has some other sticky bits set, but I tried in my own > home directory, with the same results. > > Then, I played a bit with unix sockets. What I found was interesting. > > If I use fchmod on the actual socket, it seems not possible to adjust > the permissions after creation time. That means post bind, the > permissions are locked for fd, at least as far as fchmod goes. > > HOWEVER, if I independently call 'chmod(path, mode)' the mode bits are > changed without factoring the umask (as I expect). There might need to > be some additional work to get this to do the right thing under linux, > but I think it's possible without going through the clone path. Looking > at the old patches I had, I did a traversal and eventually fchmodat(), > which probably does the right thing (but I haven't dusted those patches > off to actually try them out). > > Ben, Terry, how would you feel if I did some minor necromancy on that > code? Any objections? If you can do something that avoids fork and path-traversal issues, that would be great.
diff --git a/lib/socket-util-unix.c b/lib/socket-util-unix.c index 59f63fc..54147d1 100644 --- a/lib/socket-util-unix.c +++ b/lib/socket-util-unix.c @@ -263,42 +263,28 @@ static int bind_unix_socket(int fd, struct sockaddr *sun, socklen_t sun_len) { const mode_t mode = 0770; /* Allow both user and group access. */ - if (LINUX) { - /* On Linux, the fd's permissions become the file's permissions. - * fchmod() does not affect other files, like umask() does. */ - if (fchmod(fd, mode)) { - return errno; - } - - /* Must be after fchmod(). */ - if (bind(fd, sun, sun_len)) { - return errno; - } - return 0; + /* On unix sockets, only the umask affects permissions. The + * umask is process-wide rather than thread-specific, so we have to use + * a subprocess for safety. */ + pid_t pid = fork(); + + if (!pid) { + umask(mode ^ 0777); + _exit(bind(fd, sun, sun_len) ? errno : 0); + } else if (pid > 0) { + int status; + int error; + + do { + error = waitpid(pid, &status, 0) < 0 ? errno : 0; + } while (error == EINTR); + + return (error ? error + : WIFEXITED(status) ? WEXITSTATUS(status) + : WIFSIGNALED(status) ? EINTR + : ECHILD /* WTF? */); } else { - /* On FreeBSD and NetBSD, only the umask affects permissions. The - * umask is process-wide rather than thread-specific, so we have to use - * a subprocess for safety. */ - pid_t pid = fork(); - - if (!pid) { - umask(mode ^ 0777); - _exit(bind(fd, sun, sun_len) ? errno : 0); - } else if (pid > 0) { - int status; - int error; - - do { - error = waitpid(pid, &status, 0) < 0 ? errno : 0; - } while (error == EINTR); - - return (error ? error - : WIFEXITED(status) ? WEXITSTATUS(status) - : WIFSIGNALED(status) ? EINTR - : ECHILD /* WTF? */); - } else { - return errno; - } + return errno; } }
Unix sockets were not being created with the permission 0770, instead using the current umask value. The manpage for fchmod() states that that if filedes refers to a socket, the behavior is undefined. Insetad, use the same code as *BSD to ensure the 0770 permission is set on unix sockets. Signed-off-by: Terry Wilson <twilson@redhat.com> --- lib/socket-util-unix.c | 56 +++++++++++++++++++------------------------------- 1 file changed, 21 insertions(+), 35 deletions(-)