boazsegev / iodine

iodine - HTTP / WebSockets Server for Ruby with Pub/Sub support
MIT License
912 stars 51 forks source link

unix socket not writeable due umask #136

Closed raxoft closed 1 year ago

raxoft commented 1 year ago

System Information

Description

The server socket is not writeable by default, as it seems to honor umask.

Testing code

umask 022
iodine -b /tmp/test.socket &
ls -l /tmp/test.socket

Expected behavior

The socket created by the server should be writeable by anyone, regardless of the current umask.

Actual behavior

The socket is created with the bits masked out by the umask. Despite the fact that I found explicit fchmod(fd, 0777) call in the fio library after the socket is created. For some reason the fchmod doesn't seem to take effect.

As workaround, one can set the umask to 0 prior launching iodine, but that is far from ideal solution.

boazsegev commented 1 year ago

Hi @raxoft ,

Thank you for opening this issue and pointing it out.

I couldn't really recreate the issue, as I test on Linux using Docker, so there's really only one user... but testing with ls -l helped.

I am pushing a patch that will hopefully fix the problem.

Two quick questions:

  1. Could you possibly describe your use-case?

    I am worried that opening the Unix socket to everyone (though this was my original intent) might be a source of security concerns. Perhaps limiting access to the socket (although originally unintended) is safer?

  2. Could you possibly test if the patch solves your issue before I post the update?

Thanks! Bo.

raxoft commented 1 year ago

Thanks for quick response. Unfortunately the fix doesn't work. The chmod apparently has the same effect as fchmod, i.e., none in this case. I guess the solution is to temporarily set umask to 0 and then restore it (the umask call conveniently returns the previous value exactly for this purpose). That's for example how puma does it.

As for your other question - my use case is bog standard IMO. A webserver app behind an nginx proxy. The app runs under a dedicated user, while the nginx runs under standard system web service account. That's as standard setup as there can be.

Regarding the security, this is really a non-issue. The other web servers such as puma or unicorn always create their socket as writeable by anyone by default. If you want to restrict access, it is much easier to put the socket in a dedicated directory (which is often done anyway) and restrict access to that, rather then relying on permissions (and ownership) of the socket itself which is created anew every time.

However, this brought to my attention another issue. I used /tmp for the sake of the test only, as no sane person would normally place the public socket into world writeable directory. But when testing your patch, I did ls -l /tmp (rather than just the full socket path) and to my surprise there were two sockets in the /tmp dir. One named as I did name it, but the other one appears to be some automatic socket created by facil.io library itself, called like facil-io-sock-12907 (the pid changes every time, of course). Now I don't know what purpose this socket serves, but I am 100% sure that's not a good idea to have it directly in /tmp. In fact, it appears in /tmp regardless of where my specified socket is. It is there even when iodine listens on a port. I know this is another issue, but while you are looking into the socket code, you may want to check this one as well.

Thanks again for looking into this. I am available to any further questions, just not necessarily instantly. Thanks.

raxoft commented 1 year ago

Without checking the code, I guess this might be the socket you somehow use for communicating between the parent and the workers. If that's the case and it really needs to be in /tmp, it should be put under its own directory in /tmp which is set to chmod 0700, and the socket should be created in there. And of course that dir name should be created in the way which meets all the rules of safe temp file creation (i.e., unguessable suffix, safe against any races, etc.). That's a whole topic on its own.

boazsegev commented 1 year ago

Hi @raxoft ,

Thank you for both testing and for the observations.

I pushed a new patch using umask (I'm not sure why chmod didn't do anything) and I also back ported some of the naming consideration (but not the implementation) for the pub/sub socket (the one in tmp).

I hope this solves your issue.

Indeed, as you have guessed, the other socket is the one used to exchange pub/sub data and facilitate IPC. The 0.8.x version has a 64bit random suffix and the socket is in the running folder... but my development of the 0.8.x version is somewhat slower than I had hoped, although it is as much fun as it always .

Cheers, Bo.

raxoft commented 1 year ago

Thanks @boazsegev, this patch works fine.

BTW, when you are now using umask call in that code, you may actually want to pass it in as a parameter, so the public socket uses 0777 while your internal socket uses 0700 (btw, I would personally stick to the octal notation, as seeing the access bits in hex feels completely alien). That would remedy some of the issues with this internal socket, in addition to the random bits you have added, which are definitely welcome, too.

Thanks again and looking forward to the update release.

raxoft commented 1 year ago

Actually, writing about octal notation above made me think about the code again, so I checked the diff now and you are using mask 0x1FF, which actually disables all bits. So it seems the chmod (which you kept in) is working after all - not sure why it didn't seem to work before, perhaps I just somehow messed up which version I ran during testing, sorry about that.

boazsegev commented 1 year ago

Thank you @raxoft. Patch was released.

raxoft commented 1 year ago

Thanks for the release. However, I am afraid that the internal fio socket should not be publicly writeable, as I mentioned above. Do you plan to change that or should I perhaps prepare a patch for that? Thanks.