arachnys / cabot

Self-hosted, easily-deployable monitoring and alerts service - like a lightweight PagerDuty
MIT License
5.6k stars 592 forks source link

Refactor ICMPStatusCheck run method #571

Closed JeanFred closed 7 years ago

JeanFred commented 7 years ago

The ICMPStatusCheck was performed by shelling out to the ping executable, by building a string for the command and supplying it to subprocess.Popen.

This is dangerous because it allowed for shell injections, as the content of the command is partly user controlled: one could set the instance address to 8.8.8.8; rm -rf /, which would be happily executed.

This is avoided by passing a list of strings to subprocess, so that the arguments are all passed to ping. The above example then results in ping: bad address '8.8.8.8 ; ls -l'

The issue was detected by the Bandit linter: B602:subprocess_popen_with_shell_equals_true

Additionnally, we can simplify the flow by using check_output while redirecting stderr to STDOUT, and catching the CalledProcessError when the command fails.

This also adds some unit tests for this check.

See also: https://security.openstack.org/guidelines/dg_use-subprocess-securely.html

codecov-io commented 7 years ago

Codecov Report

Merging #571 into master will increase coverage by 0.77%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #571      +/-   ##
==========================================
+ Coverage   79.76%   80.53%   +0.77%     
==========================================
  Files          43       43              
  Lines        2846     2846              
  Branches      174      173       -1     
==========================================
+ Hits         2270     2292      +22     
+ Misses        517      495      -22     
  Partials       59       59
Impacted Files Coverage Δ
cabot/cabotapp/models/base.py 79.4% <100%> (+3.52%) :arrow_up:
cabot/cabotapp/tasks.py 61.66% <0%> (+5%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 2e52785...541a2c5. Read the comment docs.