minvws / nl-kat-coordination

OpenKAT scans networks, finds vulnerabilities and creates accessible reports. It integrates the most widely used network tools and scanning software into a modular framework, accesses external databases such as shodan, and combines the information from all these sources into clear reports. It also includes lots of cat hair.
https://openkat.nl
European Union Public License 1.2
127 stars 58 forks source link

New boefje containerized setup ignores stderr #3559

Closed zcrt closed 3 weeks ago

zcrt commented 2 months ago

Describe the bug There are some boefjes using a subprocess call that captures both stderr and stdout. However, often stderr is ignored and only stdout is returned. This makes debugging a badly functioning boefje hard.

Expected behavior Storing both stderr and stdout (and possibly even stdin) securely.

Examples https://github.com/minvws/nl-kat-coordination/blob/2583519911c816fd6a82116a7d5b2aa4ffcfeef8/boefjes/boefjes/plugins/kat_dnssec/main.py#L20 https://github.com/minvws/nl-kat-coordination/blob/2583519911c816fd6a82116a7d5b2aa4ffcfeef8/boefjes/boefjes/plugins/kat_nmap_tcp/main.py#L21

underdarknl commented 1 month ago

Adding stderror to the output stream seems logical. however, that needs to be done on a 'per boefje' basis. Returning stdout also makes better sense than parsing the output in the boefje itself, and then presenting a reworked version to the normalizer, as I'd rather keep the original output on file and work with that. We do need to make sure that we can differentiate between the wanted (stdout) output, and the unwanted (stderror) output, as both these raw files will receive the boefje/$boefjename mimetype.

underdarknl commented 1 month ago

related https://github.com/minvws/nl-kat-coordination/issues/1373

noamblitz commented 1 month ago

Adding stderror to the output stream seems logical. however, that needs to be done on a 'per boefje' basis. Returning stdout also makes better sense than parsing the output in the boefje itself, and then presenting a reworked version to the normalizer, as I'd rather keep the original output on file and work with that. We do need to make sure that we can differentiate between the wanted (stdout) output, and the unwanted (stderror) output, as both these raw files will receive the boefje/$boefjename mimetype.

Fortunately, in the meantime between creating this issue and now, we have reworked the boefje and normalizer to use the openkat/nmap-output for nmap and openkat/dnssec-output for dnssec. Therefore, we can safely return error/boefje next to that without interfering with the normalizer. However, We need to check that when the boefje errors, it will still output stout, which will let the normalizer have an empty result set, removing all old observations.

underdarknl commented 1 month ago

Fortunately, in the meantime between creating this issue and now, we have reworked the boefje and normalizer to use the openkat/nmap-output for nmap and openkat/dnssec-output for dnssec. Therefore, we can safely return error/boefje next to that without interfering with the normalizer. However, We need to check that when the boefje errors, it will still output stout, which will let the normalizer have an empty result set, removing all old observations.

That would only happen 'if' the same normalizer is started on the expected output file, with the expected 'openkat/nmap-output' mimetype, right? If that raw file isn't there, since we don't return it if an error occurs no such cleanup should happen.

dekkers commented 1 month ago

The problem we have and what I think is currently the most important part of this to fix is that the boefje succeeds when the command fails and the errors aren't saved. The command signals failure by exiting with a nonzero exit code. We should make sure the boefje fails if the status is nonzero.

I think the best way to fix this is to call output.check_returncode() and then catch and handle CalledProcessError in images/oci_adapter.py. The CalledProcessError exception includes the stderr so we can save the stderr. This way we also don't have a lot of boiler plate code we have to copypaste into every boefje.