greenbone / ospd-openvas

ospd-openvas is an OSP server implementation to allow GVM to remotely control an OpenVAS Scanner
GNU Affero General Public License v3.0
67 stars 58 forks source link

OpenVAS does not clear stale pid files when current pid matches old pid #725

Closed Ajedi32 closed 2 years ago

Ajedi32 commented 2 years ago

Expected behavior

When starting ospd-openvas in Docker after a hard shutdown, OpenVAS recognizes that the PID file at /run/ospd/ospd-openvas.pid is stale, removes it, and continues to start.

Actual behavior

OpenVAS displays the following error:

OSPD[1] 2022-08-09 15:25:51,151: ERROR: (ospd.misc) There is an already running process. See /run/ospd/ospd-openvas.pid.

Steps to reproduce

  1. Follow the instructions at https://greenbone.github.io/docs/latest/22.4/container/index.html to set up the Greenbone Community Containers.
  2. Kill the ospd-openvas suddenly with docker compose <options> kill ospd-openvas
  3. Attempt to restart the container with docker compose <options> up ospd-openvas

GVM versions

gsa: 22.04.0

gvm: 22.4.0~dev1

openvas-scanner: 22.4.1~dev1

gvm-libs: 22.4.1~dev1

Environment

Operating system:

$ uname -a && cat /etc/lsb-release 
Linux PC2287 5.10.16.3-microsoft-standard-WSL2 #1 SMP Fri Apr 2 22:23:49 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
DISTRIB_ID=Ubuntu
DISTRIB_RELEASE=20.04
DISTRIB_CODENAME=focal
DISTRIB_DESCRIPTION="Ubuntu 20.04.3 LTS"

Installation method / source: Official docker-compose.yml file on https://greenbone.github.io/docs/latest/22.4/container/index.html

Proposed fix

There was already an attempt to fix this in 200079a7a7857c3c267f93554cf7cd2811ec6b31. The fix doesn't work in a docker environment because with docker OpenVAS runs in a pid namespace where it always sees its pid as 1. Therefore the check at https://github.com/greenbone/ospd-openvas/blob/c10b8c266034d569267618097951dab1a31317d8/ospd/misc.py#L121 yields a false positive.

That line should be updated to also check if the pid in the file matches the pid of the current process, and clear the file if it does.

nichtsfrei commented 2 years ago

Thank you for finding and reporting this bug.

Unfortunately it is not enough to verify if the PID used in the file is the same as used since they're both 1 as well.

However since OSPD-OpenVAS depends on redis it is very unlikely that it has within a VM or bare metal PID 1 with https://github.com/greenbone/ospd-openvas/pull/728 it will assume that it runs within a container and treat it as it is another process and try to start anyway.

When the mentioned PR is merged into main (unstable docker image) it will be merged into stable and then it should work after pulling new images.

Ajedi32 commented 2 years ago

it is not enough to verify if the PID used in the file is the same as used since they're both 1 as well

I don't understand the problem there. If they're both 1 then you delete the PID file and start the server, since by definition the current process can't have the same PID as a completely different process currently running on the same machine, so clearly that's a stale PID file. That's the whole reason I was proposing that check in the first place.

Turning off the duplicate process detection when running in a container seems okay, since it would be very difficult to accidentally start two instances of OpenVAS inside the same container anyway, but it feels a lot more hacky than just checking whether the current PID matches the one in the file, particularly since this issue could theoretically happen outside of a Docker environment too if the new OpenVAS's PID just happens to match the PID of an old killed OpenVAS instance by pure chance (after a sudden power loss, for example).

nichtsfrei commented 2 years ago

/run/ will be cleaned up when booting after e.g. a sudden power outtake but you're right in the sense that if the process_name is the same and the pid we can ignore it and it's a lot cleaner than a magic number. Dunno why I didn't get it in the morning but should be fixed now.