diff --git a/docs/usage.md b/docs/usage.md index 9b6b420..cd50c84 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -81,7 +81,12 @@ Taint follows values between functions and across files through function summari into objects (containers, attributes, instances of the project's own classes) and through closures. Flask, FastAPI and Django route handlers, class-based views and registered URL patterns receive HTTP input; click and Typer commands receive `argv` input; `input()`, -`sys.argv`, environment variables and the responses of HTTP clients are further sources. +`sys.argv`, environment variables, the output of local processes and the responses of +HTTP clients are further sources. The first three are operator-controlled: a command-line +tool opens the paths it is given, a command or a path built from the environment is the +operator's own doing, and a path read from a local process is not a traversal, so those +sources do not carry the corresponding kinds; a downloaded script piped into a shell +stays a command injection. ## Rules diff --git a/src/coretrace_python/bundled/models/python_stdlib/python_stdlib.py b/src/coretrace_python/bundled/models/python_stdlib/python_stdlib.py index 8661b47..e3d84c5 100644 --- a/src/coretrace_python/bundled/models/python_stdlib/python_stdlib.py +++ b/src/coretrace_python/bundled/models/python_stdlib/python_stdlib.py @@ -8,6 +8,9 @@ from coretrace_python.semantic.symbols import SymbolId from coretrace_python.taint import Model, Sanitizer, Sink, Source, TaintKind, Validator +_ENVIRONMENT_KINDS = TaintKind.ALL & ~(TaintKind.COMMAND | TaintKind.PATH) +_PROCESS_OUTPUT_KINDS = TaintKind.ALL & ~TaintKind.PATH + def _sym(path: str) -> SymbolId: return SymbolId(f"python.{path}") @@ -18,13 +21,16 @@ class PythonStdlibModels(ModelPlugin): models: ClassVar[tuple[Model, ...]] = ( Source(_sym("builtins.input"), "stdin"), Source(_sym("sys.stdin"), "stdin"), - # A command-line tool is expected to open the paths it is given. + # Operator-controlled inputs. A command-line tool is expected to open the paths + # it is given; the environment is set by whoever runs the program, so a command + # or a path built from it is not an injection; the output of a local process is + # not a path either, but a downloaded script piped into a shell is a real flaw. Source(_sym("sys.argv"), "argv", TaintKind.ALL & ~TaintKind.PATH), - Source(_sym("os.environ"), "environment"), - Source(_sym("subprocess.run.stdout"), "process-output"), - Source(_sym("subprocess.check_output"), "process-output"), - Source(_sym("subprocess.getoutput"), "process-output"), - Source(_sym("subprocess.Popen.communicate"), "process-output"), + Source(_sym("os.environ"), "environment", _ENVIRONMENT_KINDS), + Source(_sym("subprocess.run.stdout"), "process-output", _PROCESS_OUTPUT_KINDS), + Source(_sym("subprocess.check_output"), "process-output", _PROCESS_OUTPUT_KINDS), + Source(_sym("subprocess.getoutput"), "process-output", _PROCESS_OUTPUT_KINDS), + Source(_sym("subprocess.Popen.communicate"), "process-output", _PROCESS_OUTPUT_KINDS), Sink(_sym("os.system"), TaintKind.COMMAND), Sink(_sym("os.popen"), TaintKind.COMMAND), Sink(_sym("subprocess.run"), TaintKind.COMMAND), diff --git a/src/coretrace_python/plugins/secrets.py b/src/coretrace_python/plugins/secrets.py index f885f92..3559466 100644 --- a/src/coretrace_python/plugins/secrets.py +++ b/src/coretrace_python/plugins/secrets.py @@ -36,7 +36,11 @@ _PLACEHOLDER_WORDS = frozenset( {"", "changeme", "change_me", "password", "passwd", "secret", "token", "example", "xxx", "todo", "none", "null"} ) -_NOT_CREDENTIAL_SUFFIXES = ("_name", "_field", "_file", "_path", "_url", "_id", "_env", "_var", "_header", "_param") +_NOT_CREDENTIAL_SUFFIXES = ( + "_name", "_field", "_file", "_path", "_url", "_id", "_env", "_var", "_header", "_param", + # ``token_type = "bearer"``: the kind of a credential, not one. + "_type", "_kind", "_scheme", "_format", "_algorithm", "_method", "_mode", +) # MD5, SHA-1, SHA-256 and SHA-512 digests, as hex. _DIGEST_LENGTHS = frozenset({32, 40, 64, 128}) _DIGEST_WORDS = ("hash", "digest", "sha", "md5", "checksum", "commit", "etag", "fingerprint", "revision", "version") diff --git a/tests/regression/expected/InsecureGram.json b/tests/regression/expected/InsecureGram.json index 987f25b..b5146fc 100644 --- a/tests/regression/expected/InsecureGram.json +++ b/tests/regression/expected/InsecureGram.json @@ -12,12 +12,6 @@ "rule": "insecure-deserialization", "function": "insecure_deserialize" }, - { - "file": "app/auth.py", - "line": 40, - "rule": "hardcoded-credential", - "function": "login" - }, { "file": "app/command.py", "line": 12, diff --git a/tests/regression/expected/full-stack-fastapi-template.json b/tests/regression/expected/full-stack-fastapi-template.json index dec398e..7ec4134 100644 --- a/tests/regression/expected/full-stack-fastapi-template.json +++ b/tests/regression/expected/full-stack-fastapi-template.json @@ -48,12 +48,6 @@ "rule": "xss", "function": "recover_password_html_content" }, - { - "file": "backend/app/models.py", - "line": 123, - "rule": "hardcoded-credential", - "function": null - }, { "file": "backend/tests/api/routes/test_login.py", "line": 31, diff --git a/tests/regression/expected/httpie.json b/tests/regression/expected/httpie.json index 47d25ca..53a5688 100644 --- a/tests/regression/expected/httpie.json +++ b/tests/regression/expected/httpie.json @@ -24,12 +24,6 @@ "rule": "ssrf", "function": "collect_messages" }, - { - "file": "httpie/internal/daemons.py", - "line": 121, - "rule": "command-injection", - "function": "spawn_daemon" - }, { "file": "httpie/internal/update_warnings.py", "line": 44, @@ -53,12 +47,6 @@ "line": 163, "rule": "missing-timeout", "function": "test_quiet_with_output_redirection" - }, - { - "file": "tests/test_uploads.py", - "line": 103, - "rule": "command-injection", - "function": "stdin_processes" } ] } diff --git a/tests/regression/expected/luigi.json b/tests/regression/expected/luigi.json index 252f634..7839d6f 100644 --- a/tests/regression/expected/luigi.json +++ b/tests/regression/expected/luigi.json @@ -30,18 +30,6 @@ "rule": "command-injection", "function": "_run_job" }, - { - "file": "luigi/contrib/lsf.py", - "line": 263, - "rule": "path-traversal", - "function": "_run_job" - }, - { - "file": "luigi/contrib/lsf.py", - "line": 274, - "rule": "path-traversal", - "function": "_run_job" - }, { "file": "luigi/contrib/pai.py", "line": 229, @@ -60,12 +48,6 @@ "rule": "missing-timeout", "function": "run" }, - { - "file": "luigi/contrib/pig.py", - "line": 134, - "rule": "command-injection", - "function": "track_and_progress" - }, { "file": "luigi/contrib/salesforce.py", "line": 258, @@ -138,18 +120,6 @@ "rule": "missing-timeout", "function": "_get_batch_info" }, - { - "file": "luigi/contrib/scalding.py", - "line": 211, - "rule": "command-injection", - "function": "run_job" - }, - { - "file": "luigi/contrib/sge.py", - "line": 291, - "rule": "path-traversal", - "function": "_run_job" - }, { "file": "luigi/contrib/sge.py", "line": 296, @@ -162,12 +132,6 @@ "rule": "hardcoded-credential", "function": null }, - { - "file": "test/cmdline_test.py", - "line": 213, - "rule": "command-injection", - "function": "_run_cmdline" - }, { "file": "test/contrib/beam_dataflow_test.py", "line": 161, diff --git a/tests/test_operator_sources.py b/tests/test_operator_sources.py new file mode 100644 index 0000000..a9cad30 --- /dev/null +++ b/tests/test_operator_sources.py @@ -0,0 +1,79 @@ +"""Acceptance tests for operator-controlled sources (issue #71). + +Environment variables and the output of local processes are set by whoever runs the +program, not by a remote attacker: like ``argv`` they are sources, since they may carry a +URL to fetch or a statement to run, but a command or a path built from them is the +operator's own doing (luigi's cluster wrappers, httpie's daemon). The environment does +not carry ``COMMAND`` or ``PATH``; process output does not carry ``PATH`` but keeps +``COMMAND``, since piping a downloaded script into a shell is a real vulnerability. +A credential-like name followed by ``_type``, ``_scheme`` or the like names a kind of +credential, not one. + +Expected to remain red until the stdlib models restrict those sources. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +from coretrace_python import engine +from coretrace_python.findings import Finding +from coretrace_python.semantic.symbols import SymbolId +from coretrace_python.source import SourceManager +from coretrace_python.taint import TaintKind + +REPO = Path(__file__).resolve().parent.parent +PLUGINS = REPO / "src" / "coretrace_python" / "bundled" + + +def source_kinds(symbol: str) -> TaintKind: + manager = engine.build_manager(engine.build_hir(SourceManager().add_source("e.py", ""))) + table = engine.plugin_models(l.plugin for l in engine.load_plugins([PLUGINS], manager)) + return table.source(SymbolId(f"python.{symbol}")).kinds # type: ignore[union-attr] + + +MISSING = None if not source_kinds("os.environ") & TaintKind.COMMAND else "environment still carries COMMAND" + + +@pytest.fixture(autouse=True) +def require_operator_sources() -> None: + if MISSING is not None: + pytest.fail(f"operator-controlled sources are not modelled yet: {MISSING}") + + +def check(text: str) -> tuple[Finding, ...]: + return engine.check(SourceManager().add_source("m.py", text), [PLUGINS]) + + +def rules(findings: tuple[Finding, ...]) -> list[str]: + return sorted(f.rule_id for f in findings) + + +def test_source_kinds_reflect_who_controls_them() -> None: + assert source_kinds("os.environ") == TaintKind.ALL & ~(TaintKind.COMMAND | TaintKind.PATH) + assert source_kinds("subprocess.check_output") == TaintKind.ALL & ~TaintKind.PATH + assert source_kinds("sys.argv") == TaintKind.ALL & ~TaintKind.PATH + assert source_kinds("builtins.input") == TaintKind.ALL + + +def test_environment_does_not_inject_commands_or_paths_but_still_reaches_urls_and_statements() -> None: + assert check("import os\n\ndef run():\n os.system(os.environ['CMD'])\n") == () + assert check("import os\n\ndef run():\n open(os.environ['CONFIG']).read()\n") == () + assert rules(check("import os\nfrom urllib.request import urlopen\n\ndef run():\n urlopen(os.environ['TARGET'])\n")) == ["ssrf"] + assert rules( + check("import os\nimport sqlite3\n\ndef run():\n sqlite3.connect('db').cursor().execute(os.environ['QUERY'])\n") + ) == ["sql-injection"] + + +def test_process_output_does_not_traverse_paths_but_still_injects_commands() -> None: + assert check("import subprocess\n\ndef run():\n open(subprocess.check_output(['pwd']).strip())\n") == () + assert rules(check("import os\nimport subprocess\n\ndef run():\n os.system(subprocess.check_output(['ls']))\n")) == [ + "command-injection" + ] + + +def test_names_of_credential_kinds_are_not_credentials() -> None: + assert check("token_type = 'bearer'\nauth_scheme = 'Bearer'\nhash_algorithm = 'sha256'\npassword_kind = 'plain'\n") == () + assert rules(check("token = 'bearer-abcdef123456'\n")) == ["hardcoded-credential"]