[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Re: Vulnerability in pcs or is it in more generic code?

On Fri, 2022-09-09 at 22:41 +0200, Ola Lundqvist wrote:

> I see that I was not clear what I meant with "in general" :-)

Woops, sorry for the noise :)

> Here I found how the generic source code looks like:
> https://rubydoc.info/gems/thin/1.3.1/Thin%2FBackends%2FUnixServer:connect
> You can see the umask(0) there.
> That is what I think is insecure, not pcs itself.


> But I think Thin::Backends::UnixServer#connect is still insecure.

It looks like the issue was introduced in this pull request:


It sounds like unicorn might have the same issue as thin.

Looking at the unicorn code, it sets the default umask to 0
when the umask option is not set, so not quite as bad but still.

The justification for the default umask of 0 in unicorn is:

   #   Typically UNIX domain sockets are created with more liberal
   #   file permissions than the rest of the application.  By default,
   #   we create UNIX domain sockets to be readable and writable by
   #   all local users to give them the same accessibility as
   #   locally-bound TCP listeners.

So the choice of umask 0 is deliberate in unicorn and the thin folks
copied that choice and dropped the possibility of overriding it,
since they did't have an options argument for the function.

Since UNIX domain sockets are often used for situations that are not
like localhost TCP sockets, that was probably the wrong choice, but
the unicorn/thin folks are likely from the web developer community,
which is mostly focused on HTTP and TCP and often use localhost for
development, so it was the right choice within their bubble.

I feel like the APIs of both thin and unicorn need redesigning so that
the choice of umask to use must be made by the caller. Then the web
developer community can use 0 and others will choose a safe value.

Really the web developer community shouldn't use 0 either but I expect
that convincing them of that might not be possible.

I'm not sure how palatable the API change will be to thin/unicorn.



Attachment: signature.asc
Description: This is a digitally signed message part

Reply to: