From 38d204860f8fe5e0eb15ba294efed56953161023 Mon Sep 17 00:00:00 2001 From: Ignace Mouzannar Date: Mon, 9 Mar 2026 21:58:22 -0400 Subject: [PATCH 1/2] Fix configuration handling and enhance path ACL checks --- etc/lshell.conf | 2 +- lshell/checkconfig.py | 37 ++-- lshell/completion.py | 3 + lshell/configschema.py | 9 +- lshell/policy.py | 13 +- lshell/sec.py | 187 +++++++++++++++++---- test/test_config.py | 57 +++++++ test/test_file_extension.py | 27 +++ test/test_parser_jobs_unit.py | 15 ++ test/test_path.py | 26 +++ test/test_policy.py | 91 ++++++++++ test/test_security_attack_surface_unit.py | 82 +++++++++ test/test_security_hardening_functional.py | 32 ++++ test/test_ssh.py | 24 +++ test/test_unit.py | 63 ++++++- 15 files changed, 606 insertions(+), 62 deletions(-) diff --git a/etc/lshell.conf b/etc/lshell.conf index c85530d..35ecd64 100644 --- a/etc/lshell.conf +++ b/etc/lshell.conf @@ -44,7 +44,7 @@ loglevel : 2 ## will allow users to execute code (e.g. /bin/sh) from within the application, ## thus easily escaping lshell. See variable 'path_noexec' to use an alternative ## path to library. -allowed : ['ls','echo','ll','vim','tail','sleep','touch','mkdir','cat','export', 'pwd', './shutdown.sh', './shitdown.sh', 'shutdown.sh', 'shitdown.sh'] +allowed : ['ls','echo','ll','vim','tail','sleep','touch','mkdir','cat','export', 'pwd'] #allowed : ['echo test'] # this will allow only the command 'echo test' ## A list of the allowed commands that are permitted to execute other diff --git a/lshell/checkconfig.py b/lshell/checkconfig.py index 93bff2c..5e7831b 100644 --- a/lshell/checkconfig.py +++ b/lshell/checkconfig.py @@ -1,4 +1,4 @@ -""" This module contains the checkconfig class of lshell """ +"""This module contains the checkconfig class of lshell""" import sys import os @@ -353,12 +353,13 @@ def get_config_sub(self, section): self.minusplus(self.conf_raw, key, stuff) ) elif configschema.is_all_literal(stuff): - if key == "allowed_shell_escape": + if key == "allowed": + self.conf_raw.update({key: self.expand_all()}) + else: self.log.critical( - "lshell: config: 'allowed_shell_escape' cannot be set to 'all'" + f"lshell: config: '{key}' cannot be set to 'all'" ) sys.exit(1) - self.conf_raw.update({key: self.expand_all()}) elif stuff and key == "path": liste = ["", ""] for path in self._parse_config_value(stuff, key): @@ -371,8 +372,8 @@ def get_config_sub(self, section): self._parse_config_value(stuff, key), list ): self.conf_raw.update({key: stuff}) - # case allowed is set to 'all' - elif key == "allowed" and split[0].strip() == "'all'": + # case allowed/sudo_commands is set to all + elif key == "allowed" and configschema.is_all_literal(split[0]): self.conf_raw.update({key: self.expand_all()}) elif key == "allowed_shell_escape" and configschema.is_all_literal( split[0] @@ -472,7 +473,9 @@ def _parse_config_value(self, value, key=""): def _parse_history_file(self): """Parse and validate history_file from raw config.""" - history_value = self.conf_raw["history_file"].replace("%u", self.conf["username"]) + history_value = self.conf_raw["history_file"].replace( + "%u", self.conf["username"] + ) if ( isinstance(history_value, str) and len(history_value) >= 2 @@ -555,7 +558,9 @@ def get_config_user(self): if len(self.conf_raw[item]) == 0: self.conf[item] = "" else: - self.conf[item] = self._parse_config_value(self.conf_raw[item], item) + self.conf[item] = self._parse_config_value( + self.conf_raw[item], item + ) except KeyError: if item in [ "allowed", @@ -718,15 +723,15 @@ def get_config_user(self): self.conf["allowed"].append(item) # case sudo_commands set to 'all', expand to all 'allowed' commands - if ( - "sudo_commands" in self.conf_raw - and configschema.is_all_literal(str(self.conf_raw["sudo_commands"])) + if "sudo_commands" in self.conf_raw and configschema.is_all_literal( + str(self.conf_raw["sudo_commands"]) ): - # exclude native commands and sudo(8) - exclude = builtincmd.builtins_list + ["sudo"] - self.conf["sudo_commands"] = [ - x for x in self.conf["allowed"] if x not in exclude - ] + # Keep shell-internal builtins out of sudo all-expansion while + # preserving `ls`, which is executed via exec_cmd in lshell. + exclude = [cmd for cmd in builtincmd.builtins_list if cmd != "ls"] + ["sudo"] + self.conf["sudo_commands"] = list( + dict.fromkeys(x for x in self.conf["allowed"] if x not in exclude) + ) # sort lsudo commands self.conf["sudo_commands"].sort() diff --git a/lshell/completion.py b/lshell/completion.py index 939b9cb..13bc277 100644 --- a/lshell/completion.py +++ b/lshell/completion.py @@ -83,6 +83,9 @@ def complete_change_dir(conf, text, line, begidx, endidx): if instance.startswith(directory) and instance.startswith(tocomplete): # Extract the next unmatched segment of the allowed path remaining_path = instance[len(directory) :].lstrip("/") + # Nothing left to suggest for this allowed path. + if not remaining_path: + continue if "/" in remaining_path: next_segment = remaining_path.split("/", 1)[0] + "/" else: diff --git a/lshell/configschema.py b/lshell/configschema.py index d0184dd..a032e63 100644 --- a/lshell/configschema.py +++ b/lshell/configschema.py @@ -85,9 +85,12 @@ def parse_config_value(value, key=""): Raises ValueError with user-friendly field-level errors. """ - if isinstance(value, str) and key in {"allowed", "sudo_commands"}: - if value.strip() == "all": - return "all" + if ( + isinstance(value, str) + and key in {"allowed", "sudo_commands"} + and is_all_literal(value) + ): + return "all" try: evaluated = ast.literal_eval(value) diff --git a/lshell/policy.py b/lshell/policy.py index e000f25..c27a332 100644 --- a/lshell/policy.py +++ b/lshell/policy.py @@ -185,7 +185,10 @@ def _merge_section(conf_raw, section, section_items, key_sources, trace): raise ValueError( "'allowed_shell_escape' cannot be set to 'all'" ) - conf_raw.update({key: _expand_all()}) + if key == "allowed": + conf_raw.update({key: _expand_all()}) + else: + raise ValueError(f"'{key}' cannot be set to 'all'") trace.append( { "section": section, @@ -231,7 +234,7 @@ def _merge_section(conf_raw, section, section_items, key_sources, trace): } ) previous = conf_raw.get(key) - elif key == "allowed" and split[0].strip() == "'all'": + elif key == "allowed" and configschema.is_all_literal(split[0]): conf_raw.update({key: _expand_all()}) trace.append( { @@ -357,8 +360,10 @@ def _build_runtime_policy(conf_raw, username): if "sudo_commands" in conf_raw and configschema.is_all_literal( str(conf_raw["sudo_commands"]) ): - exclude = builtincmd.builtins_list + ["sudo"] - policy["sudo_commands"] = [x for x in policy["allowed"] if x not in exclude] + exclude = [cmd for cmd in builtincmd.builtins_list if cmd != "ls"] + ["sudo"] + policy["sudo_commands"] = list( + dict.fromkeys(x for x in policy["allowed"] if x not in exclude) + ) policy["allowed"] += policy["allowed_shell_escape"] diff --git a/lshell/sec.py b/lshell/sec.py index bc73591..32001bd 100644 --- a/lshell/sec.py +++ b/lshell/sec.py @@ -110,19 +110,136 @@ def tokenize_command(command): def expand_shell_wildcards(item): - """Expand shell wildcards in the item and return the expanded path""" - - # Expand shell variables like $HOME - item = os.path.expanduser(item) - item = os.path.expandvars(item) - item = os.path.realpath(item) # this is a hack - needs to be reviewed - # test if item is a directory - expanded_items = glob.glob(item, recursive=True) + """Expand shell wildcards and return all candidate filesystem paths.""" + + # Expand shell variables like $HOME first. + expanded_item = os.path.expanduser(item) + expanded_item = os.path.expandvars(expanded_item) + + # Expand wildcard patterns against the filesystem and validate all matches. + expanded_items = glob.glob(expanded_item, recursive=True) if expanded_items: - # Return all matches instead of just the first one - item = expanded_items[0] + return [os.path.realpath(match) for match in expanded_items] + + # If no glob match exists, still validate the canonical target path. + return [os.path.realpath(expanded_item)] + + +def _split_path_acl_entries(path_acl): + """Convert legacy path ACL string format to canonical path entries.""" + if not path_acl: + return [] + + entries = [] + for token in str(path_acl).split("|"): + candidate = token.strip() + if not candidate: + continue + entries.append(os.path.realpath(candidate)) + return entries + + +def _is_path_within_base(path, base): + """Return True when path is equal to or nested under base.""" + try: + return os.path.commonpath([path, base]) == base + except ValueError: + # Different mount/drive semantics: treat as not matching. + return False + + +def _is_path_allowed(candidate, allowed_roots, denied_roots): + """Return True when candidate path passes allow/deny ACL precedence. + + Specificity rule: + - most specific matching prefix wins; + - ties favor deny. + This preserves historical expectation that: + ['/'] - ['/var'] + ['/var/log'] + allows /var/log while still denying /var. + """ + + def _specificity(path): + normalized = os.path.normpath(path) + if normalized == os.sep: + return 0 + return len([segment for segment in normalized.split(os.sep) if segment]) + + matching_allows = [root for root in allowed_roots if _is_path_within_base(candidate, root)] + matching_denies = [root for root in denied_roots if _is_path_within_base(candidate, root)] + + # Legacy behavior: empty allow-list means unrestricted unless denied. + if not allowed_roots: + return not bool(matching_denies) + + if not matching_allows: + return False + + best_allow = max(_specificity(root) for root in matching_allows) + best_deny = max(_specificity(root) for root in matching_denies) if matching_denies else -1 + + return best_allow > best_deny + - return item +def _format_path_for_message(path): + """Format path in user-facing messages with historical trailing-slash behavior.""" + if os.path.isdir(path) and not path.endswith("/"): + return f"{path}/" + return path + + +def _looks_like_path_token(token): + """Heuristic: return True if a token appears to reference a filesystem path.""" + if not token: + return False + if token.startswith(("/", ".", "~")): + return True + if "/" in token or "\\" in token: + return True + if any(char in token for char in ["*", "?", "[", "]"]): + return True + return False + + +def _path_tokens_from_line(line): + """Extract path-like tokens from command segments, excluding bare command names.""" + segments = utils.split_commands(line) + if not segments: + return [] + + path_tokens = [] + for segment in segments: + try: + tokens = shlex.split(segment, posix=True) + except ValueError: + tokens = tokenize_command(segment) + if not tokens: + continue + + index = 0 + while index < len(tokens) and _is_assignment_word(tokens[index]): + index += 1 + if index >= len(tokens): + continue + + command = tokens[index] + args = tokens[index + 1 :] + + if command == "cd" and args: + # `cd var` style operands are path targets even without slashes. + path_tokens.append(args[0]) + continue + + if args: + path_tokens.extend(token for token in args if _looks_like_path_token(token)) + continue + + # Single token mode (used by completion/policy path checks): + # only treat it as a path when it looks path-like. + if _looks_like_path_token(command): + path_tokens.append(command) + + return path_tokens def check_path(line, conf, completion=None, ssh=None, strict=None): @@ -130,32 +247,32 @@ def check_path(line, conf, completion=None, ssh=None, strict=None): are allowed to see this path. If user is not allowed, it calls warn_count. In case of completion, it only returns 0 or 1. """ - allowed_path_re = str(conf["path"][0]) - denied_path_re = str(conf["path"][1][:-1]) - - line = tokenize_command(line) - - for item in line: - tomatch = expand_shell_wildcards(item) - if os.path.isdir(tomatch) and tomatch[-1] != "/": - tomatch += "/" - match_allowed = re.findall(allowed_path_re, tomatch) - if denied_path_re: - match_denied = re.findall(denied_path_re, tomatch) - else: - match_denied = None - - # if path not allowed - # case path executed: warn, and return 1 - # case completion: return 1 - if not match_allowed or match_denied: - if not completion: - ret, conf = warn_count("path", tomatch, conf, strict=strict, ssh=ssh) - return 1, conf + allowed_roots = _split_path_acl_entries(conf["path"][0]) + denied_roots = _split_path_acl_entries(conf["path"][1]) + + path_tokens = _path_tokens_from_line(line) + + for item in path_tokens: + candidates = expand_shell_wildcards(item) + for candidate in candidates: + if not _is_path_allowed(candidate, allowed_roots, denied_roots): + if not completion: + message_path = _format_path_for_message(candidate) + ret, conf = warn_count( + "path", message_path, conf, strict=strict, ssh=ssh + ) + return 1, conf if not completion: - if not re.findall(allowed_path_re, os.getcwd() + "/"): - ret, conf = warn_count("path", tomatch, conf, strict=strict, ssh=ssh) + current_dir = os.path.realpath(os.getcwd()) + if not _is_path_allowed(current_dir, allowed_roots, denied_roots): + ret, conf = warn_count( + "path", + _format_path_for_message(current_dir), + conf, + strict=strict, + ssh=ssh, + ) os.chdir(conf["home_path"]) conf["promptprint"] = utils.updateprompt(os.getcwd(), conf) return 1, conf diff --git a/test/test_config.py b/test/test_config.py index 28a235f..532d58d 100644 --- a/test/test_config.py +++ b/test/test_config.py @@ -45,6 +45,18 @@ def test_55_allowed_all_minus_list(self): self.assertEqual(expected, output) self.do_exit(child) + def test_55b_allowed_all_unquoted_allows_non_default_command(self): + """F55b | allowed=all (unquoted) should allow commands outside default list.""" + child = pexpect.spawn(f"{LSHELL} --config {CONFIG} --allowed all") + child.expect(PROMPT) + + child.sendline("uname") + child.expect(PROMPT) + output = child.before.decode("utf-8") + self.assertNotIn("lshell: unknown syntax:", output) + self.assertIn("Linux", output) + self.do_exit(child) + def test_56_path_minus_specific_path(self): """F56 | allow paths except for the specified path""" @@ -184,3 +196,48 @@ def test_64_custom_messages_override_warning_output(self): "*** You have 1 warning(s) left, before getting kicked out.", output ) self.do_exit(child) + + def test_65_allowed_shell_escape_plus_minus_chain(self): + """F65 | +/- merge on allowed_shell_escape impacts command allow-list.""" + child = pexpect.spawn( + f"{LSHELL} --config {CONFIG} " + "--allowed \"[]\" " + "--allowed_shell_escape \"['echo'] + ['printf'] - ['echo']\" " + "--forbidden \"[]\" --warning_counter 2 --strict 1" + ) + child.expect(PROMPT) + + child.sendline("echo blocked") + child.expect(PROMPT) + denied_output = child.before.decode("utf-8") + self.assertIn('lshell: forbidden command: "echo"', denied_output) + self.assertIn("lshell: warning: 1 violation remaining", denied_output) + + child.sendline("printf ALLOWED") + child.expect(PROMPT) + allowed_output = child.before.decode("utf-8") + self.assertIn("ALLOWED", allowed_output) + + self.do_exit(child) + + def test_66_sudo_commands_all_quoted_reflected_in_policy_sudo(self): + """F66 | sudo_commands='all' should expose effective sudo allow-list.""" + child = pexpect.spawn( + f"{LSHELL} --config {CONFIG} " + "--allowed \"['ls','echo','cat']\" " + "--sudo_commands \"'all'\"" + ) + child.expect(PROMPT) + + child.sendline("policy-sudo") + child.expect(PROMPT) + output = child.before.decode("utf-8") + self.assertIn("Sudo access : enabled", output) + self.assertIn("Allowed via sudo", output) + allowed_line = [ + line for line in output.splitlines() if "Allowed via sudo" in line + ][0] + self.assertIn("echo", allowed_line) + self.assertIn("ls", allowed_line) + + self.do_exit(child) diff --git a/test/test_file_extension.py b/test/test_file_extension.py index e1b9d51..42c2cfd 100644 --- a/test/test_file_extension.py +++ b/test/test_file_extension.py @@ -104,3 +104,30 @@ def test_63_extensionless_filename_is_forbidden(self): self.assertEqual(expected, output) self.assertFalse(os.path.exists(target)) self.do_exit(child) + + def test_64_allowed_file_extensions_plus_minus_chain(self): + """F64 | +/- merge on allowed_file_extensions controls warning outcome.""" + f_name = "test_60_allowed_extension_success" + log_file = f"{TOPDIR}/test/testfiles/{f_name}.log" + conf_file = CONFIG + + child = pexpect.spawn( + f"{LSHELL} --config {CONFIG} " + "--allowed \"+ ['cat']\" " + "--allowed_file_extensions \"['.log'] + ['.conf'] - ['.log']\" " + "--warning_counter 2 --strict 1" + ) + child.expect(PROMPT) + + child.sendline(f"cat {log_file}") + child.expect(PROMPT) + denied_output = child.before.decode("utf-8") + self.assertIn("forbidden file extension ['.log']", denied_output) + self.assertIn("lshell: warning: 1 violation remaining", denied_output) + + child.sendline(f"cat {conf_file}") + child.expect(PROMPT) + allowed_output = child.before.decode("utf-8") + self.assertIn("[global]", allowed_output) + + self.do_exit(child) diff --git a/test/test_parser_jobs_unit.py b/test/test_parser_jobs_unit.py index 56d45c7..e27c77e 100644 --- a/test/test_parser_jobs_unit.py +++ b/test/test_parser_jobs_unit.py @@ -219,6 +219,21 @@ def test_complete_list_dir_returns_empty_when_path_denied(self): self.assertEqual(result, []) + def test_complete_change_dir_denied_path_does_not_suggest_root_slash(self): + """Denied cd completion should not suggest a standalone '/' segment.""" + conf = { + "home_path": "/home/testuser", + "path": ["/var/log/|", ""], + } + with patch("lshell.completion.os.getcwd", return_value="/home/testuser"), patch( + "lshell.completion.sec.check_path", return_value=(1, conf) + ): + result = completion.complete_change_dir( + conf, "", "cd /var/log/", len("cd "), len("cd /var/log/") + ) + + self.assertNotIn("/", result) + def test_completenames_dot_slash_with_basename_text(self): """Complete ./ commands when readline provides text without ./ prefix.""" conf = {"allowed": ["ls", "./shutdown.sh", "./show.sh"]} diff --git a/test/test_path.py b/test/test_path.py index 654c8ec..be33993 100644 --- a/test/test_path.py +++ b/test/test_path.py @@ -118,3 +118,29 @@ def test_21_allow_slash(self): result = child.before.decode("utf8").split("\n")[1].strip() self.assertEqual(expected, result) self.do_exit(child) + + def test_22_path_plus_minus_reallow_and_warning_messages(self): + """F22 | path +/- chain should re-allow child path and keep warning countdown.""" + child = pexpect.spawn( + f"{LSHELL} --config {CONFIG} " + "--path \"['/'] - ['/var/','/var/lib/'] + ['/var/log']\" " + "--warning_counter 2 --strict 1" + ) + child.expect(PROMPT) + + child.sendline("cd /var") + child.expect(PROMPT) + output_1 = child.before.decode("utf8") + self.assertIn('lshell: forbidden path: "/var/"', output_1) + self.assertIn("lshell: warning: 1 violation remaining", output_1) + + child.sendline("cd /var/log") + child.expect(f"{USER}:/var/log\\$") + + child.sendline("cd /var/lib") + child.expect(f"{USER}:/var/log\\$") + output_2 = child.before.decode("utf8") + self.assertIn('lshell: forbidden path: "/var/lib/"', output_2) + self.assertIn("lshell: warning: 0 violations remaining", output_2) + + self.do_exit(child) diff --git a/test/test_policy.py b/test/test_policy.py index d1f1026..fc769f9 100644 --- a/test/test_policy.py +++ b/test/test_policy.py @@ -149,6 +149,44 @@ def test_policy_command_decision_exempts_builtin_ls_from_extension_filter(self): decision = policy.policy_command_decision("ls /tmp", runtime_policy) self.assertTrue(decision["allowed"]) + def test_resolve_policy_allowed_all_unquoted_expands(self): + """EX03e | allowed=all (unquoted) should expand successfully.""" + with tempfile.TemporaryDirectory() as tempdir: + config = self._write_config( + tempdir, + """ + [global] + logpath : /tmp + loglevel : 0 + + [default] + allowed : all + forbidden : [';'] + warning_counter : 2 + """, + ) + result = policy.resolve_policy(config, "bleh", []) + self.assertIn("ls", result["policy"]["allowed"]) + + def test_resolve_policy_allowed_all_minus_list(self): + """EX03f | allowed supports all - [item] merge semantics.""" + with tempfile.TemporaryDirectory() as tempdir: + config = self._write_config( + tempdir, + """ + [global] + logpath : /tmp + loglevel : 0 + + [default] + allowed : all - ['echo'] + forbidden : [';'] + warning_counter : 2 + """, + ) + result = policy.resolve_policy(config, "bleh", []) + self.assertNotIn("echo", result["policy"]["allowed"]) + def test_build_resolved_rows_order_and_user_origin(self): """EX04 | rows are ordered and user section label is explicit.""" result = { @@ -268,6 +306,59 @@ def test_resolve_policy_falls_back_to_grp_user_section(self): result = policy.resolve_policy(config, "testuser", []) self.assertIn("grp:testuser", result["applied_sections"]) + def test_resolve_policy_plus_minus_supported_for_all_list_merge_keys(self): + """EX08b | policy mode applies +/- merges on every merge-capable list key.""" + with tempfile.TemporaryDirectory() as tempdir: + config = self._write_config( + tempdir, + """ + [global] + logpath : /tmp + loglevel : 0 + + [default] + allowed : ['basecmd'] + allowed_shell_escape : ['ase_base'] + allowed_file_extensions : ['.log'] + forbidden : [';'] + overssh : ['scp', 'rsync'] + path : ['/'] + warning_counter : 2 + + [user:bleh] + allowed : + ['pluscmd'] - ['basecmd'] + allowed_shell_escape : + ['ase_plus'] - ['ase_base'] + allowed_file_extensions : + ['.txt'] - ['.log'] + forbidden : + ['#'] - [';'] + overssh : + ['ls'] - ['scp'] + path : - ['/var', '/etc'] + ['/var/log'] + """, + ) + + result = policy.resolve_policy(config, "bleh", []) + runtime_policy = result["policy"] + + self.assertIn("pluscmd", runtime_policy["allowed"]) + self.assertNotIn("basecmd", runtime_policy["allowed"]) + + self.assertEqual(set(runtime_policy["allowed_shell_escape"]), {"ase_plus"}) + self.assertEqual(set(runtime_policy["allowed_file_extensions"]), {".txt"}) + + self.assertIn("#", runtime_policy["forbidden"]) + self.assertNotIn(";", runtime_policy["forbidden"]) + + self.assertIn("rsync", runtime_policy["overssh"]) + self.assertIn("ls", runtime_policy["overssh"]) + self.assertNotIn("scp", runtime_policy["overssh"]) + + self.assertTrue(runtime_policy["path"][0].startswith("/|")) + self.assertIn( + f"{os.path.realpath('/var/log')}/|", + runtime_policy["path"][0], + ) + self.assertIn(f"{os.path.realpath('/var')}/|", runtime_policy["path"][1]) + self.assertIn(f"{os.path.realpath('/etc')}/|", runtime_policy["path"][1]) + def test_main_without_command_returns_success(self): """EX09 | policy show without command should print resolved values and exit 0.""" with tempfile.TemporaryDirectory() as tempdir: diff --git a/test/test_security_attack_surface_unit.py b/test/test_security_attack_surface_unit.py index b323ac9..51bc8b7 100644 --- a/test/test_security_attack_surface_unit.py +++ b/test/test_security_attack_surface_unit.py @@ -7,6 +7,7 @@ import io import os +import tempfile import unittest from contextlib import redirect_stderr from unittest.mock import patch @@ -99,6 +100,87 @@ def test_check_path_forbidden_decrements_counter_even_when_not_strict(self): self.assertEqual(ret, 1) self.assertEqual(_conf["warning_counter"], starting_counter - 1) + def test_check_path_uses_canonical_prefix_not_substring_regex(self): + """Do not allow sibling paths that only share a string prefix.""" + with tempfile.TemporaryDirectory(prefix="lshell-path-prefix-", dir="/tmp") as tmpdir: + allowed_dir = os.path.join(tmpdir, "allow") + sibling_dir = os.path.join(tmpdir, "allow-evil") + os.makedirs(allowed_dir, exist_ok=True) + os.makedirs(sibling_dir, exist_ok=True) + + conf = CheckConfig( + self.args + [f"--path=['{allowed_dir}']", "--strict=0"] + ).returnconf() + ret, _conf = sec.check_path(f"ls {sibling_dir}", conf, strict=0) + self.assertEqual(ret, 1) + + def test_check_path_validates_all_glob_matches(self): + """Validate every wildcard expansion result, not only the first match.""" + with tempfile.TemporaryDirectory(prefix="lshell-path-glob-", dir="/tmp") as tmpdir: + allowed_dir = os.path.join(tmpdir, "a_allowed") + blocked_dir = os.path.join(tmpdir, "z_blocked") + os.makedirs(allowed_dir, exist_ok=True) + os.makedirs(blocked_dir, exist_ok=True) + + conf = CheckConfig( + self.args + [f"--path=['{allowed_dir}']", "--strict=0"] + ).returnconf() + ret, _conf = sec.check_path(f"ls {tmpdir}/*", conf, strict=0) + self.assertEqual(ret, 1) + + def test_check_path_allow_root_minus_var_allows_cd_root(self): + """Root allow-list entry '/' must stay valid when minus-paths are present.""" + conf = CheckConfig( + self.args + ["--path=['/'] - ['/var']", "--strict=0"] + ).returnconf() + ret, _conf = sec.check_path("cd /", conf, strict=0) + self.assertEqual(ret, 0) + + def test_check_path_root_minus_var_denies_relative_cd_var(self): + """Relative cd target should still be validated against path ACL.""" + conf = CheckConfig( + self.args + ["--path=['/'] - ['/var']", "--strict=0"] + ).returnconf() + previous_cwd = os.getcwd() + try: + os.chdir("/") + ret, _conf = sec.check_path("cd var", conf, strict=0) + self.assertEqual(ret, 1) + finally: + os.chdir(previous_cwd) + + def test_check_path_more_specific_allow_overrides_broader_deny(self): + """Specific allow path should win over broader deny prefix.""" + conf = CheckConfig( + self.args + + ["--path=['/'] - ['/var','/var/lib'] + ['/var/log']", "--strict=0"] + ).returnconf() + ret, _conf = sec.check_path("cd /var/log", conf, strict=0) + self.assertEqual(ret, 0) + + def test_check_path_does_not_treat_command_word_as_path(self): + """Command names like 'cd' must not be reported as denied filesystem paths.""" + conf = CheckConfig( + self.args + ["--path=['/tmp']", "--strict=0", "--quiet=0"] + ).returnconf() + + with patch("lshell.sec.warn_count", return_value=(1, conf)) as mock_warn: + ret, _conf = sec.check_path("cd /var/log", conf, strict=0) + + self.assertEqual(ret, 1) + self.assertTrue(mock_warn.called) + warned_path = mock_warn.call_args.args[1] + self.assertNotEqual(warned_path, os.path.realpath("cd")) + self.assertNotEqual(os.path.basename(warned_path.rstrip("/")), "cd") + + def test_check_path_completion_still_validates_single_path_token(self): + """Single-token path checks (completion/policy mode) must keep working.""" + conf = CheckConfig( + self.args + ["--path=['/home', '/var']", "--strict=0"] + ).returnconf() + ret, _conf = sec.check_path("/tmp", conf, completion=1, strict=0) + self.assertEqual(ret, 1) + def test_checkconfig_rejects_allowed_shell_escape_all(self): """Reject allowed_shell_escape=all to avoid bypassing noexec globally.""" with self.assertRaises(SystemExit): diff --git a/test/test_security_hardening_functional.py b/test/test_security_hardening_functional.py index 118d135..b657871 100644 --- a/test/test_security_hardening_functional.py +++ b/test/test_security_hardening_functional.py @@ -138,3 +138,35 @@ def test_bash_env_persistence_should_not_inject_into_future_commands(self): finally: if os.path.exists(bash_env): os.remove(bash_env) + + def test_path_acl_glob_checks_all_matches_and_blocks_forbidden_target(self): + """Glob path checks must fail closed when any expanded item is forbidden.""" + with tempfile.TemporaryDirectory(prefix="lshell-path-hardening-") as tmpdir: + allowed_dir = os.path.join(tmpdir, "a_allowed") + blocked_dir = os.path.join(tmpdir, "z_blocked") + os.makedirs(allowed_dir, exist_ok=True) + os.makedirs(blocked_dir, exist_ok=True) + + result = self._run_lsh_script( + script_body=f"echo {tmpdir}/*\necho SAFE\n", + extra_shell_args=f"--path \"['{allowed_dir}']\" --strict 0", + ) + + combined = result.stdout + result.stderr + self.assertIn(f'lshell: forbidden path: "{blocked_dir}/"', combined) + + def test_path_acl_prefix_confusion_is_blocked(self): + """ACL allow for /path/allow must not allow sibling /path/allow-evil.""" + with tempfile.TemporaryDirectory(prefix="lshell-path-prefix-hardening-") as tmpdir: + allowed_dir = os.path.join(tmpdir, "allow") + sibling_dir = os.path.join(tmpdir, "allow-evil") + os.makedirs(allowed_dir, exist_ok=True) + os.makedirs(sibling_dir, exist_ok=True) + + result = self._run_lsh_script( + script_body=f"echo {sibling_dir}\necho SAFE\n", + extra_shell_args=f"--path \"['{allowed_dir}']\" --strict 0", + ) + + combined = result.stdout + result.stderr + self.assertIn(f'lshell: forbidden path: "{sibling_dir}/"', combined) diff --git a/test/test_ssh.py b/test/test_ssh.py index 0decf1e..78699d1 100644 --- a/test/test_ssh.py +++ b/test/test_ssh.py @@ -123,3 +123,27 @@ def test_57_overssh_all_minus_list(self): output = self.child.before.decode("utf-8").strip() self.assertEqual(expected, output) + + def test_58_overssh_plus_minus_chain_controls_warning_and_allow(self): + """F58 | overssh +/- chain should deny removed command and allow added one.""" + if not os.environ.get("SSH_CLIENT"): + os.environ["SSH_CLIENT"] = "random" + + denied = pexpect.spawn( + f"{LSHELL} --config {CONFIG} " + "--overssh \"['ls'] + ['echo'] - ['ls']\" " + "-c 'ls'" + ) + denied.expect(pexpect.EOF) + denied_output = denied.before.decode("utf-8").strip() + self.assertIn('lshell: forbidden char/command over SSH: "ls"', denied_output) + self.assertIn("This incident has been reported.", denied_output) + + allowed = pexpect.spawn( + f"{LSHELL} --config {CONFIG} " + "--overssh \"['ls'] + ['echo'] - ['ls']\" " + "-c 'echo 1'" + ) + allowed.expect(pexpect.EOF, timeout=10) + allowed_output = allowed.before.decode("utf-8") + self.assertIn("1", allowed_output) diff --git a/test/test_unit.py b/test/test_unit.py index 3e940fd..9b123b8 100644 --- a/test/test_unit.py +++ b/test/test_unit.py @@ -89,6 +89,36 @@ def test_11_checkconfig_configoverwrite(self): userconf = CheckConfig(args).returnconf() return self.assertEqual(userconf["strict"], 123) + def test_11b_merge_plus_minus_supported_for_all_list_merge_keys(self): + """U12b | +/- merge semantics are applied for all merge-capable list keys.""" + args = self.args + [ + "--allowed=['basecmd'] + ['pluscmd'] - ['basecmd']", + "--allowed_shell_escape=['ase_base'] + ['ase_plus'] - ['ase_base']", + "--allowed_file_extensions=['.log'] + ['.txt'] - ['.log']", + "--forbidden=[';'] + ['#'] - [';']", + "--overssh=['scp', 'rsync'] + ['ls'] - ['scp']", + "--path=['/'] - ['/var','/etc'] + ['/var/log']", + ] + userconf = CheckConfig(args).returnconf() + + self.assertIn("pluscmd", userconf["allowed"]) + self.assertNotIn("basecmd", userconf["allowed"]) + + self.assertEqual(set(userconf["allowed_shell_escape"]), {"ase_plus"}) + self.assertEqual(set(userconf["allowed_file_extensions"]), {".txt"}) + + self.assertIn("#", userconf["forbidden"]) + self.assertNotIn(";", userconf["forbidden"]) + + self.assertIn("rsync", userconf["overssh"]) + self.assertIn("ls", userconf["overssh"]) + self.assertNotIn("scp", userconf["overssh"]) + + self.assertTrue(userconf["path"][0].startswith("/|")) + self.assertIn(f"{os.path.realpath('/var/log')}/|", userconf["path"][0]) + self.assertIn(f"{os.path.realpath('/var')}/|", userconf["path"][1]) + self.assertIn(f"{os.path.realpath('/etc')}/|", userconf["path"][1]) + def test_13_multiple_aliases_with_separator(self): """U13 | multiple aliases using &&, || and ; separators""" # enable &, | and ; characters @@ -103,14 +133,41 @@ def test_14_sudo_all_commands_expansion(self): """U14 | sudo_commands set to 'all' is equal to allowed variable""" args = self.args + ["--sudo_commands=all"] userconf = CheckConfig(args).returnconf() - # exclude internal and sudo(8) commands - exclude = builtincmd.builtins_list + ["sudo"] - allowed = [x for x in userconf["allowed"] if x not in exclude] + # exclude shell-internal builtins and sudo(8), but keep `ls` + exclude = [cmd for cmd in builtincmd.builtins_list if cmd != "ls"] + ["sudo"] + allowed = list(dict.fromkeys(x for x in userconf["allowed"] if x not in exclude)) # sort lists to compare userconf["sudo_commands"].sort() allowed.sort() return self.assertEqual(allowed, userconf["sudo_commands"]) + def test_14b_allowed_all_unquoted_expands(self): + """U14b | allowed=all (unquoted) expands to executable allow-list.""" + args = self.args + ["--allowed=all"] + userconf = CheckConfig(args).returnconf() + self.assertIsInstance(userconf["allowed"], list) + self.assertIn("ls", userconf["allowed"]) + + def test_14c_allowed_all_quoted_expands(self): + """U14c | allowed='all' (quoted) expands to executable allow-list.""" + args = self.args + ["--allowed='all'"] + userconf = CheckConfig(args).returnconf() + self.assertIsInstance(userconf["allowed"], list) + self.assertIn("ls", userconf["allowed"]) + + def test_14d_sudo_all_quoted_expansion(self): + """U14d | sudo_commands='all' (quoted) expands against effective allowed list.""" + args = self.args + ["--sudo_commands='all'"] + userconf = CheckConfig(args).returnconf() + self.assertIn("echo", userconf["sudo_commands"]) + self.assertIn("ll", userconf["sudo_commands"]) + self.assertIn("ls", userconf["sudo_commands"]) + self.assertEqual( + userconf["sudo_commands"].count("ls"), + 1, + msg="sudo_commands all-expansion must not duplicate ls", + ) + def test_16_allowed_ld_preload_builtin(self): """U16 | builtin commands should NOT be prepended with LD_PRELOAD""" args = self.args + ["--allowed=['echo','export']"] From d3b3dac96a9ab09648b55f495a7894f048757a86 Mon Sep 17 00:00:00 2001 From: Ignace Mouzannar Date: Mon, 9 Mar 2026 22:04:33 -0400 Subject: [PATCH 2/2] Enhance documentation with best practices and accessibility guidelines in config, README and man page --- README.md | 15 +++++++++++ etc/lshell.conf | 67 ++++++++++++++++++++++++++----------------------- man/lshell.1 | 46 ++++++++++++++++++++++++++++----- 3 files changed, 90 insertions(+), 38 deletions(-) diff --git a/README.md b/README.md index 9baba75..eae13b4 100644 --- a/README.md +++ b/README.md @@ -122,6 +122,14 @@ Make sure lshell is present in `/etc/shells`. The main template is [`etc/lshell.conf`](etc/lshell.conf). Full reference is available in the man page. +### Best practices + +- Prefer an explicit `allowed` allow-list instead of `'all'`. +- Keep `allowed_shell_escape` short and audit every entry. Never add tools that execute arbitrary commands (for example `find`, `vim`, `xargs`). +- Use `allowed_file_extensions` when users are expected to work with a known set of file types. +- Keep `warning_counter` enabled (avoid `-1` unless you intentionally want warning-only behavior). +- Use `policy-show` during reviews to validate effective policy before assigning it to users. + ### Section model and precedence Supported section types: @@ -155,6 +163,7 @@ For local executables, add explicit relative paths (for example `./deploy.sh`). `warning_counter` is decremented on forbidden command/path/character attempts. When `strict = 1`, unknown syntax/commands also decrement `warning_counter`. +`strict = 1` is typically preferred for higher-assurance restricted environments. ### `messages` @@ -199,6 +208,12 @@ messages : { - `allowed_shell_escape`: explicit list of commands allowed to run child programs. Do not set it to `'all'`. - `allowed_file_extensions`: optional allow-list for file extensions passed in command lines. +### Prompt accessibility + +- Keep the default prompt text-based and readable in monochrome terminals. +- If you use ANSI colors in `prompt` or `$LPS1`, avoid color-only meaning (for example, include separators like `user@host:path`). +- Verify contrast and readability over SSH clients commonly used in your environment. + ### `umask` Set a persistent session umask in config: diff --git a/etc/lshell.conf b/etc/lshell.conf index 35ecd64..c90c653 100644 --- a/etc/lshell.conf +++ b/etc/lshell.conf @@ -3,7 +3,7 @@ # $Id: lshell.conf,v 1.27 2010-10-18 19:05:17 ghantoos Exp $ [global] -## log directory (default /var/log/lshell/ ) +## log directory (default: /var/log/lshell/) logpath : /var/log/lshell/ ## set log level to 0, 1, 2, 3 or 4 (0: no logs, 1: least verbose, ## 4: log all commands) @@ -26,49 +26,49 @@ loglevel : 2 ## using the -exec flag. #path_noexec : '/usr/libexec/sudo/sudo_noexec.so' -## include a directory containing multiple configuration files. These files -## can only contain default/user/group configuration. The global configuration will -## only be loaded from the default configuration file. +## include a directory containing multiple configuration files. +## these files can only contain default/user/group configuration. +## global configuration is only loaded from this main configuration file. ## e.g. splitting users into separate files #include_dir : /etc/lshell.d/*.conf [default] -## a list of the allowed commands without execution privileges or 'all' to -## allow all commands in user's PATH +## a list of allowed commands, or 'all' to allow every command in user's PATH +## best practice: prefer an explicit allow-list instead of 'all' ## local commands must be explicitly listed with their relative path ## (e.g. './backup.sh') ## -## if sudo(8) is installed and sudo_noexec.so is available, it will be loaded -## before running every command, preventing it from running further commands +## if sudo(8) is installed and sudo_noexec.so is available, it will be loaded +## before running every command, preventing it from running further commands ## itself. If not available, beware of commands like vim/find/more/etc. that ## will allow users to execute code (e.g. /bin/sh) from within the application, ## thus easily escaping lshell. See variable 'path_noexec' to use an alternative -## path to library. +## path to the library. allowed : ['ls','echo','ll','vim','tail','sleep','touch','mkdir','cat','export', 'pwd'] #allowed : ['echo test'] # this will allow only the command 'echo test' -## A list of the allowed commands that are permitted to execute other +## a list of allowed commands that are permitted to execute other ## programs (e.g. shell scripts with exec(3)). Setting this variable to 'all' -## is NOT allowed. Warning do not put here any command that can execute +## is NOT allowed. warning: do not put here any command that can execute ## arbitrary commands (e.g. find, vim, xargs) ## ## Important: commands defined in 'allowed_shell_escape' override their ## definition in the 'allowed' variable #allowed_shell_escape : ['man','zcat'] -## A list of allowed file extensions that can be provided in the command line. -## If a list of allowed extensions is provided, all other file extensions will be disallowed. +## a list of allowed file extensions that can be provided in the command line. +## if set, all other file extensions are denied. #allowed_file_extensions : ['.tmp', '.log'] -## a list of forbidden character or commands +## a list of forbidden characters or commands forbidden : [';','&', '|','`','>','<', '$(','${'] -## a list of allowed command to use with sudo(8) -## if set to ´all', all the 'allowed' commands will be accessible through sudo(8) +## a list of allowed commands to use with sudo(8) +## if set to 'all', all values from 'allowed' are accessible through sudo(8) sudo_commands : ['ls', 'more'] -## number of warnings when user enters a forbidden value before getting -## exited from lshell, set to -1 to disable. +## number of warnings before lshell terminates the session. +## set to -1 to disable the counter. warning_counter : 2 ## command aliases list (similar to bash’s alias directive) @@ -104,10 +104,12 @@ aliases : {'ll':'ls -l'} ## introduction text to print (when entering lshell) #intro : "== My personal intro ==\nWelcome to lshell\nType '?' or 'help' to get the list of allowed commands" -## configure your prompt using %u or %h (default: username) -# prompt : "%u@%h" -## colored prompt using ANSI escape codes colors (light red user, light cyan host) -prompt : "\033[91m%u\033[97m@\033[96m%h\033[0m" +## configure your prompt using %u (username) and %h (hostname) +## accessibility tip: use plain text by default; color-only distinction can be +## hard to read for some users and terminals. +prompt : "%u@%h" +## optional colorized prompt using ANSI escape codes (light red user, light cyan host): +#prompt : "\033[91m%u\033[97m@\033[96m%h\033[0m" ## set prompt path style (0, 1, or 2; default: 0) @@ -120,18 +122,19 @@ prompt : "\033[91m%u\033[97m@\033[96m%h\033[0m" ## a value in seconds for the session timer #timer : 5 -## list of path to restrict the user "geographicaly" -## warning: many commands like vi and less allow to break this restriction +## list of paths to restrict where the user can operate +## warning: commands like vi and less can bypass this restriction #path : ['/etc','/var/log','/var/lib'] -## set the home folder of your user. If not specified the home_path is set to +## set the home folder for your user. if not specified, home_path is set to ## the $HOME environment variable +## deprecated: prefer setting the home directory with system account tools. #home_path : '/home/bla/' ## update the environment variable $PATH of the user #env_path : '/usr/local/bin:/usr/sbin' -## a list of path; all executable files inside these path will be allowed +## a list of paths; all executable files inside these paths are allowed #allowed_cmd_path: ['/home/bla/bin','/home/bla/stuff/libexec'] ## add environment variables @@ -150,14 +153,14 @@ prompt : "\033[91m%u\033[97m@\033[96m%h\033[0m" ## forbid scp download #scp_download : 0 -## allow of forbid the use of sftp (set to 1 or 0) +## allow or forbid the use of sftp (set to 1 or 0) ## this option will not work if you are using OpenSSH's internal-sftp service #sftp : 1 -## list of command allowed to execute over ssh (e.g. rsync, rdiff-backup, etc.) +## list of commands allowed to execute over ssh (e.g. rsync, rdiff-backup) #overssh : ['ls', 'rsync'] -## logging strictness. If set to 1, unknown syntax/commands are considered +## strictness mode. if set to 1, unknown syntax/commands are considered ## forbidden and decrement warning_counter (which can kick the user out). ## If set to 0, they are reported as unknown syntax only. strict : 0 @@ -174,7 +177,7 @@ strict : 0 ## - allowed: +['scp', 'env', 'pwd', 'groups', 'unset', 'unalias'] #winscp: 0 -## history file maximum size +## history file maximum size #history_size : 100 ## set history file name (default is /home/%u/.lhistory) @@ -193,8 +196,8 @@ strict : 0 ## changes there (such as umask) do not persist in the lshell parent process #login_script : "/path/to/myscript.sh" -## disable user exit, this could be useful when lshell is spawned from another -## none-restricted shell (e.g. bash) +## disable user exit; useful when lshell is spawned from another +## non-restricted shell (e.g. bash) #disable_exit : 0 ## show/hide policy introspection builtins: diff --git a/man/lshell.1 b/man/lshell.1 index ca46fdc..e5471d4 100644 --- a/man/lshell.1 +++ b/man/lshell.1 @@ -24,7 +24,7 @@ or with .I /etc/shells and .I /etc/passwd -, it becomes very easy to restrict a user's access to a limited set of commands. +, it is straightforward to restrict a user's access to a limited set of commands. .SH OPTIONS .TP @@ -35,7 +35,7 @@ Specify configuration file Specify the log directory .TP .B \-- \fI\fR -where is any config file parameter +where is any configuration file parameter .TP .B \-h, --help Show help message @@ -185,8 +185,8 @@ Local executables must be explicitly listed with a relative path (e.g., './deplo .BR \ \ \ \ - To allow all commands in the user's PATH, use the value 'all'. .RE -if sudo(8) is installed and sudo_noexec.so is available, it will be loaded -before running every command, preventing it from running further commands +if sudo(8) is installed and sudo_noexec.so is available, it will be loaded +before running every command, preventing it from running further commands itself. If not available, beware of commands like vim/find/more/etc. that will allow users to execute code (e.g. /bin/sh) from within the application, thus easily escaping lshell. See variable 'path_noexec' to use an alternative @@ -198,7 +198,7 @@ a list of the allowed commands that are permitted to execute other programs allowed. Warning: do not put here any command that can execute arbitrary commands (e.g. find, vim, xargs). -important: commands defined in 'allowed_shell_escape' override their definition +Important: commands defined in 'allowed_shell_escape' override their definition in the \'allowed\' variable. .TP .I allowed_file_extensions @@ -206,7 +206,7 @@ a list of allowed file extensions that can be provided in the command line. If a list of allowed extensions is provided, all other file extensions will be disallowed. .TP .I allowed_cmd_path -a list of path; all executable files inside these path will be allowed +a list of paths; all executable files inside these paths are allowed .TP .I disable_exit disable user exit, this could be useful when lshell is spawned from another @@ -464,6 +464,40 @@ enable/disable policy introspection builtins. If set to 1 (default), users can run \fBpolicy-show\fR, \fBpolicy-path\fR, \fBpolicy-sudo\fR (and aliases \fBlpath\fR, \fBlsudo\fR). If set to 0, these commands are hidden. +.SH BEST PRACTICES +.TP +.B Least privilege +Prefer explicit allow-lists in \fIallowed\fR and avoid using \fB'all'\fR unless +strictly required. +.TP +.B Shell escape control +Keep \fIallowed_shell_escape\fR minimal and do not include tools that execute +arbitrary commands (for example: find, vim, xargs). +.TP +.B Extension policy +Use \fIallowed_file_extensions\fR when users should handle only known file types. +.TP +.B Warning policy +Keep \fIwarning_counter\fR enabled and consider \fIstrict=1\fR for stronger +enforcement in high-assurance environments. +.TP +.B Validate before rollout +Use \fBlshell policy-show\fR to review final merged policy and command decisions +before assigning a profile to users. + +.SH ACCESSIBILITY +.TP +.B Prompt readability +Use a readable text prompt format (for example \fB%u@%h\fR) as the default. +.TP +.B Color usage +If using ANSI colors in \fIprompt\fR or \fB$LPS1\fR, avoid color-only meaning so +the prompt remains understandable in monochrome or low-contrast terminals. +.TP +.B Message clarity +When customizing \fImessages\fR, keep phrasing concise and actionable so users +understand how to recover from denied commands. + .SH SHELL BUILTIN COMMANDS Here is the set of commands that are always available with lshell: .TP