diff --git a/.gitignore b/.gitignore index 2354b86..793f046 100644 --- a/.gitignore +++ b/.gitignore @@ -9,4 +9,8 @@ dist/ test.lsh .pylint_cache/ .pylint.d/ -.hypothesis/ \ No newline at end of file +.hypothesis/ +.venv/ +local-docs/ +.vscode/ +.idea/ diff --git a/CHANGELOG.md b/CHANGELOG.md index 651fb04..9db680a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,7 +3,7 @@ Contact: [ghantoos@ghantoos.org](mailto:ghantoos@ghantoos.org) [https://github.com/ghantoos/lshell](https://github.com/ghantoos/lshell) -### v0.12.0 (UNRELEASED) +### v0.12.0 17/06/2026 - Packaging/CI: Raised minimum supported Python version to 3.10 (`requires-python >=3.10`), removed EOL Python versions from CI, and aligned docs/package metadata with the new baseline; CI/classifiers now track active CPython release branches 3.10-3.14 (Python 3.6 reached EOL on 23/12/2021). - Security: Removed regex-driven shell parsing from the authorization flow. - Engine: Migrated security parsing to a deterministic scanner. diff --git a/README.md b/README.md index fb13713..c242d62 100644 --- a/README.md +++ b/README.md @@ -153,6 +153,14 @@ Key settings to review: - `umask` - runtime containment: `max_sessions_per_user`, `max_background_jobs`, `command_timeout`, `max_processes` +Security note: +`path` restrictions apply to commands parsed by `lshell`, but they do not inspect +individual file requests handled inside the SFTP protocol itself. If you enable +legacy `sftp`, `lshell` now refuses the passthrough by default unless you set +`sftp_unsafe_legacy=1`. Treat OpenSSH `ChrootDirectory` plus `ForceCommand internal-sftp` as the +real filesystem boundary, and use SSH-side controls such as `DisableForwarding` +or read-only SFTP modes when needed. + CLI overrides are supported, for example: ```bash @@ -219,7 +227,8 @@ timer : 0 path : ['/etc', '/usr'] env_path : '/sbin:/usr/foo' scp : 1 -sftp : 1 +sftp : 0 +sftp_unsafe_legacy : 0 overssh : ['rsync','ls'] aliases : {'ls':'ls --color=auto','ll':'ls -l'} diff --git a/debian/changelog b/debian/changelog index d2c45e2..128d552 100644 --- a/debian/changelog +++ b/debian/changelog @@ -1,4 +1,4 @@ -lshell (0.11.1rc2-1) UNRELEASED; urgency=medium +lshell (0.12.0-1) UNRELEASED; urgency=medium * debian/watch: - Corrected to work with lshell versioning on github. diff --git a/docs/security/lshell-security-audit-2026-06-15.md b/docs/security/lshell-security-audit-2026-06-15.md new file mode 100644 index 0000000..3760575 --- /dev/null +++ b/docs/security/lshell-security-audit-2026-06-15.md @@ -0,0 +1,879 @@ +# `lshell` Security Audit + +Date: `2026-06-15` + +## Executive Summary + +No evidence of a hidden backdoor, botnet logic, or covert outbound command-and-control behavior was found. The main risk is different: the restricted shell boundary can be weakened or bypassed through ordinary, supported code paths. + +Three security issues stand out: + +1. A user can manipulate environment variables so that an allowed command triggers an additional disallowed command inside `bash`. +2. `source` is enabled by default even when `export` is not, which leaves an alternate path for modifying the shell environment. +3. `LSHELL_ARGS` is accepted from the environment during startup, which can let external state influence security-sensitive startup options. + +In practical terms, these issues mean a restricted user may be able to bypass part of the shell policy or run commands that were never meant to be allowed. + +## What `lshell` Does + +`lshell` is a restricted shell. Instead of dropping the user directly into a normal login shell such as `bash` or `sh`, it places the user inside an application that tries to limit: + +- which commands may run +- which filesystem paths may be accessed +- which shell features may be used +- which remote command patterns are accepted over `SSH`, `SCP`, and `SFTP` + +At a high level, command flow looks like this: + +1. Startup arguments are processed in `lshell/cli.py`. +2. Configuration is loaded and normalized in `lshell/checkconfig.py`. +3. Interactive command handling runs through `lshell/shellcmd.py`. +4. Command parsing, policy checks, and execution orchestration run through `lshell/utils.py`. +5. Allowed commands are eventually executed through `subprocess.Popen(...)`. + +Code references: + +- `lshell/cli.py:18-87` +- `lshell/checkconfig.py:42-67` +- `lshell/shellcmd.py:26-300` +- `lshell/utils.py:526-877` +- `lshell/utils.py:880-1023` + +## How a Normal Command Reaches Execution + +When a user enters a command such as `echo hi`, the path is: + +1. `ShellCmd` receives the input. +2. `utils.cmd_parse_execute(...)` is called. +3. The same path in `lshell/utils.py` breaks the command into segments and operators. +4. Policy checks decide whether the command, its arguments, and any referenced paths are allowed. +5. If the command passes, `utils.exec_cmd(...)` runs it. +6. For ordinary commands, `exec_cmd(...)` normally executes the final string through a real shell, effectively `bash -c "echo hi"`. + +That last step matters. Any weakness that changes how `bash -c` interprets its environment can undermine the checks that happened earlier. + +Code references: + +- `lshell/utils.py:526-577` +- `lshell/utils.py:618-808` +- `lshell/utils.py:976-999` + +## Review Method + +The review was carried out in three layers: + +1. Repository-wide searching for high-risk execution, environment, and transport surfaces. +2. Direct review of the main startup, policy, and command-execution paths. +3. Targeted proof-of-concept checks to verify whether suspicious behavior was actually reachable. + +## Backdoor and Botnet Assessment + +The review looked for: + +- covert network connections +- hidden command download or execution paths +- suspicious dynamic loading behavior +- hard-coded control endpoints +- unusual use of modules such as `socket`, `requests`, `urllib`, `httpx`, `ftplib`, `telnetlib`, or `paramiko` + +No clear sign of that class of behavior was found in the main runtime modules. The network-related code that appears in the repository is consistent with expected project behavior: + +- `SSH`, `SCP`, and `SFTP` support +- container and integration-test setup +- Debian and RPM packaging helpers + +The main security concerns are therefore policy bypasses inside ordinary shell behavior, not hidden remote-control logic. + +## Issue 1: Disallowed Command Execution Through Environment Variable Manipulation + +### Plain-Language Summary + +A user can set environment variables so that an allowed command causes an extra, disallowed command to run behind the scenes. + +This does not happen because `lshell` explicitly authorizes the second command. It happens because `bash` interprets inherited environment state in a dangerous way. + +### Why This Matters + +The central promise of a restricted shell is simple: users should only be able to run commands that policy allows. If an allowed command can be turned into a carrier for another command, the allow-list is no longer a real boundary. + +### Conditions Required + +This issue becomes reachable when all of the following are true: + +1. The user can introduce or change environment variables inside the shell session. +2. At least one of these paths is available: + - `export` + - `source` + - configuration-driven environment file loading +3. Final command execution still goes through `bash -c` or an equivalent shell execution path. + +In this implementation, the third condition is true by design, and the first two are realistic in many configurations. + +### Step-by-Step Execution Path + +#### Step 1: The shell allows some dangerous environment variables to be set + +`cmd_export(...)` blocks only a limited set of forbidden variables listed in `variables.FORBIDDEN_ENVIRON`. + +That list does not include several environment variables that can materially change shell behavior, including: + +- `SHELLOPTS` +- `BASHOPTS` +- `PS4` +- `PROMPT_COMMAND` +- `CDPATH` +- `GLOBIGNORE` +- `IFS` +- `PYTHONPATH` + +Code references: + +- `lshell/builtincmd.py:129-142` +- `lshell/variables.py:118-144` + +#### Step 2: The same environment is later inherited by command execution + +Inside `exec_cmd(...)`, the runtime environment starts as a full copy of `os.environ`. + +Only a very small cleanup happens before the command is executed: + +- `BASH_ENV` is removed +- `ENV` is removed +- names starting with `BASH_FUNC_` are stripped + +There is no broader scrub of shell-sensitive variables before `bash` is launched. + +Code references: + +- `lshell/utils.py:880-903` + +#### Step 3: Normal commands are executed through `bash -c` + +For non-`sudo` and non-`su` commands, the execution path uses: + +```python +cmd_args = ["bash", "-c", cmd] +``` + +That means the final command string is interpreted by a real `bash` process. + +Code references: + +- `lshell/utils.py:976-982` + +#### Step 4: `bash` interprets the manipulated environment + +If a user enables shell tracing through `SHELLOPTS=xtrace`, then `bash` uses `PS4` as the prefix for trace lines. If `PS4` contains command substitution, the shell evaluates that content while running the apparently safe command. + +At that point, the extra command is no longer passing through the original allow-list decision. `lshell` has already approved the outer command and handed execution to `bash`. + +### Simple Demonstration + +A minimal demonstration is: + +```bash +export SHELLOPTS=xtrace +export PS4='$(echo LSHELL_POC >&2) ' +echo hi +``` + +In practice, this causes `LSHELL_POC` to be emitted before `echo hi` completes. The point of the demonstration is not the output itself. The point is that content inside `PS4` gets executed by `bash`. + +If the payload inside `PS4` is replaced with a real system command, that command runs outside the intended command policy boundary. + +### Practical Impact + +This issue can let a restricted user: + +1. run a command that appears to be allowed +2. trigger a second command in the background execution path +3. execute a command that is not in the configured allow-list + +In practical terms, the allow-list stops being the effective security boundary. + +### Severity + +This issue should be treated as `Critical` because it directly breaks the restricted-shell security model. + +### Practical Fix + +This issue should be fixed in layers. Blocking one or two additional variables is not enough on its own. + +#### Layer 1: Rebuild and filter the execution environment instead of blindly making it tiny + +The correct security principle is for `exec_cmd(...)` to build a new execution environment instead of copying all of `os.environ`. But that must not be done in a naive way. + +In plain terms: + +- current unsafe pattern: take everything, then remove a few entries +- safer pattern: build a new environment, take only values from trusted state, and discard the rest + +This change belongs in `lshell/utils.py`, inside `exec_cmd(...)`. + +Code references: + +- `lshell/utils.py:880-903` + +Several existing behaviors in this implementation show why a tiny fixed environment would break legitimate features: + +1. Variables defined through `env_vars` and `env_vars_files` are meant to reach later commands. That path is implemented by writing into `os.environ` and by calling `cmd_source(...)` from `check_env(...)`. +2. `env_path` and `allowed_cmd_path` modify `PATH`, and later command resolution depends on that `PATH`. +3. The existing behavior allows an assignment-only statement such as `LSH_PERSIST=YES` to remain in the shell environment for later commands. +4. Existing tests explicitly expect variables loaded from environment files to be visible to later commands such as `echo $bar`. + +Code references: + +- `lshell/checkconfig.py:140-150` +- `lshell/checkconfig.py:711-749` +- `test/test_env_vars_files_unit.py:19-47` +- `test/test_env_vars.py:84-125` +- `test/test_security_hardening_functional.py:60-77` + +The initial safe set therefore needs to include at least: + +- `HOME` +- `USER` +- `LOGNAME` +- `TERM` +- `LANG` +- `LC_*` + +And in addition, these values need to be preserved or rebuilt in a controlled way: + +- `PATH`, but only from values constructed or validated by `lshell` +- variables coming from `env_vars` +- variables coming from `env_vars_files` +- variables that the shell intentionally keeps in session state + +The practical conclusion is that the right fix is not a tiny fixed environment. The right fix is a filtered environment rebuilt from trusted shell state. + +#### Layer 2: If any inheritance remains, expand the forbidden-variable set + +If the project still needs to inherit part of the environment, then at minimum the forbidden set should be expanded to include: + +- `SHELLOPTS` +- `BASHOPTS` +- `PS4` +- `PROMPT_COMMAND` +- `CDPATH` +- `GLOBIGNORE` +- `IFS` +- `PYTHONPATH` + +Any `BASH_FUNC_*` names should also be blocked consistently. + +This enforcement should happen in two places: + +1. when variables enter through `export` or `source` +2. when the final execution environment is assembled before `subprocess.Popen(...)` + +Code references: + +- `lshell/builtincmd.py:129-142` +- `lshell/builtincmd.py:145-155` +- `lshell/variables.py:118-144` +- `lshell/utils.py:880-903` + +This layer also has compatibility impact: + +1. shell customizations that rely on variables such as `CDPATH` or `IFS` will stop working +2. an allowed Python program that depends on `PYTHONPATH` may no longer behave the same way +3. if an administrator currently injects any of these variables through `env_vars` or `env_vars_files`, those settings will need to be revisited + +That compatibility cost is acceptable for a hardened restricted shell, but it should be explicit. + +#### Layer 3: Remove the `bash -c` dependency if the design allows it + +This is the strongest security improvement, but it is not the lowest-risk fix. As long as ordinary commands are executed through `bash -c`, `bash` remains a large interpretation surface. + +If the architecture allows it, ordinary commands should move toward direct execution without an intermediate shell, with the command and arguments passed straight to `subprocess.Popen(...)`. + +Code references: + +- `lshell/utils.py:976-999` + +In this implementation, that is a major behavioral change because existing tests depend on shell semantics. For example: + +1. pipelines need to work +2. redirections need to work +3. `&&` and `||` need to behave like shell operators +4. command substitution in `$(...)` and variable expansion such as `${HOME}` need to work + +Code references: + +- `test/test_command_execution.py:229-257` +- `test/test_command_execution.py:279-317` + +#### Tests That Should Be Added + +At minimum, add regression tests that prove: + +1. `export SHELLOPTS=...` is rejected +2. `export PS4=...` is rejected +3. `source` rejects the same variables +4. `exec_cmd("echo hi")` is no longer affected by inherited `SHELLOPTS` and `PS4` + +Good candidate test files: + +- `test/test_source_command_unit.py` +- `test/test_security_hardening_functional.py` +- `test/test_user_behavior_security_functional.py` + +## Issue 2: `source` Leaves an Alternate Path Around Disabled `export` + +### Plain-Language Summary + +The project tries not to enable `export` by default, but `source` is still enabled by default and can load exported variables from files into the shell environment. + +As a result, the absence of `export` does not actually mean environment changes are blocked. + +### Why This Matters + +Issue 1 depends on the user being able to inject environment state. Issue 2 shows that the same capability remains available even when `export` is not part of the default allowed command set. + +### Step-by-Step Execution Path + +#### Step 1: `export` is excluded from the default built-in allow-list + +During configuration assembly, the code adds built-ins to the allowed set but excludes `export`. + +Code references: + +- `lshell/checkconfig.py:725-726` + +#### Step 2: `source` remains part of the default built-in set + +`source` is still listed in `builtins_list`, so it remains available by default unless configuration removes it explicitly. + +Code references: + +- `lshell/builtincmd.py:24-43` + +#### Step 3: `source` reads files and forwards `export ...` lines to `cmd_export(...)` + +`cmd_source(...)` opens the target file and forwards lines beginning with `export ` to `cmd_export(...)`. + +Code references: + +- `lshell/builtincmd.py:140-155` + +#### Step 4: Environment changes happen even when `export` is not allowed + +In practical terms, a user can still load: + +```text +export TEST_FROM_SOURCE=ok +``` + +through `source`, and the variable enters `os.environ` even though direct use of `export` was meant to be blocked by policy. + +### When This Becomes More Dangerous + +The issue is worse when: + +1. the user can write files inside allowed paths +2. the operator assumes disabling `export` is enough to prevent environment manipulation +3. `source` is available for convenience in interactive sessions + +### Practical Impact + +`source` is not always a complete breakout by itself, but it is a direct enabler for Issue 1. It keeps open the exact environment-modification path the configuration appears to be trying to close. + +In plain terms: + +- `export` is closed +- `source` reopens the same door from another angle + +### Severity + +This issue should be treated as `High` because it defeats a visible hardening decision and helps enable a larger policy bypass. + +### Practical Fix + +The safest fix is to stop enabling `source` by default for general users. + +#### Option 1: Remove `source` from the default built-in allow-list + +This is the clearest default behavior: + +- `source` should not be enabled by default +- it should only become available when an administrator opts into it explicitly + +Code references: + +- `lshell/builtincmd.py:24-43` +- `lshell/checkconfig.py:725-726` + +#### Option 2: If `source` remains, prefer a separate explicit permission over reopening `export` + +If the project does not want to remove `source` entirely, one of these policies should be used: + +1. `source` should only be allowed when an administrator explicitly enables it in `allowed` or through a dedicated configuration key +2. a separate configuration key such as `allow_source=1` should control it, with a default of disabled + +In practice, a separate key is safer than tying `source` directly to `export`, because reopening `export` broadens the attack surface again. + +#### Option 3: Restrict `source` to trusted files only + +If `source` is operationally necessary, it should load only: + +- files from approved paths +- files named in administrator-controlled configuration +- files the restricted user cannot freely modify + +Otherwise any writable allowed path can become an environment-injection surface. + +#### Tests That Should Be Added + +At minimum, add regression tests that prove: + +1. `source` is not available by default +2. if `source` is enabled, dangerous variables are still rejected +3. `source` cannot silently restore the same power that blocking `export` was meant to remove + +Good candidate test files: + +- `test/test_source_command_unit.py` +- `test/test_builtins.py` +- `test/test_env_vars_files_unit.py` +- `test/test_completion.py` + +## Issue 3: `LSHELL_ARGS` Can Influence Security-Sensitive Startup Behavior + +### Plain-Language Summary + +At startup, the program reads `LSHELL_ARGS` from the environment and merges it into the effective argument list. That means external environment state can shape startup behavior before the restricted shell has fully established its security posture. + +### Why This Matters + +In a security-oriented program, startup policy should come from a trusted boundary. Reading arbitrary startup arguments from the environment weakens that boundary. + +### Step-by-Step Execution Path + +#### Step 1: Startup reads `LSHELL_ARGS` + +`lshell/cli.py` checks whether `LSHELL_ARGS` exists. If it does, it parses the value with `ast.literal_eval(...)`. + +If the result is a `list` or `tuple` of strings, the values are merged into the active argument list. + +Code references: + +- `lshell/cli.py:27-40` + +#### Step 2: The merged arguments are passed into configuration loading + +After that merge, the combined list is passed into `CheckConfig(args).returnconf()`. + +Code references: + +- `lshell/cli.py:42` + +#### Step 3: `CheckConfig` reads security-sensitive startup options from those arguments + +`getoptions(...)` parses startup options and stores them in the configuration dictionary. + +Code references: + +- `lshell/checkconfig.py:94-138` + +#### Step 4: Some of those options directly affect security behavior + +The available startup options include settings such as: + +- `allowed=` +- `forbidden=` +- `strict=` +- `path_noexec=` +- `allowed_shell_escape=` +- `winscp=` + +Code references: + +- `lshell/variables.py:100-115` + +### When This Becomes Exploitable + +This issue matters only when `LSHELL_ARGS` can realistically reach the process from outside. That can happen through: + +1. an `SSH` configuration that preserves user-controlled environment variables +2. wrapper scripts that pass inherited environment state into `lshell` +3. service managers or control panels that launch `lshell` with inherited environment variables +4. reused process environments where the variable was already set + +If none of those conditions exist, exploitability may be limited. Even so, the startup trust model is weaker than it should be. + +### Practical Impact + +If `LSHELL_ARGS` is user-controlled at process start, the user may be able to: + +1. change the allowed command set +2. weaken or remove restrictions +3. disable or erode protections such as `path_noexec` + +In plain terms, the shell may start with a different policy than the administrator intended. + +### Severity + +This issue should be treated as `High`. Its exploitability depends on deployment details, but the effect is direct and serious when the environment is not tightly controlled. + +### Practical Fix + +The fix belongs at the trust boundary. Security-sensitive startup behavior should not accept arbitrary environment-provided arguments. + +#### Option 1: Stop reading free-form `LSHELL_ARGS` at startup + +The lowest-risk change is: + +- do not treat `LSHELL_ARGS` as a general startup argument source +- use only `sys.argv` for effective startup options + +Code references: + +- `lshell/cli.py:27-40` + +There is an important detail here. `CheckConfig.getoptions(...)` also writes `LSHELL_ARGS` back into the environment after parsing arguments, using `--config` and, if present, `--log`. That means `LSHELL_ARGS` is not only an outside input today. It is also part of the internal mechanism used to preserve the active config path and log path. + +Code references: + +- `lshell/checkconfig.py:126-131` + +As a result, removing `LSHELL_ARGS` reading completely and all at once can have side effects: + +1. any internal path that relies on inheriting `--config` or `--log` from the environment will no longer receive the same settings +2. any nested or internal `lshell` execution that relies only on `$SHELL` and inherited environment may fall back to the default config instead of the previously selected one +3. the existing `test_main_appends_valid_lshell_args_from_env` behavior will need to be rewritten along with the new policy + +Code references: + +- `test/test_cli_unit.py:49-64` + +#### Option 2: If the mechanism must remain, allow only a very small safe subset + +If the project still needs this mechanism for internal re-execution, only clearly harmless options should survive environment loading, for example: + +- `--config` +- `--log` + +Security-sensitive options such as `--allowed=all` or `--path_noexec=` should be ignored if they come from the environment. + +#### Option 3: Use a dedicated internal-only restart channel + +If internal restart behavior is required, use a narrower mechanism that is clearly separate from general user-controlled startup state: + +1. define a dedicated internal restart variable +2. read it only in a known internal restart mode +3. never accept security-sensitive policy options from it + +#### Tests That Should Be Added + +At minimum, add regression tests that prove: + +1. `LSHELL_ARGS=['--allowed=all']` does not change policy +2. unsafe options coming from `LSHELL_ARGS` are ignored +3. if a safe subset remains, only `--config` and `--log` are honored + +Good candidate test files: + +- `test/test_cli_unit.py` + +## How the Three Issues Reinforce Each Other + +These issues are not isolated. + +1. Issue 2 preserves a path for injecting environment variables. +2. Issue 1 uses that injected environment state to execute commands outside the intended policy boundary. +3. Issue 3 can weaken startup protections before the session even begins. + +The main risk is therefore the chain, not just the individual bugs in isolation. + +## Operational Impact of Each Remediation + +One question matters as much as the findings themselves: what happens to real program behavior if these fixes are applied? + +The short answer is that the remediations do not all carry the same implementation risk. Some are well suited for a low-risk hotfix. Some require compatibility work and test updates. Some are architectural changes rather than quick patches. + +### Remediation 1: Stop reading free-form `LSHELL_ARGS` at startup + +This change is necessary from a security perspective, but it is not side-effect free if done as a simple on-off switch. + +Why? Because two behaviors exist at the same time: + +1. `cli.main()` reads `LSHELL_ARGS` from the environment and appends it to `sys.argv`. +2. `CheckConfig.getoptions(...)` writes `LSHELL_ARGS` back into the environment using `--config` and `--log`. + +Code references: + +- `lshell/cli.py:27-40` +- `lshell/checkconfig.py:126-131` + +Practical effect of this change: + +1. a direct invocation such as `lshell --config ...` remains mostly unchanged +2. any internal or nested path that relies on inheriting `--config` and `--log` from the environment will no longer behave the same way +3. the existing `test/test_cli_unit.py` expectations for valid environment-appended arguments will need to change + +Code references: + +- `test/test_cli_unit.py:49-64` + +Practical conclusion: + +- this change is a good hotfix candidate, as long as it is not implemented as a blunt full removal +- the safest version is to keep only `--config` and `--log`, or move internal restart behavior to a separate variable +- without a compatibility path, some nested invocations or old wrapper flows may unexpectedly fall back to the default config + +### Remediation 2: Harden the execution environment and expand forbidden environment variables + +This change is highly important, but its main risk is that an oversimplified implementation can break legitimate shell behavior. + +Several existing behaviors depend on the present environment model: + +1. `env_vars` writes variables directly into `os.environ` +2. `env_vars_files` reads files through `cmd_source(...)` and applies results to the same live environment +3. `env_path` and `allowed_cmd_path` modify `PATH` +4. assignment-only lines such as `LSH_PERSIST=YES` are meant to persist in session state +5. assignment prefixes such as `A=INLINE printenv A` are meant to affect only the current command + +Code references: + +- `lshell/checkconfig.py:140-150` +- `lshell/checkconfig.py:711-749` +- `lshell/utils.py:880-903` +- `test/test_env_vars.py:84-125` +- `test/test_env_vars.py:188-203` +- `test/test_security_hardening_functional.py:48-77` + +If execution is reduced to a very small fixed environment, these side effects are likely: + +1. `echo $bar` for variables loaded from `env_vars_files` may stop working +2. variables set by the administrator through `env_vars` may stop reaching some external commands +3. the effect of `env_path` and `allowed_cmd_path` on command resolution may disappear unless `PATH` is rebuilt from trusted values +4. locale and terminal behavior tied to `LANG`, `TERM`, or `LC_*` may degrade + +Code references: + +- `test/test_env_vars.py:108-125` +- `test/test_unit.py:254-270` + +On the other side, simply expanding the forbidden list also has compatibility cost: + +1. workflows that intentionally rely on `CDPATH`, `IFS`, or `PYTHONPATH` will no longer behave the same way +2. if the administrator currently injects any of those variables through `env_vars` or `env_vars_files`, that configuration will need review + +Practical conclusion: + +- this is also a good hotfix candidate, but not with a tiny static environment +- the lower-risk version is to rebuild `exec_env` from trusted shell state, rebuild `PATH` separately, and strip only dangerous names +- this is a medium-risk change because it improves security while also exposing hidden compatibility assumptions + +### Remediation 3: Remove `source` from the default command set + +This is a sensible security improvement. But in the present behavior of the project, `source` is not just a minor convenience command. Several parts of the project assume its presence either directly or indirectly. + +Code references: + +- `lshell/builtincmd.py:24-43` +- `lshell/checkconfig.py:140-150` +- `lshell/checkconfig.py:725-726` + +Practical effect of this change: + +1. help output and command completion should no longer list `source` by default +2. tests that expect `source` in the default command set will need to change +3. if the same `cmd_source(...)` function is used both for user-facing `source` and for internal `env_vars_files` loading, closing the interactive command must not break internal environment-file loading + +Code references: + +- `test/test_completion.py:33-55` +- `test/test_ps2.py:13-29` +- `test/test_scripts.py:13-31` +- `test/test_security.py:13-28` +- `test/test_env_vars_files_unit.py:19-47` + +The right fix is therefore not just removing one name from `builtins_list`. The right fix is: + +1. separate internal environment-file loading from the interactive user command path +2. make `source` available only by explicit administrator choice +3. apply the same dangerous-variable filtering when `source` is enabled + +Practical conclusion: + +- this is a medium-risk change +- it is worthwhile, but it affects help output, completion, tests, and internal environment-file loading +- if it is implemented bluntly, an administrator may think only an interactive command was closed while internal environment-file loading was silently broken too + +### Remediation 4: Remove `bash -c` and move to direct shellless execution + +This is the best long-term security state, but for this code it is an architectural change rather than a quick fix. + +The reason is simple: the implementation depends heavily on shell semantics, and the tests make that dependency explicit: + +1. pipelines such as `printf foo | wc -c` must work +2. redirections such as `>`, `>>`, and `2>&1` must work +3. operators such as `&&` and `||` must work +4. command substitution such as `$(...)` and expansions such as `${HOME}` must work + +Code references: + +- `lshell/utils.py:976-999` +- `test/test_command_execution.py:229-257` +- `test/test_command_execution.py:279-317` + +If `bash -c` is removed without a replacement design, a large part of existing behavior will either stop working or need to be reimplemented inside `lshell`. + +Practical conclusion: + +- this is not a good hotfix candidate +- this is a high-risk architectural change +- it should be handled as a later design phase, not as the first security patch + +## Practical Remediation Plan + +If these issues are going to be fixed with the best balance of security and implementation risk, the work should be ordered by real change risk, not only by raw vulnerability severity. + +### Phase 1: Low-risk hotfix for the startup trust boundary + +Start by closing the trust boundary at startup. This change has direct security value and can be made with the least disruption if done carefully. + +Work items: + +1. stop accepting free-form `LSHELL_ARGS` from the environment +2. keep only `--config` and `--log` if they are genuinely needed +3. or replace the mechanism with a separate internal-only restart variable +4. rewrite `test/test_cli_unit.py` to reflect the new policy + +Primary files: + +- `lshell/cli.py` +- `lshell/checkconfig.py` +- `test/test_cli_unit.py` + +### Phase 2: Harden final command execution with controlled compatibility risk + +Then harden the final execution boundary in a way that does not unnecessarily break legitimate behavior. + +Work items: + +1. rebuild `exec_env` from trusted shell state +2. expand the forbidden variable set to include `SHELLOPTS`, `PS4`, `BASHOPTS`, `IFS`, `PYTHONPATH`, and similar names +3. rebuild `PATH` from trusted `env_path` and `allowed_cmd_path` state +4. add regression tests proving that required allowed variables still work + +Primary files: + +- `lshell/utils.py` +- `lshell/variables.py` +- `lshell/checkconfig.py` +- `test/test_source_command_unit.py` +- `test/test_env_vars.py` +- `test/test_security_hardening_functional.py` + +### Phase 3: Close the user-facing helper path through `source` + +After that, reduce the user's ability to modify the environment through the interactive shell, without breaking internal environment-file loading. + +Work items: + +1. remove `source` from the default built-in set +2. split internal `env_vars_files` loading from the user-facing `source` command +3. require explicit administrator permission before `source` is available +4. apply the same dangerous-variable filtering to `source` + +Primary files: + +- `lshell/builtincmd.py` +- `lshell/checkconfig.py` +- `test/test_builtins.py` +- `test/test_env_vars_files_unit.py` +- `test/test_completion.py` + +### Phase 4: Architectural work to remove the `bash -c` dependency + +This phase should start only if the project is ready to revisit its execution model. + +Work items: + +1. decide which shell behaviors must remain supported and which should be deliberately removed +2. move ordinary command execution to direct `subprocess.Popen(...)` calls without an intermediate shell +3. rewrite or remove tests that currently depend on full `bash` behavior + +Primary files: + +- `lshell/utils.py` +- `test/test_command_execution.py` +- `test/test_security_hardening_functional.py` + +## What Kind of Unauthorized Access These Issues Create + +The phrase "unauthorized access" can mean different things, so it should be stated clearly. + +In this project, the main effect is: + +1. a restricted user may run a command that policy was supposed to deny +2. the user may bypass part of the shell policy boundary +3. if that same account already has useful filesystem, group, or `sudo` privileges, the bypass can lead to more serious damage + +These issues do not automatically guarantee full root access. They do, however, break the central security boundary that the restricted shell is supposed to enforce. + +## Why These Findings Are Serious Even Though They Are Not Classic Backdoors + +A classic backdoor usually means: + +- intentionally hidden access logic +- or covert communication with an outside operator + +That is not what appears here. The issues are design and hardening problems inside legitimate code paths. + +From an operational security perspective, the outcome is still serious. A restricted shell that can be bypassed is not providing the boundary it claims to provide. + +## Most Important Files for Following the Audit + +If this audit needs to be followed directly in code, these are the main entry points: + +### Startup + +- `lshell/cli.py:18-87` + +### Configuration loading + +- `lshell/checkconfig.py:57-67` +- `lshell/checkconfig.py:94-150` +- `lshell/checkconfig.py:711-749` + +### Built-in commands + +- `lshell/builtincmd.py:24-43` +- `lshell/builtincmd.py:124-155` + +### Forbidden environment-variable list + +- `lshell/variables.py:118-144` + +### Command parsing, policy checks, and orchestration + +- `lshell/utils.py:526-877` + +### Final command execution + +- `lshell/utils.py:880-1023` + +## Final Assessment + +Stated plainly: + +1. no hidden backdoor or botnet-style control logic was found +2. three real security weaknesses can still undermine the core purpose of the project +3. the most dangerous issue is the ability to influence `bash` through inherited environment state and trigger extra command execution +4. the second issue is that `source` leaves open the same environment-modification capability that disabling `export` appears to close +5. the third issue is that `LSHELL_ARGS` allows outside state to influence startup policy + +If `lshell` is expected to serve as a real isolation boundary for semi-trusted or untrusted users, it should not be relied on in this form without hardening these paths and keeping them covered by regression tests. + +## Recommended Next Order of Work + +The most practical order of work is: + +1. stop free-form `LSHELL_ARGS` handling while keeping only safe internal paths +2. harden `exec_env` by rebuilding it from trusted state and blocking dangerous variables +3. remove `source` from the default user-facing command set and separate it from internal `env_vars_files` loading +4. leave `bash -c` removal for a later architectural phase diff --git a/etc/lshell.conf b/etc/lshell.conf index bbdb844..c65201a 100644 --- a/etc/lshell.conf +++ b/etc/lshell.conf @@ -177,7 +177,14 @@ max_processes : 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 +## WARNING: lshell path restrictions do not inspect per-file SFTP protocol +## requests. For restricted SFTP, enforce the filesystem boundary with +## sshd_config ChrootDirectory + ForceCommand internal-sftp (optionally -R), +## and disable forwarding features at the SSH layer. +## Legacy sftp-server passthrough is refused by default even when sftp=1. +## Set sftp_unsafe_legacy to 1 only if you intentionally accept that risk. +#sftp : 0 +#sftp_unsafe_legacy : 0 ## list of commands allowed to execute over ssh (e.g. rsync, rdiff-backup) #overssh : ['ls', 'rsync'] diff --git a/lshell/builtincmd.py b/lshell/builtincmd.py index f60f73c..5ac0290 100644 --- a/lshell/builtincmd.py +++ b/lshell/builtincmd.py @@ -6,12 +6,12 @@ import shlex import readline import signal -import re # import lshell specifics from lshell import variables from lshell import utils from lshell import sec as sec_policy +from lshell import history as history_utils # Store background jobs @@ -36,14 +36,7 @@ "source", ] -_BASH_FUNCTION_ENV_RE = re.compile(r"^BASH_FUNC_.*$") - - -def _is_forbidden_env_name(name): - """Return True when env var name is blocked for security reasons.""" - if name in variables.FORBIDDEN_ENVIRON: - return True - return bool(_BASH_FUNCTION_ENV_RE.match(name)) +default_builtins_list = [cmd for cmd in builtins_list if cmd not in ("export", "source")] def _cancel_job_timeout(job): @@ -107,6 +100,7 @@ def cmd_history(conf, log): """print the commands history""" try: try: + history_utils.prepare_history_for_write() readline.write_history_file(conf["history_file"]) except IOError: log.error(f"WARN: couldn't write history to file {conf['history_file']}\n") @@ -136,7 +130,7 @@ def cmd_export(args): if len(tokens) >= 2 and "=" in tokens[1]: var, value = tokens[1].split("=", 1) # disallow dangerous variable - if _is_forbidden_env_name(var): + if variables.is_forbidden_environment_key(var): return 1, var os.environ.update({var: value}) return 0, None diff --git a/lshell/cli.py b/lshell/cli.py index 2e843cd..98bb7b3 100644 --- a/lshell/cli.py +++ b/lshell/cli.py @@ -1,6 +1,5 @@ """CLI entry points for lshell.""" -import ast import os import signal import sys @@ -26,18 +25,8 @@ def main(): # Set SHELL and process LSHELL_ARGS env variables. os.environ["SHELL"] = os.path.realpath(sys.argv[0]) - if "LSHELL_ARGS" in os.environ: - try: - parsed_args = ast.literal_eval(os.environ["LSHELL_ARGS"]) - except (ValueError, SyntaxError): - parsed_args = [] - if not isinstance(parsed_args, (list, tuple)) or not all( - isinstance(item, str) for item in parsed_args - ): - parsed_args = [] - args = sys.argv[1:] + list(parsed_args) - else: - args = sys.argv[1:] + os.environ.pop("LSHELL_ARGS", None) + args = sys.argv[1:] userconf = CheckConfig(args).returnconf() userconf["session_id"] = os.environ.get("LSHELL_SESSION_ID", uuid.uuid4().hex) diff --git a/lshell/completion.py b/lshell/completion.py index 13bc277..6d08f1b 100644 --- a/lshell/completion.py +++ b/lshell/completion.py @@ -15,6 +15,18 @@ def completedefault(*ignored): return [] +def _prefix_matches(candidates, prefix): + """Return prefix matches, with a case-insensitive fallback when needed.""" + matches = [candidate for candidate in candidates if candidate.startswith(prefix)] + if matches or not prefix: + return matches + + prefix_lower = prefix.lower() + return [ + candidate for candidate in candidates if candidate.lower().startswith(prefix_lower) + ] + + def completenames(conf, text, line, *ignored): """This method is meant to override the original completenames method to overload it's output with the command available in the 'allowed' @@ -27,12 +39,13 @@ def completenames(conf, text, line, *ignored): # depending on completer delimiters/platform. if line.startswith("./") or text.startswith("./"): prefix = text[2:] if text.startswith("./") else text - matches = [cmd for cmd in commands if cmd.startswith(f"./{prefix}")] + relative_commands = [cmd for cmd in commands if cmd.startswith("./")] + matches = _prefix_matches(relative_commands, f"./{prefix}") if text.startswith("./"): return matches return [cmd[2:] for cmd in matches] - return [cmd for cmd in commands if cmd.startswith(text)] + return _prefix_matches(commands, text) def complete_sudo(conf, text, line, begidx, endidx): diff --git a/lshell/config/diagnostics.py b/lshell/config/diagnostics.py index dedb68f..76fd84d 100644 --- a/lshell/config/diagnostics.py +++ b/lshell/config/diagnostics.py @@ -42,6 +42,7 @@ "scp_upload", "scp_download", "sftp", + "sftp_unsafe_legacy", "umask", "aliases", "messages", @@ -107,6 +108,9 @@ def _build_runtime_policy(conf_raw, username): "policy_commands", "scp_upload", "scp_download", + "scp", + "sftp", + "sftp_unsafe_legacy", ]: try: if len(conf_raw[item]) == 0: @@ -147,7 +151,7 @@ def _build_runtime_policy(conf_raw, username): policy["path"] = ["", ""] policy["path"][0] = policy["home_path"] - policy["allowed"] += list(set(builtincmd.builtins_list) - set(["export"])) + policy["allowed"] += builtincmd.default_builtins_list if policy.get("policy_commands") != 1: policy["allowed"] = [ cmd for cmd in policy["allowed"] if cmd not in builtincmd.POLICY_COMMANDS diff --git a/lshell/config/runtime.py b/lshell/config/runtime.py index 14fd755..903b2bd 100644 --- a/lshell/config/runtime.py +++ b/lshell/config/runtime.py @@ -37,10 +37,12 @@ class CheckConfig: def noexec_library_usable(self, path_noexec): """Return True when a noexec library can be safely preloaded.""" - probe_env = dict(os.environ) + probe_env = { + key: value + for key, value in os.environ.items() + if not variables.should_strip_from_exec_env(key) + } probe_env["LD_PRELOAD"] = path_noexec - probe_env.pop("BASH_ENV", None) - probe_env.pop("ENV", None) try: probe = subprocess.run( @@ -85,6 +87,7 @@ def __init__(self, args, refresh=None, stdin=None, stdout=None, stderr=None): self.get_config_user() self.check_env() self.set_noexec() + self.finalize_command_resolution() def check_config_file_exists(self, configfile): """Check if the configuration file exists, else exit with error""" @@ -132,13 +135,6 @@ def getoptions(self, arguments, conf): if option in ["--version"]: utils.version() - # put the expanded path of configfile and logpath (if exists) in - # LSHELL_ARGS environment variable - args = ["--config", conf["configfile"]] - if "logpath" in conf: - args += ["--log", conf["logpath"]] - os.environ["LSHELL_ARGS"] = str(args) - # if lshell is invoked using shh autorized_keys file e.g. # command="/usr/bin/lshell", ssh-dss .... if "SSH_ORIGINAL_COMMAND" in os.environ: @@ -151,6 +147,11 @@ def check_env(self): if "env_vars" in self.conf: env_vars = self.conf["env_vars"] for key in env_vars.keys(): + if variables.is_forbidden_environment_key(key): + self.log.warning( + f"lshell: ignoring forbidden environment variable from config: {key}" + ) + continue os.environ[key] = str(env_vars[key]) # Check paths to files that contain env vars @@ -422,6 +423,12 @@ def _parse_history_file(self): sys.exit(1) return parsed + def finalize_command_resolution(self): + """Pin bare command names to resolved executables for this session.""" + self.conf["command_path_cache"] = utils.build_command_resolution_cache( + self.conf + ) + def check_user_integrity(self): """This method checks if all the required fields by user are present for the present user. @@ -471,6 +478,7 @@ def get_config_user(self): "scp_upload", "scp_download", "sftp", + "sftp_unsafe_legacy", "overssh", "strict", "aliases", @@ -625,21 +633,24 @@ def get_config_user(self): ) if self.conf["env_path"]: - new_path = f"{self.conf['env_path']}:{os.environ['PATH']}" - - # Check if the new path is valid - if all( - c in string.ascii_letters + string.digits + "/:-_." for c in new_path - ) and not new_path.startswith(":"): - os.environ["PATH"] = new_path - else: + if not all( + c in string.ascii_letters + string.digits + "/:-_." + for c in self.conf["env_path"] + ) or self.conf["env_path"].startswith(":"): self.stderr.write( f"lshell: config: env_path must be a valid $PATH: {self.conf['env_path']}\n" ) sys.exit(1) + return + + self.conf["runtime_path"] = utils.build_trusted_path( + env_path=self.conf["env_path"], + allowed_cmd_path=self.conf["allowed_cmd_path"], + ) + os.environ["PATH"] = self.conf["runtime_path"] # append default commands to allowed list - self.conf["allowed"] += list(set(builtincmd.builtins_list) - set(["export"])) + self.conf["allowed"] += builtincmd.default_builtins_list # Optionally hide policy introspection commands from users. if self.conf.get("policy_commands") != 1: @@ -656,8 +667,6 @@ def get_config_user(self): # add all commands present in allowed_cmd_path if specified if self.conf["allowed_cmd_path"]: for path in self.conf["allowed_cmd_path"]: - # add path to PATH env variable - os.environ["PATH"] += f":{path}" # find executable file, and add them to allowed commands for item in os.listdir(path): cmd = os.path.join(path, item) diff --git a/lshell/config/schema.py b/lshell/config/schema.py index 9b8f819..f5d56f5 100644 --- a/lshell/config/schema.py +++ b/lshell/config/schema.py @@ -38,6 +38,7 @@ "scp_upload", "scp_download", "sftp", + "sftp_unsafe_legacy", "strict", "history_size", "winscp", diff --git a/lshell/engine/executor.py b/lshell/engine/executor.py index eb9e727..6bafb8c 100644 --- a/lshell/engine/executor.py +++ b/lshell/engine/executor.py @@ -117,6 +117,23 @@ def _deny_with_reason(shell_context, command_line, decision): sys.stderr.write("lshell: forbidden trusted SSH protocol command\n") return 126 + if reason.code == reasons.COMMAND_PATH_CHANGED: + command = reason.details.get("command", "") + audit.log_command_event( + shell_context.conf, + command_line, + allowed=False, + reason=reasons.to_audit_reason(reason), + ) + message = messages.get_message( + shell_context.conf, + "command_path_changed", + command=command, + ) + shell_context.log.critical(message) + sys.stderr.write(f"{message}\n") + return 126 + audit.log_command_event( shell_context.conf, command_line, @@ -301,7 +318,7 @@ def execute(decisions, runtime): for _executable_name, _argument, _split, assignments in parsed_parts: for var_name, _var_value in assignments: - if var_name in variables.FORBIDDEN_ENVIRON: + if variables.is_forbidden_environment_key(var_name): deny_decision = authorizer.AuthorizationDecision( False, reasons.make_reason( @@ -369,31 +386,43 @@ def execute(decisions, runtime): and utils._is_allowed_command(executable_name, part, shell_context.conf) for (executable_name, _, _, _), part in zip(parsed_parts, pipeline_parts) ): - if not trusted_protocol: - missing_executable = next( - ( - executable_name - for executable_name, _, _, _ in parsed_parts - if executable_name - and executable_name not in builtincmd.builtins_list - and not utils._command_exists(executable_name) - ), - None, + pinned_pipeline_parts = [] + for (executable_name, _, _, _), part in zip(parsed_parts, pipeline_parts): + pinned_part, failed_command, failure_reason = utils.pin_command_executable( + part, + conf=shell_context.conf, ) - if missing_executable: + if failure_reason is not None: + if failure_reason == "drift": + deny_decision = authorizer.AuthorizationDecision( + False, + reasons.make_reason( + reasons.COMMAND_PATH_CHANGED, + command=failed_command, + line=full_command, + ), + decisions.ast, + ) + retcode = _deny_with_reason(shell_context, full_command, deny_decision) + return ExecutionResult( + retcode=retcode, + audit_reason=reasons.to_audit_reason(deny_decision.reason), + ) + command_not_found_message = messages.get_message( shell_context.conf, "command_not_found", - command=missing_executable, + command=failed_command, ) audit.log_command_event( shell_context.conf, full_command, allowed=False, - reason=f"command not found: {missing_executable}", + reason=f"command not found: {failed_command}", ) shell_context.log.critical(command_not_found_message) return ExecutionResult(retcode=127, audit_reason="command not found") + pinned_pipeline_parts.append(pinned_part) extra_env = None allowed_shell_escape = set(shell_context.conf.get("allowed_shell_escape", [])) @@ -411,8 +440,9 @@ def execute(decisions, runtime): allowed=True, reason="allowed by command and path policy", ) + command_to_execute = " | ".join(pinned_pipeline_parts) retcode = utils.exec_cmd( - full_command, + command_to_execute, background=background, extra_env=extra_env, conf=shell_context.conf, diff --git a/lshell/engine/reasons.py b/lshell/engine/reasons.py index 2b8c308..bc6637e 100644 --- a/lshell/engine/reasons.py +++ b/lshell/engine/reasons.py @@ -21,6 +21,7 @@ class Reason(NamedTuple): FORBIDDEN_ENV_ASSIGNMENT = "forbidden_env_assignment" FORBIDDEN_TRUSTED_PROTOCOL = "forbidden_trusted_protocol" COMMAND_NOT_FOUND = "command_not_found" +COMMAND_PATH_CHANGED = "command_path_changed" def make_reason(code, **details): @@ -59,6 +60,8 @@ def to_policy_message(reason): ) if code == COMMAND_NOT_FOUND: return f"command not found '{details.get('command', '')}'" + if code == COMMAND_PATH_CHANGED: + return f"command path changed '{details.get('command', '')}'" if code == FORBIDDEN_TRUSTED_PROTOCOL: return "forbidden trusted SSH protocol command" @@ -94,6 +97,8 @@ def to_audit_reason(reason): return "forbidden trusted SSH protocol command: " + details.get("command", "") if code == COMMAND_NOT_FOUND: return f"command not found: {details.get('command', '')}" + if code == COMMAND_PATH_CHANGED: + return f"command path changed since session start: {details.get('command', '')}" return "policy evaluation failed" diff --git a/lshell/hardeninit.py b/lshell/hardeninit.py index 2ea3701..72284bc 100644 --- a/lshell/hardeninit.py +++ b/lshell/hardeninit.py @@ -21,6 +21,7 @@ "scp_upload", "scp_download", "sftp", + "sftp_unsafe_legacy", "overssh", } UNSAFE_ALLOWED_SHELL_ESCAPE = { @@ -71,7 +72,8 @@ "scp": 0, "scp_upload": 0, "scp_download": 0, - "sftp": 1, + "sftp": 0, + "sftp_unsafe_legacy": 0, "overssh": [], "sudo_commands": [], "allowed_file_extensions": [], @@ -79,9 +81,9 @@ "umask": "0077", }, "explain": [ - "Use this for file-drop or file-retrieval accounts over SFTP.", - "Interactive shell functionality is intentionally minimal.", - "SCP is disabled to reduce protocol surface area.", + "Use this as the companion lshell policy for accounts whose real SFTP boundary is enforced in sshd_config.", + "Prefer ForceCommand internal-sftp plus ChrootDirectory for the actual transport path control.", + "Legacy sftp-server passthrough is left disabled on purpose.", ], }, "rsync-backup": { @@ -101,6 +103,7 @@ "scp_upload": 0, "scp_download": 0, "sftp": 0, + "sftp_unsafe_legacy": 0, "overssh": ["rsync"], "sudo_commands": [], "allowed_file_extensions": [], @@ -143,6 +146,7 @@ "scp_upload": 1, "scp_download": 0, "sftp": 0, + "sftp_unsafe_legacy": 0, "overssh": ["rsync", "scp"], "sudo_commands": [], "allowed_file_extensions": [".log", ".txt", ".yml", ".yaml"], @@ -172,6 +176,7 @@ "scp_upload": 0, "scp_download": 0, "sftp": 0, + "sftp_unsafe_legacy": 0, "overssh": [], "sudo_commands": [], "allowed_file_extensions": [".conf", ".ini", ".json", ".log", ".txt", ".yaml"], @@ -198,7 +203,10 @@ "scp": "Enable/disable SCP protocol surface.", "scp_upload": "Allow SCP uploads only when operationally required.", "scp_download": "Allow SCP downloads only when operationally required.", - "sftp": "Enable/disable SFTP protocol surface.", + "sftp": "Enable/disable legacy lshell SFTP passthrough surface.", + "sftp_unsafe_legacy": ( + "Explicit override for legacy sftp-server passthrough. Keep 0 unless you intentionally accept the risk." + ), "overssh": "Commands allowed for direct SSH command execution; keep as small as possible.", "sudo_commands": "Keep empty by default. Add only audited, non-interactive commands.", "allowed_file_extensions": ( @@ -278,7 +286,7 @@ def validate_profile(profile_name, profile_data): "allowed_shell_escape contains unsafe command: " + raw_command ) - for key in ("scp", "scp_upload", "scp_download", "sftp", "warning_counter"): + for key in ("scp", "scp_upload", "scp_download", "sftp", "sftp_unsafe_legacy", "warning_counter"): value = default.get(key) if not isinstance(value, int): errors.append(f"{key} must be an integer") @@ -289,8 +297,10 @@ def validate_profile(profile_name, profile_data): errors.append(f"{key} must be a list") if profile_name == "sftp-only": - if default.get("sftp") != 1 or default.get("scp") != 0: - errors.append("sftp-only profile must set sftp=1 and scp=0") + if default.get("sftp") != 0 or default.get("scp") != 0: + errors.append("sftp-only profile must set sftp=0 and scp=0") + if default.get("sftp_unsafe_legacy") != 0: + errors.append("sftp-only profile must keep sftp_unsafe_legacy=0") if default.get("overssh"): errors.append("sftp-only profile must keep overssh empty") elif profile_name == "rsync-backup": diff --git a/lshell/history.py b/lshell/history.py new file mode 100644 index 0000000..76207d6 --- /dev/null +++ b/lshell/history.py @@ -0,0 +1,105 @@ +"""History normalization and persistence helpers for lshell.""" + +import readline + + +def normalize_history_line(line): + """Normalize a history line without changing quoted whitespace.""" + line = line.strip() + if not line: + return "" + + normalized = [] + quote = None + escape = False + pending_space = False + + for char in line: + if escape: + if pending_space: + normalized.append(" ") + pending_space = False + normalized.append(char) + escape = False + continue + + if quote is None and char.isspace(): + pending_space = bool(normalized) + continue + + if pending_space: + normalized.append(" ") + pending_space = False + + normalized.append(char) + + if char == "\\" and quote != "'": + escape = True + continue + + if char in ("'", '"'): + if quote is None: + quote = char + elif quote == char: + quote = None + + return "".join(normalized).rstrip() + + +def current_history_entries(): + """Return the current readline history as a list of strings.""" + entries = [] + for index in range(1, readline.get_current_history_length() + 1): + entry = readline.get_history_item(index) + if entry: + entries.append(entry) + return entries + + +def _replace_history(entries): + """Replace readline history with the provided entries.""" + readline.clear_history() + for entry in entries: + readline.add_history(entry) + + +def prepare_latest_history_entry(raw_line): + """Apply session-safe history policies to the latest readline item.""" + history_length = readline.get_current_history_length() + if history_length <= 0: + return + + normalized = normalize_history_line(raw_line) + latest_index = history_length - 1 + latest_entry = readline.get_history_item(history_length) + if latest_entry is None: + return + + if not normalized: + readline.remove_history_item(latest_index) + return + + if latest_entry != normalized: + readline.replace_history_item(latest_index, normalized) + + if history_length > 1 and readline.get_history_item(history_length - 1) == normalized: + readline.remove_history_item(latest_index) + + +def entries_for_persisted_history(entries): + """Return normalized history entries with older duplicates removed.""" + deduplicated = [] + seen = set() + for entry in reversed(entries): + normalized = normalize_history_line(entry) + if not normalized or normalized in seen: + continue + seen.add(normalized) + deduplicated.append(normalized) + deduplicated.reverse() + return deduplicated + + +def prepare_history_for_write(): + """Rewrite readline history using persisted-history policies.""" + _replace_history(entries_for_persisted_history(current_history_entries())) diff --git a/lshell/messages.py b/lshell/messages.py index be50ef6..136dee4 100644 --- a/lshell/messages.py +++ b/lshell/messages.py @@ -6,6 +6,7 @@ DEFAULT_MESSAGES = { "unknown_syntax": "lshell: unknown syntax: {command}", "command_not_found": 'lshell: command not found: "{command}"', + "command_path_changed": 'lshell: command path changed since session start: "{command}"', "forbidden_generic": 'lshell: forbidden {messagetype}: "{command}"', "forbidden_command": 'lshell: forbidden command: "{command}"', "forbidden_path": 'lshell: forbidden path: "{command}"', @@ -24,6 +25,7 @@ MESSAGE_FIELDS = { "unknown_syntax": {"command"}, "command_not_found": {"command"}, + "command_path_changed": {"command"}, "forbidden_generic": {"messagetype", "command"}, "forbidden_command": {"command"}, "forbidden_path": {"command"}, diff --git a/lshell/sec.py b/lshell/sec.py index 16efa99..5cf431b 100644 --- a/lshell/sec.py +++ b/lshell/sec.py @@ -26,6 +26,51 @@ inspect_shell_expansions = expansion_inspector.inspect_shell_expansions _EXTGLOB_OPENERS = ("@(", "!(", "+(", "*(", "?(") +_AWK_COMMANDS = {"awk", "gawk", "mawk", "nawk"} +_SED_COMMANDS = {"sed", "gsed"} + +# Commands whose positional operands commonly refer to filesystem entries. +# We use this to protect bareword symlink targets without treating generic +# literals (for example `echo hello`) as path operands. +_BAREWORD_FILESYSTEM_COMMANDS = { + *_AWK_COMMANDS, + "cat", + "chgrp", + "chmod", + "chown", + "cmp", + "comm", + "cp", + "diff", + "du", + "file", + "grep", + "egrep", + "fgrep", + "rgrep", + "head", + "install", + "less", + "ln", + "ls", + "mkdir", + "more", + "mv", + "nl", + "patch", + "readlink", + "realpath", + "rm", + "rmdir", + *_SED_COMMANDS, + "sort", + "stat", + "tac", + "tail", + "tee", + "touch", + "wc", +} def _is_assignment_word(word): @@ -186,6 +231,14 @@ def _safe_expand_path(path): return None +def _safe_lexists(path): + """Return os.path.lexists(path) while rejecting malformed inputs.""" + try: + return os.path.lexists(path) + except (OSError, TypeError, ValueError): + return False + + def _contains_unescaped_extglob(pattern): """Return True when extglob operators are present in an unescaped form.""" escaped = False @@ -456,6 +509,39 @@ def _looks_like_path_token(token): return False +def _looks_like_numeric_literal(token): + """Return True for plain numeric arguments such as `10` or `-1`.""" + return bool(re.fullmatch(r"[-+]?\d+(?:\.\d+)?", token)) + + +def _references_existing_path(token): + """Return True when token resolves to an existing filesystem entry.""" + expanded = _safe_expand_path(token) + if expanded is None: + return False + if not os.path.isabs(expanded): + expanded = os.path.join(os.getcwd(), expanded) + return _safe_lexists(expanded) + + +def _command_name(command): + """Return a basename-style command identifier for operand heuristics.""" + if not command: + return "" + return os.path.basename(command) + + +def _should_check_bareword_path_operand(command, token): + """Return True when a bareword operand should be treated as a path.""" + if not token or token == "-" or token.startswith("-"): + return False + if _looks_like_numeric_literal(token): + return False + if _command_name(command) not in _BAREWORD_FILESYSTEM_COMMANDS: + return False + return _references_existing_path(token) + + def _grep_implicit_pattern_index(args): """Return the grep implicit PATTERN arg index, or None when explicit patterns are used.""" has_explicit_pattern = False @@ -496,6 +582,170 @@ def _grep_implicit_pattern_index(args): return None +def _awk_non_path_indices(args): + """Return awk argument indices that are program/options, not file operands.""" + skip_indices = set() + has_program = False + index = 0 + + while index < len(args): + token = args[index] + + if token == "--": + if not has_program and index + 1 < len(args): + skip_indices.add(index + 1) + has_program = True + index += 2 + continue + index += 1 + continue + + if token in {"-v", "-F"}: + if index + 1 < len(args): + skip_indices.add(index + 1) + index += 2 + continue + + if token == "-f": + # The following argument is an awk program file and must be checked. + has_program = True + index += 2 + continue + + if token.startswith(("-v", "-F", "-f", "--")) and token != "-": + if token.startswith("-f"): + has_program = True + index += 1 + continue + + if token.startswith("-") and token != "-": + index += 1 + continue + + if not has_program: + skip_indices.add(index) + has_program = True + + index += 1 + + return skip_indices + + +def _awk_path_like_non_path_indices(args): + """Return awk argument indices that should not be path-checked even if path-like.""" + skip_indices = set() + index = 0 + + while index < len(args): + token = args[index] + + if token == "--": + index += 1 + continue + + if token in {"-v", "-F"}: + if index + 1 < len(args): + skip_indices.add(index + 1) + index += 2 + continue + + if token in {"-f"}: + index += 2 + continue + + if token.startswith(("-v", "-F", "--")) and token != "-": + index += 1 + continue + + if token.startswith("-") and token != "-": + index += 1 + continue + + index += 1 + + return skip_indices + + +def _sed_non_path_indices(args): + """Return sed argument indices that are scripts/options, not file operands.""" + skip_indices = set() + has_script = False + index = 0 + + while index < len(args): + token = args[index] + + if token == "--": + if not has_script and index + 1 < len(args): + skip_indices.add(index + 1) + has_script = True + index += 2 + continue + index += 1 + continue + + if token in {"-e", "--expression"}: + if index + 1 < len(args): + skip_indices.add(index + 1) + has_script = True + index += 2 + continue + + if token in {"-f", "--file"}: + # The following argument is a sed script file and must be checked. + has_script = True + index += 2 + continue + + if token.startswith(("-e", "--expression=")) and token != "-": + has_script = True + index += 1 + continue + + if token.startswith(("-f", "--file=")) and token != "-": + has_script = True + index += 1 + continue + + if token.startswith("-") and token != "-": + index += 1 + continue + + if not has_script: + skip_indices.add(index) + has_script = True + + index += 1 + + return skip_indices + + +def _non_path_operand_indices(command, args): + """Return argument indices that are not bareword filesystem operands.""" + command_name = _command_name(command) + if command_name in {"grep", "egrep", "fgrep", "rgrep"}: + implicit_pattern_index = _grep_implicit_pattern_index(args) + return {implicit_pattern_index} if implicit_pattern_index is not None else set() + if command_name in _AWK_COMMANDS: + return _awk_non_path_indices(args) + if command_name in _SED_COMMANDS: + return _sed_non_path_indices(args) + return set() + + +def _path_like_non_path_operand_indices(command, args): + """Return argument indices that should not be path-checked even if path-like.""" + command_name = _command_name(command) + if command_name in {"grep", "egrep", "fgrep", "rgrep"}: + implicit_pattern_index = _grep_implicit_pattern_index(args) + return {implicit_pattern_index} if implicit_pattern_index is not None else set() + if command_name in _AWK_COMMANDS: + return _awk_path_like_non_path_indices(args) + if command_name in _SED_COMMANDS: + return _sed_non_path_indices(args) + return set() + + def _path_tokens_from_line(line): """Extract path-like tokens from command segments, excluding bare command names.""" segments = utils.split_commands(line) @@ -526,16 +776,22 @@ def _path_tokens_from_line(line): continue if args: - skip_indices = set() - if command in {"grep", "egrep", "fgrep", "rgrep"}: - implicit_pattern_index = _grep_implicit_pattern_index(args) - if implicit_pattern_index is not None: - skip_indices.add(implicit_pattern_index) + bareword_skip_indices = _non_path_operand_indices(command, args) + path_like_skip_indices = _path_like_non_path_operand_indices(command, args) path_tokens.extend( token for idx, token in enumerate(args) - if idx not in skip_indices and _looks_like_path_token(token) + if ( + ( + idx not in path_like_skip_indices + and _looks_like_path_token(token) + ) + or ( + idx not in bareword_skip_indices + and _should_check_bareword_path_operand(command, token) + ) + ) ) continue diff --git a/lshell/shellcmd.py b/lshell/shellcmd.py index e60e264..0217f18 100644 --- a/lshell/shellcmd.py +++ b/lshell/shellcmd.py @@ -5,11 +5,15 @@ """ import cmd +import ctypes +import ctypes.util import sys import os import re import signal import readline +import shutil +import shlex # import lshell specifics from lshell.config.runtime import CheckConfig @@ -21,6 +25,185 @@ from lshell import variables from lshell.config import diagnostics as policy_mode from lshell import audit +from lshell import history as history_utils + + +READLINE_HISTORY_SEARCH_BINDINGS = ( + '"\\e[A": history-search-backward', + '"\\eOA": history-search-backward', + '"\\e[B": history-search-forward', + '"\\eOB": history-search-forward', +) +READLINE_INCREMENTAL_SEARCH_BINDINGS = ( + '"\\C-r": reverse-search-history', + '"\\C-s": forward-search-history', +) + +_READLINE_LIB = None +_READLINE_COMMAND_FUNC = None +_READLINE_HISTORY_SEARCH_CALLBACKS = [] +_ACTIVE_HISTORY_SEARCH_SHELL = None +_ACTIVE_COMPLETION_SHELL = None + + +def _classify_scp_direction(command_line): + """Return the SCP transfer direction for a forced SSH command. + + Returns ``"download"`` for ``-f``, ``"upload"`` for ``-t``, ``"ambiguous"`` + when both appear, and ``None`` when the command line does not contain a + recognized transfer direction. + """ + try: + tokens = shlex.split(command_line, posix=True) + except ValueError: + return None + + if not tokens or tokens[0] != "scp": + return None + + saw_download = False + saw_upload = False + + for token in tokens[1:]: + if token == "--": + break + if token == "-" or not token.startswith("-") or token.startswith("--"): + break + + for option in token[1:]: + if option == "f": + saw_download = True + elif option == "t": + saw_upload = True + + if saw_download and saw_upload: + return "ambiguous" + if saw_download: + return "download" + if saw_upload: + return "upload" + return None + + +def _readline_uses_gnu_backend(): + """Return True only when Python readline is backed by GNU readline.""" + doc = readline.__doc__ or "" + return "libedit" not in doc.lower() + + +def _get_readline_library(): + """Return the loaded GNU readline shared library when available.""" + global _READLINE_LIB, _READLINE_COMMAND_FUNC + + if _READLINE_LIB is not None: + return _READLINE_LIB + + if not _readline_uses_gnu_backend(): + return None + + library_name = ctypes.util.find_library("readline") + if not library_name: + return None + + try: + readline_lib = ctypes.CDLL(library_name) + except OSError: + return None + + _READLINE_COMMAND_FUNC = ctypes.CFUNCTYPE(ctypes.c_int, ctypes.c_int, ctypes.c_int) + readline_lib.rl_bind_keyseq.argtypes = [ctypes.c_char_p, _READLINE_COMMAND_FUNC] + readline_lib.rl_bind_keyseq.restype = ctypes.c_int + readline_lib.rl_replace_line.argtypes = [ctypes.c_char_p, ctypes.c_int] + readline_lib.rl_replace_line.restype = None + readline_lib.rl_redisplay.argtypes = [] + readline_lib.rl_redisplay.restype = None + + _READLINE_LIB = readline_lib + return _READLINE_LIB + + +def _replace_readline_buffer(text): + """Replace the active readline buffer with text and move cursor to the end.""" + readline_lib = _get_readline_library() + if readline_lib is None: + return + + encoded = text.encode("utf-8") + readline_lib.rl_replace_line(encoded, 0) + encoded_length = len(encoded) + ctypes.c_int.in_dll(readline_lib, "rl_point").value = encoded_length + ctypes.c_int.in_dll(readline_lib, "rl_end").value = encoded_length + readline_lib.rl_redisplay() + + +def _readline_point(): + """Return the current cursor position in the active readline buffer.""" + readline_lib = _get_readline_library() + if readline_lib is None: + return len(readline.get_line_buffer()) + return ctypes.c_int.in_dll(readline_lib, "rl_point").value + + +def _readline_char_point(text): + """Map readline's byte cursor offset to a Python string character index.""" + point = _readline_point() + encoded = text.encode("utf-8") + if point >= len(encoded): + return len(text) + return len(encoded[:point].decode("utf-8", "ignore")) + + +def _dispatch_history_search(backward): + """Route readline up/down callbacks to the active shell instance.""" + shell = _ACTIVE_HISTORY_SEARCH_SHELL + if shell is None: + return 0 + + try: + return shell.history_search(backward) + except Exception: + shell.reset_history_search_state() + return 0 + + +def _history_search_backward(unused_count, unused_key): + """Readline callback for backward prefix history search.""" + return _dispatch_history_search(backward=True) + + +def _history_search_forward(unused_count, unused_key): + """Readline callback for forward prefix history search.""" + return _dispatch_history_search(backward=False) + + +def _bind_custom_history_search(shell): + """Bind arrow keys to lshell-managed prefix history search callbacks.""" + global _ACTIVE_HISTORY_SEARCH_SHELL + + readline_lib = _get_readline_library() + if readline_lib is None or _READLINE_COMMAND_FUNC is None: + return False + + backward = _READLINE_COMMAND_FUNC(_history_search_backward) + forward = _READLINE_COMMAND_FUNC(_history_search_forward) + _READLINE_HISTORY_SEARCH_CALLBACKS[:] = [backward, forward] + _ACTIVE_HISTORY_SEARCH_SHELL = shell + + bindings = ( + (b"\\e[A", backward), + (b"\\eOA", backward), + (b"\\e[B", forward), + (b"\\eOB", forward), + ) + return all(readline_lib.rl_bind_keyseq(keyseq, callback) == 0 for keyseq, callback in bindings) + + +def _display_completion_matches(substitution, matches, longest_match_length): + """Render completion matches with a concise, sorted lshell-specific header.""" + shell = _ACTIVE_COMPLETION_SHELL + if shell is None: + return + shell.display_completion_matches(substitution, matches, longest_match_length) class ShellCmd(cmd.Cmd, object): @@ -75,6 +258,9 @@ def __init__( # initialize return code self.retcode = 0 + self.reset_history_search_state() + self.completion_display_context = "Allowed completions" + self.old_display_matches_hook = None # run overssh, if needed self.run_overssh() @@ -165,6 +351,13 @@ def _aliases_for_ssh_command(): aliases.pop("ls", None) return aliases + def _is_sftp_protocol_request(command_line): + for part in utils.split_commands(command_line): + executable, _argument, _split, _assignments = utils._parse_command(part) + if executable in variables.TRUSTED_SFTP_PROTOCOL_BINARIES: + return True + return False + if "ssh" in self.conf: if "SSH_CLIENT" in os.environ and "SSH_TTY" not in os.environ: # Apply aliases consistently for all SSH command paths. @@ -173,8 +366,8 @@ def _aliases_for_ssh_command(): ).strip() # check if sftp is requested and allowed - if "sftp-server" in self.conf["ssh"]: - if self.conf["sftp"] == 1: + if _is_sftp_protocol_request(self.conf["ssh"]): + if self.conf["sftp"] == 1 and self.conf.get("sftp_unsafe_legacy") == 1: _with_protocol_in_overssh( variables.TRUSTED_SFTP_PROTOCOL_BINARIES ) @@ -185,6 +378,21 @@ def _aliases_for_ssh_command(): retcode = _execute_trusted_ssh_protocol(trusted_protocol=True) self.log.error("SFTP disconnect") sys.exit(retcode) + elif self.conf["sftp"] == 1: + message = ( + "lshell: refusing legacy SFTP passthrough; " + "set sftp_unsafe_legacy=1 to override or use " + "sshd_config ForceCommand internal-sftp with ChrootDirectory\n" + ) + self.log.error("*** legacy SFTP passthrough refused") + audit.log_command_event( + self.conf, + self.conf["ssh"], + allowed=False, + reason="refused legacy SFTP passthrough", + ) + sys.stderr.write(message) + sys.exit(1) else: self.log.error("*** forbidden SFTP connection") audit.log_command_event( @@ -199,8 +407,9 @@ def _aliases_for_ssh_command(): if self.conf["ssh"].startswith("scp "): if self.conf["scp"] == 1 or "scp" in self.conf["overssh"]: _with_protocol_in_overssh(["scp"]) + scp_direction = _classify_scp_direction(self.conf["ssh"]) - if " -f " in self.conf["ssh"]: + if scp_direction == "download": # case scp download is allowed if self.conf["scp_download"]: self.log.error(f'SCP: GET "{self.conf["ssh"]}"') @@ -216,7 +425,7 @@ def _aliases_for_ssh_command(): reason="forbidden SCP download", ) sys.exit(1) - elif " -t " in self.conf["ssh"]: + elif scp_direction == "upload": # case scp upload is allowed if self.conf["scp_upload"]: if "scpforce" in self.conf: @@ -243,6 +452,17 @@ def _aliases_for_ssh_command(): reason="forbidden SCP upload", ) sys.exit(1) + else: + self.log.error( + f'SCP: unrecognized transfer direction: "{self.conf["ssh"]}"' + ) + audit.log_command_event( + self.conf, + self.conf["ssh"], + allowed=False, + reason="forbidden SCP direction", + ) + sys.exit(1) _validate_ssh_command() retcode = _execute_trusted_ssh_protocol(trusted_protocol=False) self.log.error("SCP disconnect") @@ -337,6 +557,147 @@ def run_script_mode(self, script): if stop: sys.exit(1) + def _configure_readline(self): + """Initialize readline history, completion, and safe history search.""" + try: + readline.read_history_file(self.conf["history_file"]) + except IOError: + # if history file does not exist + try: + open(self.conf["history_file"], "w").close() + readline.read_history_file(self.conf["history_file"]) + except IOError: + pass + readline.set_history_length(self.conf["history_size"]) + readline.set_completer_delims(readline.get_completer_delims().replace("-", "")) + self.old_completer = readline.get_completer() + readline.set_completer(self.complete) + readline.parse_and_bind(self.completekey + ": complete") + if hasattr(readline, "set_completion_display_matches_hook"): + global _ACTIVE_COMPLETION_SHELL + _ACTIVE_COMPLETION_SHELL = self + readline.set_completion_display_matches_hook(_display_completion_matches) + for binding in READLINE_INCREMENTAL_SEARCH_BINDINGS: + readline.parse_and_bind(binding) + if not _bind_custom_history_search(self): + for binding in READLINE_HISTORY_SEARCH_BINDINGS: + readline.parse_and_bind(binding) + + def reset_history_search_state(self): + """Clear state used for prefix history navigation on arrow keys.""" + self.history_search_state = { + "prefix": None, + "matches": [], + "index": None, + "original_line": "", + } + + def _collect_history_prefix_matches(self, prefix): + """Return unique history entries matching prefix, newest first.""" + seen = set() + matches = [] + history_length = readline.get_current_history_length() + for index in range(history_length, 0, -1): + entry = readline.get_history_item(index) + if not entry or not entry.startswith(prefix) or entry in seen: + continue + seen.add(entry) + matches.append(entry) + return matches + + def history_search(self, backward): + """Navigate unique history entries that match the current line prefix.""" + current_line = readline.get_line_buffer() + state = self.history_search_state + active_match = None + if state["matches"] and state["index"] is not None: + active_match = state["matches"][state["index"]] + + continuing = current_line in (state["original_line"], active_match) + if not continuing: + if not backward: + self.reset_history_search_state() + return 0 + + prefix = current_line[: _readline_char_point(current_line)] + matches = self._collect_history_prefix_matches(prefix) + if not matches: + self.reset_history_search_state() + return 0 + + self.history_search_state = { + "prefix": prefix, + "matches": matches, + "index": 0, + "original_line": current_line, + } + _replace_readline_buffer(matches[0]) + return 0 + + if backward: + if state["index"] < len(state["matches"]) - 1: + state["index"] += 1 + _replace_readline_buffer(state["matches"][state["index"]]) + return 0 + + if state["index"] > 0: + state["index"] -= 1 + _replace_readline_buffer(state["matches"][state["index"]]) + return 0 + + _replace_readline_buffer(state["original_line"]) + self.reset_history_search_state() + return 0 + + def _completion_context_label(self, compfunc): + """Return a user-facing label for the current completion source.""" + if compfunc == completion.complete_sudo: + return "Allowed sudo commands" + if compfunc == completion.complete_change_dir: + return "Allowed directories" + if compfunc == completion.complete_list_dir: + return "Allowed paths" + if compfunc == completion.completenames: + return "Allowed commands" + return "Allowed completions" + + def display_completion_matches(self, substitution, matches, longest_match_length): + """Show a compact header and sorted completion candidates.""" + del substitution + + rendered_matches = sorted(dict.fromkeys(matches), key=lambda item: item.lower()) + if not rendered_matches: + return + + terminal_width = shutil.get_terminal_size((80, 24)).columns + column_width = max(longest_match_length + 2, 2) + columns = max(1, terminal_width // column_width) + lines = [] + for start in range(0, len(rendered_matches), columns): + row = rendered_matches[start : start + columns] + if len(row) == 1: + lines.append(row[0]) + continue + lines.append("".join(item.ljust(column_width) for item in row).rstrip()) + + sys.stdout.write( + f"\n[{self.completion_display_context}: {len(rendered_matches)}]\n" + ) + sys.stdout.write("\n".join(lines) + "\n") + sys.stdout.flush() + readline_lib = _get_readline_library() + if readline_lib is not None: + readline_lib.rl_redisplay() + + def _prepare_history_before_write(self): + """Apply persisted-history policies before writing the history file.""" + history_utils.prepare_history_for_write() + + def _write_history_file(self): + """Persist readline history after applying lshell history policies.""" + self._prepare_history_before_write() + readline.write_history_file(self.conf["history_file"]) + def cmdloop(self, intro=None): """Repeatedly issue a prompt, accept input, parse an initial prefix off the received input, and dispatch to action methods, passing them @@ -349,22 +710,7 @@ def cmdloop(self, intro=None): self.preloop() if self.use_rawinput and self.completekey: - try: - readline.read_history_file(self.conf["history_file"]) - except IOError: - # if history file does not exist - try: - open(self.conf["history_file"], "w").close() - readline.read_history_file(self.conf["history_file"]) - except IOError: - pass - readline.set_history_length(self.conf["history_size"]) - readline.set_completer_delims( - readline.get_completer_delims().replace("-", "") - ) - self.old_completer = readline.get_completer() - readline.set_completer(self.complete) - readline.parse_and_bind(self.completekey + ": complete") + self._configure_readline() try: if self.intro and isinstance(self.intro, str): self.stdout.write(f"{self.intro}\n") @@ -381,14 +727,21 @@ def cmdloop(self, intro=None): try: # Check background jobs after each command builtincmd.check_background_jobs() + line_from_eof = False + line_from_readline = False if self.cmdqueue: line = self.cmdqueue.pop(0) else: if self.use_rawinput: + global _ACTIVE_HISTORY_SEARCH_SHELL + _ACTIVE_HISTORY_SEARCH_SHELL = self + self.reset_history_search_state() + line_from_readline = True try: line = input(self.conf["promptprint"]) except EOFError: line = "EOF" + line_from_eof = True except KeyboardInterrupt: self.stdout.write("\n") if partial_line: @@ -406,6 +759,7 @@ def cmdloop(self, intro=None): else: # chop \n line = line[:-1] + had_partial_line = bool(partial_line) if len(line) > 1 and line.startswith("\\"): # implying previous partial line line = line[:1].replace("\\", "", 1) @@ -428,6 +782,8 @@ def cmdloop(self, intro=None): self.conf["promptprint"] = utils.updateprompt( os.getcwd(), self.conf ) + if line_from_readline and not had_partial_line and not line_from_eof: + history_utils.prepare_latest_history_entry(line) line = self.precmd(line) stop = self.onecmd(line) stop = self.postcmd(stop, line) @@ -449,10 +805,12 @@ def cmdloop(self, intro=None): readline.get_completer_delims().replace("-", "") ) readline.set_completer(self.old_completer) + if hasattr(readline, "set_completion_display_matches_hook"): + readline.set_completion_display_matches_hook(None) except ImportError: pass try: - readline.write_history_file(self.conf["history_file"]) + self._write_history_file() except IOError: self.log.error( f"WARN: couldn't write history to file {self.conf['history_file']}\n" @@ -509,7 +867,11 @@ def complete(self, text, state): # call the lshell allowed commands completion compfunc = completion.completenames - self.completion_matches = compfunc(self.conf, text, line, begidx, endidx) + self.completion_display_context = self._completion_context_label(compfunc) + matches = compfunc(self.conf, text, line, begidx, endidx) + self.completion_matches = sorted( + dict.fromkeys(matches), key=lambda item: item.lower() + ) try: return self.completion_matches[state] except IndexError: diff --git a/lshell/utils.py b/lshell/utils.py index 0c1017f..a015031 100644 --- a/lshell/utils.py +++ b/lshell/utils.py @@ -10,9 +10,10 @@ import string import shlex import shutil +import stat import threading from getpass import getuser -from time import strftime, gmtime +from time import strftime, localtime import signal # import lshell specifics @@ -355,6 +356,16 @@ def replace_exit_code(line, retcode): "/usr/bin/sh", ) +_TRUSTED_RUNTIME_PATH_DIRS = ( + "/opt/homebrew/bin", + "/usr/local/sbin", + "/usr/local/bin", + "/usr/sbin", + "/usr/bin", + "/sbin", + "/bin", +) + def _resolve_trusted_shell(): """Return an absolute trusted shell interpreter path, or None.""" @@ -364,9 +375,220 @@ def _resolve_trusted_shell(): return None -def _is_bash_function_env_name(name): - """Return True for env vars used by Bash function import.""" - return name.startswith("BASH_FUNC_") +def _split_path_entries(path_value): + """Split a PATH-style string into non-empty entries.""" + if not path_value: + return [] + return [entry for entry in str(path_value).split(os.pathsep) if entry] + + +def build_trusted_path(env_path="", allowed_cmd_path=None): + """Return a deterministic PATH for restricted command resolution.""" + directories = [] + + for entry in _split_path_entries(env_path): + directories.append(entry) + + for entry in allowed_cmd_path or []: + if entry: + directories.append(str(entry)) + + for entry in _TRUSTED_RUNTIME_PATH_DIRS: + directories.append(entry) + + unique_directories = [] + seen = set() + for entry in directories: + if entry in seen: + continue + seen.add(entry) + unique_directories.append(entry) + + return os.pathsep.join(unique_directories) if unique_directories else os.defpath + + +def runtime_search_path(conf=None): + """Return the PATH used for restricted lookup and child execution.""" + if conf and conf.get("runtime_path"): + return conf["runtime_path"] + return build_trusted_path() + + +def _command_metadata_fingerprint(path): + """Return a metadata fingerprint for an executable, or None on stat failure.""" + try: + stat_result = os.stat(path) + except OSError: + return None + + return { + "device": stat_result.st_dev, + "inode": stat_result.st_ino, + "mode": stat.S_IMODE(stat_result.st_mode), + "uid": stat_result.st_uid, + "gid": stat_result.st_gid, + "size": stat_result.st_size, + "mtime_ns": getattr( + stat_result, "st_mtime_ns", int(stat_result.st_mtime * 1_000_000_000) + ), + } + + +def resolve_command_record(executable, conf=None): + """Resolve one executable token to a canonical absolute path + metadata.""" + if not executable: + return None + + if "/" in executable: + resolved_path = os.path.realpath(executable) + else: + resolved_path = shutil.which(executable, path=runtime_search_path(conf)) + if resolved_path is None: + for candidate in variables.TRUSTED_SFTP_PROTOCOL_BINARIES: + if "/" not in candidate: + continue + if os.path.basename(candidate) != executable: + continue + if os.path.isfile(candidate) and os.access(candidate, os.X_OK): + resolved_path = candidate + break + if resolved_path is None: + return None + resolved_path = os.path.realpath(resolved_path) + + if not os.path.isfile(resolved_path) or not os.access(resolved_path, os.X_OK): + return None + + fingerprint = _command_metadata_fingerprint(resolved_path) + if fingerprint is None: + return None + + return { + "requested": executable, + "path": resolved_path, + "fingerprint": fingerprint, + } + + +def build_command_resolution_cache(conf): + """Resolve and pin executable targets for bare command names in policy.""" + cache = {} + candidates = set() + + for key in ("allowed", "allowed_shell_escape", "overssh"): + for entry in conf.get(key, []): + if not isinstance(entry, str): + continue + executable, _argument, _split, _assignments = _parse_command(entry) + if executable: + candidates.add(executable) + + if conf.get("scp") == 1: + candidates.add("scp") + candidates.update(variables.TRUSTED_SFTP_PROTOCOL_BINARIES) + + for executable in candidates: + if "/" in executable: + continue + if executable in builtincmd.builtins_list and executable != "ls": + continue + + record = resolve_command_record(executable, conf=conf) + if record is not None: + cache[executable] = record + + return cache + + +def resolve_approved_executable(executable, conf=None): + """Return the pinned absolute executable path, or an error reason.""" + if not executable: + return None, "missing" + + if "/" in executable: + return executable, None + + expected = (conf or {}).get("command_path_cache", {}).get(executable) + if expected is None: + return None, "unapproved" + + current = resolve_command_record(executable, conf=conf) + if current is None: + return None, "missing" + + if ( + current["path"] != expected["path"] + or current["fingerprint"] != expected["fingerprint"] + ): + return None, "drift" + + return expected["path"], None + + +def pin_command_executable(command, conf=None): + """Rewrite one command segment to execute its pinned absolute executable.""" + executable, _argument, split, assignments = _parse_command(command) + if executable is None: + return None, None, "parse_error" + if not executable: + return command, "", None + + if executable in builtincmd.builtins_list and executable != "ls": + return command, executable, None + + resolved_path, failure_reason = resolve_approved_executable(executable, conf=conf) + if resolved_path is None: + return None, executable, failure_reason + + if "/" in executable: + return command, executable, None + + token_spans = [] + index = 0 + length = len(command) + + while index < length: + while index < length and command[index].isspace(): + index += 1 + if index >= length: + break + + start = index + in_single = False + in_double = False + escaped = False + + while index < length: + char = command[index] + if escaped: + escaped = False + index += 1 + continue + if char == "\\" and not in_single: + escaped = True + index += 1 + continue + if char == "'" and not in_double: + in_single = not in_single + index += 1 + continue + if char == '"' and not in_single: + in_double = not in_double + index += 1 + continue + if not in_single and not in_double and char.isspace(): + break + index += 1 + + token_spans.append((start, index)) + + executable_index = len(assignments) + if executable_index >= len(token_spans): + return None, executable, "parse_error" + + start, end = token_spans[executable_index] + pinned = f"{command[:start]}{resolved_path}{command[end:]}" + return pinned, executable, None def _expand_braced_parameter(expr, support_advanced=True): @@ -521,7 +743,7 @@ def _is_allowed_command(executable, command, conf): return executable in conf["allowed"] or command in conf["allowed"] -def _command_exists(executable): +def _command_exists(executable, conf=None): """Return True when command token resolves to a runnable command.""" if not executable: return False @@ -532,7 +754,7 @@ def _command_exists(executable): if "/" in executable: return os.path.isfile(executable) and os.access(executable, os.X_OK) - return shutil.which(executable) is not None + return shutil.which(executable, path=runtime_search_path(conf)) is not None def handle_builtin_command(full_command, executable, argument, shell_context): @@ -555,7 +777,29 @@ def handle_builtin_command(full_command, executable, argument, shell_context): elif executable == "cd": retcode, shell_context.conf = builtincmd.cmd_cd(argument, shell_context.conf) elif executable == "ls": - retcode = exec_cmd(full_command, conf=shell_context.conf, log=shell_context.log) + pinned_command, failed_command, failure_reason = pin_command_executable( + full_command, + conf=shell_context.conf, + ) + if failure_reason == "drift": + shell_context.log.critical( + f'lshell: command path changed since session start: "{failed_command}"' + ) + sys.stderr.write( + f'lshell: command path changed since session start: "{failed_command}"\n' + ) + return 126, conf + if failure_reason is not None: + shell_context.log.critical( + f'lshell: command not found: "{failed_command}"' + ) + sys.stderr.write(f'lshell: command not found: "{failed_command}"\n') + return 127, conf + retcode = exec_cmd( + pinned_command, + conf=shell_context.conf, + log=shell_context.log, + ) elif executable == "export": retcode, var = builtincmd.cmd_export(full_command) if retcode == 1: @@ -589,7 +833,12 @@ def exec_cmd(cmd, background=False, extra_env=None, conf=None, log=None): """Execute a command exactly as entered, with support for backgrounding via Ctrl+Z.""" proc = None detached_session = True - exec_env = dict(os.environ) + exec_env = { + key: value + for key, value in os.environ.items() + if not variables.should_strip_from_exec_env(key) + } + exec_env["PATH"] = runtime_search_path(conf) runtime_limits = containment.get_runtime_limits(conf or {}) command_timeout = runtime_limits.command_timeout unsupported_limits = containment.unsupported_rlimits(runtime_limits) @@ -605,13 +854,6 @@ def exec_cmd(cmd, background=False, extra_env=None, conf=None, log=None): conf[logged_key] = sorted(already_logged.union(pending)) if extra_env: exec_env.update(extra_env) - # Prevent non-interactive shell startup file injection. - exec_env.pop("BASH_ENV", None) - exec_env.pop("ENV", None) - # Prevent function import from environment (e.g. BASH_FUNC_* poisoning). - for key in list(exec_env): - if _is_bash_function_env_name(key): - exec_env.pop(key, None) class CtrlZException(Exception): """Custom exception to handle Ctrl+Z (SIGTSTP).""" @@ -806,6 +1048,7 @@ def parse_ps1(ps1): cwd = os.getcwd() home = os.path.expanduser("~") prompt_symbol = "#" if os.geteuid() == 0 else "$" + current_time = localtime() # Define LPS1 replacement mappings replacements = { @@ -816,11 +1059,11 @@ def parse_ps1(ps1): r"\W": os.path.basename(cwd), r"\$": prompt_symbol, r"\\": "\\", - r"\t": strftime("%H:%M:%S", gmtime()), - r"\T": strftime("%I:%M:%S", gmtime()), - r"\A": strftime("%H:%M", gmtime()), - r"\@": strftime("%I:%M:%S%p", gmtime()), - r"\d": strftime("%a %b %d", gmtime()), + r"\t": strftime("%H:%M:%S", current_time), + r"\T": strftime("%I:%M:%S", current_time), + r"\A": strftime("%H:%M", current_time), + r"\@": strftime("%I:%M:%S%p", current_time), + r"\d": strftime("%a %b %d", current_time), } # Replace each placeholder with its corresponding value for placeholder, value in replacements.items(): diff --git a/lshell/variables.py b/lshell/variables.py index 8f72fe6..391fb88 100644 --- a/lshell/variables.py +++ b/lshell/variables.py @@ -3,7 +3,7 @@ import sys import os -__version__ = "0.12.0rc1" +__version__ = "0.12.0" # Required config variable list per user required_config = ["allowed", "forbidden", "warning_counter"] @@ -96,6 +96,7 @@ "scp_upload=", "scp_download=", "sftp=", + "sftp_unsafe_legacy=", "overssh=", "strict=", "scpforce=", @@ -141,8 +142,36 @@ "LD_AUDIT", "NIS_PATH", "PATH", + "BASHOPTS", + "CDPATH", + "GLOBIGNORE", + "IFS", + "PROMPT_COMMAND", + "PS4", + "PYTHONPATH", + "SHELLOPTS", ) +EXEC_ENV_REMOVE = tuple(item for item in FORBIDDEN_ENVIRON if item != "PATH") + ( + "BASH_ENV", + "ENV", + "LSHELL_ARGS", +) + + +def is_forbidden_environment_key(name): + """Return True when a variable name must not be user-controlled.""" + return bool(name) and ( + name in FORBIDDEN_ENVIRON or name.startswith("BASH_FUNC_") + ) + + +def should_strip_from_exec_env(name): + """Return True when a variable must not reach a child shell.""" + return bool(name) and ( + name in EXEC_ENV_REMOVE or name.startswith("BASH_FUNC_") + ) + # Single source of truth for trusted SFTP protocol executables. TRUSTED_SFTP_PROTOCOL_BINARIES = ( "sftp-server", diff --git a/man/lshell.1 b/man/lshell.1 index 1cf624b..c1cd87b 100644 --- a/man/lshell.1 +++ b/man/lshell.1 @@ -434,6 +434,16 @@ allow or forbid the use of sftp connection - set to 1 or 0. WARNING: This option will not work if you are using OpenSSH's \ internal-sftp service (e.g. when configured in chroot) + +WARNING: lshell path restrictions do not inspect per-file requests once the \ +SFTP protocol is handed to sftp-server. Use sshd_config controls such as \ +ChrootDirectory, ForceCommand internal-sftp, DisableForwarding, or read-only \ +SFTP modes to enforce the real filesystem boundary. +.TP +.I sftp_unsafe_legacy +explicit override for legacy sftp-server passthrough. Even when \ +\fBsftp=1\fR, lshell refuses legacy passthrough by default unless this value \ +is set to 1. Prefer SSH-side \fBinternal-sftp\fR plus \fBChrootDirectory\fR. .TP .I sudo_commands a list of the allowed commands that can be used with sudo(8). If set to \ @@ -621,7 +631,8 @@ strict : 1 scp : 0 scp_upload : 0 scp_download : 0 -sftp : 1 +sftp : 0 +sftp_unsafe_legacy : 0 overssh : [] [user:alice] @@ -700,7 +711,8 @@ timer : 0 path : ['/etc', '/usr'] env_path : ':/sbin:/usr/bin/' scp : 1 # or 0 -sftp : 1 # or 0 +sftp : 0 # keep 1 only with explicit legacy override +sftp_unsafe_legacy : 0 overssh : ['rsync','ls'] aliases : {'ls':'ls \-\-color=auto','ll':'ls \-l'} diff --git a/rpm/lshell.spec b/rpm/lshell.spec index bcad6b6..5a90899 100644 --- a/rpm/lshell.spec +++ b/rpm/lshell.spec @@ -1,5 +1,5 @@ Name: lshell -Version: 0.11.0 +Version: 0.12.0 Release: 1%{?dist} Summary: Limited shell implementation in Python diff --git a/test/test_builtins.py b/test/test_builtins.py index 9f4d5c8..1094796 100644 --- a/test/test_builtins.py +++ b/test/test_builtins.py @@ -1,6 +1,7 @@ """Functional tests for lshell built-in commands""" import os +import tempfile import unittest import subprocess from getpass import getuser @@ -160,47 +161,61 @@ def test_source_nonexistent_file(self): def test_source_valid_file(self): """F69 | Test sourcing a valid environment file sets variables""" - # Start lshell and source the environment file - child = pexpect.spawn(f"{LSHELL} --config {CONFIG} --allowed \"+['source']\"") - child.expect(PROMPT) - - # Source the file and check if the variable is set - child.sendline(f"source {SOURCE_FIXTURE}") - child.expect(PROMPT) - child.sendline("echo $SOURCE_DOUBLE_QUOTED") - child.expect(PROMPT) - - output = child.before.decode("utf-8").split("\n")[1].strip() - expected_output = "hello world" - - assert ( - output == expected_output - ), f"Expected '{expected_output}', got '{output}'" - - # Clean up and end session - self.do_exit(child) + with tempfile.NamedTemporaryFile( + "w", delete=False, dir=os.path.expanduser("~") + ) as env_file: + env_file.write('export SOURCE_DOUBLE_QUOTED="hello world"\n') + env_path = env_file.name + + try: + child = pexpect.spawn( + f"{LSHELL} --config {CONFIG} --allowed \"+['source']\"" + ) + child.expect(PROMPT) + + child.sendline(f"source {env_path}") + child.expect(PROMPT) + child.sendline("echo $SOURCE_DOUBLE_QUOTED") + child.expect(PROMPT) + + output = child.before.decode("utf-8").split("\n")[1].strip() + expected_output = "hello world" + + assert ( + output == expected_output + ), f"Expected '{expected_output}', got '{output}'" + self.do_exit(child) + finally: + os.remove(env_path) def test_source_overwrite_variable(self): """F70 | Test sourcing a file overwrites existing environment variables""" - # Start lshell, set initial variable, and source file to overwrite it - child = pexpect.spawn(f"{LSHELL} --config {CONFIG} --allowed \"+['source']\"") - child.expect(PROMPT) - - # Set initial variable and source the file - child.sendline("export SOURCE_SIMPLE='initial_value'") - child.expect(PROMPT) - child.sendline(f"source {SOURCE_FIXTURE}") - child.expect(PROMPT) - child.sendline("echo $SOURCE_SIMPLE") - child.expect(PROMPT) - - output = child.before.decode("utf-8").split("\n")[1].strip() - expected_output = "value" - - assert ( - output == expected_output - ), f"Expected '{expected_output}', got '{output}'" - - # Clean up and end session - self.do_exit(child) + with tempfile.NamedTemporaryFile( + "w", delete=False, dir=os.path.expanduser("~") + ) as env_file: + env_file.write("export SOURCE_SIMPLE=value\n") + env_path = env_file.name + + try: + child = pexpect.spawn( + f"{LSHELL} --config {CONFIG} --allowed \"+['source','export']\"" + ) + child.expect(PROMPT) + + child.sendline("export SOURCE_SIMPLE='initial_value'") + child.expect(PROMPT) + child.sendline(f"source {env_path}") + child.expect(PROMPT) + child.sendline("echo $SOURCE_SIMPLE") + child.expect(PROMPT) + + output = child.before.decode("utf-8").split("\n")[1].strip() + expected_output = "value" + + assert ( + output == expected_output + ), f"Expected '{expected_output}', got '{output}'" + self.do_exit(child) + finally: + os.remove(env_path) diff --git a/test/test_cli_unit.py b/test/test_cli_unit.py index 969cda2..9d5c3e9 100644 --- a/test/test_cli_unit.py +++ b/test/test_cli_unit.py @@ -48,10 +48,12 @@ def returnconf(self): return captured["args"] - def test_main_appends_valid_lshell_args_from_env(self): - """Append safely parsed list arguments from LSHELL_ARGS env var.""" - args = self._run_main_and_capture_args("['--config', '/tmp/lshell.conf']") - self.assertEqual(args, ["--quiet=1", "--config", "/tmp/lshell.conf"]) + def test_main_ignores_lshell_args_env_even_when_well_formed(self): + """Startup should ignore LSHELL_ARGS entirely.""" + args = self._run_main_and_capture_args( + "['--config', '/tmp/lshell.conf', '--allowed=all', '--log', '/tmp/lshell.log']" + ) + self.assertEqual(args, ["--quiet=1"]) def test_main_ignores_invalid_or_unsafe_lshell_args_env(self): """Ignore malformed, non-sequence, or non-string entries in LSHELL_ARGS.""" @@ -59,6 +61,7 @@ def test_main_ignores_invalid_or_unsafe_lshell_args_env(self): "__import__('os').system('id')", "'--config'", "['--config', 123]", + "['--allowed=all']", ] for value in invalid_values: with self.subTest(value=value): diff --git a/test/test_command_execution.py b/test/test_command_execution.py index 42c6188..e0db7db 100644 --- a/test/test_command_execution.py +++ b/test/test_command_execution.py @@ -2,6 +2,7 @@ import os import random +import re import unittest from getpass import getuser import pexpect @@ -17,9 +18,26 @@ class TestFunctions(unittest.TestCase): """Functional tests for lshell""" + @staticmethod + def _normalize_ls_error_prefix(text): + """Collapse pinned absolute ls paths back to the user-facing command name.""" + return re.sub(r"^/\S*/ls:", "ls:", text) + + def _clean_env(self, extra=None): + """Return a sanitized environment for deterministic child shells.""" + env = os.environ.copy() + env.pop("LSHELL_ARGS", None) + env.pop("LPS1", None) + if extra: + env.update(extra) + return env + def setUp(self): """spawn lshell with pexpect and return the child""" - self.child = pexpect.spawn(f"{LSHELL} --config {CONFIG} --strict 1") + self.child = pexpect.spawn( + f"{LSHELL} --config {CONFIG} --strict 1", + env=self._clean_env(), + ) self.child.expect(PROMPT) def tearDown(self): @@ -48,7 +66,10 @@ def test_external_echo_command_string(self): def test_exitcode_with_separator_external_cmd(self): """F16(a) | external command exit codes with separator""" - child = pexpect.spawn(f"{LSHELL} " f"--config {CONFIG} " '--forbidden "[]"') + child = pexpect.spawn( + f"{LSHELL} " f"--config {CONFIG} " '--forbidden "[]"', + env=self._clean_env(), + ) child.expect(PROMPT) expected_1 = ( @@ -63,14 +84,17 @@ def test_exitcode_with_separator_external_cmd(self): result_1 = result[1].strip() result_2 = result[2].strip() result_3 = result[3].strip() - self.assertEqual(expected_1, result_1) + self.assertEqual(expected_1, self._normalize_ls_error_prefix(result_1)) self.assertEqual(expected_2, result_2) self.assertEqual(expected_3, result_3) self.do_exit(child) def test_exitcode_with_separator_external_cmd_b(self): """F16(b) | external command exit codes with separator""" - child = pexpect.spawn(f"{LSHELL} " f"--config {CONFIG} " '--forbidden "[]"') + child = pexpect.spawn( + f"{LSHELL} " f"--config {CONFIG} " '--forbidden "[]"', + env=self._clean_env(), + ) child.expect(PROMPT) expected_1 = ( @@ -83,13 +107,16 @@ def test_exitcode_with_separator_external_cmd_b(self): result = child.before.decode("utf8").split("\n") result_1 = result[1].strip() result_2 = result[2].strip() - self.assertEqual(expected_1, result_1) + self.assertEqual(expected_1, self._normalize_ls_error_prefix(result_1)) self.assertEqual(expected_2, result_2) self.do_exit(child) def test_exitcode_without_separator_external_cmd(self): """F17 | external command exit codes without separator""" - child = pexpect.spawn(f"{LSHELL} " f"--config {CONFIG} " '--forbidden "[]"') + child = pexpect.spawn( + f"{LSHELL} " f"--config {CONFIG} " '--forbidden "[]"', + env=self._clean_env(), + ) child.expect(PROMPT) expected = "2" @@ -103,7 +130,10 @@ def test_exitcode_without_separator_external_cmd(self): def test_cd_and_command(self): """F24 | cd && command should not be interpreted by internal function""" - child = pexpect.spawn(f"{LSHELL} " f"--config {CONFIG} --forbidden \"-['&']\"") + child = pexpect.spawn( + f"{LSHELL} " f"--config {CONFIG} --forbidden \"-['&']\"", + env=self._clean_env(), + ) child.expect(PROMPT) expected = "OK" @@ -115,7 +145,7 @@ def test_cd_and_command(self): def test_ls_non_existing_directory_and_echo(self): """Test: ls non_existing_directory && echo nothing""" - child = pexpect.spawn(f"{LSHELL} --config {CONFIG}") + child = pexpect.spawn(f"{LSHELL} --config {CONFIG}", env=self._clean_env()) child.expect(PROMPT) child.sendline("ls non_existing_directory && echo nothing") @@ -128,7 +158,10 @@ def test_ls_non_existing_directory_and_echo(self): def test_ls_and_echo_ok(self): """Test: ls && echo OK""" - child = pexpect.spawn(f"{LSHELL} --config {CONFIG} --forbidden \"-['&']\"") + child = pexpect.spawn( + f"{LSHELL} --config {CONFIG} --forbidden \"-['&']\"", + env=self._clean_env(), + ) child.expect(PROMPT) child.sendline("ls && echo OK") @@ -141,7 +174,10 @@ def test_ls_and_echo_ok(self): def test_ls_non_existing_directory_or_echo_ok(self): """Test: ls non_existing_directory || echo OK""" - child = pexpect.spawn(f"{LSHELL} --config {CONFIG} --forbidden \"-['|']\"") + child = pexpect.spawn( + f"{LSHELL} --config {CONFIG} --forbidden \"-['|']\"", + env=self._clean_env(), + ) child.expect(PROMPT) child.sendline("ls non_existing_directory || echo OK") @@ -154,7 +190,7 @@ def test_ls_non_existing_directory_or_echo_ok(self): def test_ls_or_echo_nothing(self): """Test: ls || echo nothing""" - child = pexpect.spawn(f"{LSHELL} --config {CONFIG}") + child = pexpect.spawn(f"{LSHELL} --config {CONFIG}", env=self._clean_env()) child.expect(PROMPT) child.sendline("ls || echo nothing") @@ -168,7 +204,8 @@ def test_ls_or_echo_nothing(self): def test_multicmd_with_wrong_arg_should_fail(self): """F20 | Allowing 'echo asd': Test 'echo qwe' should fail""" child = pexpect.spawn( - f"{LSHELL} " f"--config {CONFIG} " "--allowed \"['echo asd']\"" + f"{LSHELL} " f"--config {CONFIG} " "--allowed \"['echo asd']\"", + env=self._clean_env(), ) child.expect(PROMPT) @@ -183,7 +220,8 @@ def test_multicmd_with_wrong_arg_should_fail(self): def test_multicmd_with_near_exact_arg_should_fail(self): """F41 | Allowing 'echo asd': Test 'echo asds' should fail""" child = pexpect.spawn( - f"{LSHELL} " f"--config {CONFIG} " "--allowed \"['echo asd']\"" + f"{LSHELL} " f"--config {CONFIG} " "--allowed \"['echo asd']\"", + env=self._clean_env(), ) child.expect(PROMPT) @@ -198,7 +236,8 @@ def test_multicmd_with_near_exact_arg_should_fail(self): def test_multicmd_without_arg_should_fail(self): """F42 | Allowing 'echo asd': Test 'echo' should fail""" child = pexpect.spawn( - f"{LSHELL} " f"--config {CONFIG} " "--allowed \"['echo asd']\"" + f"{LSHELL} " f"--config {CONFIG} " "--allowed \"['echo asd']\"", + env=self._clean_env(), ) child.expect(PROMPT) @@ -214,7 +253,8 @@ def test_multicmd_asd_should_pass(self): """F43 | Allowing 'echo asd': Test 'echo asd' should pass""" child = pexpect.spawn( - f"{LSHELL} " f"--config {CONFIG} " "--allowed \"['echo asd']\"" + f"{LSHELL} " f"--config {CONFIG} " "--allowed \"['echo asd']\"", + env=self._clean_env(), ) child.expect(PROMPT) @@ -230,7 +270,8 @@ def test_pipeline_is_shell_compatible(self): """F45 | Pipeline should pass stdout between commands.""" child = pexpect.spawn( f"{LSHELL} --config {CONFIG} " - "--forbidden \"-['|']\" --allowed \"+['printf', 'wc']\"" + "--forbidden \"-['|']\" --allowed \"+['printf', 'wc']\"", + env=self._clean_env(), ) child.expect(PROMPT) @@ -244,7 +285,8 @@ def test_redirection_is_shell_compatible(self): """F46 | Redirections should be handled by shell semantics.""" child = pexpect.spawn( f"{LSHELL} --config {CONFIG} --path \"['/tmp']\" " - "--forbidden \"-['>','<','&']\" --allowed \"+['cat']\"" + "--forbidden \"-['>','<','&']\" --allowed \"+['cat']\"", + env=self._clean_env(), ) child.expect(PROMPT) @@ -263,7 +305,8 @@ def test_allowed_missing_binary_uses_lshell_error(self): child = pexpect.spawn( f"{LSHELL} --config {CONFIG} " f"--allowed \"+['{missing_cmd}']\" " - '--forbidden "[]"' + '--forbidden "[]"', + env=self._clean_env(), ) child.expect(PROMPT) @@ -282,7 +325,8 @@ def test_operator_matrix_fuzz(self): f"{LSHELL} --config {CONFIG} --strict 1 " "--path \"['/tmp']\" " '--forbidden "[]" ' - "--allowed \"+['printf','wc','cat','pwd','true','false']\"" + "--allowed \"+['printf','wc','cat','pwd','true','false']\"", + env=self._clean_env(), ) temp_file = f"/tmp/lshell_matrix_{os.getpid()}.txt" @@ -325,7 +369,10 @@ def expect_clean_prompt(): def test_multiline_and_interrupt_storm(self): """F70 | Repeated multiline and Ctrl-C should recover cleanly.""" - child = pexpect.spawn(f'{LSHELL} --config {CONFIG} --strict 1 --forbidden "[]"') + child = pexpect.spawn( + f'{LSHELL} --config {CONFIG} --strict 1 --forbidden "[]"', + env=self._clean_env(), + ) def expect_clean_prompt(): child.expect(PROMPT) @@ -365,7 +412,8 @@ def test_history_randomized_session_consistency(self): child = pexpect.spawn( f"{LSHELL} --config {CONFIG} --strict 1 " '--forbidden "[]" ' - "--allowed \"+['printf','wc','pwd','true','false']\"" + "--allowed \"+['printf','wc','pwd','true','false']\"", + env=self._clean_env(), ) rng = random.Random(7101) executed_commands = [] @@ -407,7 +455,8 @@ def test_background_job_lifecycle(self): child = pexpect.spawn( f"{LSHELL} --config {CONFIG} --strict 1 " '--forbidden "[]" ' - "--allowed \"+['sleep']\"" + "--allowed \"+['sleep']\"", + env=self._clean_env(), ) def expect_clean_prompt(): diff --git a/test/test_completion.py b/test/test_completion.py index 57b4009..df7c64b 100644 --- a/test/test_completion.py +++ b/test/test_completion.py @@ -1,26 +1,44 @@ """Functional tests for lshell completion""" import os +import shutil +import tempfile import unittest -import subprocess from getpass import getuser import pexpect # pylint: disable=wrong-import-order +from lshell import completion +from lshell.config.runtime import CheckConfig + TOPDIR = os.path.abspath(os.path.join(os.path.dirname(__file__), "..")) CONFIG = f"{TOPDIR}/test/testfiles/test.conf" LSHELL = f"{TOPDIR}/bin/lshell" USER = getuser() PROMPT = f"{USER}:~\\$" +PROMPT_ANY_DIR = f"{USER}:.+\\$" class TestFunctions(unittest.TestCase): """Functional tests for lshell""" + def _clean_env(self, extra=None): + """Return a sanitized environment for deterministic child shells.""" + env = os.environ.copy() + env.pop("LSHELL_ARGS", None) + env.pop("LPS1", None) + if extra: + env.update(extra) + return env + def setUp(self): """spawn lshell with pexpect and return the child""" - self.child = pexpect.spawn(f"{LSHELL} --config {CONFIG} --strict 1") + self.child = pexpect.spawn( + f"{LSHELL} --config {CONFIG} --strict 1", + env=self._clean_env(), + ) self.child.expect(PROMPT) + self.child.setwinsize(2000, 200) def tearDown(self): self.child.close() @@ -30,11 +48,34 @@ def do_exit(self, child): child.sendline("exit") child.expect(pexpect.EOF) + def _make_home_completion_fixture(self, fixture_name): + """Create an isolated directory under the user's home for completion tests.""" + home_dir = os.path.expanduser("~") + root_dir = tempfile.mkdtemp(prefix=f"lshell-{fixture_name}-", dir=home_dir) + dir1_name = "alpha-dir" + dir2_name = "beta-dir" + file1_name = "alpha-file" + file2_name = "beta-file" + os.mkdir(os.path.join(root_dir, dir1_name)) + os.mkdir(os.path.join(root_dir, dir2_name)) + open(os.path.join(root_dir, file1_name), "w", encoding="utf-8").close() + open(os.path.join(root_dir, file2_name), "w", encoding="utf-8").close() + return { + "root_dir": root_dir, + "root_name": os.path.basename(root_dir), + "dirs": {f"{dir1_name}/", f"{dir2_name}/"}, + "display_entries": { + f"{dir1_name}/", + f"{dir2_name}/", + f"{file1_name} ", + f"{file2_name} ", + }, + } + def test_cmd_completion_tab_tab(self): """F15 | command completion: tab to list commands""" - self.child.sendline("\t\t") - self.child.expect(PROMPT) - result = self.child.before.decode("utf8").strip() + conf = CheckConfig([f"--config={CONFIG}", "--quiet=1"]).returnconf() + result = completion.completenames(conf, "", "") for command in [ "bg", @@ -46,159 +87,79 @@ def test_cmd_completion_tab_tab(self): "history", "jobs", "lshow", - "source", ]: self.assertIn(command, result) def test_path_completion_tilda(self): """F14 | path completion with ~/""" - # Create two random directories in the home directory - home_dir = f"/home/{USER}" - test_num = 14 - dir1 = f"{home_dir}/test_{test_num}_dir_1" - dir2 = f"{home_dir}/test_{test_num}_dir_2" - file1 = f"{home_dir}/test_{test_num}_file_1" - file2 = f"{home_dir}/test_{test_num}_file_2" - os.mkdir(dir1) - os.mkdir(dir2) - open(file1, "w").close() - open(file2, "w").close() - - # test dir list - command = "find . -maxdepth 1 -type d -printf '%f/\n'" - p_dir_list = subprocess.Popen( - command, - shell=True, - stdin=subprocess.PIPE, - stdout=subprocess.PIPE, - ) - stdout_p_dir_list = p_dir_list.stdout - expected = stdout_p_dir_list.read().decode("utf8").strip().split() - # Normalize expected to relative paths - expected = set(expected) - expected = set(expected) - expected.discard("./") - - self.child.sendline("cd ~/\t\t") - self.child.expect(PROMPT) - output = ( - self.child.before.decode("utf8").strip().split("\n", 1)[1].strip().split() - ) - output = set(output) - # github action hackish-fix... - output.discard(".ghcup/") - - self.assertEqual(expected, output) - - # cleanup - os.rmdir(dir1) - os.rmdir(dir2) - os.remove(file1) - os.remove(file2) + fixture = self._make_home_completion_fixture("completion-dir") + try: + conf = CheckConfig([f"--config={CONFIG}", "--quiet=1"]).returnconf() + output = set( + completion.complete_change_dir( + conf, + "", + f"cd ~/{fixture['root_name']}/", + 0, + len(f"cd ~/{fixture['root_name']}/"), + ) + ) + for directory in fixture["dirs"]: + self.assertIn(directory, output) + finally: + shutil.rmtree(fixture["root_dir"]) def test_file_completion_tilda(self): """F15 | file completion ls with ~/""" - # Create two random directories in the home directory - home_dir = f"/home/{USER}" - test_num = 15 - dir1 = f"{home_dir}/test_{test_num}_dir_1" - dir2 = f"{home_dir}/test_{test_num}_dir_2" - file1 = f"{home_dir}/test_{test_num}_file_1" - file2 = f"{home_dir}/test_{test_num}_file_2" - os.mkdir(dir1) - os.mkdir(dir2) - open(file1, "w").close() - open(file2, "w").close() - - # test file list - command = "find . -maxdepth 1 -printf '%P%y\n' | sed 's|d$|/|;s|f$||'" - p_file_list = subprocess.Popen( - command, - shell=True, - stdin=subprocess.PIPE, - stdout=subprocess.PIPE, - ) - stdout_p_file_list = p_file_list.stdout - expected = stdout_p_file_list.read().decode("utf8").strip().split() - expected = set(expected) - expected.discard("/") - - self.child.sendline("ls ~/\t\t") - self.child.expect(PROMPT) - output = ( - self.child.before.decode("utf8").strip().split("\n", 1)[1].strip().split() - ) - output = set(output) - # github action hackish-fix... - output.discard(".ghcup/") - if ".ghcupl" in expected: - output.add(".ghcupl") - - self.assertEqual(expected, output) - - # cleanup - os.rmdir(dir1) - os.rmdir(dir2) - os.remove(file1) - os.remove(file2) + fixture = self._make_home_completion_fixture("completion-file") + try: + conf = CheckConfig([f"--config={CONFIG}", "--quiet=1"]).returnconf() + output = set( + completion.complete_list_dir( + conf, + "", + f"ls ~/{fixture['root_name']}/", + 0, + len(f"ls ~/{fixture['root_name']}/"), + ) + ) + for entry in fixture["display_entries"]: + self.assertIn(entry, output) + finally: + shutil.rmtree(fixture["root_dir"]) def test_file_completion_with_arg(self): """F15 | file completion ls with ~/""" - # Create two random directories in the home directory - home_dir = f"/home/{USER}" - test_num = 16 - dir1 = f"{home_dir}/test_{test_num}_dir_1" - dir2 = f"{home_dir}/test_{test_num}_dir_2" - file1 = f"{home_dir}/test_{test_num}_file_1" - file2 = f"{home_dir}/test_{test_num}_file_2" - os.mkdir(dir1) - os.mkdir(dir2) - open(file1, "w").close() - open(file2, "w").close() - - # test file list - command = "find . -maxdepth 1 -printf '%P%y\n' | sed 's|d$|/|;s|f$||'" - p_file_list = subprocess.Popen( - command, - shell=True, - stdin=subprocess.PIPE, - stdout=subprocess.PIPE, - ) - stdout_p_file_list = p_file_list.stdout - expected = stdout_p_file_list.read().decode("utf8").strip().split() - expected = set(expected) - expected.discard("/") - - self.child.sendline("ls -l ~/\t\t") - self.child.expect(PROMPT) - output = ( - self.child.before.decode("utf8").strip().split("\n", 1)[1].strip().split() - ) - output = set(output) - # github action hackish-fix... - output.discard(".ghcup/") - if ".ghcupl" in expected: - output.add(".ghcupl") - - self.assertEqual(expected, output) - - # cleanup - os.rmdir(dir1) - os.rmdir(dir2) - os.remove(file1) - os.remove(file2) + fixture = self._make_home_completion_fixture("completion-arg") + try: + conf = CheckConfig([f"--config={CONFIG}", "--quiet=1"]).returnconf() + output = set( + completion.complete_list_dir( + conf, + "", + f"ls -l ~/{fixture['root_name']}/", + 0, + len(f"ls -l ~/{fixture['root_name']}/"), + ) + ) + for entry in fixture["display_entries"]: + self.assertIn(entry, output) + finally: + shutil.rmtree(fixture["root_dir"]) def test_cmd_completion_dot_slash(self): """F26 | command completion: tab to list ./foo1 ./foo2""" child = pexpect.spawn( - f"{LSHELL} " f"--config {CONFIG} " "--allowed \"+ ['./foo1', './foo2']\"" + f"{LSHELL} " f"--config {CONFIG} " "--allowed \"+ ['./foo1', './foo2']\"", + env=self._clean_env(), ) child.expect(PROMPT) - expected = "./\x07foo\x07\r\nfoo1 foo2" child.sendline("./\t\t\t") child.expect(PROMPT) result = child.before.decode("utf8").strip() - self.assertEqual(expected, result) + self.assertIn("Allowed commands", result) + self.assertIn("foo1", result) + self.assertIn("foo2", result) self.do_exit(child) diff --git a/test/test_engine_security_regressions_unit.py b/test/test_engine_security_regressions_unit.py index a4a51cd..d0ea683 100644 --- a/test/test_engine_security_regressions_unit.py +++ b/test/test_engine_security_regressions_unit.py @@ -1,6 +1,7 @@ """Security regression coverage for the canonical engine.""" import os +import tempfile import unittest from lshell.config.runtime import CheckConfig @@ -106,6 +107,64 @@ def test_path_specific_allow_beats_broader_deny(self): self.assertFalse(denied_decision.allowed) self.assertEqual(denied_decision.reason.code, reasons.FORBIDDEN_PATH) + def test_authorizer_blocks_bareword_symlink_operand_for_cat(self): + """Canonical authorizer must deny bareword symlinks escaping allowed roots.""" + previous_cwd = os.getcwd() + try: + with tempfile.TemporaryDirectory(prefix="lshell-engine-symlink-", dir="/tmp") as tmpdir: + link_path = os.path.join(tmpdir, "passwdlink") + os.symlink("/etc/passwd", link_path) + os.chdir(tmpdir) + + decision = authorizer.authorize_line( + "cat passwdlink", + self._policy( + allowed=["cat"], + strict=1, + path=[f"{tmpdir}|", ""], + ), + mode="policy", + check_current_dir=False, + ) + + self.assertFalse(decision.allowed) + self.assertEqual(decision.reason.code, reasons.FORBIDDEN_PATH) + finally: + os.chdir(previous_cwd) + + def test_authorizer_blocks_bareword_symlink_operand_for_text_filters(self): + """Canonical authorizer must deny filter file operands escaping allowed roots.""" + previous_cwd = os.getcwd() + try: + with tempfile.TemporaryDirectory(prefix="lshell-engine-filter-", dir="/tmp") as tmpdir: + link_path = os.path.join(tmpdir, "passwdlink") + os.symlink("/etc/passwd", link_path) + os.chdir(tmpdir) + + for command in ( + "awk 1 passwdlink", + "awk -f passwdlink", + "sed -n 1p passwdlink", + "sed -f passwdlink", + "sort passwdlink", + ): + with self.subTest(command=command): + decision = authorizer.authorize_line( + command, + self._policy( + allowed=["awk", "sed", "sort"], + strict=1, + path=[f"{tmpdir}|", ""], + ), + mode="policy", + check_current_dir=False, + ) + + self.assertFalse(decision.allowed) + self.assertEqual(decision.reason.code, reasons.FORBIDDEN_PATH) + finally: + os.chdir(previous_cwd) + def test_runtime_blocks_forbidden_env_assignment(self): """Runtime should keep forbidden env-assignment protection.""" conf = CheckConfig(self.args + ["--forbidden=[]", "--strict=0"]).returnconf() diff --git a/test/test_extension_parser_unit.py b/test/test_extension_parser_unit.py index 86724f5..60a9c1e 100644 --- a/test/test_extension_parser_unit.py +++ b/test/test_extension_parser_unit.py @@ -105,11 +105,12 @@ def test_cmd_parse_execute_treats_ls_as_builtin(self, mock_exec_cmd): shell_context = DummyShellContext(conf) retcode = utils.cmd_parse_execute("ls /tmp", shell_context=shell_context) self.assertEqual(retcode, 0) - mock_exec_cmd.assert_called_once_with( - "ls /tmp", - conf=ANY, - log=ANY, - ) + mock_exec_cmd.assert_called_once() + called_command = mock_exec_cmd.call_args.args[0] + self.assertEqual(os.path.basename(called_command.split()[0]), "ls") + self.assertTrue(called_command.endswith(" /tmp")) + self.assertIs(mock_exec_cmd.call_args.kwargs["conf"], conf) + self.assertIs(mock_exec_cmd.call_args.kwargs["log"], shell_context.log) if __name__ == "__main__": diff --git a/test/test_file_extension.py b/test/test_file_extension.py index 9dfab95..4479146 100644 --- a/test/test_file_extension.py +++ b/test/test_file_extension.py @@ -10,6 +10,7 @@ TOPDIR = os.path.abspath(os.path.join(os.path.dirname(__file__), "..")) CONFIG = f"{TOPDIR}/test/testfiles/test.conf" LSHELL = f"{TOPDIR}/bin/lshell" +TESTFILES_PATH_ARG = f'--path "[\'{TOPDIR}/test/testfiles\']"' USER = getuser() PROMPT = f"{USER}:~\\$" @@ -32,7 +33,8 @@ def test_allowed_extension_success(self): child = pexpect.spawn( f"{LSHELL} --config {CONFIG} " "--allowed \"+ ['cat']\" " - "--allowed_file_extensions \"['.log']\"" + "--allowed_file_extensions \"['.log']\" " + f"{TESTFILES_PATH_ARG}" ) child.expect(PROMPT) @@ -70,7 +72,8 @@ def test_allowed_extension_empty(self): child = pexpect.spawn( f"{LSHELL} --config {CONFIG} " "--allowed \"+ ['cat']\" " - '--allowed_file_extensions "[]"' + '--allowed_file_extensions "[]" ' + f"{TESTFILES_PATH_ARG}" ) child.expect(PROMPT) @@ -113,7 +116,8 @@ def test_allowed_file_extensions_plus_minus_chain(self): f"{LSHELL} --config {CONFIG} " "--allowed \"+ ['cat']\" " "--allowed_file_extensions \"['.log'] + ['.conf'] - ['.log']\" " - "--warning_counter 2 --strict 1" + "--warning_counter 2 --strict 1 " + f"{TESTFILES_PATH_ARG}" ) child.expect(PROMPT) diff --git a/test/test_hardeninit_functional.py b/test/test_hardeninit_functional.py index c9906d5..912b463 100644 --- a/test/test_hardeninit_functional.py +++ b/test/test_hardeninit_functional.py @@ -27,7 +27,8 @@ def test_harden_init_writes_config_file(self): with open(output_path, "r", encoding="utf-8") as handle: rendered = handle.read() self.assertIn("[default]", rendered) - self.assertIn("sftp : 1", rendered) + self.assertIn("sftp : 0", rendered) + self.assertIn("sftp_unsafe_legacy : 0", rendered) self.assertIn("strict : 1", rendered) def test_harden_init_writes_scoped_group_and_user_sections(self): diff --git a/test/test_hardeninit_unit.py b/test/test_hardeninit_unit.py index c3d5dc3..80352be 100644 --- a/test/test_hardeninit_unit.py +++ b/test/test_hardeninit_unit.py @@ -71,7 +71,8 @@ def test_main_stdout_flag(self): self.assertEqual(code, 0) rendered = stdout.getvalue() self.assertIn("[default]", rendered) - self.assertIn("sftp : 1", rendered) + self.assertIn("sftp : 0", rendered) + self.assertIn("sftp_unsafe_legacy : 0", rendered) def test_main_stdout_group_and_user_flags_render_scoped_sections(self): """--group/--user render [grp:*]/[user:*] sections and skip [default].""" diff --git a/test/test_history_size_unit.py b/test/test_history_size_unit.py index 2d55276..06cf86b 100644 --- a/test/test_history_size_unit.py +++ b/test/test_history_size_unit.py @@ -3,10 +3,16 @@ import io import os import unittest -from unittest.mock import mock_open, patch +from unittest.mock import ANY, call, mock_open, patch +from lshell import history as history_utils from lshell.config.runtime import CheckConfig -from lshell.shellcmd import ShellCmd +from lshell.shellcmd import ( + READLINE_HISTORY_SEARCH_BINDINGS, + READLINE_INCREMENTAL_SEARCH_BINDINGS, + ShellCmd, + _readline_char_point, +) TOPDIR = f"{os.path.dirname(os.path.realpath(__file__))}/../" CONFIG = f"{TOPDIR}/test/testfiles/test.conf" @@ -106,6 +112,207 @@ def test_cmdloop_applies_history_size_when_history_file_missing(self): self.assertEqual(mock_read.call_count, 2) mock_len.assert_called_once_with(11) + def test_cmdloop_does_not_normalize_history_when_input_returns_eof(self): + """Ctrl-D/EOF should not rewrite the latest history entry as literal EOF.""" + conf = CheckConfig(self.args + ["--strict=0"]).returnconf() + shell = ShellCmd( + conf, + args=[], + stdin=io.StringIO(), + stdout=io.StringIO(), + stderr=io.StringIO(), + ) + + with patch("lshell.shellcmd.readline.read_history_file"): + with patch("lshell.shellcmd.readline.set_history_length"): + with patch( + "lshell.shellcmd.readline.get_completer_delims", + return_value=" \t\n", + ): + with patch("lshell.shellcmd.readline.set_completer_delims"): + with patch("lshell.shellcmd.readline.get_completer", return_value=None): + with patch("lshell.shellcmd.readline.set_completer"): + with patch("lshell.shellcmd.readline.parse_and_bind"): + with patch("lshell.shellcmd.readline.write_history_file"): + with patch( + "lshell.shellcmd.readline.set_completion_display_matches_hook" + ): + with patch( + "lshell.shellcmd.history_utils.prepare_latest_history_entry" + ) as mock_prepare: + with patch( + "builtins.input", + side_effect=EOFError, + ): + with patch( + "lshell.shellcmd.sys.exit", + side_effect=SystemExit, + ): + with self.assertRaises(SystemExit): + shell.cmdloop() + + mock_prepare.assert_not_called() + + def test_cmdloop_does_not_normalize_history_for_cmdqueue_entries(self): + """Queued commands should not be treated as freshly entered readline history.""" + conf = CheckConfig(self.args + ["--strict=0"]).returnconf() + shell = ShellCmd( + conf, + args=[], + stdin=io.StringIO(), + stdout=io.StringIO(), + stderr=io.StringIO(), + ) + shell.cmdqueue = ["exit"] + + with patch("lshell.shellcmd.readline.read_history_file"): + with patch("lshell.shellcmd.readline.set_history_length"): + with patch( + "lshell.shellcmd.readline.get_completer_delims", + return_value=" \t\n", + ): + with patch("lshell.shellcmd.readline.set_completer_delims"): + with patch("lshell.shellcmd.readline.get_completer", return_value=None): + with patch("lshell.shellcmd.readline.set_completer"): + with patch("lshell.shellcmd.readline.parse_and_bind"): + with patch("lshell.shellcmd.readline.write_history_file"): + with patch( + "lshell.shellcmd.readline.set_completion_display_matches_hook" + ): + with patch( + "lshell.shellcmd.history_utils.prepare_latest_history_entry" + ) as mock_prepare: + with patch( + "lshell.shellcmd.sys.exit", + side_effect=SystemExit, + ): + with self.assertRaises(SystemExit): + shell.cmdloop() + + mock_prepare.assert_not_called() + + def test_cmdloop_binds_prefix_history_search_to_arrow_keys(self): + """Fallback arrow bindings should be installed when custom hooks are unavailable.""" + conf = CheckConfig(self.args + ["--history_size=7", "--strict=0"]).returnconf() + shell = ShellCmd( + conf, + args=[], + stdin=io.StringIO(), + stdout=io.StringIO(), + stderr=io.StringIO(), + ) + shell.cmdqueue = ["exit"] + + with patch("lshell.shellcmd._bind_custom_history_search", return_value=False) as mock_custom: + with patch("lshell.shellcmd.readline.read_history_file"): + with patch("lshell.shellcmd.readline.set_history_length"): + with patch( + "lshell.shellcmd.readline.get_completer_delims", + return_value=" \t\n", + ): + with patch("lshell.shellcmd.readline.set_completer_delims"): + with patch("lshell.shellcmd.readline.get_completer", return_value=None): + with patch("lshell.shellcmd.readline.set_completer"): + with patch("lshell.shellcmd.readline.parse_and_bind") as mock_bind: + with patch("lshell.shellcmd.readline.write_history_file"): + with patch( + "lshell.shellcmd.readline.set_completion_display_matches_hook" + ) as mock_hook: + with patch( + "lshell.shellcmd.sys.exit", + side_effect=SystemExit, + ): + with self.assertRaises(SystemExit): + shell.cmdloop() + + expected_calls = [call(f"{shell.completekey}: complete")] + expected_calls.extend(call(binding) for binding in READLINE_INCREMENTAL_SEARCH_BINDINGS) + expected_calls.extend(call(binding) for binding in READLINE_HISTORY_SEARCH_BINDINGS) + mock_bind.assert_has_calls(expected_calls) + mock_custom.assert_called_once_with(shell) + mock_hook.assert_any_call(ANY) + mock_hook.assert_any_call(None) + + def test_history_search_skips_duplicate_matches_and_restores_original_line(self): + """Prefix search should deduplicate matches and restore the typed line on final down.""" + conf = CheckConfig(self.args + ["--strict=0"]).returnconf() + shell = ShellCmd( + conf, + args=[], + stdin=io.StringIO(), + stdout=io.StringIO(), + stderr=io.StringIO(), + ) + + history_items = { + 1: "echo alpha", + 2: "echo alpha beta", + 3: "echo alpha", + } + replaced = [] + with patch("lshell.shellcmd.readline.get_current_history_length", return_value=3): + with patch("lshell.shellcmd.readline.get_history_item", side_effect=history_items.get): + with patch("lshell.shellcmd._replace_readline_buffer", side_effect=replaced.append): + with patch("lshell.shellcmd.readline.get_line_buffer", return_value="echo a"): + shell.history_search(True) + with patch( + "lshell.shellcmd.readline.get_line_buffer", + return_value="echo alpha", + ): + shell.history_search(True) + with patch( + "lshell.shellcmd.readline.get_line_buffer", + return_value="echo alpha beta", + ): + shell.history_search(False) + with patch( + "lshell.shellcmd.readline.get_line_buffer", + return_value="echo alpha", + ): + shell.history_search(False) + + self.assertEqual( + replaced, + ["echo alpha", "echo alpha beta", "echo alpha", "echo a"], + ) + self.assertEqual(shell.history_search_state["matches"], []) + self.assertIsNone(shell.history_search_state["index"]) + + def test_readline_char_point_converts_utf8_byte_offset_to_character_index(self): + """Byte-based readline cursor offsets should map safely back to string indices.""" + with patch("lshell.shellcmd._readline_point", return_value=2): + self.assertEqual(_readline_char_point("éx"), 1) + + def test_normalize_history_line_reduces_unquoted_blanks_only(self): + """History normalization should keep quoted spacing while reducing outer blanks.""" + line = ' echo "alpha beta" gamma ' + self.assertEqual( + history_utils.normalize_history_line(line), + 'echo "alpha beta" gamma', + ) + + def test_prepare_latest_history_entry_replaces_and_drops_consecutive_duplicate(self): + """Latest history item should be normalized and removed when it duplicates the prior item.""" + with patch("lshell.history.readline.get_current_history_length", return_value=2): + with patch( + "lshell.history.readline.get_history_item", + side_effect=lambda index: {1: "echo alpha", 2: "echo alpha"}[index], + ): + with patch("lshell.history.readline.replace_history_item") as mock_replace: + with patch("lshell.history.readline.remove_history_item") as mock_remove: + history_utils.prepare_latest_history_entry("echo alpha") + + mock_replace.assert_called_once_with(1, "echo alpha") + mock_remove.assert_called_once_with(1) + + def test_entries_for_persisted_history_reduce_blanks_and_keep_latest_duplicate(self): + """Persisted history should drop older duplicates after normalization.""" + entries = ["echo alpha", "help", "echo alpha", 'echo "x y"'] + self.assertEqual( + history_utils.entries_for_persisted_history(entries), + ["help", "echo alpha", 'echo "x y"'], + ) + if __name__ == "__main__": unittest.main() diff --git a/test/test_messages_unit.py b/test/test_messages_unit.py index 3c15fda..84eb786 100644 --- a/test/test_messages_unit.py +++ b/test/test_messages_unit.py @@ -66,6 +66,20 @@ def test_command_not_found_message(self): "missing: catt", ) + def test_command_path_changed_message(self): + """Render the command-path-drift message from default and custom config.""" + conf = {} + self.assertEqual( + messages.get_message(conf, "command_path_changed", command="ls"), + 'lshell: command path changed since session start: "ls"', + ) + + custom_conf = {"messages": {"command_path_changed": "changed: {command}"}} + self.assertEqual( + messages.get_message(custom_conf, "command_path_changed", command="ls"), + "changed: ls", + ) + def test_forbidden_command_message(self): """Render the forbidden command message from default and custom config.""" conf = {} @@ -267,6 +281,7 @@ def test_validate_messages_config_accepts_all_supported_keys(self): overrides = { "unknown_syntax": "a {command}", "command_not_found": "a {command}", + "command_path_changed": "a {command}", "forbidden_generic": "a {messagetype} {command}", "forbidden_command": "a {command}", "forbidden_path": "a {command}", diff --git a/test/test_parser_jobs_unit.py b/test/test_parser_jobs_unit.py index c39df55..fb0788e 100644 --- a/test/test_parser_jobs_unit.py +++ b/test/test_parser_jobs_unit.py @@ -300,6 +300,18 @@ def test_completenames_dot_slash_with_prefixed_text(self): result = completion.completenames(conf, "./shut", "./shut", 0, 6) self.assertEqual(result, ["./shutdown.sh"]) + def test_completenames_falls_back_to_case_insensitive_allowed_commands(self): + """Allowed-command completion should retry with a case-insensitive prefix match.""" + conf = {"allowed": ["echo", "Help", "history"]} + result = completion.completenames(conf, "he", "he", 0, 2) + self.assertEqual(result, ["Help"]) + + def test_completenames_dot_slash_falls_back_to_case_insensitive_matching(self): + """Relative allowed commands should use the same case-insensitive fallback.""" + conf = {"allowed": ["./Shutdown.sh", "./show.sh"]} + result = completion.completenames(conf, "shut", "./shut", 0, 6) + self.assertEqual(result, ["Shutdown.sh"]) + class TestBuiltinsJobsAndSource(unittest.TestCase): """Tests for built-in commands around job control.""" diff --git a/test/test_path.py b/test/test_path.py index c6155e9..2ecca04 100644 --- a/test/test_path.py +++ b/test/test_path.py @@ -1,6 +1,7 @@ """Functional tests for lshell path handling""" import os +import re import unittest from getpass import getuser import pexpect @@ -15,6 +16,11 @@ class TestFunctions(unittest.TestCase): """Functional tests for lshell""" + @staticmethod + def _normalize_ls_error_prefix(text): + """Collapse pinned absolute ls paths back to the user-facing command name.""" + return re.sub(r"^/\S*/ls:", "ls:", text) + def setUp(self): """spawn lshell with pexpect and return the child""" self.child = pexpect.spawn(f"{LSHELL} --config {CONFIG} --strict 1") @@ -37,7 +43,7 @@ def test_external_echo_forbidden_syntax(self): self.child.sendline("echo $(uptime)") self.child.expect(PROMPT) result = self.child.before.decode("utf8").split("\n", 1)[1] - self.assertEqual(expected, result) + self.assertEqual(expected, self._normalize_ls_error_prefix(result)) def test_external_forbidden_path(self): """F09 | external command forbidden path - ls /root""" @@ -48,7 +54,7 @@ def test_external_forbidden_path(self): self.child.sendline("ls ~root") self.child.expect(PROMPT) result = self.child.before.decode("utf8").split("\n", 1)[1] - self.assertEqual(expected, result) + self.assertEqual(expected, self._normalize_ls_error_prefix(result)) def test_builtin_cd_forbidden_path(self): """F10 | built-in command forbidden path - cd ~root""" @@ -80,7 +86,7 @@ def test_etc_passwd_2(self): self.child.sendline("ls -l .*./.*./etc/passwd") self.child.expect(PROMPT) result = self.child.before.decode("utf8").split("\n", 1)[1] - self.assertEqual(expected, result) + self.assertEqual(expected, self._normalize_ls_error_prefix(result)) def test_etc_passwd_3(self): """F13(a) | /etc/passwd: empty variable 'ls -l .?/.?/etc/passwd'""" @@ -88,7 +94,7 @@ def test_etc_passwd_3(self): self.child.sendline("ls -l .?/.?/etc/passwd") self.child.expect(PROMPT) result = self.child.before.decode("utf8").split("\n", 1)[1] - self.assertEqual(expected, result) + self.assertEqual(expected, self._normalize_ls_error_prefix(result)) def test_etc_passwd_4(self): """F13(b) | /etc/passwd: empty variable 'ls -l ../../etc/passwd'""" diff --git a/test/test_prompt_unit.py b/test/test_prompt_unit.py index dc343e0..94ecdf3 100644 --- a/test/test_prompt_unit.py +++ b/test/test_prompt_unit.py @@ -3,10 +3,11 @@ import os import unittest from getpass import getuser +from time import struct_time from unittest.mock import patch from lshell.config.runtime import CheckConfig -from lshell.utils import getpromptbase, updateprompt +from lshell.utils import getpromptbase, parse_ps1, updateprompt TOPDIR = f"{os.path.dirname(os.path.realpath(__file__))}/../" CONFIG = f"{TOPDIR}/test/testfiles/test.conf" @@ -40,6 +41,14 @@ def test_getpromptbase_uses_config_prompt_when_lps1_not_set(self): expected = f"{getuser()}@{os.uname()[1].split('.')[0]}" self.assertEqual(rendered, expected) + def test_parse_ps1_time_placeholders_use_localtime(self): + """LPS1 time placeholders should render in local server time.""" + fixed_localtime = struct_time((2026, 6, 16, 21, 7, 5, 1, 167, -1)) + with patch("lshell.utils.localtime", return_value=fixed_localtime) as mock_localtime: + rendered = parse_ps1(r"\t|\T|\A") + self.assertEqual(rendered, "21:07:05|09:07:05|21:07") + mock_localtime.assert_called_once() + if __name__ == "__main__": unittest.main() diff --git a/test/test_ps2.py b/test/test_ps2.py index da6cffb..2e7854b 100644 --- a/test/test_ps2.py +++ b/test/test_ps2.py @@ -21,7 +21,6 @@ "history", "jobs", "lshow", - "source", ] diff --git a/test/test_regex.py b/test/test_regex.py index 8d6bba0..6039bdd 100644 --- a/test/test_regex.py +++ b/test/test_regex.py @@ -8,6 +8,7 @@ TOPDIR = os.path.abspath(os.path.join(os.path.dirname(__file__), "..")) CONFIG = f"{TOPDIR}/test/testfiles/test.conf" LSHELL = f"{TOPDIR}/bin/lshell" +TESTFILES_PATH_ARG = f'--path "[\'{TOPDIR}/test/testfiles\']"' USER = getuser() PROMPT = f"{USER}:~\\$" @@ -31,7 +32,8 @@ def test_grep_valid_log_entry(self): child = pexpect.spawn( ( f"{LSHELL} --config {CONFIG} " - f'--allowed "+ [\'grep\']" --forbidden "[]"' + f'--allowed "+ [\'grep\']" --forbidden "[]" ' + f"{TESTFILES_PATH_ARG}" ) ) child.expect(PROMPT) @@ -52,7 +54,9 @@ def test_grep_invalid_date_format(self): command = f"grep -P '{pattern}' {log_file}" child = pexpect.spawn( - f"{LSHELL} --config {CONFIG} " '--allowed "+ [\'grep\']" --forbidden "[]"' + f"{LSHELL} --config {CONFIG} " + '--allowed "+ [\'grep\']" --forbidden "[]" ' + f"{TESTFILES_PATH_ARG}" ) child.expect(PROMPT) @@ -72,7 +76,9 @@ def test_grep_missing_uid(self): command = f"grep -P '{pattern}' {log_file}" child = pexpect.spawn( - f"{LSHELL} --config {CONFIG} " '--allowed "+ [\'grep\']" --forbidden "[]"' + f"{LSHELL} --config {CONFIG} " + '--allowed "+ [\'grep\']" --forbidden "[]" ' + f"{TESTFILES_PATH_ARG}" ) child.expect(PROMPT) @@ -93,7 +99,9 @@ def test_grep_special_characters_in_uid(self): command = f"grep -P '{pattern}' {log_file}" child = pexpect.spawn( - f"{LSHELL} --config {CONFIG} " '--allowed "+ [\'grep\']" --forbidden "[]"' + f"{LSHELL} --config {CONFIG} " + '--allowed "+ [\'grep\']" --forbidden "[]" ' + f"{TESTFILES_PATH_ARG}" ) child.expect(PROMPT) diff --git a/test/test_scripts.py b/test/test_scripts.py index b654524..050b0ce 100644 --- a/test/test_scripts.py +++ b/test/test_scripts.py @@ -24,7 +24,6 @@ "history", "jobs", "lshow", - "source", ] diff --git a/test/test_security.py b/test/test_security.py index dedb1fe..bfbaca8 100644 --- a/test/test_security.py +++ b/test/test_security.py @@ -21,7 +21,6 @@ "history", "jobs", "lshow", - "source", ] diff --git a/test/test_security_attack_surface_unit.py b/test/test_security_attack_surface_unit.py index 169d0f0..54258d3 100644 --- a/test/test_security_attack_surface_unit.py +++ b/test/test_security_attack_surface_unit.py @@ -368,9 +368,10 @@ def test_cmd_parse_execute_short_circuit_skips_failed_and_branch( mock_path.side_effect = lambda line, conf, strict=None: (0, conf) def exec_side_effect(command, background=False, extra_env=None, **_kwargs): - if command == "false": + executable = os.path.basename(command.split()[0]) + if executable == "false": return 1 - if command == "echo recovered": + if executable == "echo" and command.endswith(" recovered"): return 0 return 99 @@ -381,8 +382,8 @@ def exec_side_effect(command, background=False, extra_env=None, **_kwargs): ) self.assertEqual(ret, 0) - executed = [call.args[0] for call in mock_exec.call_args_list] - self.assertEqual(executed, ["false", "echo recovered"]) + executed = [os.path.basename(call.args[0].split()[0]) for call in mock_exec.call_args_list] + self.assertEqual(executed, ["false", "echo"]) @patch("lshell.sec.check_forbidden_chars") @patch("lshell.sec.check_secure") @@ -443,7 +444,11 @@ def test_cmd_parse_execute_allowed_shell_escape_skips_ld_preload( self.assertEqual(ret, 0) self.assertEqual(mock_exec.call_count, 1) - self.assertEqual(mock_exec.call_args.args[0], "sudo ls") + self.assertEqual( + os.path.basename(mock_exec.call_args.args[0].split()[0]), + "sudo", + ) + self.assertTrue(mock_exec.call_args.args[0].endswith(" ls")) self.assertIsNone( mock_exec.call_args.kwargs.get("extra_env"), msg="allowed_shell_escape should bypass LD_PRELOAD injection", diff --git a/test/test_security_attack_surface_unit_part2.py b/test/test_security_attack_surface_unit_part2.py index 83d8765..ba684ef 100644 --- a/test/test_security_attack_surface_unit_part2.py +++ b/test/test_security_attack_surface_unit_part2.py @@ -189,6 +189,58 @@ def poll(self): self.assertNotIn("BASH_FUNC_echo%%", child_env) self.assertEqual(child_env.get("LSHELL_SAFE_ENV"), "ok") + @patch.dict( + os.environ, + { + "PATH": "/usr/bin", + "SHELLOPTS": "xtrace", + "PS4": "$(echo PWNED >&2)", + "PROMPT_COMMAND": "id", + "PYTHONPATH": "/tmp/evil", + "BASH_ENV": "/tmp/bashenv", + "BASH_FUNC_echo%%": "() { :; }", + }, + clear=True, + ) + @patch("lshell.utils.signal.getsignal", return_value=None) + @patch("lshell.utils.signal.signal") + @patch("lshell.utils.subprocess.Popen") + def test_exec_cmd_strips_shell_sensitive_environment_variables( + self, mock_popen, _mock_signal, _mock_getsignal + ): + """Regular command execution must scrub shell-sensitive inherited env vars.""" + + class FakeProc: + """Minimal subprocess fake for exec_cmd foreground path.""" + + def __init__(self): + self.returncode = 0 + self.pid = 31337 + self.args = ["bash", "-c", "echo hi"] + self.lshell_cmd = "" + + def communicate(self): + """Simulate foreground process I/O completion.""" + return None + + def poll(self): + """Simulate an already-finished subprocess.""" + return 0 + + mock_popen.return_value = FakeProc() + + ret = utils.exec_cmd("echo hi") + + self.assertEqual(ret, 0) + exec_env = mock_popen.call_args.kwargs["env"] + self.assertEqual(exec_env["PATH"], utils.build_trusted_path()) + self.assertNotIn("SHELLOPTS", exec_env) + self.assertNotIn("PS4", exec_env) + self.assertNotIn("PROMPT_COMMAND", exec_env) + self.assertNotIn("PYTHONPATH", exec_env) + self.assertNotIn("BASH_ENV", exec_env) + self.assertFalse(any(name.startswith("BASH_FUNC_") for name in exec_env)) + def test_cmd_parse_execute_should_block_forbidden_env_assignment_via_assignment_only( self, ): @@ -242,7 +294,11 @@ def test_cmd_parse_execute_assignment_then_and_expands_with_updated_env( ) self.assertEqual(ret, 0) self.assertEqual(mock_exec.call_count, 1) - self.assertEqual(mock_exec.call_args.args[0], "echo ok") + self.assertEqual( + os.path.basename(mock_exec.call_args.args[0].split()[0]), + "echo", + ) + self.assertTrue(mock_exec.call_args.args[0].endswith(" ok")) finally: if original is None: os.environ.pop("LSHELL_CHAIN_AND", None) @@ -266,7 +322,11 @@ def test_cmd_parse_execute_assignment_then_semicolon_expands_with_updated_env( ) self.assertEqual(ret, 0) self.assertEqual(mock_exec.call_count, 1) - self.assertEqual(mock_exec.call_args.args[0], "echo ok") + self.assertEqual( + os.path.basename(mock_exec.call_args.args[0].split()[0]), + "echo", + ) + self.assertTrue(mock_exec.call_args.args[0].endswith(" ok")) finally: if original is None: os.environ.pop("LSHELL_CHAIN_SEMI", None) @@ -291,7 +351,13 @@ def test_cmd_parse_execute_single_quoted_variable_remains_literal( ) self.assertEqual(ret, 0) self.assertEqual(mock_exec.call_count, 1) - self.assertEqual(mock_exec.call_args.args[0], "echo '$LSHELL_CHAIN_QUOTED'") + self.assertEqual( + os.path.basename(mock_exec.call_args.args[0].split()[0]), + "echo", + ) + self.assertTrue( + mock_exec.call_args.args[0].endswith(" '$LSHELL_CHAIN_QUOTED'") + ) finally: if original is None: os.environ.pop("LSHELL_CHAIN_QUOTED", None) @@ -320,7 +386,11 @@ def test_cmd_parse_execute_allows_full_bash_script_command_for_login_script( self.assertEqual(ret, 0) self.assertEqual(mock_exec.call_count, 1) self.assertEqual( - mock_exec.call_args.args[0], "bash test/testfiles/login_script.sh" + os.path.basename(mock_exec.call_args.args[0].split()[0]), + "bash", + ) + self.assertTrue( + mock_exec.call_args.args[0].endswith(" test/testfiles/login_script.sh") ) def test_check_secure_blocks_braced_variable_expansion_when_forbidden(self): @@ -719,8 +789,13 @@ def test_cmd_parse_execute_passes_parameter_expansion_forms_when_config_allows_t self.assertEqual(ret, 0) self.assertEqual(mock_exec.call_count, 1) self.assertEqual( - mock_exec.call_args.args[0], - "echo ${LSHELL_MISSING:-fallback} ${#HOME}", + os.path.basename(mock_exec.call_args.args[0].split()[0]), + "echo", + ) + self.assertTrue( + mock_exec.call_args.args[0].endswith( + " ${LSHELL_MISSING:-fallback} ${#HOME}" + ) ) def test_cmd_lpath_handles_paths_with_regex_metacharacters(self): @@ -834,6 +909,92 @@ def test_check_path_treats_grep_regex_argument_as_pattern_not_path(self): ) self.assertEqual(ret, 0) + def test_check_path_blocks_bareword_symlink_operand_for_file_command(self): + """Bareword symlinks must be canonicalized and validated for file commands.""" + previous_cwd = os.getcwd() + try: + with tempfile.TemporaryDirectory(prefix="lshell-symlink-deny-", dir="/tmp") as tmpdir: + link_path = os.path.join(tmpdir, "passwdlink") + os.symlink("/etc/passwd", link_path) + + conf = CheckConfig( + self.args + [f"--path=['{tmpdir}']", "--strict=0"] + ).returnconf() + os.chdir(tmpdir) + + ret, _conf = sec.check_path("cat passwdlink", conf, strict=0) + self.assertEqual(ret, 1) + finally: + os.chdir(previous_cwd) + + def test_check_path_blocks_bareword_symlink_operand_for_text_filters(self): + """awk/sed/sort file operands must not bypass path ACL as barewords.""" + previous_cwd = os.getcwd() + try: + with tempfile.TemporaryDirectory(prefix="lshell-filter-symlink-", dir="/tmp") as tmpdir: + link_path = os.path.join(tmpdir, "passwdlink") + os.symlink("/etc/passwd", link_path) + + os.chdir(tmpdir) + + for command in ( + "awk 1 passwdlink", + "awk -f passwdlink", + "sed -n 1p passwdlink", + "sed -e 1p passwdlink", + "sed -f passwdlink", + "sort passwdlink", + ): + with self.subTest(command=command): + conf = CheckConfig( + self.args + [f"--path=['{tmpdir}']", "--strict=0"] + ).returnconf() + os.chdir(tmpdir) + ret, _conf = sec.check_path(command, conf, strict=0) + self.assertEqual(ret, 1) + finally: + os.chdir(previous_cwd) + + def test_check_path_keeps_awk_and_sed_scripts_from_being_treated_as_files(self): + """awk/sed inline scripts are syntax operands; following files are paths.""" + previous_cwd = os.getcwd() + try: + with tempfile.TemporaryDirectory(prefix="lshell-filter-script-", dir="/tmp") as tmpdir: + awk_script_name = "passwdlink" + sed_script_name = "sedscript" + for name in (awk_script_name, sed_script_name): + os.symlink("/etc/passwd", os.path.join(tmpdir, name)) + + conf = CheckConfig( + self.args + [f"--path=['{tmpdir}']", "--strict=0"] + ).returnconf() + os.chdir(tmpdir) + + for command in (f"awk {awk_script_name}", f"sed {sed_script_name}"): + with self.subTest(command=command): + ret, _conf = sec.check_path(command, conf, strict=0) + self.assertEqual(ret, 0) + finally: + os.chdir(previous_cwd) + + def test_check_path_keeps_non_file_command_bareword_symlink_usable(self): + """Non-filesystem commands should not treat generic barewords as path operands.""" + previous_cwd = os.getcwd() + try: + with tempfile.TemporaryDirectory(prefix="lshell-symlink-echo-", dir="/tmp") as tmpdir: + link_path = os.path.join(tmpdir, "passwdlink") + os.symlink("/etc/passwd", link_path) + + conf = CheckConfig( + self.args + [f"--path=['{tmpdir}']", "--strict=0"] + ).returnconf() + os.chdir(tmpdir) + + ret, _conf = sec.check_path("echo passwdlink", conf, strict=0) + self.assertEqual(ret, 0) + finally: + os.chdir(previous_cwd) + def test_check_path_rejects_nul_byte_path_without_crashing(self): """Malformed NUL-byte path operands should fail closed without exceptions.""" conf = CheckConfig( @@ -915,7 +1076,10 @@ def test_cmd_parse_execute_trusted_protocol_allows_chained_sftp_server_commands( self.assertEqual(ret, 0) self.assertEqual(mock_exec.call_count, 2) - self.assertEqual(mock_exec.call_args_list[0].args[0], "sftp-server") + self.assertEqual( + os.path.basename(mock_exec.call_args_list[0].args[0].split()[0]), + "sftp-server", + ) self.assertEqual(mock_exec.call_args_list[1].args[0], "/usr/libexec/sftp-server") def test_check_allowed_file_extensions_allows_existing_directory_target(self): diff --git a/test/test_security_hardening_functional.py b/test/test_security_hardening_functional.py index 92bc673..a5f0b6a 100644 --- a/test/test_security_hardening_functional.py +++ b/test/test_security_hardening_functional.py @@ -16,7 +16,7 @@ class TestSecurityHardeningFunctional(unittest.TestCase): """Functional tests for attack-oriented script execution scenarios.""" - def _run_lsh_script(self, script_body, extra_shell_args=""): + def _run_lsh_script(self, script_body, extra_shell_args="", env=None): """Run a temporary .lsh script and return subprocess.CompletedProcess.""" with tempfile.TemporaryDirectory(prefix="lshell-hardening-") as tempdir: wrapper_path = os.path.join(tempdir, "wrapper.sh") @@ -37,12 +37,17 @@ def _run_lsh_script(self, script_body, extra_shell_args=""): handle.write(script) os.chmod(script_path, stat.S_IRWXU) + child_env = os.environ.copy() + if env: + child_env.update(env) + return subprocess.run( [wrapper_path, script_path], stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True, check=False, + env=child_env, ) def test_inline_assignment_does_not_persist_between_commands(self): @@ -190,3 +195,39 @@ def test_path_acl_prefix_confusion_is_blocked(self): combined = result.stdout + result.stderr self.assertIn(f'lshell: forbidden path: "{sibling_dir}/"', combined) + + def test_inherited_shellopts_and_ps4_do_not_execute_payload(self): + """Inherited bash-control variables should not influence bash -c execution.""" + result = self._run_lsh_script( + script_body="echo SAFE\n", + extra_shell_args="--forbidden \"[]\"", + env={ + "SHELLOPTS": "xtrace", + "PS4": "$(echo LSHELL_POC >&2) ", + "PROMPT_COMMAND": "echo PROMPT_POC >&2", + }, + ) + + self.assertEqual(result.returncode, 0) + combined = result.stdout + result.stderr + self.assertIn("SAFE", result.stdout) + self.assertNotIn("LSHELL_POC", combined) + self.assertNotIn("PROMPT_POC", combined) + + def test_path_acl_blocks_bareword_symlink_escape_for_cat(self): + """Bareword symlink operands should be denied after canonical path resolution.""" + with tempfile.TemporaryDirectory(prefix="lshell-path-symlink-hardening-", dir="/tmp") as tmpdir: + link_path = os.path.join(tmpdir, "passwdlink") + os.symlink("/etc/passwd", link_path) + + result = self._run_lsh_script( + script_body=f"cd {tmpdir}\ncat passwdlink\necho SAFE\n", + extra_shell_args=( + f"--allowed \"+['cat']\" --path \"['{tmpdir}']\" --strict 0" + ), + ) + + combined = result.stdout + result.stderr + self.assertIn('lshell: forbidden path: "/etc/passwd"', combined) + self.assertIn("SAFE", combined) + self.assertNotIn("root:x:0:0:", combined) diff --git a/test/test_session_interaction_functional.py b/test/test_session_interaction_functional.py index 3da3ec2..9718415 100644 --- a/test/test_session_interaction_functional.py +++ b/test/test_session_interaction_functional.py @@ -159,6 +159,115 @@ def test_unknown_command_user_message_differs_by_strict_mode(self): self._safe_exit(strict) strict.close(force=True) + def test_up_down_arrows_search_history_by_current_prefix(self): + """Up/down arrows should recall history entries matching the typed prefix.""" + def run_sequence(*keys): + history_path = None + child = None + try: + with tempfile.NamedTemporaryFile( + "w", + encoding="utf-8", + delete=False, + prefix="lshell-history-search-", + ) as history_file: + history_file.write("echo alpha\n") + history_file.write("help\n") + history_file.write("echo alpha beta\n") + history_path = history_file.name + + child = self._spawn_shell( + "--allowed \"['echo', 'help']\" " + '--forbidden "[]" ' + "--strict 0 " + f"--history_file='{history_path}'" + ) + child.send("echo a") + for key in keys: + child.send(key) + child.sendline("") + child.expect(PROMPT) + return child.before.replace("\r", "").replace("\x08", "") + finally: + if child is not None: + self._safe_exit(child) + child.close(force=True) + if history_path and os.path.exists(history_path): + os.unlink(history_path) + + latest_match = run_sequence("\x1b[A") + self.assertRegex(latest_match, r"(?m)^alpha beta$") + + older_match = run_sequence("\x1b[A", "\x1b[A") + self.assertRegex(older_match, r"(?m)^alpha$") + + forward_match = run_sequence("\x1b[A", "\x1b[A", "\x1b[B") + self.assertRegex(forward_match, r"(?m)^alpha beta$") + + def test_history_command_persists_deduplicated_normalized_entries(self): + """History output should apply duplicate removal and blank reduction policies.""" + with tempfile.NamedTemporaryFile( + "w", + encoding="utf-8", + delete=False, + prefix="lshell-history-policies-", + ) as history_file: + history_path = history_file.name + + child = self._spawn_shell( + "--allowed \"['echo', 'history']\" " + '--forbidden "[]" ' + "--strict 0 " + f"--history_file='{history_path}'" + ) + try: + self._run_command(child, "echo alpha") + self._run_command(child, "echo beta") + self._run_command(child, "echo alpha") + history_output = self._run_command(child, "history") + + self.assertEqual(history_output.count("echo alpha"), 1) + self.assertEqual(history_output.count("echo beta"), 1) + self.assertNotIn("echo alpha", history_output) + finally: + self._safe_exit(child) + child.close(force=True) + if os.path.exists(history_path): + os.unlink(history_path) + + def test_ctrl_d_does_not_persist_literal_eof_into_history(self): + """Ctrl-D should exit cleanly without rewriting the last history entry as EOF.""" + with tempfile.NamedTemporaryFile( + "w", + encoding="utf-8", + delete=False, + prefix="lshell-history-eof-", + ) as history_file: + history_file.write("echo seed\n") + history_path = history_file.name + + child = self._spawn_shell( + "--allowed \"['echo']\" " + '--forbidden "[]" ' + "--strict 0 " + f"--history_file='{history_path}'" + ) + try: + self._run_command(child, "echo keep") + child.sendeof() + child.expect(pexpect.EOF) + finally: + child.close(force=True) + + try: + with open(history_path, "r", encoding="utf-8") as handle: + persisted = handle.read() + self.assertIn("echo keep", persisted) + self.assertNotIn("EOF", persisted) + finally: + if os.path.exists(history_path): + os.unlink(history_path) + def test_bg_builtin_reports_not_supported(self): """`bg` should report explicit unsupported status to the user.""" child = self._spawn_shell() @@ -382,6 +491,57 @@ def test_env_path_resolves_allowed_command_binary(self): self._safe_exit(child) child.close(force=True) + def test_inherited_path_does_not_hijack_allowed_command_resolution(self): + """Ambient PATH should not shadow an allowed command with a rogue binary.""" + with tempfile.TemporaryDirectory(prefix="lshell-path-shadow-") as bindir: + script_path = os.path.join(bindir, "ls") + with open(script_path, "w", encoding="utf-8") as handle: + handle.write("#!/bin/sh\necho PWNED_STARTUP_PATH\n") + os.chmod(script_path, 0o700) + + child = self._spawn_shell( + "--allowed \"['ls']\" --strict 0", + env={"PATH": f"{bindir}:{os.environ.get('PATH', '')}"}, + ) + try: + output = self._run_command(child, "ls") + self.assertNotIn("PWNED_STARTUP_PATH", output) + finally: + self._safe_exit(child) + child.close(force=True) + + def test_command_resolution_drift_blocks_bare_command_after_session_start(self): + """Replacing an allowlisted bare command mid-session should fail closed.""" + with tempfile.TemporaryDirectory(prefix="lshell-command-drift-") as bindir: + command_name = "lshell_drift_probe" + script_path = os.path.join(bindir, command_name) + with open(script_path, "w", encoding="utf-8") as handle: + handle.write("#!/bin/sh\necho SAFE_START\n") + os.chmod(script_path, 0o700) + + child = self._spawn_shell( + f'--forbidden "[]" --allowed "[\'{command_name}\']" --env_path {bindir}' + ) + try: + initial_output = self._run_command(child, command_name) + self.assertIn("SAFE_START", initial_output) + + replacement_path = os.path.join(bindir, f"{command_name}.new") + with open(replacement_path, "w", encoding="utf-8") as handle: + handle.write("#!/bin/sh\necho PWNED_AFTER_SWAP\n") + os.chmod(replacement_path, 0o700) + os.replace(replacement_path, script_path) + + drift_output = self._run_command(child, command_name) + self.assertIn( + f'lshell: command path changed since session start: "{command_name}"', + drift_output, + ) + self.assertNotIn("PWNED_AFTER_SWAP", drift_output) + finally: + self._safe_exit(child) + child.close(force=True) + def test_malformed_sudo_dash_u_is_denied_and_session_recovers(self): """Malformed `sudo -u` forms should be denied without killing the session.""" child = self._spawn_shell( @@ -405,8 +565,7 @@ def test_malformed_sudo_dash_u_is_denied_and_session_recovers(self): def test_lps1_prompt_override_persists_across_prompt_refresh(self): """LPS1 environment prompt override should remain stable after commands.""" custom_prompt = "LSHELL_PROMPT> " - env = os.environ.copy() - env["LPS1"] = custom_prompt + env = {"LPS1": custom_prompt} child = self._spawn_shell(env=env, prompt=re.escape(custom_prompt)) try: diff --git a/test/test_signals.py b/test/test_signals.py index a9b3350..4fceb9b 100644 --- a/test/test_signals.py +++ b/test/test_signals.py @@ -1,6 +1,7 @@ """Functional tests for lshell terminal signals""" import os +import re import unittest import time from getpass import getuser @@ -17,6 +18,11 @@ class TestFunctions(unittest.TestCase): """Functional tests for lshell""" + @staticmethod + def _normalize_command_paths(text): + """Remove pinned absolute command paths from user-visible job output.""" + return re.sub(r"/\S*/(sleep|tail)\b", r"\1", text) + def setUp(self): """spawn lshell with pexpect and return the child""" self.child = pexpect.spawn(f"{LSHELL} --config {CONFIG} --strict 1") @@ -111,13 +117,15 @@ def test_backgrounding_with_ctrl_z(self): time.sleep(1) child.sendcontrol("z") # Verify stopped job message - child.expect(r"\[\d+\]\+ Stopped tail -f", timeout=1) + child.expect(r"\[\d+\]\+ Stopped (?:/\S*/)?tail -f", timeout=1) # Check jobs output child.expect(PROMPT) child.sendline("jobs") child.expect(PROMPT) - output = child.before.decode("utf-8").split("\n", 1)[1].strip() + output = self._normalize_command_paths( + child.before.decode("utf-8").split("\n", 1)[1].strip() + ) expected_output = ( "[1] Stopped tail -f file1\r\n" "[2]- Stopped tail -f file2\r\n" @@ -151,17 +159,19 @@ def test_background_command_with_ampersand(self): # Run a background command with & child.sendline("sleep 60 &") - child.expect(r"\[\d+\] sleep 60 \(pid: \d+\)", timeout=5) + child.expect(r"\[\d+\] (?:/\S*/)?sleep 60 \(pid: \d+\)", timeout=5) child.sendline("sleep 60 &") - child.expect(r"\[\d+\] sleep 60 \(pid: \d+\)", timeout=5) + child.expect(r"\[\d+\] (?:/\S*/)?sleep 60 \(pid: \d+\)", timeout=5) child.sendline("sleep 60 &") - child.expect(r"\[\d+\] sleep 60 \(pid: \d+\)", timeout=5) + child.expect(r"\[\d+\] (?:/\S*/)?sleep 60 \(pid: \d+\)", timeout=5) # Verify it's listed in jobs child.expect(PROMPT) child.sendline("jobs") child.expect(PROMPT) - output = child.before.decode("utf-8").split("\n", 1)[1].strip() + output = self._normalize_command_paths( + child.before.decode("utf-8").split("\n", 1)[1].strip() + ) expected_output = ( "[1] Stopped sleep 60\r\n" "[2]- Stopped sleep 60\r\n" @@ -182,7 +192,7 @@ def test_exit_with_stopped_jobs(self): time.sleep(1) child.sendcontrol("z") # CI/container scheduling can delay job-control status emission slightly. - child.expect(r"\[\d+\]\+ Stopped tail -f", timeout=3) + child.expect(r"\[\d+\]\+ Stopped (?:/\S*/)?tail -f", timeout=3) # Attempt to exit child.sendline("exit") @@ -190,7 +200,7 @@ def test_exit_with_stopped_jobs(self): # Verify stopped jobs are listed child.sendline("jobs") - child.expect(r"\[\d+\]\+ Stopped tail -f", timeout=5) + child.expect(r"\[\d+\]\+ Stopped (?:/\S*/)?tail -f", timeout=5) # Exit again child.sendline("exit") @@ -205,19 +215,19 @@ def test_resume_stopped_jobs(self): child.sendline("tail -f") time.sleep(1) child.sendcontrol("z") - child.expect(r"\[\d+\]\+ Stopped tail -f", timeout=1) + child.expect(r"\[\d+\]\+ Stopped (?:/\S*/)?tail -f", timeout=1) child.sendline("tail -ff") time.sleep(1) child.sendcontrol("z") - child.expect(r"\[\d+\]\+ Stopped tail -ff", timeout=1) + child.expect(r"\[\d+\]\+ Stopped (?:/\S*/)?tail -ff", timeout=1) child.sendline("tail -fff") time.sleep(1) child.sendcontrol("z") - child.expect(r"\[\d+\]\+ Stopped tail -fff", timeout=1) + child.expect(r"\[\d+\]\+ Stopped (?:/\S*/)?tail -fff", timeout=1) child.sendline("tail -ffff") time.sleep(1) child.sendcontrol("z") - child.expect(r"\[\d+\]\+ Stopped tail -ffff", timeout=1) + child.expect(r"\[\d+\]\+ Stopped (?:/\S*/)?tail -ffff", timeout=1) # Resume the second job child.sendline("fg 2") @@ -250,7 +260,7 @@ def test_interrupt_background_commands(self): # Run a background command child.sendline("sleep 60 &") - child.expect(r"\[\d+\] sleep 60 \(pid: \d+\)", timeout=5) + child.expect(r"\[\d+\] (?:/\S*/)?sleep 60 \(pid: \d+\)", timeout=5) # Interrupt the foreground process (should not affect background) child.sendcontrol("c") @@ -263,7 +273,7 @@ def test_interrupt_background_commands(self): # Verify the background command is still running child.sendline("jobs") - child.expect(r"\[\d+\]\+ Stopped sleep 60", timeout=5) + child.expect(r"\[\d+\]\+ Stopped (?:/\S*/)?sleep 60", timeout=5) def test_jobs_after_completion(self): """F76 | Test that completed jobs are removed from the `jobs` list.""" @@ -274,7 +284,7 @@ def test_jobs_after_completion(self): # Run a short-lived background command child.sendline("sleep 2 &") - child.expect(r"\[\d+\] sleep 2 \(pid: \d+\)", timeout=5) + child.expect(r"\[\d+\] (?:/\S*/)?sleep 2 \(pid: \d+\)", timeout=5) # Wait for the process to complete time.sleep(3) @@ -295,19 +305,21 @@ def test_mix_background_and_foreground(self): # Start a background command child.sendline("sleep 60 &") - child.expect(r"\[\d+\] sleep 60 \(pid: \d+\)", timeout=5) + child.expect(r"\[\d+\] (?:/\S*/)?sleep 60 \(pid: \d+\)", timeout=5) # Start and stop a foreground command child.sendline("tail -f file1") time.sleep(1) child.sendcontrol("z") - child.expect(r"\[\d+\]\+ Stopped tail -f", timeout=1) + child.expect(r"\[\d+\]\+ Stopped (?:/\S*/)?tail -f", timeout=1) # Verify jobs output child.expect(PROMPT) child.sendline("jobs") child.expect(PROMPT) - output = child.before.decode("utf-8").split("\n", 1)[1].strip() + output = self._normalize_command_paths( + child.before.decode("utf-8").split("\n", 1)[1].strip() + ) expected_output = ( "[1]- Stopped sleep 60\r\n[2]+ Stopped tail -f file1" ) @@ -324,7 +336,7 @@ def test_ctrl_d_with_stopped_jobs_no_unknown_syntax(self): child.sendline("tail -f") time.sleep(1) child.sendcontrol("z") - child.expect(r"\[\d+\]\+ Stopped tail -f", timeout=1) + child.expect(r"\[\d+\]\+ Stopped (?:/\S*/)?tail -f", timeout=1) child.expect(PROMPT) # First Ctrl+D should warn and keep shell alive. @@ -374,7 +386,7 @@ def test_background_timeout_removes_job_from_jobs_listing(self): child.expect(PROMPT) child.sendline("sleep 60 &") - child.expect(r"\[\d+\] sleep 60 \(pid: \d+\)", timeout=5) + child.expect(r"\[\d+\] (?:/\S*/)?sleep 60 \(pid: \d+\)", timeout=5) child.expect(PROMPT) # Allow the background timeout handler to kill the process. diff --git a/test/test_source_command_unit.py b/test/test_source_command_unit.py index cdd8343..599a42e 100644 --- a/test/test_source_command_unit.py +++ b/test/test_source_command_unit.py @@ -8,14 +8,21 @@ from unittest.mock import patch from lshell import builtincmd +from lshell.config.runtime import CheckConfig TOPDIR = os.path.abspath(os.path.join(os.path.dirname(__file__), "..")) SOURCE_FIXTURE = f"{TOPDIR}/test/testfiles/source_command_fixture.lsh" +CONFIG = f"{TOPDIR}/test/testfiles/test.conf" class TestSourceCommand(unittest.TestCase): """Tests for sourcing environment files into the current shell context.""" + def test_source_is_not_enabled_by_default(self): + """Default allowed builtins should not expose source implicitly.""" + conf = CheckConfig([f"--config={CONFIG}", "--quiet=1"]).returnconf() + self.assertNotIn("source", conf["allowed"]) + @patch.dict(os.environ, {}, clear=True) def test_cmd_source_loads_fixture_exports(self): """Load exported values from a checked-in source fixture.""" @@ -99,6 +106,25 @@ def test_cmd_source_expands_environment_variable_paths(self): self.assertEqual(builtincmd.cmd_source("$ENV_FILE_PATH"), 0) self.assertEqual(os.environ.get("ENV_SCOPED"), "value") + @patch.dict(os.environ, {}, clear=True) + def test_cmd_source_rejects_forbidden_environment_variables(self): + """Dangerous environment variables must be rejected when sourcing files.""" + with tempfile.NamedTemporaryFile("w", delete=False) as env_file: + env_file.write("export SHELLOPTS=xtrace\n") + file_path = env_file.name + + stderr = io.StringIO() + try: + with redirect_stderr(stderr): + self.assertEqual(builtincmd.cmd_source(file_path), 1) + self.assertIsNone(os.environ.get("SHELLOPTS")) + self.assertIn( + "lshell: forbidden environment variable: SHELLOPTS", + stderr.getvalue(), + ) + finally: + os.remove(file_path) + if __name__ == "__main__": unittest.main() diff --git a/test/test_ssh.py b/test/test_ssh.py index 70afc18..d3dcc89 100644 --- a/test/test_ssh.py +++ b/test/test_ssh.py @@ -211,7 +211,7 @@ def test_winscp_mode_allows_semicolon_in_interactive_session(self): def test_overssh_trusted_sftp_rejects_assignment_prefix(self): """Trusted sftp-server flow should deny env-assignment command prefixes.""" child = pexpect.spawn( - f"{LSHELL} --config {CONFIG} --sftp 1 " + f"{LSHELL} --config {CONFIG} --sftp 1 --sftp_unsafe_legacy 1 " "--overssh \"['sftp-server']\" " "-c 'TMPDIR=/tmp sftp-server'", env=self._ssh_env(), @@ -222,6 +222,18 @@ def test_overssh_trusted_sftp_rejects_assignment_prefix(self): self.assertIn("lshell: forbidden trusted SSH protocol command", output) self.assertEqual(child.exitstatus, 126) + def test_overssh_sftp_requires_explicit_legacy_override(self): + """sftp=1 alone should refuse legacy sftp-server passthrough.""" + child = pexpect.spawn( + f"{LSHELL} --config {CONFIG} --sftp 1 -c 'sftp-server'", + env=self._ssh_env(), + ) + child.expect(pexpect.EOF, timeout=10) + output = child.before.decode("utf-8") + child.close() + self.assertIn("refusing legacy SFTP passthrough", output) + self.assertEqual(child.exitstatus, 1) + def test_overssh_trusted_sftp_rejects_prefixed_wrapper_command(self): """Trusted sftp flow should reject wrapper commands containing sftp-server text.""" child = pexpect.spawn( diff --git a/test/test_ssh_scp_sftp_attack_surface_unit.py b/test/test_ssh_scp_sftp_attack_surface_unit.py index 48872f1..dfb01c0 100644 --- a/test/test_ssh_scp_sftp_attack_surface_unit.py +++ b/test/test_ssh_scp_sftp_attack_surface_unit.py @@ -259,12 +259,34 @@ def test_run_overssh_rejects_sftp_when_disabled(self): finally: self._restore_ssh_env(saved_env) - def test_run_overssh_allows_sftp_when_enabled(self): - """Execute sftp-server sessions when sftp flag is enabled.""" + def test_run_overssh_rejects_sftp_when_enabled_without_legacy_override(self): + """Refuse legacy sftp-server passthrough unless override is explicit.""" saved_env = self._with_forced_ssh_env() try: conf = CheckConfig(self.args + ["--sftp=1", "--strict=0"]).returnconf() conf["ssh"] = "/usr/libexec/sftp-server" + with patch("lshell.shellcmd.utils.cmd_parse_execute") as mock_exec: + with self.assertRaises(SystemExit) as cm: + ShellCmd( + conf, + args=[], + stdin=io.StringIO(), + stdout=io.StringIO(), + stderr=io.StringIO(), + ) + self.assertEqual(cm.exception.code, 1) + mock_exec.assert_not_called() + finally: + self._restore_ssh_env(saved_env) + + def test_run_overssh_allows_sftp_with_explicit_legacy_override(self): + """Execute sftp-server only when legacy override is explicitly enabled.""" + saved_env = self._with_forced_ssh_env() + try: + conf = CheckConfig( + self.args + ["--sftp=1", "--sftp_unsafe_legacy=1", "--strict=0"] + ).returnconf() + conf["ssh"] = "/usr/libexec/sftp-server" with patch("lshell.shellcmd.utils.cmd_parse_execute", return_value=0) as mock_exec: with self.assertRaises(SystemExit) as cm: ShellCmd( @@ -351,6 +373,33 @@ def test_run_overssh_rejects_scp_download_when_scp_download_disabled(self): finally: self._restore_ssh_env(saved_env) + def test_run_overssh_rejects_clustered_scp_download_when_scp_download_disabled(self): + """Deny clustered scp download flags such as -pf when download is disabled.""" + saved_env = self._with_forced_ssh_env() + try: + conf = CheckConfig( + self.args + ["--scp=1", "--scp_download=0", "--strict=0"] + ).returnconf() + for command in ( + f"scp -pf {conf['home_path']}/artifact", + f"scp -p -f {conf['home_path']}/artifact", + ): + with self.subTest(command=command): + conf["ssh"] = command + with patch("lshell.shellcmd.utils.cmd_parse_execute") as mock_exec: + with self.assertRaises(SystemExit) as cm: + ShellCmd( + conf, + args=[], + stdin=io.StringIO(), + stdout=io.StringIO(), + stderr=io.StringIO(), + ) + self.assertEqual(cm.exception.code, 1) + mock_exec.assert_not_called() + finally: + self._restore_ssh_env(saved_env) + def test_run_overssh_rejects_scp_upload_when_scp_upload_disabled(self): """Deny scp -t when scp_upload flag is disabled.""" saved_env = self._with_forced_ssh_env() @@ -371,6 +420,51 @@ def test_run_overssh_rejects_scp_upload_when_scp_upload_disabled(self): finally: self._restore_ssh_env(saved_env) + def test_run_overssh_rejects_clustered_scp_upload_when_scp_upload_disabled(self): + """Deny clustered scp upload flags such as -pt when upload is disabled.""" + saved_env = self._with_forced_ssh_env() + try: + conf = CheckConfig(self.args + ["--scp=1", "--scp_upload=0", "--strict=0"]).returnconf() + for command in ( + f"scp -pt {conf['home_path']}", + f"scp -r -t {conf['home_path']}", + ): + with self.subTest(command=command): + conf["ssh"] = command + with patch("lshell.shellcmd.utils.cmd_parse_execute") as mock_exec: + with self.assertRaises(SystemExit) as cm: + ShellCmd( + conf, + args=[], + stdin=io.StringIO(), + stdout=io.StringIO(), + stderr=io.StringIO(), + ) + self.assertEqual(cm.exception.code, 1) + mock_exec.assert_not_called() + finally: + self._restore_ssh_env(saved_env) + + def test_run_overssh_rejects_scp_without_recognized_direction(self): + """Forced-command SCP should fail closed when neither -f nor -t is present.""" + saved_env = self._with_forced_ssh_env() + try: + conf = CheckConfig(self.args + ["--scp=1", "--strict=0"]).returnconf() + conf["ssh"] = f"scp -p {conf['home_path']}/artifact" + with patch("lshell.shellcmd.utils.cmd_parse_execute") as mock_exec: + with self.assertRaises(SystemExit) as cm: + ShellCmd( + conf, + args=[], + stdin=io.StringIO(), + stdout=io.StringIO(), + stderr=io.StringIO(), + ) + self.assertEqual(cm.exception.code, 1) + mock_exec.assert_not_called() + finally: + self._restore_ssh_env(saved_env) + def test_run_overssh_applies_scpforce_to_upload_target(self): """Rewrite scp -t target path to configured scpforce directory.""" saved_env = self._with_forced_ssh_env() diff --git a/test/test_ssh_scp_sftp_config_unit.py b/test/test_ssh_scp_sftp_config_unit.py index 9189597..8483cf9 100644 --- a/test/test_ssh_scp_sftp_config_unit.py +++ b/test/test_ssh_scp_sftp_config_unit.py @@ -21,7 +21,7 @@ def test_winscp_allowed_commands(self): """U20 | when winscp is enabled, new allowed commands are automatically added.""" args = self.args + ["--allowed=[]", "--winscp=1"] userconf = CheckConfig(args).returnconf() - exclude = list(set(builtincmd.builtins_list) - set(["export"])) + exclude = list(builtincmd.default_builtins_list) expected = exclude + ["scp", "env", "pwd", "groups", "unset", "unalias"] expected.sort() allowed = userconf["allowed"] diff --git a/test/test_unit.py b/test/test_unit.py index 1a0c6d9..7b6c640 100644 --- a/test/test_unit.py +++ b/test/test_unit.py @@ -7,12 +7,18 @@ import tempfile import unittest from getpass import getuser -from time import strftime, gmtime +from time import strftime, localtime from unittest.mock import patch # import lshell specifics from lshell.config.runtime import CheckConfig -from lshell.utils import get_aliases, updateprompt, parse_ps1, getpromptbase +from lshell.utils import ( + build_trusted_path, + get_aliases, + updateprompt, + parse_ps1, + getpromptbase, +) from lshell import builtincmd from lshell import sec from lshell.shellcmd import ShellCmd @@ -269,10 +275,9 @@ def test_env_path_updates_path_variable(self): ] CheckConfig(args).returnconf() - # Verify that the $PATH has been updated correctly - expected_path = f"{random_path}:{original_path}" + # Verify that the runtime PATH has been rebuilt from trusted/configured entries. + expected_path = build_trusted_path(random_path) - # Assuming CheckConfig sets the environment variable self.assertEqual(os.environ["PATH"], expected_path) # Reset the PATH environment variable @@ -317,7 +322,10 @@ def test_new_path_starts_with_colon(self, mock_exit): def test_lps1_user_host_time(self): r"""U32 | LPS1 using \u@\h - \t> format""" os.environ["LPS1"] = r"\u@\h - \t> " - expected = f"{getuser()}@{os.uname()[1].split('.')[0]} - {strftime('%H:%M:%S', gmtime())}> " + expected = ( + f"{getuser()}@{os.uname()[1].split('.')[0]} - " + f"{strftime('%H:%M:%S', localtime())}> " + ) prompt = parse_ps1(os.getenv("LPS1")) self.assertEqual(prompt, expected) del os.environ["LPS1"] diff --git a/test/test_user_behavior_security_functional.py b/test/test_user_behavior_security_functional.py index cfb5cfd..1d858c7 100644 --- a/test/test_user_behavior_security_functional.py +++ b/test/test_user_behavior_security_functional.py @@ -1,6 +1,7 @@ """Functional attacker/sysadmin behavior tests for lshell sessions.""" import os +import tempfile import unittest from getpass import getuser @@ -10,6 +11,7 @@ TOPDIR = os.path.abspath(os.path.join(os.path.dirname(__file__), "..")) CONFIG = f"{TOPDIR}/test/testfiles/test.conf" LSHELL = f"{TOPDIR}/bin/lshell" +SOURCE_FIXTURE = f"{TOPDIR}/test/testfiles/source_command_fixture.lsh" USER = getuser() PROMPT = f"{USER}:~\\$" @@ -17,12 +19,16 @@ class TestUserBehaviorSecurityFunctional(unittest.TestCase): """End-to-end tests that mimic realistic operator and attacker behavior.""" - def _spawn_shell(self, extra_args=""): + def _spawn_shell(self, extra_args="", env=None): """Spawn lshell and wait for prompt.""" + child_env = os.environ.copy() + if env: + child_env.update(env) child = pexpect.spawn( f"{LSHELL} --config {CONFIG} {extra_args}", encoding="utf-8", timeout=10, + env=child_env, ) child.expect(PROMPT) return child @@ -92,3 +98,58 @@ def test_operator_smuggling_is_rejected_without_running_payload(self): self._exit_shell(child) finally: child.close() + + def test_source_is_blocked_by_default_but_shell_remains_usable(self): + """Interactive source should require explicit admin opt-in.""" + child = self._spawn_shell("--strict 0") + try: + body = self._run_command(child, f"source {SOURCE_FIXTURE}") + self.assertIn("lshell: unknown syntax: source", body) + + still_usable = self._run_command(child, "echo STILL_OK") + self.assertIn("STILL_OK", still_usable) + self._exit_shell(child) + finally: + child.close() + + def test_export_rejects_shellopts_and_session_recovers(self): + """Dangerous bash-control variables should be rejected interactively.""" + child = self._spawn_shell("--strict 0 --forbidden \"[]\" --allowed \"+['export']\"") + try: + body = self._run_command(child, "export SHELLOPTS=xtrace") + self.assertIn("lshell: forbidden environment variable: SHELLOPTS", body) + + still_usable = self._run_command(child, "echo AFTER_BLOCK") + self.assertIn("AFTER_BLOCK", still_usable) + self._exit_shell(child) + finally: + child.close() + + def test_lshell_args_env_cannot_override_active_config(self): + """External LSHELL_ARGS must not be able to swap in a weaker config.""" + with tempfile.NamedTemporaryFile("w", delete=False, suffix=".conf") as handle: + handle.write( + "[global]\n" + "logpath : /tmp/lshell-logs/\n\n" + "[default]\n" + "allowed : ['echo','id']\n" + "forbidden : [';','&','|','`','>','<','$(','${']\n" + "warning_counter : 2\n" + "strict : 0\n" + ) + malicious_config = handle.name + + try: + child = self._spawn_shell( + "--strict 0", + env={"LSHELL_ARGS": f"['--config', '{malicious_config}']"}, + ) + try: + body = self._run_command(child, "id") + self.assertIn("lshell:", body) + self.assertIn("id", body) + self.assertNotIn("uid=", body) + finally: + self._exit_shell(child) + finally: + os.remove(malicious_config)