Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Harden systemd services for freshclam and clamd #859

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

eternaltyro
Copy link

The default systemd service files clamav-freshclam and clamav-daemon (at least as installed in Arch Linux) do not contain any Sandboxing options configured. Therefore, the service units are marked as UNSAFE when systemd-analyze security[1] is run.

➜  ~ systemd-analyze security
UNIT                                 EXPOSURE PREDICATE HAPPY
..
clamav-daemon.service                     9.6 UNSAFE    😨
clamav-freshclam.service                  9.6 UNSAFE    😨
..

By including some basic security options[2], we can increase the security level of the services - even if only a little bit - from systemd's perspective.

For the daemon, I have used ProtectSystem and ProtectHome directives to make the filesystem read-only - which should be okay when scanning files. However, we have to make an exception for the log path using ReadWritePaths. Additionally, this configuration would interfere with the --remove option if the user chooses to auto-remove infected files.

Other options protect sysconfig, kernel modules, and the hardware clock.

Adverse effects are less pronounced if we apply similar configuration to freshclam service - which ostensibly only updates a specific set of files on disk. I referred to the Installation manual[3] to ascertain the default file path to which fresh databases are downloaded.

With all these changes applied, the result of systemd-analyze security looks like this:

➜  ~ systemd-analyze security
UNIT                                 EXPOSURE PREDICATE HAPPY
..
clamav-daemon.service                     7.8 EXPOSED   🙁
clamav-freshclam.service                  7.8 EXPOSED   🙁
..

While this seems like a definite improvement, we need to take user experience, distribution-specific defaults, and best-practices (as determined by ClamAV itself) to ensure that this changeset is valid.

[1] https://www.freedesktop.org/software/systemd/man/systemd-analyze.html
[2] https://www.freedesktop.org/software/systemd/man/systemd.exec.html
[3] https://docs.clamav.net/manual/Installing/Packages.html

@eternaltyro
Copy link
Author

Addresses issue #858

Copy link
Contributor

@micahsnyder micahsnyder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have essentially one concern, about documentation for people impacted by the NoExecPaths=.

In addition, I imagine we should add similar security options for the clamav-clamonacc.service.in file:
https://github.com/Cisco-Talos/clamav/blob/719a8b2feacbbb7e91ed57805640cd450a58ab22/clamonacc/clamav-clamonacc.service.in

Would you be interested in working on that one as well?

Everything else looks good to me. I'm a systemd novice though, so if anyone else sees something concerning or opportunities to improve it, please speak up.

clamd/clamav-daemon.service.in Show resolved Hide resolved
freshclam/clamav-freshclam.service.in Outdated Show resolved Hide resolved
@eternaltyro
Copy link
Author

In addition, I imagine we should add similar security options for the clamav-clamonacc.service.in file: https://github.com/Cisco-Talos/clamav/blob/719a8b2feacbbb7e91ed57805640cd450a58ab22/clamonacc/clamav-clamonacc.service.in

Would you be interested in working on that one as well?

Made the changes here: eternaltyro@814f1e2

@eternaltyro eternaltyro requested a review from micahsnyder April 4, 2023 17:47
@eternaltyro
Copy link
Author

@micahsnyder Thank you so much for your review and apologies for the delay in addressing these. I addressed the issues you raised - including hardening the on-access scan service unit file. Please review when you get a chance.

Copy link
Contributor

@micahsnyder micahsnyder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How much did you test? I'm having some trouble with the service files.

clamonacc/clamav-clamonacc.service.in Outdated Show resolved Hide resolved
clamonacc/clamav-clamonacc.service.in Outdated Show resolved Hide resolved
clamonacc/clamav-clamonacc.service.in Outdated Show resolved Hide resolved
freshclam/clamav-freshclam.service.in Show resolved Hide resolved
clamd/clamav-daemon.service.in Outdated Show resolved Hide resolved
clamd/clamav-daemon.service.in Outdated Show resolved Hide resolved
@eternaltyro
Copy link
Author

Thank you @micahsnyder for the review. I'll go through them this weekend. I will respond to individual suggestions soon. But I need to try and simulate the permission-denied errors in order to figure out what is going on there. The rest of the changes you have suggested seem reasonable. Please give me a couple of days to fix these.

The default systemd service files `clamav-freshclam` and `clamav-daemon`
(at least as installed in Arch Linux) do not contain any Sandboxing
options configured. Therefore, the service units are marked as `UNSAFE`
when `systemd-analyze security`[1] is run.

```
➜  ~ systemd-analyze security
UNIT                                 EXPOSURE PREDICATE HAPPY
..
clamav-daemon.service                     9.6 UNSAFE    😨
clamav-freshclam.service                  9.6 UNSAFE    😨
..
```

By including some basic security options[2], we can increase the
security level of the services - even if only a little bit - from
systemd's perspective.

For the daemon, I have used `ProtectSystem` and `ProtectHome` directives
to make the filesystem read-only - which should be okay when scanning
files. However, we have to make an exception for the log path using
`ReadWritePaths`. Additionally, this configuration would interfere with
the `--remove` option if the user chooses to auto-remove infected files.

Other options protect sysconfig, kernel modules, and the hardware clock.

Adverse effects are less pronounced if we apply similar configuration to
freshclam service - which ostensibly only updates a specific set of
files on disk. I referred to the Installation manual[3] to ascertain the
default file path to which fresh databases are downloaded.

With all these changes applied, the result of `systemd-analyze security`
looks like this:

```
➜  ~ systemd-analyze security
UNIT                                 EXPOSURE PREDICATE HAPPY
..
clamav-daemon.service                     7.8 EXPOSED   🙁
clamav-freshclam.service                  7.8 EXPOSED   🙁
..
```

While this seems like a definite improvement, we need to take user
experience, distribution-specific defaults, and best-practices (as
determined by ClamAV itself) to ensure that this changeset is valid.

[1] https://www.freedesktop.org/software/systemd/man/systemd-analyze.html
[2] https://www.freedesktop.org/software/systemd/man/systemd.exec.html
[3] https://docs.clamav.net/manual/Installing/Packages.html
In systemd unit files, I had missed some paths to commands that are
potentially executed in response to events. These commands are arbitrary
and configurable in clamd.conf and freshclam.conf.

Each of these options invoke an appropriate path to a configured
executable when - for example - a scan is complete or signature update
fails. In order for these executables to run, systemd should allow it.
It is necessary to add these paths to `ExecPaths` in systemd service
unit files.

This change adds comments instructing users and administrators how to do
that and generally helps make sense of the defaults.

- Plus some formatting changes
Harden ClamAV OnAccess service systemd unit file.

- Removed default move options to be consistent with the behaviour of
  the rest of the service files
- Added hardening parameters for service
- Added Reload and Stop signals for graceful reload and stop
This commit includes four changes:

1. Wait for clamd process using `--wait` and `--ping` switches instead
   of using a bash test for the presence of clamd.ctl socket
2. Use the PreStart directive to create log and quarantine directories
3. Add shared library path to ExecPaths allow-list
4. Add quarantine directory path to ReadWritePaths allow-list
@eternaltyro eternaltyro force-pushed the enhance/harden-systemd-service branch from 814f1e2 to e69b611 Compare April 30, 2023 20:51
- Add LogsDirectory= and ConfigurationDirectory= directives to the
  service files. This creates the log directory under /var/log and the
  configuration directory under /etc as specified in the unit file.
- Add LogsDirectory path to ReadWritePaths=
- Add /run and /var/run to ReadWritePaths=
- Add shared library path to ReadWritePaths=
- Add Alias to all three services

Known Issues:
- Terminating ClamOnAcc service is really slow and times out. SIGKILL
  takes over where SIGTERM fails to stop the process
- For ClamOnAcc to function effectively on files in $HOME, it needs the
  --fdpass switch
- The PIDFILE and Lockfile paths for services are not managed by Systemd
  yet.
@eternaltyro eternaltyro requested a review from micahsnyder May 1, 2023 17:39
@eternaltyro
Copy link
Author

For some reason ClamOnAcc keeps dying without specifying a reason. I'll investigate further. Do these service files work fine for anybody else?

@micahsnyder
Copy link
Contributor

I'm having some trouble with the clamav-daemon.service. I'm seeing

clamav-daemon.service: Failed to execute /usr/local/sbin/clamd: Permission denied
clamav-daemon.service: Failed to step EXEC spawning /usr/local/sbin/clamd: Permission denied

I don't see any issue with the service file so I am a bit confused.

@@ -10,8 +10,30 @@ After=clamav-daemon.service syslog.target network.target
[Service]
Type=simple
User=root
ExecStartPre=/bin/bash -c "while [ ! -S /run/clamav/clamd.ctl ]; do sleep 1; done"
ExecStart=@prefix@/sbin/clamonacc -F --log=/var/log/clamav/clamonacc.log --move=/root/quarantine
ExecStartPre=/usr/bin/install --owner=root --group=root --directory /var/local/quarantine
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find that this fails if /var/local/quarantine does not exist.

I created it and did sudo chown clamav /var/local/quarantine and then tried again. This time it failed with `Failed to execute /usr/bin/install: Permission denied", which seems similar to the failure I'm seeing with trying to start cthe clamd service.

I'm at a loss as the ExecPaths in both service files look correct to me.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, let me investigate.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you give me more information about your setup so I can attempt to replicate the issue?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm testing on an ubuntu 22.04 VM where I've installed systemd and libsystemd-dev before building clamav with these instructions and installing to the default prefix. Like:
mkdir build && cd build && cmake .. && make -j12 && sudo make install

I had to set up:

  • /usr/local/etc/freshclam.conf and /usr/local/etc/clamd.conf with the basics
  • sudo groupadd clamav && sudo useradd -g clamav -s /bin/false -c "Clam Antivirus" clamav
  • sudo mkdir /usr/local/share/clamav && sudo chown clamav /usr/local/share/clamav
  • sudo mkdir /var/log/clamav && sudo chown clamav /var/log/clamav

I think that's it. Then after reloading with sudo systemctl daemon-reload, I tried starting the various services.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you give me more information about your setup so I can attempt to replicate the issue?

You did a lot of work on this. I don't want to close/cancel this PR unless you've lost interest in solving the remaining issues.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@micahsnyder Sorry, this completely slipped my mind. I'll work on this over the weekend. I already have a VM that I can test this in.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To begin with, I tested the default installation on Debian 12. I found one issue where if /usr/lib/x86_64-linux-gnu had to be in the ExecPaths or clamav-daemon won't start. I also found /dev/null in the list of open files by the daemon - preferably for writing. I don't know if /dev/null and other char-files need to be in the read-only exception in the unit file.

@ProBackup-nl
Copy link

1.9 OK while still being able to start and use clamav-daemon.service on /tmp was possible on Arch Linux its clamav package by appending these lines to your hardened .service file:

NoNewPrivileges=yes
#DynamicUser=yes ;fails opening log
PrivateDevices=yes
SystemCallArchitectures=native
#CapabilityBoundingSet=~CAP_SYS_PACCT CAP_KILL CAP_DAC_READ_SEARCH CAP_FOWNER CAP_IPC_OWNER CAP_BPF CAP_LINUX_IMMUTABLE CAP_IPC_LOCK CAP_SY...
CapabilityBoundingSet=CAP_CHOWN CAP_SETGID CAP_SETUID CAP_DAC_OVERRIDE
MemoryDenyWriteExecute=yes
RestrictNamespaces=~user pid net uts mnt cgroup ipc
RestrictSUIDSGID=yes
ProtectHostname=yes
LockPersonality=yes
RestrictAddressFamilies=none
RestrictRealtime=yes
#ProtectProc= ;root is unaffected by this setting
ProcSubset=pid
PrivateNetwork=yes
PrivateUsers=no
SystemCallFilter=~@clock @cpu-emulation @debug @module @mount @obsolete @raw-io @reboot @resources @swap
#SystemCallFilter=@privileged
IPAddressDeny=any

@ProBackup-nl
Copy link

ProBackup-nl commented Jan 23, 2024

@eternaltyro Is there any update on solving the remaining issues? Or can I start a new merge request for a hardened clamav.service.in based upon your work including my updates and modification to address /tmp scanning?

@eternaltyro
Copy link
Author

@ProBackup-nl What's your SystemD version?

@ProBackup-nl
Copy link

ProBackup-nl commented Jan 27, 2024

@eternaltyro pacman -Q systemd outputs systemd 255.3-1. Note that I don't use clamav-freshclam-once.service neither clamav-clamonacc.service. I use clamav-daemon.service (.socket) and clamav-freshclam-once.service (.timer).

@eternaltyro
Copy link
Author

eternaltyro commented Jan 28, 2024

FTR, the current systemd versions on my systems are:

systemd 249 (249.11-0ubuntu3.12)  # Ubuntu 22.04
systemd 255 (255.3-1-arch)        # ArchLinux
systemd 252 (252.19-1~deb12u1)    # Debian 12 Bookworm

Unknown settings in systemd unit files are ignored so we can safely assume that settings corresponding to newer versions of systemd are handled safely on older systemd without breaking. I have been meaning to add many of the settings that @ProBackup-nl also suggested. I will review the ones with which I am not very familiar before adding them to the unit file.

Some settings I propose implementing in the next version because we need to research the implications of it. For example, for whatever reason the daemon can bind to TCP host:port, having IPAddressDeny=any will probably ignore any config preferences. Another example is ProtectSystem and ProtectHome which might come in the way of ClamAV being able to quarantine files. Somethings to think about.

@ProBackup-nl
Copy link

IPAddressDeny=

I don't see a reason why clamav-daemon.service needs any network access. With IPAddressDeny=any the localhost:3310 binding still works.

ProtectHome=

My current setting is ProtectHome=read-only.

FYI: On my system clamav-daemon.socket is used to scan smtp inbound files using /tmp. No quarantine is needed here. Every file scanned will be deleted anyhow. Using --stream I might even be able to skip temporary storing files in /tmp.

Service user:
Added explicit user and group settings for service files. Clamd and
Freshclam now run as clamav user. Per documentation, the service forks
off process that runs as user specfied in the config. This change makes
that explicit.

Clamonacc still runs as root since it needs to access files that are not
owned by the default clamav user. An alternative approach is to use the
--fdpass to pass the file descriptor perms to Clamd instead of having to
stream the entire file.

Other changes:
- [gen] Added explicit service types. Forking type for ClamD failed even
  without foreground switch. So the service runs as a simple daemon.
- [doc] Updated comment strings to make them concise and unambiguous.
- [sec] Added a safer permission mode for quarantine directory.
- [sec] Added several new security settings to protect the system. Since
  unknown settings are safely ignored by older versions of SystemD, the
  settings should automatically apply when SystemD version is bumped.
- [sec] Added new network security settings. Clamd and OnAcc are not
   allowed to use the networkr; only Freshclam is.
- [sec] Added restrictions to several system calls and Linux
  capabilities

TODO / Known issues:
- PreStart tasks need root user access - for example, to create the
  quarantine directory. The current approach is unreliable.
- Some settings such as PrivateUsers=yes break the service file.
- We need CMAKE/Automake substitution strings to add (multi-arch and
  arch dependent) LIBDIR to certain settings.
- ExecPaths and ReadWritePaths need locking down.
- Parameterize DATADIR - e.g. /var/log/quarantine
@eternaltyro eternaltyro force-pushed the enhance/harden-systemd-service branch from bcd1f5d to a694172 Compare January 29, 2024 07:02
@eternaltyro
Copy link
Author

I don't see a reason why clamav-daemon.service needs any network access.

One of the less advertised features of ClamAV is that it can run as a network service on the LAN or on the company WAN. Scanners can stream files to the daemon over the network to handle suspicious files. The network service also accepts control commands and is not protected very well though, so we need to have firewalls in place.

@eternaltyro
Copy link
Author

I made a lot more changes to address most of the issues here. Commit message should have most of what the changeset has. I will also do some further testing:

  1. Service should run - preferably as clamav user.
  2. Scanning a file not owned by clamav user
  3. OnAccess scanning while I randomly access files
  4. Use EICAR test file to --move to a quarantine location (ReadWritePaths + filesystem ownership tests)
  5. Freshclam tests and directory creation
  6. Logging.

@micahsnyder @ProBackup-nl please test my changes if you are able. I will probably manually test my changes on VMs. (Deb12, Ubuntu 22.04, Arch, any other?)

@ProBackup-nl
Copy link

@eternaltyro Don't expect a review from me due to Github disabled code view for old Firefox browsers somewhere in 2023. I am able to see the diff, but I am unable to see the new code after the diff is applied.

@eternaltyro
Copy link
Author

I also added security settings to the oneshot freshclam service.

@eternaltyro
Copy link
Author

➜  ~ systemd-analyze security      
UNIT                                 EXPOSURE PREDICATE HAPPY
NetworkManager.service                    7.8 EXPOSED   🙁
accounts-daemon.service                   5.5 MEDIUM    😐
archlinux-keyring-wkd-sync.service        2.0 OK        🙂
auditd.service                            8.9 EXPOSED   🙁
bluetooth.service                         6.0 MEDIUM    😐
clamav-daemon.service                     1.2 OK        🙂    <--------
dbus-broker.service                       8.7 EXPOSED   🙁
dhcpcd.service                            9.6 UNSAFE    😨

LogsDirectory specified explicitly as such is automatically configured
to be writable by systemD. So it need not be explicitly specified under
ReadWritePaths.
Specify RuntimeDirectory for clamav services `/run/clamav` to make
PIDFiles writeable.

The RuntimeDirectory ownership is changed by SystemD to match the `User`
and `Group` specified in the service unit files. ClamOnAccess runs as
root and therefore would clobber the ownership of these directories set
by other services in the family. For this reason, until a better
approach is available, RuntimeDirectory and LogsDirectory are not
managed by SystemD for ClomOnAccess service.
@eternaltyro
Copy link
Author

I made the following changes:

  1. Removed /var/log and /run/* directories from ReadWritePaths since SystemD automatically manages ownership and permission for these directories when set via LogsDirectory and RuntimeDirectory respectively.
  2. Removed LogsDirectory and RuntimeDirectory from on-access service since it is run as root user and would clobber ownership and permissions set by other clamav services.

I need to understand how OnAccess manages permissions and file ownership for me to fix the issue.

@eternaltyro
Copy link
Author

Also, the configuration comment for PidFile says this:

# 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

Which I don't think is sound advice. I think it's best for the PID file to be owned by clamav. Also, it is not immediately clear whether the OnAccess service and ClamD share a PIDFile.

@smitsohu
Copy link

smitsohu commented Jun 4, 2024

Why set the user in the systemd service file when it's already in clamd.conf ?

It would be better if clamd itself would change to a sane default.

(edit) Pragmatically, running clamd as clamav user would already be a big improvement. Ideally though it should run as its own separate user, so that freshclam and clamd cannot trace each other. This is relevant for Red Hat distributions because Yama is available there but off, and SELinux isn't configured to restrict ptrace.

Also, the configuration comment for PidFile says this:

# 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

Which I don't think is sound advice. I think it's best for the PID file to be owned by clamav. Also, it is not immediately clear whether the OnAccess service and ClamD share a PIDFile.

Yes, if systemd drops privileges before clamd is started, a root controlled directory will be getting in the way.

But if clamd is started as root this is critical, because otherwise we run into the equivalent of CVE-2020-28935.

@smitsohu
Copy link

smitsohu commented Jun 7, 2024

Replying to myself ...

Why set the user in the systemd service file when it's already in clamd.conf ?

It would be better if clamd itself would change to a sane default.

Both approaches are actually not mutually exclusive.

And if systemd changes user before executing clamd, it means that clamd will never run as root, which in turn means that less code will run as root. This is a good thing.

(edit) Pragmatically, running clamd as clamav user would already be a big improvement. Ideally though it should run as its own separate user, so that freshclam and clamd cannot trace each other. This is relevant for Red Hat distributions because Yama is available there but off, and SELinux isn't configured to restrict ptrace.

SystemCallFilter= mitigates this concern.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants