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

Add unprivileged/non-root docker image in addition to current image that starts as root #478

Open
dasMulli opened this issue Feb 21, 2022 · 41 comments

Comments

@dasMulli
Copy link

The provided docker-built container contains user clamav (100) and group clamav (101), however when trying to run as this user/group, the image fails to start (same for current :stable and :latest tags):

> docker run --rm -it --user 100:101 clamav/clamav:latest
Unable to find image 'clamav/clamav:latest' locally
latest: Pulling from clamav/clamav
Digest: sha256:8083453c1d23818ed281629943fe83318a77542ae07fc5113b6e70ff9dfda102
Status: Downloaded newer image for clamav/clamav:latest
install: can't create directory '/run/clamav': Permission denied

This is needed to run the container in Kubernetes as non-root container in order to pass stricter policies without additional need to work around them - e.g. creating PodSecurityPolicy objects to use. In the upcoming Pod Security Admissions feature, this container will be unable to be used in the Pod Security Standard "Restricted" ("Heavily restricted policy, following current Pod hardening best practices.") (see https://kubernetes.io/docs/concepts/security/pod-security-standards/).

Since I don't think ClamAV needs to do things that need root access or additional capabilities by default, being able to run as non-root user by default or having an unprivileged image would be good.

This also shows up as "HIGH" vulnerability/misconfiguration in some security scans (it's just a tool and good practices, not something to blindly make decisions on):

$ trivy repo --security-checks config https://github.com/Cisco-Talos/clamav
Enumerating objects: 52591, done.
Counting objects: 100% (52591/52591), done.
Compressing objects: 100% (25552/25552), done.
Total 52591 (delta 33169), reused 44462 (delta 26102), pack-reused 0
2022-02-21T15:48:29.073+0100    INFO    Detected config files: 1

Dockerfile (dockerfile)
=======================
Tests: 23 (SUCCESSES: 20, FAILURES: 3, EXCEPTIONS: 0)
Failures: 3 (UNKNOWN: 0, LOW: 0, MEDIUM: 2, HIGH: 1, CRITICAL: 0)

+---------------------------+------------+----------------------------------+----------+------------------------------------------------+
|           TYPE            | MISCONF ID |              CHECK               | SEVERITY |                    MESSAGE                     |
+---------------------------+------------+----------------------------------+----------+------------------------------------------------+
| Dockerfile Security Check |   DS001    | ':latest' tag used               |  MEDIUM  | Specify a tag in the                           |
|                           |            |                                  |          | 'FROM' statement for image                     |
|                           |            |                                  |          | 'index.docker.io/library/alpine'               |
|                           |            |                                  |          | -->avd.aquasec.com/appshield/ds001             |
+                           +------------+----------------------------------+----------+------------------------------------------------+
|                           |   DS002    | root user                        |   HIGH   | Specify at least 1 USER                        |
|                           |            |                                  |          | command in Dockerfile with                     |
|                           |            |                                  |          | non-root user as argument                      |
|                           |            |                                  |          | -->avd.aquasec.com/appshield/ds002             |
+                           +------------+----------------------------------+----------+------------------------------------------------+
|                           |   DS013    | 'RUN cd ...' to change directory |  MEDIUM  | RUN should not be used to change directory:    |
|                           |            |                                  |          | 'apk add --no-cache         bsd-compat-headers |
|                           |            |                                  |          |         bzip2-dev         check-dev            |
|                           |            |                                  |          |    cmake         curl-dev         file         |
@ghost
Copy link

ghost commented Mar 9, 2022

We are facing this problem too! Unprivileged would be really helpful to have as an option.

@val-ms
Copy link
Contributor

val-ms commented Mar 10, 2022

The current Dockerfile implementation is set up so that clamd and freshclam services start as root but switch to run as the clamav user after service startup.

I don't really have any objection to changing things so the container runs as the clamav user to begin with. It would just take a little work.

I have my hands full. If any volunteers would like to give it a try, I (or one of my teammates) wouldn't mind reviewing a pull-request.

@dasMulli
Copy link
Author

Poking around: it seems to run when necessary permissions are prepared in the dockerfile and all pid and lock files are moved to a dedicated run (/var/run/clamav) dir, can tinker more with it over the weekend and create a PR

@codebitts
Copy link

We are also facing this issue. As an interim solution can you share how the directories are supposed to be prepared in the docker file? Would be great if we could use the base image and add more layers for preparing the directories.

@dasMulli
Copy link
Author

dasMulli commented Apr 5, 2022

I had some issues testing all features, but the main idea was to move all mkdir, install and ln invocations from docker-entrypoint.sh to the Dockerfile, setting a fixed UID in the adduser command that is already there, changing the sed commands in the Dockerfile to configure everything into /var/run instead of /run and fixing permissions through chown where they occur.
It appeared that clamd and freshclam were working, but did not test milterd yet (at least it starts)

@codebitts
Copy link

codebitts commented Apr 5, 2022

For reference, this seems to have worked for me. Albeit with some modifications to the default configuration files as well.

FROM clamav/clamav:0.104

COPY etc /etc/clamav

RUN mkdir /var/run/clamav /run/lock && \
    chown clamav:clamav /var/run/clamav /run/clamav /var/log/clamav /var/lock /run/lock && \
    chmod 770 /var/run/clamav /run/clamav /var/log/clamav /var/lock /run/lock

USER clamav

@dasMulli
Copy link
Author

dasMulli commented Apr 6, 2022

My diff so far is main...dasMulli:feature/nonroot-container

@msamendinger
Copy link

msamendinger commented Apr 11, 2022

Also looking for a solution to run the container as non-root.
Can anyone give more insight why these lines are needed? At a first try the container seems to run without these lines but I guess they are there for a reason.

# Help tiny-init a little
mkdir -p "/run/lock"
ln -f -s "/run/lock" "/var/lock"

@dasMulli I was looking through your diff and wondering why the location for the pid file and socket get moved to /var/run/lock and /var/run/clamav and later the directory /run/clamav is created that doesn't seem to be used.
What do you think about using /run/lock/clamav and /run/clamav respectively?
When do you plan to create the PR?

@dasMulli
Copy link
Author

later the directory /run/clamav is created that doesn't seem to be used.

It is an unfinished snapshot, the permissions for /run would just be more invasive to set up.

No timeline yet but before the end of the month

@cmcdougall
Copy link
Contributor

Is there an update to this issue? I'm currently looking at trying to run the image non-root as well.

@val-ms
Copy link
Contributor

val-ms commented Jul 15, 2022

Is there an update to this issue? I'm currently looking at trying to run the image non-root as well.

Sorry, no update on this issue from the clamav development team.

@cmcdougall
Copy link
Contributor

Thanks! I've created a pull request (#666) that fixes the issue. Let me know if you need any changes!

@val-ms
Copy link
Contributor

val-ms commented Aug 3, 2022

@dasMulli and any others who are interested, able: If you're able to help test @cmcdougall's PR to verify that it meets your needs, that will be helpful.

@candrews
Copy link
Contributor

I think a new non-root user image should be published alongside the existing root user images for a few reasons:

  1. backwards compatibility. People who currently use the images expect them to be using root, so changing to non-root can (and will) surprise them. A major reason for surprise is...
  2. root images cannot run commands that root can run, such as installing packages. For example, today I can run docker run -it --entrypoint /bin/sh clamav:latest then apk add curl. If the image runs as a non-root user, I can't do that.

Publishing both root and non-root images is a common pattern. For example, nginx publishes nginxinc/nginx-unprivileged (which uses the non-root user nginx) and nginx which uses root.

Therefore, I suggest clamav publish images using a tag pattern as demonstrated by these examples:

  • latest
  • latest-unprivileged
  • 0.105.1
  • 0.105.1-unprivileged

For backwards compatibility, un-suffixed images run as root, while those with the -unprivileged suffix run as the clamav user.

@cmcdougall
Copy link
Contributor

I'd be happy to add a new Dockerfile for a separate unprivileged image. Would best practice be to have the unprivileged Dockerfile extend the root image and make the config changes? Or to keep them completely separate and dependent from each other?

Thanks!

@candrews
Copy link
Contributor

Would best practice be to have the unprivileged Dockerfile extend the root image and make the config changes? Or to keep them completely separate and dependent from each other?

I don't have a strong opinion... I think that whatever approach is most maintainable is the way to go.

@cmcdougall
Copy link
Contributor

Would best practice be to have the unprivileged Dockerfile extend the root image and make the config changes? Or to keep them completely separate and dependent from each other?

I don't have a strong opinion... I think that whatever approach is most maintainable is the way to go.

I'll update my pull request. Thanks for the suggestion!

@rmueller83
Copy link

I see no need of such an additional image. In my experience, the above mentioned case is solved by a Dockerfile like this:

FROM clamav/clamav:latest
USER root
apk add curl
USER clamav

In the end it's the decision of @micahsnyder if they want to maintain two images.

@candrews
Copy link
Contributor

the above mentioned case is solved by a Dockerfile like this:

That can be a solution where it's possible.

One scenario where that isn't possible is a use case I and my colleagues use frequently - GitLab jobs.

For example:

clamav:
  image:
    name: clamav/clamav:0.105.1  # clamav official, verified image
    entrypoint: [""]
  script:
    - |
      apk add --no-cache curl
      freshclam --quiet
      # change file permissions so clamav can access them
      chown -R clamav .
      chmod -R 777 .
      su - -s /bin/sh -c "clamscan --alert-encrypted --allmatch --archive-verbose --recursive  --suppress-ok-results $(pwd)" clamav 2>&1 | grep -E -v ': Empty file$|: Symbolic link$' | tee scan_log.txt
      scan_result=$?
      curl --fail -X POST --data-urlencode "success=$scan_result" https://<redacted>

A couple immediate reasons why this needs to run as root in this case are:

  • The files scanned aren't owned/accessible by the clamav user. The files are created by another job and have file permissions that restrict access. Therefore, unless the image runs as root so the permission can be modified, those files are unscannable.
  • curl is installed which can only be done by root.

There are other ways to do this (such as splitting up into multiple jobs), but those have trade off (such as longer latency / more compute resources used). Plus, it would be a surprising break in backwards compatibility for this approach to work for clamav/clamav:0.105.1 but suddenly stop working for clamav/clamav:0.XXX.

@cmcdougall
Copy link
Contributor

I will wait for further advice from @micahsnyder before updating my pull request

@albundy83
Copy link

Maybe if adding USER directive in Dockerfile is an issue in this MR bde960c at least you can keep all the others updates that allow running Container image under non-root user ?

@candrews
Copy link
Contributor

@cmcdougall @micahsnyder can we please progress this issue?

As I said in #478 (comment) if we go with the approach of publishing only one docker image that is rootless, my organization won't be able to use the ClamAV docker image any longer and it's going to be a big problem for us, and I suspect we're not the only ones. Please publish two images (a rootless and root one) :)

@val-ms
Copy link
Contributor

val-ms commented Sep 22, 2022

@candrews thanks for the bump on this. I spent some time today reviewing #666 and am happy with the results. I merged the PR.

However, we cannot publish a new image with this change without a new clamav software version. I could include this change in a backport fix for the next 0.105 patch version (0.105.2). But we do not have a timeline for the next patch version. Otherwise this will have to wait until the next feature release (1.0.0), which is anticipated in late October or early November.

A better plan is to migrate our Dockerfiles out of the clamav and clamav-bytecode-compiler project repositories and into their own separate repos so we can update images on demand, and will not have to coordinate new images with new software versions. I will work towards this goal, but can not provide an ETA for the transition.

@candrews
Copy link
Contributor

candrews commented Sep 22, 2022

As I said in #478 (comment), 28f8337 is a huge problem for my use case.

This is a breaking change - users will be surprised by this and will experience breakage like my organization will be experiencing.

Can you please revert that commit and reconsider?

@val-ms
Copy link
Contributor

val-ms commented Sep 22, 2022

Apologies @candrews. Too much going on at once. I skimmed the conversation far too fast, and went straight to testing the changes.

Yes, I can revert the commits.

I do not relish maintaining multiple images, but if @cmcdougall's idea to extend the image with root and make the configuration change to drop root is viable, then I say we do it that way.

With that said, we're looking at switching from Alpine to Debian (slim) so we can build multi-arch images -- something I haven't been able to do with Alpine. Ref: #673

I am certain people will experience breakage with a switch from Alpine -> Debian as well. And though I do not relish the idea of having to maintain two different images (alpine & debian), which given the above would mean having all of:

  • clamav/clamav:stable
  • clamav/clamav:stable-debian
  • clamav/clamav:stable-unprivileged
  • clamav/clamav:stable-debian-unprivileged
  • clamav/clamav:stable_base
  • clamav/clamav:stable_base-debian
  • clamav/clamav:stable_base-unprivileged
  • clamav/clamav:stable_base-debian-unprivileged

I suppose it will all be done by automation anyways...

@albundy83
Copy link

albundy83 commented Sep 23, 2022

As I said in previous comment (#478 (comment)), accepting the commit WITHOUT USER clamav directive will be a first step to be able to run root-less.
It will be also a better idea to specify group id when group is added:
From here:

addgroup -S "clamav" && \

to:

    addgroup -S -g 100 "clamav" && \

It will remove all commands that prevent image from being run under non-root user (chown and so on) that are in the docker-entrypoint.sh.

Under Kubernetes for example, specifying securityContext like this should do the job:

<...>
    spec:
      securityContext:
        runAsUser: 100
        runAsGroup: 100
        fsGroup: 100
<...>

It will also give time to @candrews to update his root stuff :-)

@msamendinger
Copy link

I agree with @albundy83. That way the image is prepared for users that want to drop privileges. They can just start the container under user clamav. Others just run default as root and won't experience any difference.
If the default should change one day can be another conversation.

@val-ms val-ms changed the title Container unable to run as non-root user Add unprivileged/non-root docker image in addition to current image that starts as root Nov 15, 2022
@val-ms
Copy link
Contributor

val-ms commented Nov 16, 2022

I have implemented most of the changes from @cmcdougall's https://github.com/Cisco-Talos/clamav/pull/666/files PR in the Dockerfiles in our new clamav-docker repo.

For example:

Today we published updated images for the 0.105 / 0.105.1 / latest / etc. as well as for the unstable tags based on these. That is to say, the latest images now have the changes from @cmcdougall to place files in /tmp instead of under root-owned directories.

So I think the next step is to build an image based off of the current image and switch users. Maybe we can implement it in the update_db_image.sh script, in a similar way as with this clamav_db_update() function: https://github.com/Cisco-Talos/clamav-docker/blob/main/clamav/0.105/alpine/scripts/update_db_image.sh#L80-L99

Note: I'm trying to replicate the same in debian-based Dockerfiles for building multi-arch images -- something I haven't been able to do successfully with Alpine. They are more of a work in progress. If you're interested, see:

@val-ms
Copy link
Contributor

val-ms commented Nov 16, 2022

We did not consider the case where a custom config is already in use and specified the /run/clamav and /run/lock paths.
For compatibility, we may need to revert this change back to using /run/clamav and /run/lock instead of /tmp and find a way to change the config to /tmp only for a downstream image, such as changing those paths in the downstream Dockerfile used to switch users.

@val-ms
Copy link
Contributor

val-ms commented Nov 16, 2022

Here is the change to (mostly) revert the base Dockerfile: Cisco-Talos/clamav-docker#1

I think the next step is going to be making a Dockerfile for the unprivileged image and incorporating that build into the Jenkinsfiles, so we can complete this ticket.

@val-ms
Copy link
Contributor

val-ms commented Nov 16, 2022

I think I have a compromise to continue with the change to use /tmp for the new config, while creating the directories necessary for folks using the /run/clamav and /run/lock settings in the original config.
I'm testing this now.

val-ms added a commit to Cisco-Talos/clamav-docker that referenced this issue Feb 7, 2023
The change to use /tmp/clamd.sock instead of /var/run/clamd.sock was an intentional 
step towards being able to build an unprivileged/rootless image as requested by 
Cisco-Talos/clamav#478

It looks like we forgot to update the documentation to note the change in the 
clamd.sock location.
val-ms added a commit to Cisco-Talos/clamav-documentation that referenced this issue Feb 7, 2023
The change to use /tmp/clamd.sock instead of /var/run/clamd.sock was an intentional 
step towards being able to build an unprivileged/rootless image as requested by 
Cisco-Talos/clamav#478

It looks like we forgot to update the documentation to note the change in the 
clamd.sock location.
@HuyDo-95g
Copy link

HuyDo-95g commented Mar 20, 2023

Has the issue been fixed yet? I'm still facing the permission denied on folder '/run/clamav' when set the user as non root.
FYI, I'm using the clamav ver 0.105.2. Also, did try ver 1.0.1.

@shm-eboks
Copy link

would love to see an update on this as well

@boindil
Copy link

boindil commented Mar 24, 2023

@HuyDo-95g
@shm-eboks

I ended up integrating my own Dockerfile and init-file for a variety of problems (proxy, non-root, additional configuration options). The progress here is very slow and I dont expect this to be solved soon at all. You might want to do the same as this is relevant for maintaining security ...

@shm-eboks
Copy link

shm-eboks commented Mar 24, 2023

@boindil

I ended up integrating my own Dockerfile and init-file for a variety of problems (proxy, non-root, additional configuration options). The progress here is very slow and I dont expect this to be solved soon at all. You might want to do the same as this is relevant for maintaining security ...

could you care to post your solution here ? - as this would greatly speed up solution to our problem as well.
and might also push this repo for a decent solution.

@boindil
Copy link

boindil commented Mar 24, 2023

@shm-eboks https://github.com/boindil/clamav-docker-rootless

have fun

@nc-ruth
Copy link

nc-ruth commented Apr 14, 2023

Is it possible to get this maintained by Cisco-Talos?

@boindil I see you have some hints and improvements in regards of the rootless image you have made. Is it possible for you to collaborate or introduce a PR that could be maintained by the community surrounding Cisco-Talos?

@boindil thank you for the contribution and sharing what you got :)

@dradoaica
Copy link

dradoaica commented Jun 17, 2023

another solution (built on top of the clamav official docker image): https://github.com/dradoaica/clamav-docker-openshift

P.S.: already built docker image: https://hub.docker.com/r/dradoaica/clamav-openshift/tags

@giorgionanfa
Copy link

another solution (built on top of the clamav official docker image): https://github.com/dradoaica/clamav-docker-openshift

P.S.: already built docker image: https://hub.docker.com/r/dradoaica/clamav-openshift/tags

@dradoaica I have found your implementation perfect to overcome this problem. I try using an even more restrictive container security context, setting readOnlyRootFilesystem: true, but run into some issues like this when trying to scan a file:

LibClamAV Error: cli_gentempfd_with_prefix: Can't create temporary file /tmp/clamav-f616090c59070abb81be20ceb9b54e0a.tmp: Read-only file system

I also tried changing folder permissions via Dockerfile, but the problem persists. Have you maybe done any analysis on this aspect or do you have any suggestions?
Thanks anyway :)

@dradoaica
Copy link

dradoaica commented May 22, 2024

readOnlyRootFilesystem

If you are using the helm chart from my repo, you need to add another volume mount for the tmp folder:

    volumeMounts:
    - mountPath: /tmp
      name: tmp-volume

  volumes:
  - name: tmp-volume
    emptyDir: {}

@albundy83
Copy link

albundy83 commented Sep 4, 2024

A clean way to start rootless under Kubernetes for example is to simply use the already built feature, here a simple Deployment:

apiVersion: apps/v1
kind: Deployment
metadata:
  name: clamav
  labels:
    app: clamav
    app.kubernetes.io/component: clamav
    app.kubernetes.io/instance: clamav
    app.kubernetes.io/managed-by: Kustomize
    app.kubernetes.io/name: clamav
spec:
  replicas: 1
  selector:
    matchLabels:
      app: clamav
  template:
    metadata:
      name: clamav
      labels:
        app: clamav
        app.kubernetes.io/component: clamav
        app.kubernetes.io/instance: clamav
        app.kubernetes.io/managed-by: Kustomize
        app.kubernetes.io/name: clamav
    spec:
      automountServiceAccountToken: false
      securityContext:
        runAsUser: 1000
        runAsGroup: 1000
        fsGroup: 1000
      containers:
        - name: clamav
          image: docker.io/clamav/clamav-debian:1.4.0-1_base
          imagePullPolicy: IfNotPresent
          ports:
            - name: clamd
              containerPort: 3310
              protocol: TCP
            - name: clamav-milter
              containerPort: 7357
              protocol: TCP
          env:
            - name: CLAMAV_NO_MILTERD
              value: 'false'
          command:
            - /bin/bash
          args:
            - -c
            - /init-unprivileged               # The very important part :)
          livenessProbe:
            exec:
              command:
                - /usr/local/bin/clamdcheck.sh
            failureThreshold: 1
            periodSeconds: 10
          readinessProbe:
            exec:
              command:
                - /usr/local/bin/clamdcheck.sh
            initialDelaySeconds: 5
            periodSeconds: 5
          startupProbe:
            exec:
              command:
                - /usr/local/bin/clamdcheck.sh
            failureThreshold: 30
            periodSeconds: 10
          resources:
            limits:
              cpu: '1'
              ephemeral-storage: 1Gi
              memory: 4Gi
            requests:
              cpu: '0.1'
              ephemeral-storage: 300Mi
              memory: 64Mi
          volumeMounts:
            - name: tmp
              mountPath: /tmp
            - name: clamav-lib
              mountPath: /var/lib/clamav
            - name: clamav-log
              mountPath: /var/log/clamav
          securityContext:
            allowPrivilegeEscalation: false
            readOnlyRootFilesystem: true
            capabilities:
              drop:
                - ALL
      volumes:
        - name: tmp
          emptyDir: {}
        - name: clamav-lib
          emptyDir: {}
        - name: clamav-log
          emptyDir: {}

If you don't want to manage logs in /var/log/clamav, just override with ConfigMap ClamAV configuration files (/etc/clamav/clamav-milter.conf, /etc/clamav/clamav-milter.conf and /etc/clamav/freshclam.conf).

Maybe this issue could be closed no ?

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

No branches or pull requests