Security audit - B411 #1025

4 files changed, 90 insertions, 8 deletions

diff --git a/glances/actions.py b/glances/actions.py
index ea94a816..02b4724f 100644
--- a/glances/actions.py
+++ b/glances/actions.py
@@ -19,10 +19,9 @@
 
 """Manage on alert actions."""
 
-from subprocess import Popen
-
 from glances.logger import logger
 from glances.timer import Timer
+from glances.secure import secure_popen
 
 try:
     import chevron
@@ -94,12 +93,16 @@ class GlancesActions(object):
             logger.info("Action triggered for {} ({}): {}".format(stat_name,
                                                                   criticity,
                                                                   cmd_full))
-            logger.debug("Action will be executed with the following command: \
-                subprocess.Popen({}, shell=False)".format(cmd_full.split(' ')))
             try:
-                Popen(cmd_full.split(' '), shell=False)
+                ret = secure_popen(cmd_full)
             except OSError as e:
-                logger.error("Can't execute the action ({})".format(e))
+                logger.error("Action error for {} ({}): {}".format(stat_name,
+                                                                   criticity,
+                                                                   e))
+            else:
+                logger.debug("Action result for {} ({}): {}".format(stat_name,
+                                                                    criticity, 
+                                                                    ret))
 
         self.set(stat_name, criticity)
 
diff --git a/glances/config.py b/glances/config.py
index 2e001b65..e8053019 100644
--- a/glances/config.py
+++ b/glances/config.py
@@ -293,7 +293,7 @@ class Config(object):
         If it did not exist, then return the default value.
 
         It allows user to define dynamic configuration key (see issue#1204)
-        Dynamic vlaue should starts and end with the ` char
+        Dynamic value should starts and end with the ` char
         Example: prefix=`hostname`
         """
         ret = default
diff --git a/glances/secure.py b/glances/secure.py
new file mode 100644
index 00000000..7a2f8d7b
--- /dev/null
+++ b/glances/secure.py
@@ -0,0 +1,73 @@
+# -*- coding: utf-8 -*-
+#
+# This file is part of Glances.
+#
+# Copyright (C) 2021 Nicolargo <nicolas@nicolargo.com>
+#
+# Glances is free software; you can redistribute it and/or modify
+# it under the terms of the GNU Lesser General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# Glances is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+"""Secures functions for Glances"""
+
+from glances.compat import nativestr
+from subprocess import Popen, PIPE
+
+
+def secure_popen(cmd):
+    """A more or less secure way to execute system command
+
+    Return: the result of the command OR an error message
+    """
+    ret = None
+
+    # Split by redirection '>'
+    cmd_split_redirect = cmd.split('>')
+    if len(cmd_split_redirect) > 2:
+        return 'Glances error: Only one file redirection allowed ({})'.format(cmd)
+    elif len(cmd_split_redirect) == 2:
+        stdout_redirect = cmd_split_redirect[1].strip()
+        cmd = cmd_split_redirect[0]
+    else:
+        stdout_redirect = None
+
+    sub_cmd_stdin = None
+    p_last = None
+    # Split by pipe '|'
+    for sub_cmd in cmd.split('|'):
+        # Split by space ' '
+        sub_cmd_split = [i for i in sub_cmd.split(' ') if i]
+        p = Popen(sub_cmd_split,
+                  shell=False,
+                  stdin=sub_cmd_stdin,
+                  stdout=PIPE,
+                  stderr=PIPE)
+        if p_last is not None:
+            # Allow p_last to receive a SIGPIPE if p exits.
+            p_last.stdout.close()
+        p_last = p
+        sub_cmd_stdin = p.stdout
+
+    p_ret = p_last.communicate()
+
+    if nativestr(p_ret[1]) == '':
+        # No error
+        ret = nativestr(p_ret[0])
+        if stdout_redirect is not None:
+            # Write result to redirection file
+            with open(stdout_redirect, "w") as stdout_redirect_file:
+                stdout_redirect_file.write(ret)
+    else:
+        # Error
+        ret = nativestr(p_ret[1])
+
+    return ret
diff --git a/unitest.py b/unitest.py
index ea91ea9e..0ed66d5b 100755
--- a/unitest.py
+++ b/unitest.py
@@ -35,11 +35,11 @@ from glances.thresholds import GlancesThresholdCritical
 from glances.thresholds import GlancesThresholds
 from glances.plugins.glances_plugin import GlancesPlugin
 from glances.compat import subsample, range
+from glances.secure import secure_popen
 
 # Global variables
 # =================
 
-
 # Init Glances core
 core = GlancesMain()
 
@@ -376,6 +376,12 @@ class TestGlances(unittest.TestCase):
         bar.percent = 101
         self.assertGreaterEqual(bar.percent, bar.max_value)
 
+    def test_100_secure(self):
+        """Test secure functions"""
+        print('INFO: [TEST_100] Secure functions')
+        self.assertEqual(secure_popen('echo -n TEST'), 'TEST')
+        self.assertEqual(secure_popen('echo FOO | grep FOO'), 'FOO\n')
+
     def test_999_the_end(self):
         """Free all the stats"""
         print('INFO: [TEST_999] Free the stats')