Various security improvements

[clutch2sft]

I am writing this issue to submit a pull request/s against it.

I will submit code fixes to mitigate these risks:

Command Injection Risk:

The _exec_command function uses subprocess.Popen with commands split by spaces. This can potentially be risky if any part of the command is influenced by user input, leading to command injection vulnerabilities. Ensure that no part of these commands can be modified by user input.

Error Handling:

The application returns a 500 Internal Server Error when a subprocess execution fails. While this is good for indicating failure, ensure that no sensitive information is leaked through error messages or stack traces.

Logging:

The code does not show any logging mechanism. Proper logging of access and errors is crucial for monitoring and identifying potential security breaches.

Input Validation:

The application seems to lack explicit input validation, especially for the key parameter. Ensuring strict validation of all inputs is a fundamental security practice.

Security Headers:

Implement appropriate HTTP security headers in the Flask application to mitigate common web vulnerabilities.

Thank you,

Gregg

[nemesifier]

I am writing this issue to submit a pull request/s against it. I will submit code fixes to mitigate these risks:

Thanks!

Command Injection Risk:

The _exec_command function uses subprocess.Popen with commands split by spaces. This can potentially be risky if any part of the command is influenced by user input, leading to command injection vulnerabilities. Ensure that no part of these commands can be modified by user input.

Fair enough.

Error Handling:

The application returns a 500 Internal Server Error when a subprocess execution fails. While this is good for indicating failure, ensure that no sensitive information is leaked through error messages or stack traces.

AFAIK the application in production is not meant to leak any stack trace.

Logging:

The code does not show any logging mechanism. Proper logging of access and errors is crucial for monitoring and identifying potential security breaches.

See also https://github.com/openwisp/ansible-wireguard-openwisp/issues/13.

Input Validation:

The application seems to lack explicit input validation, especially for the key parameter. Ensuring strict validation of all inputs is a fundamental security practice.

Fair enough. I would like to define this a bit more though, what kind of validation do you want to introduce?

Security Headers:

Implement appropriate HTTP security headers in the Flask application to mitigate common web vulnerabilities.

Maybe this should be done in uwsgi rather than flask?

[clutch2sft]

Fair enough. I would like to define this a bit more though, what kind of validation do you want to introduce?

For now I do a check to make sure we are only calling allowed scripts, and changed the validation of the key to use hmac compare_digest. I wasn't sure if we should validate the length of the key but maybe not a bad idea - picking the right length then is the difficult task. In hindsight most of this is probably over-kill given the real work of this module is to simply trigger pre-configured tasks.... but applying common Flask security mechanisms never hurts.