[ovs-dev] Fix socket permissions on Linux
diff mbox series

Message ID 1534449309-21642-1-git-send-email-twilson@redhat.com
State Deferred
Headers show
Series
  • [ovs-dev] Fix socket permissions on Linux
Related show

Commit Message

Terry Wilson Aug. 16, 2018, 7:55 p.m. UTC
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(-)

Comments

Ben Pfaff Aug. 16, 2018, 9:57 p.m. UTC | #1
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.
Terry Wilson Aug. 16, 2018, 10:21 p.m. UTC | #2
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
Terry Wilson Aug. 16, 2018, 10:46 p.m. UTC | #3
>> 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
Aaron Conole Aug. 16, 2018, 10:58 p.m. UTC | #4
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
Ben Pfaff Aug. 16, 2018, 11:11 p.m. UTC | #5
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.
Ben Pfaff Aug. 16, 2018, 11:13 p.m. UTC | #6
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.
Terry Wilson Aug. 16, 2018, 11:51 p.m. UTC | #7
> 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
Terry Wilson Aug. 17, 2018, 3:38 a.m. UTC | #8
>> 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
Ben Pfaff Aug. 17, 2018, 5:34 p.m. UTC | #9
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.
Aaron Conole Aug. 20, 2018, 3:08 p.m. UTC | #10
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?
Ben Pfaff Aug. 20, 2018, 4:29 p.m. UTC | #11
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.

Patch
diff mbox series

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;
     }
 }