donaldzou / WGDashboard

Simple dashboard for WireGuard VPN written in Python & Vue.js
https://donaldzou.github.io/WGDashboard-Documentation/
Apache License 2.0
1.58k stars 236 forks source link

📢📢 URGENT! Docker WGDashboard Container Security Vulnerability! 📢📢 #333

Closed DaanSelen closed 2 months ago

DaanSelen commented 2 months ago

For everyone using my work on the Docker image, I have recently discovered a crucial mistake with the creation of the image.

The Private key of my templated wg0.conf file is NOT random, and was prebaked in the image. This has changed with the new version of dselen/wgdashboard:dev and dselen/wgdashboard:latest hosted on the Docker Hub.

Please update to this image if you are able to! And if you are unable to use the new image, please configure a change of the private in your instance! This is to keep your data and connections secure.

I would like to sincerely apologise for my mistake, and if anyone needs help reach out to me!

DaanSelen commented 2 months ago

To add more context, if you built your own image you are safe! But if you pulled it from a repository. You must either delete the conf volume and let the container create a new one with a unique key. Or modify the private key entry yourself.

This is how to change the key in an existing file.

privateKey=$(wg genkey)
sed -i "s|^PrivateKey =$|PrivateKey = ${privateKey}|g" /etc/wireguard/wg0.conf

You also need to change the Public key in the endpoints, you can extract the public key with: If you still have the environment variable from the first command simple run:

echo "${privateKey}" | wg pubkey

And you will get the public key printed.

NOXCIS commented 2 months ago

@DaanSelen made a fix https://github.com/donaldzou/WGDashboard/pull/334

DaanSelen commented 2 months ago

@DaanSelen made a fix #334

Hello @NOXCIS, may I ask where you fixed it? I see you removed it in its entirety?

NOXCIS commented 2 months ago

@DaanSelen

Screenshot 2024-08-23 at 1 50 05 AM

enter image description here

DaanSelen commented 2 months ago

@NOXCIS inspired by your efforts and your repository, I have made an Alpine Linux image. Please review https://github.com/DaanSelen/WGDashboard. Regarding the size limitations I got an actual running container about on the same level as the one made by your fork. However I was keen on keeping the same features as I previously had crafted for WGDashboard.

CONTAINER ID   IMAGE                       COMMAND                  CREATED          STATUS                     PORTS       NAMES                SIZE
e189c27def35   dselen/wgdashboard:alpine   "/bin/bash /entrypoi…"   4 minutes ago    Up 4 minutes (healthy)     10086/tcp   peaceful_agnesi      11.3MB (virtual 140MB)
1d477a16b01f   dselen/wgdashboard:nox      "/home/app/entrypoin…"   5 minutes ago    Up 5 minutes (unhealthy)               ecstatic_albattani   49.6MB (virtual 114MB)

Regarding CVE-2018-20225. This is at the moment still unpatched, so I cannot remove that. However your image also has this CVE present, but moved to the runtime bash script.

Regarding the image I published, which is outdated (thanks to the documentation) it still points to my own registry, which should be moved to the Docker Hub registry managed by me, refer to https://hub.docker.com/repository/docker/dselen/wgdashboard/general

I'd like to hear from you.

NOXCIS commented 2 months ago

@DaanSelen

donaldzou commented 2 months ago

@DaanSelen

  • Reimplementation of the automatic configuration creation is no issue as long as its done from the wgd.sh script. Docker treats the entrypoint script as the actual runtime script, all thats doing is calling the wgd.sh start and install arguments, and the cascade follows, etc. I have an adaption here where wireguard config are generated in the setup script and a master peer is outputted to terminal. The master peer is the sole way to access the dashboard as i only expose wireguard UDP ports in the container image.
  • CVE-2018-20225 still hasn't been patched, so the best option is mitigation. Even though the setup script isn't using --extra-index-url option when installing pip or performing the install requirements function. Its best practice to further isolate pip behind the venv, versus the docker image that acts as your first layer of defense.
  • I've recently pushed some updates to my fork to show how config generation can be better handled. However the ultimate solution will require the dashboard.py to modifed slightly to accept passed docker enviorment vars before the wg-dashboard.ini is created as external charges are overwritten after init.

I believe these are fixed?

DaanSelen commented 2 months ago

@DaanSelen

  • Reimplementation of the automatic configuration creation is no issue as long as its done from the wgd.sh script. Docker treats the entrypoint script as the actual runtime script, all thats doing is calling the wgd.sh start and install arguments, and the cascade follows, etc. I have an adaption here where wireguard config are generated in the setup script and a master peer is outputted to terminal. The master peer is the sole way to access the dashboard as i only expose wireguard UDP ports in the container image.
  • CVE-2018-20225 still hasn't been patched, so the best option is mitigation. Even though the setup script isn't using --extra-index-url option when installing pip or performing the install requirements function. Its best practice to further isolate pip behind the venv, versus the docker image that acts as your first layer of defense.
  • I've recently pushed some updates to my fork to show how config generation can be better handled. However the ultimate solution will require the dashboard.py to modifed slightly to accept passed docker enviorment vars before the wg-dashboard.ini is created as external charges are overwritten after init.

I believe these are fixed?

Hallo Donald, regarding the CVE there is no fix and automatic deployment is something we need to do from either Github action or by interval.

But yes @donaldzou i think my latest image, or the code in my fork is doing this

donaldzou commented 2 months ago

But yes @donaldzou i think my latest image, or the code in my fork is doing this

Do you mean @NOXCIS 's PR I just merged?

DaanSelen commented 2 months ago

But yes @donaldzou i think my latest image, or the code in my fork is doing this

Do you mean @NOXCIS 's PR I just merged?

No I did not, but I will look at what has changed tomorrow. Looks like a lot has changed and I do not know what specifically yet.

From what I see now, the Docker part is incomplete, Noxcis may have changed the Image compilation but all Documentation is still very outdated and all. For the inner workings on the image, it might work better, but I have to analyze it

NOXCIS commented 2 months ago

@DaanSelen @donaldzou Its been a pleasure working with the two of you. There are some aspects of the docker implementation that need to be discussed before any further changes. I.E Docs. For now I'm burnt, happy hunting 😄.

NOXCIS commented 2 months ago

@DaanSelen @donaldzou Oh and theres no attack surface for the CVE in question. Its Mitigated until pip patch.

  1. the --extra-index-url option is not utilized during pip actions anywhere
  2. Container Start sequence interrupt required for injection and exploitation.
  3. Python & Friends are not ran directly in docker file system layers
donaldzou commented 2 months ago

@DaanSelen @donaldzou Its been a pleasure working with the two of you. There are some aspects of the docker implementation that need to be discussed before any further changes. I.E Docs. For now I'm burnt, happy hunting 😄.

No problem, and thank you so much on helping this!!

NOXCIS commented 2 months ago

@DaanSelen I forgot to update the Healthcheck. HEALTHCHECK --interval=30s --timeout=10s --start-period=5s --retries=3 CMD sh -c 'pgrep gunicorn > /dev/null && pgrep tail > /dev/null' || exit 1

DaanSelen commented 2 months ago

@DaanSelen I forgot to update the Healthcheck. HEALTHCHECK --interval=30s --timeout=10s --start-period=5s --retries=3 CMD sh -c 'pgrep gunicorn > /dev/null && pgrep tail > /dev/null' || exit 1

I'll look ino the Healthcheck, but the one your proposed checks for the gunicorn process, why this over a simple curl call?

It also does not work out-of-the-box anymore. So I have to analyze the changes, and streamline them further in the coming days. And its configurations are faulty out of the box because of the Docker IP.

DaanSelen commented 2 months ago

@DaanSelen I forgot to update the Healthcheck. HEALTHCHECK --interval=30s --timeout=10s --start-period=5s --retries=3 CMD sh -c 'pgrep gunicorn > /dev/null && pgrep tail > /dev/null' || exit 1

What was the use of the multistage build? Was it to mitigate the Pip installation? Because the package is still present on your changed version. Docker Scout just does not show it.

06b0265790ba:/opt/wireguarddashboard/src# pip --version
pip 24.0 from /usr/lib/python3.12/site-packages/pip (python 3.12)
NOXCIS commented 2 months ago

@DaanSelen 1. The curl call does not work under in prod or dev.

  1. checking for both the gunicorn and tails processes is a better health check, because it works.
  2. Screenshot 2024-08-26 at 8 43 42 AM

    The mitigation step is to keep pip install away from any active execution layers. Which in the case of docker is the entrypoint script to which the pip install is apart of the child process and not the parent entrypoint shell.

DaanSelen commented 2 months ago

@NOXCIS Can you elaborate more than "It does not work" and "because it works".

How does the curl call not work? It calls the web service and if it gets a response it marks it as healthy, true this is shallow. But so it looking if there is a Gunicorn process.

Please I would like to know why you think so!

NOXCIS commented 2 months ago

@DaanSelen Honestly dont know why its not working for me, i just know that i don't need to have curl installed in the image. Its barebones and failsafe to check for gunicorn.

NOXCIS commented 2 months ago

@DaanSelen objective is to have a final docker image that is so bare bones it nerfs privilege escalation and lateral movement from within the container if someone like me found a away in through the frontend. Hence why i was adamant on not shipping an image with a c lib, compiler, git, curl or anything that would be useful for exploitation.

DaanSelen commented 2 months ago

@DaanSelen objective is to have a final docker image that is so bare bones it nerfs privilege escalation and lateral movement from within the container if someone like me found a away in through the frontend. Hence why i was adamant on not shipping an image with a c lib, compiler, git, curl or anything that would be useful for exploitation.

I know that the less packages the better, but right now its a bit of a conflict between functionality and security. In my opinion the Docker Image should be made around WGDashboard, not WGDashboard around the docker image. The WGDashboard itself should be secure enough, otherwise exploiting a Docker container is a small task compared to exploiting a system which has WGDashboard installed as a SystemD Daemon.

NOXCIS commented 2 months ago

@DaanSelen The Docker image is built around WGDashboard. The docker image is just the alpine OS with with WGDashboard & dependencies . Essentially a quasi VM whose sole purpose is to run WGDashboard. Full bare metal functionality still works, docker and Bare Metal share the same install sequence function in wgd.sh and call separate functions for their respective start sequences. Full functionality and full security can be done without compromise, its been done here. But security should never be compromised for the sake of "functionality" with a VPN Server Dashboard.

In short, the Dashboard runs with all of its functionality, however passing environment variables from docker compose is not an original function of the dashboard. Neither is hard coding said passed vars into the dockerfile a part of original functionality. In v3 they were somewhat necessary as there was no way to create configs from within the dashboard. Doing so pokes holes in everything and allows for issues such as this one we are conversating under because the issue is hard baked into the image it self for everyone to pull from. Docker environment vars should be passed into a python venv with .env file to prevent vars passed from the docker space being executed in the python venv space.

As for the docker exploitation, It shares the host system kernel with its containers. I.E exploit docker exploit the host system at the root level.

DaanSelen commented 2 months ago

@DaanSelen The Docker image is built around WGDashboard. The docker image is just the alpine OS with with WGDashboard & dependencies . Essentially a quasi VM whose sole purpose is to run WGDashboard. Full bare metal functionality still works, docker and Bare Metal share the same install sequence function in wgd.sh and call separate functions for their respective start sequences. Full functionality and full security can be done without compromise, its been done here. But security should never be compromised for the sake of "functionality" with a VPN Server Dashboard.

In short, the Dashboard runs with all of its functionality, however passing environment variables from docker compose is not an original function of the dashboard. Neither is hard coding said passed vars into the dockerfile a part of original functionality. In v3 they were somewhat necessary as there was no way to create configs from within the dashboard. Doing so pokes holes in everything and allows for issues such as this one we are conversating under because the issue is hard baked into the image it self for everyone to pull from. Docker environment vars should be passed into a python venv with .env file to prevent vars passed from the docker space being executed in the python venv space.

As for the docker exploitation, It shares the host system kernel with its containers. I.E exploit docker exploit the host system at the root level.

Thanks for the info, and its true that some features in the Docker container are unique compared to the WGDashboard itself.