Move PID file out of user-writable directory

[orlitzky]

In ed630ab the PID file was moved to /run/clamav. Since /run/clamav is typically writable by the clamav user (so that he can create the socket after dropping privileges), this reintroduces the vulnerability fixed in 5b168b5.

The socket needs to be writable by clamd after it starts, but the PID file doesn't. The PID should go in /run/clamd.pid, or better yet, @RUNSTATEDIR@/clamd.pid.

I mentioned this on the mailing list when the release was announced, but I don't want it to get lost.

[micahsnyder]

The actual clamd.pid filepath is set when you create your own clamd.conf file.

That commit just changed suggested filepaths. The intention had been to make it align with how the clamav docker image was using it, although now that I look I see we're actually using /tmp/*.pid, because people want to start the whole container as the clamav user instead of starting as root and dropping privs.

I think a good compromise would be to list both filepath options in the sample config with some explanation as to which you may choose.

For the one that requires root to run, we could switch to use @RUNSTATEDIR@, but only if we switch to have CMake generate the sample config in the configure-step of the build process.

[orlitzky]

I don't think the docker image needs a PID file at all. Problem solved :)

[micahsnyder]

TBH I don't know why the pidfiles are enabled for the docker image. But I am hesitant to go remove that. Someone probably found a way to use it. 🤷

[orlitzky]

More generally, the only situation where a PID file is useful is when you have a dumb init system (namely sysvinit) and a daemon that drops privileges and forks.

In the docker image there is no init system, you're just launching clamd in the foreground and forgetting about it, so the PID can't really be useful. And everybody else is using systemd or OpenRC who can track the PID themselves; again no PID file is needed in those cases.

In the only situation where a PID is actually needed, you want it to live in @RUNSTATEDIR@. (Almost) all other uses are cargo cult. And the ones that aren't are even more insane.

[Kangie]

I'm with @orlitzky here - I can't see any real value added by the PID file in docker and it will resolve this issue just to drop it.

[micahsnyder]

Incidentally, we do have one suggestion on using the pid file, here Cisco-Talos/clamav-docker#45

[orlitzky]

Incidentally, we do have one suggestion on using the pid file, here Cisco-Talos/clamav-docker#45

A PID is used there, but not a PID file. The clamdpid=$! line uses a magic variable to obtain the PID of the process that was just launched, and then wait $clamdpidpasses its value to wait. Since all of this is happening within the same script, there's no reason to store the PID in a persistent location to be retrieved at a later time from another process (as is the case with dumb init systems).

[micahsnyder]

Oh silly me. I didn't read close enough. I was in a bit of a scramble and just saw "pid" 🤦. So long as clamd's --foreground option is used that will also work okay.

[Kangie]

Hi Micah,

Any update / thoughts on this one? I've started patching our configs downstream but I'd really prefer to avoid maintaining that if we can come up with an acceptable solution for everyone :).

[Kangie]

@micahsnyder ping

[micahsnyder]

@Kangie @orlitzky would this make you feel better? Cisco-Talos/clamav-docker#69

[micahsnyder]

Sorry my phrasing wasn't great. I mean, would this change resolve your security concerns about the pid file in the Docker images?

[orlitzky]

I don't think either of us necessarily cares what goes on in docker, but it's certainly an improvement to skip the PID file there if it doesn't need one and if it simplifies the reasoning about PID files in general.

The problem is that the suggestion,

# This file will be owned by root, as long as clamd was started by root.
# It is recommended that the directory where this file is stored is
# also owned by root to keep other users from tampering with it.
# Default: disabled
#PidFile /run/clamav/clamd.pid

has some risk of creating a vulnerability if /run/clamav is writable by anyone other than root. It's a strictly worse choice than /run/clamd.pid:

• The permissions on /run are already safe
• The permissions on /run/clamav are likely to be unsafe if it just happens to exist; the main reason people create subdirectories of /run is to mess with the permissions
• Users don't have to worry about creating the subdirectory at boot
• It doesn't conflict with the example for the local socket

For the local socket, the sample config suggests

# Path to a local socket file the daemon will listen on.
# Default: disabled (must be specified by a user)
#LocalSocket /run/clamav/clamd.sock
#LocalSocket /tmp/clamd.sock

The first one will only work if /run/clamav is NOT safe for a PID file, because clamd needs to create the socket there after dropping permissions. And the second (which I am just now noticing), is just plain unsafe, because /tmp will typically be world-writable.

If you are very smart you can figure this stuff out for yourself but I can see no reason not to suggest /run for the PID file instead and save people the trouble.