diff --git a/CHANGELOG.md b/CHANGELOG.md index bccc191..28a29fd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,18 @@ # Changelog +## Unreleased + +### Detection + +- A module-level name bound to an attribute, an item or a call of another module-level name resolves in functions, as the same value does in a function body. `cursor = conn.cursor()` after `conn = sqlite3.connect(...)`, `db = client.shop` and `run = os.system` used to leave the calls made through them unknown, so a query built from request data and executed through a module-level cursor was not reported. Names resolve in statement order. The regression corpus is unchanged (#155). +- `nosql-injection` reports attacker input that may hold a structure reaching a NoSQL sink, such as a MongoDB filter, where a JSON body giving `{"$ne": null}` matches any value. Only input that may hold a structure carries `NOSQL`: JSON, the raw bodies a `json.loads` decodes, files, WebSocket messages and standard input. Text cannot hold an operator. The text sources of Flask, bottle and tornado, the URL parameters of their routes, the command line, the environment and `input()` no longer carry `NOSQL`. Neither does an entry-point parameter annotated with scalars (`str`, `Optional[int]`, `Annotated[str, Query()]`, `list[str]`, …), which FastAPI validates. Two limits remain. Django and aiohttp request objects stay tainted as a whole. The keys of a dict are not followed, so a string concatenated into `$where` is not reported. The engine ships no NoSQL sink: model plugins declare them (#158). + +### Plugins + +- `TEXT_KINDS` holds every kind but `NOSQL`: what text input carries. A model plugin declares its text sources with it (#158). + +- A `Members(symbol, dynamic=None, defined=(), typed=())` model says what the attributes and items of a class's instances give, for libraries whose objects give others by names the project chooses. A pymongo client gives the database it is dotted or indexed with, a database gives a collection. The names used to end up in the symbol (`MongoClient.shop.users.find`), or an item vanished into its container's symbol (`MongoClient.find`), so no model could list them. With the model, every path to a collection denotes the collection class, the one a parameter annotated `Collection` denotes: `client.shop.users`, `client["shop"]["users"]`, `client.get_database("shop").get_collection("users")`, a module-level `db = client.shop`, an attribute inherited from the client class. A sink on `Collection.find` covers them all. Symbol resolution now depends on these models, which the engine provides to every module and worker (#157). + ## 0.11.0 (2026-09-25) ### Detection diff --git a/docs/plugins.md b/docs/plugins.md index eac5fa4..30379c3 100644 --- a/docs/plugins.md +++ b/docs/plugins.md @@ -140,7 +140,7 @@ class BottleModels(ModelPlugin): | Model | Declares | |---|---| -| `Source(symbol, label, kinds=ALL)` | A call or attribute whose value is attacker-controlled; `label` names it in messages (`http`, `stdin`, `argv`, `http-response`). `kinds` restricts what it can inject. | +| `Source(symbol, label, kinds=ALL)` | A call or attribute whose value is attacker-controlled; `label` names it in messages (`http`, `stdin`, `argv`, `http-response`). `kinds` restricts what it can inject. Text input, such as a form field, a header or a URL parameter, carries `TEXT_KINDS`, every kind but `NOSQL`: a string cannot hold a query operator. | | `Sink(symbol, kinds, positions=(), keywords=())` | A call dangerous for `kinds`. `positions` limits a kind to the given argument indexes, and `keywords` to the given keyword names; a restricted kind reaches only the arguments listed there. SQL sinks read the statement only, and the SSRF sinks of HTTP clients read the URL, by position or as `url=`. A starred argument or `**kwargs` reaches no restricted kind. | | `Sanitizer(symbol, kinds)` | A call whose result is safe for `kinds`, including when the call is inside a project function the data goes through. | | `EntryPoint(symbol, label, kinds=ALL)` | A decorator or a class base whose functions receive their parameters as `label` input, such as `flask.Flask.route`. | @@ -152,6 +152,7 @@ class BottleModels(ModelPlugin): | `TemplateRender(symbol, position=0, keyword="template_name")` | A call rendering, with autoescaping, the template named by its argument at `position` or `keyword`, such as `render_to_string`. What it returns carries no `HTML` when the name is written in the call and the project's template escapes everything it prints (see the usage guide). | | `Validator(symbol, kinds=ALL, argument=0)` | A callable whose truth proves its argument safe for `kinds`, such as `re.fullmatch`. A flow guarded by it is refuted when it carries no other kind into the sink, and is a hotspot otherwise: a redirect validator does not make a shell command safe. `symbol` may be a function of the project, by its project symbol (`python.hc.accounts.views._allow_redirect`), which then counts wherever it is called, in its own module too. | | `AuthorizationGuard(symbol, label)` | A decorator or a condition restricting who reaches the code, such as `login_required`; a flow behind it is a hotspot. | +| `Members(symbol, dynamic=None, defined=(), typed=())` | What the attributes and items of an instance of the class `symbol` give, for objects that give others by name. `typed` maps an attribute to the class of what it gives, called or not, since a call denotes its callee: pymongo's `get_database` gives a `Database`, as Flask-PyMongo's `db` property does. `dynamic`, when set, is the class of every item and of every attribute outside `defined` and `typed`: a pymongo client gives a `Database` by name (`client.shop`, `client["shop"]`), a database a `Collection`. Every path to a collection then denotes the collection class, so a sink on `Collection.find` covers them all. Without `dynamic`, an attribute keeps its path and an item its container's symbol, as for any class. | Two plugins may describe the same symbol. An identical model is registered once; two `Sink` models merge their kinds, positions and keywords, so a plugin can add `SQL` to a sink diff --git a/docs/usage.md b/docs/usage.md index 56b1678..a39f17e 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -112,6 +112,7 @@ variable is not treated that way yet. | `command-injection` | Shell or process execution (`os.system`, `subprocess` with a string, …). | | `code-injection` | The code `eval` or `exec` runs; the globals and locals passed with it are data. `dangerous-eval` reports every such call, this rule the ones attacker input reaches. | | `sql-injection` | A database statement (`cursor.execute`, SQLAlchemy `text`, Django `raw`, …). Parameters of a parameterised query are not statements. | +| `nosql-injection` | A NoSQL query, such as a MongoDB filter, given attacker input that may hold a structure: a JSON body giving `{"$ne": null}` matches any value. The input that may hold a structure is JSON, the raw bodies a `json.loads` decodes, files, WebSocket messages and standard input. Text cannot hold an operator: form fields, query parameters, headers, cookies, URL parameters, the command line, the environment, and an entry-point parameter annotated with scalars (`str`, `int`, `Optional[str]`, `Annotated[str, Query()]`, `list[str]`, …), which FastAPI validates. Limits: a Django or aiohttp request object is tainted as a whole, so `request.GET` counts like `request.body`; the keys of a dict are not followed, so a string concatenated into `$where` is not reported. The analyzer ships no NoSQL sink: model plugins declare them. | | `path-traversal` | A file system path (`open`, `send_file`, `os.remove`, …). | | `ssrf` | The URL of an HTTP client request (Requests, httpx, `urllib`), by position or as `url=`; the body, headers and query parameters are not the destination. | | `xss` | An HTTP response body built without escaping. A Django template rendered by `render_to_string` escapes, where the analyzer can establish it (below). | diff --git a/src/coretrace_python/bundled/models/bottle/bottle_models.py b/src/coretrace_python/bundled/models/bottle/bottle_models.py index 2936074..b622de9 100644 --- a/src/coretrace_python/bundled/models/bottle/bottle_models.py +++ b/src/coretrace_python/bundled/models/bottle/bottle_models.py @@ -8,13 +8,15 @@ from coretrace_python.plugins import ModelPlugin from coretrace_python.semantic.symbols import SymbolId -from coretrace_python.taint import EntryPoint, Model, Sanitizer, Sink, Source, TaintKind +from coretrace_python.taint import TEXT_KINDS, EntryPoint, Model, Sanitizer, Sink, Source, TaintKind _METHODS = ("route", "get", "post", "put", "delete", "patch") -_REQUEST_ATTRIBUTES = ( - "query", "forms", "params", "json", "body", "headers", "cookies", "GET", "POST", "files", +# Text, which cannot hold a query operator; JSON, raw bodies and files may hold a structure. +_TEXT_ATTRIBUTES = ( + "query", "forms", "params", "headers", "cookies", "GET", "POST", "url", "path", "fullpath", "query_string", "get_cookie", "get_header", ) +_PAYLOAD_ATTRIBUTES = ("json", "body", "files") _FIRST_ONLY = lambda kind: ((kind, (0,)),) @@ -25,9 +27,11 @@ def _sym(path: str) -> SymbolId: class BottleModels(ModelPlugin): name: ClassVar[str] = "bottle-models" models: ClassVar[tuple[Model, ...]] = ( - *(EntryPoint(_sym(f"bottle.{method}"), "http") for method in _METHODS), - *(EntryPoint(_sym(f"bottle.Bottle.{method}"), "http") for method in _METHODS), - *(Source(_sym(f"bottle.request.{attribute}"), "http") for attribute in _REQUEST_ATTRIBUTES), + # A route's parameters are segments of its URL. + *(EntryPoint(_sym(f"bottle.{method}"), "http", TEXT_KINDS) for method in _METHODS), + *(EntryPoint(_sym(f"bottle.Bottle.{method}"), "http", TEXT_KINDS) for method in _METHODS), + *(Source(_sym(f"bottle.request.{attribute}"), "http", TEXT_KINDS) for attribute in _TEXT_ATTRIBUTES), + *(Source(_sym(f"bottle.request.{attribute}"), "http") for attribute in _PAYLOAD_ATTRIBUTES), Sink(_sym("bottle.redirect"), TaintKind.REDIRECT, _FIRST_ONLY(TaintKind.REDIRECT)), # ``static_file(filename, root)``: the root is the application's own. Sink(_sym("bottle.static_file"), TaintKind.PATH, _FIRST_ONLY(TaintKind.PATH)), diff --git a/src/coretrace_python/bundled/models/cli/cli_models.py b/src/coretrace_python/bundled/models/cli/cli_models.py index 72ac98f..e6561f5 100644 --- a/src/coretrace_python/bundled/models/cli/cli_models.py +++ b/src/coretrace_python/bundled/models/cli/cli_models.py @@ -6,11 +6,11 @@ from coretrace_python.plugins import ModelPlugin from coretrace_python.semantic.symbols import SymbolId -from coretrace_python.taint import EntryPoint, Model, TaintKind +from coretrace_python.taint import TEXT_KINDS, EntryPoint, Model, TaintKind -# A command-line tool is expected to open the paths it is given: argv input carries every -# kind but PATH. -ARGV_KINDS = TaintKind.ALL & ~TaintKind.PATH +# A command-line tool is expected to open the paths it is given: argv input is text, +# without PATH. +ARGV_KINDS = TEXT_KINDS & ~TaintKind.PATH _COMMANDS = ( "click.command", diff --git a/src/coretrace_python/bundled/models/flask/flask_models.py b/src/coretrace_python/bundled/models/flask/flask_models.py index 765cf20..2d33563 100644 --- a/src/coretrace_python/bundled/models/flask/flask_models.py +++ b/src/coretrace_python/bundled/models/flask/flask_models.py @@ -7,6 +7,7 @@ from coretrace_python.plugins import ModelPlugin from coretrace_python.semantic.symbols import SymbolId from coretrace_python.taint import ( + TEXT_KINDS, AuthorizationGuard, EntryPoint, Model, @@ -17,10 +18,10 @@ TaintKind, ) -_REQUEST_ATTRIBUTES = ( - "args", "form", "values", "json", "data", "cookies", "headers", "files", - "get_json", "get_data", "url", "full_path", "path", "query_string", "stream", -) +# Strings, or mappings of strings: text, which cannot hold a query operator. +_TEXT_ATTRIBUTES = ("args", "form", "values", "cookies", "headers", "url", "full_path", "path", "query_string") +# JSON, and the raw bodies and files a ``json.loads`` may decode into a structure. +_PAYLOAD_ATTRIBUTES = ("json", "get_json", "data", "get_data", "stream", "files") _TARGET_ONLY = ((TaintKind.REDIRECT, (0,)),) @@ -33,11 +34,13 @@ def _sym(path: str) -> SymbolId: class FlaskModels(ModelPlugin): name: ClassVar[str] = "flask-models" models: ClassVar[tuple[Model, ...]] = ( - *(Source(_sym(f"flask.request.{attribute}"), "http") for attribute in _REQUEST_ATTRIBUTES), - EntryPoint(_sym("flask.Flask.route"), "http"), - EntryPoint(_sym("flask.Blueprint.route"), "http"), - RouteRegistrar(_sym("flask.Flask.add_url_rule"), 2, "http", keyword="view_func"), - RouteRegistrar(_sym("flask.Blueprint.add_url_rule"), 2, "http", keyword="view_func"), + *(Source(_sym(f"flask.request.{attribute}"), "http", TEXT_KINDS) for attribute in _TEXT_ATTRIBUTES), + *(Source(_sym(f"flask.request.{attribute}"), "http") for attribute in _PAYLOAD_ATTRIBUTES), + # A route's parameters are segments of its URL. + EntryPoint(_sym("flask.Flask.route"), "http", TEXT_KINDS), + EntryPoint(_sym("flask.Blueprint.route"), "http", TEXT_KINDS), + RouteRegistrar(_sym("flask.Flask.add_url_rule"), 2, "http", TEXT_KINDS, keyword="view_func"), + RouteRegistrar(_sym("flask.Blueprint.add_url_rule"), 2, "http", TEXT_KINDS, keyword="view_func"), Sink(_sym("flask.render_template_string"), TaintKind.HTML), Sink(_sym("flask.make_response"), TaintKind.HTML), Sink(_sym("flask.Response"), TaintKind.HTML), 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 ec2bbb5..e0a52e6 100644 --- a/src/coretrace_python/bundled/models/python_stdlib/python_stdlib.py +++ b/src/coretrace_python/bundled/models/python_stdlib/python_stdlib.py @@ -7,6 +7,7 @@ from coretrace_python.plugins import ModelPlugin from coretrace_python.semantic.symbols import SymbolId from coretrace_python.taint import ( + TEXT_KINDS, Model, SafeArgument, Sanitizer, @@ -16,7 +17,7 @@ Validator, ) -_ENVIRONMENT_KINDS = TaintKind.ALL & ~(TaintKind.COMMAND | TaintKind.PATH) +_ENVIRONMENT_KINDS = TEXT_KINDS & ~(TaintKind.COMMAND | TaintKind.PATH) # SafeLoader and BaseLoader build plain data only, in Python and in C, under every spelling. _SAFE_YAML_LOADERS = ( "python.yaml.SafeLoader", @@ -38,13 +39,14 @@ def _sym(path: str) -> SymbolId: class PythonStdlibModels(ModelPlugin): name: ClassVar[str] = "python-stdlib-models" models: ClassVar[tuple[Model, ...]] = ( - Source(_sym("builtins.input"), "stdin"), + # A line of text; the stream may be a payload ``json.load`` decodes. + Source(_sym("builtins.input"), "stdin", TEXT_KINDS), Source(_sym("sys.stdin"), "stdin"), # 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("sys.argv"), "argv", TEXT_KINDS & ~TaintKind.PATH), 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), diff --git a/src/coretrace_python/bundled/models/tornado/tornado_models.py b/src/coretrace_python/bundled/models/tornado/tornado_models.py index 6960d14..918e216 100644 --- a/src/coretrace_python/bundled/models/tornado/tornado_models.py +++ b/src/coretrace_python/bundled/models/tornado/tornado_models.py @@ -8,12 +8,13 @@ from coretrace_python.plugins import ModelPlugin from coretrace_python.semantic.symbols import SymbolId -from coretrace_python.taint import EntryPoint, Model, Sanitizer, Sink, Source, TaintKind +from coretrace_python.taint import TEXT_KINDS, EntryPoint, Model, Sanitizer, Sink, Source, TaintKind _HANDLERS = ("tornado.web.RequestHandler", "tornado.websocket.WebSocketHandler") -_REQUEST_METHODS = ( +# Text, which cannot hold a query operator; the request object holds the raw body too. +_TEXT_METHODS = ( "get_argument", "get_arguments", "get_query_argument", "get_query_arguments", - "get_body_argument", "get_body_arguments", "get_cookie", "request", "path_args", "path_kwargs", + "get_body_argument", "get_body_arguments", "get_cookie", "path_args", "path_kwargs", ) _CLIENTS = ("tornado.httpclient.AsyncHTTPClient.fetch", "tornado.httpclient.HTTPClient.fetch") _TARGET_ONLY = ((TaintKind.REDIRECT, (0,)),) @@ -26,8 +27,12 @@ def _sym(path: str) -> SymbolId: class TornadoModels(ModelPlugin): name: ClassVar[str] = "tornado-models" models: ClassVar[tuple[Model, ...]] = ( - *(EntryPoint(_sym(handler), "http") for handler in _HANDLERS), - *(Source(_sym(f"{handler}.{method}"), "http") for handler in _HANDLERS for method in _REQUEST_METHODS), + # A request handler's parameters are segments of its URL; a WebSocket handler also + # receives the messages, raw payloads a ``json.loads`` may decode. + EntryPoint(_sym("tornado.web.RequestHandler"), "http", TEXT_KINDS), + EntryPoint(_sym("tornado.websocket.WebSocketHandler"), "http"), + *(Source(_sym(f"{handler}.{method}"), "http", TEXT_KINDS) for handler in _HANDLERS for method in _TEXT_METHODS), + *(Source(_sym(f"{handler}.request"), "http") for handler in _HANDLERS), *(Sink(_sym(f"{handler}.write"), TaintKind.HTML) for handler in _HANDLERS), *(Sink(_sym(f"{handler}.finish"), TaintKind.HTML) for handler in _HANDLERS), *(Sink(_sym(f"{handler}.redirect"), TaintKind.REDIRECT, _TARGET_ONLY) for handler in _HANDLERS), diff --git a/src/coretrace_python/bundled/security/nosql_injection/nosql_injection.py b/src/coretrace_python/bundled/security/nosql_injection/nosql_injection.py new file mode 100644 index 0000000..fda5452 --- /dev/null +++ b/src/coretrace_python/bundled/security/nosql_injection/nosql_injection.py @@ -0,0 +1,20 @@ +"""NoSQL injection: attacker input that may hold a structure reaching a NOSQL sink, such +as a MongoDB filter, where a JSON body giving ``{"$ne": null}`` matches any value. Text +input carries no ``NOSQL``: a string cannot hold an operator. The engine ships no NOSQL +sink; model plugins declare them.""" + +from __future__ import annotations + +from typing import ClassVar + +from coretrace_python.findings import Severity +from coretrace_python.plugins import TaintDetector +from coretrace_python.taint import TaintKind + + +class NoSqlInjectionPlugin(TaintDetector): + name: ClassVar[str] = "nosql-injection" + rule_id: ClassVar[str] = "nosql-injection" + kind: ClassVar[TaintKind] = TaintKind.NOSQL + severity: ClassVar[Severity] = Severity.HIGH + title: ClassVar[str] = "NoSQL injection" diff --git a/src/coretrace_python/bundled/security/nosql_injection/plugin.toml b/src/coretrace_python/bundled/security/nosql_injection/plugin.toml new file mode 100644 index 0000000..85bada1 --- /dev/null +++ b/src/coretrace_python/bundled/security/nosql_injection/plugin.toml @@ -0,0 +1,9 @@ +name = "nosql-injection" +version = "1.0.0" +plugin_api = ">=1,<2" +requires = ["taint.flows", "findings.refutation"] +provides = ["vulnerability.nosql-injection"] + +[entrypoint] +module = "nosql_injection" +class = "NoSqlInjectionPlugin" diff --git a/src/coretrace_python/engine.py b/src/coretrace_python/engine.py index 49066da..416d881 100644 --- a/src/coretrace_python/engine.py +++ b/src/coretrace_python/engine.py @@ -109,7 +109,7 @@ from coretrace_python.semantic import SEMANTIC_ANALYSES from coretrace_python.semantic.imports import ImportAnalysis, ImportResolutionError, ImportTable from coretrace_python.semantic.scopes import ScopeAnalysis, ScopeError -from coretrace_python.semantic.symbols import SymbolAnalysis, SymbolId +from coretrace_python.semantic.symbols import MembersAnalysis, SymbolAnalysis, SymbolId from coretrace_python.source import SourceFile, SourceId, SourceManager, SourceSpan, decode_text from coretrace_python.taint import ( EntryPoint, @@ -257,6 +257,7 @@ def build_manager( manager = _register_all(module) table = (models or SecurityModelRegistry()).freeze() manager.provide(SecurityModelAnalysis, table) + manager.provide(MembersAnalysis, table.members_by_class()) manager.provide(ClearingAnalysis, table.clearing()) manager.provide(ProjectSummaries, SummaryIndex()) manager.provide(RegisteredRoutes, _routes_of(manager)) @@ -311,6 +312,7 @@ def analyze_file(source: SourceFile, plugin_roots: Sequence[Path]) -> FileAnalys registry = load_plugins(plugin_roots, manager) table = plugin_models(loaded.plugin for loaded in registry) manager.provide(SecurityModelAnalysis, table) + manager.provide(MembersAnalysis, table.members_by_class()) manager.provide(ClearingAnalysis, table.clearing()) manager.provide(ProjectSummaries, SummaryIndex()) manager.provide(RegisteredRoutes, _routes_of(manager)) @@ -402,8 +404,10 @@ def analyze_project( advisories, origins = _merge_advisories(_contributions(registry, plugins, root, file_advisories)) affected = affected_symbols(dependencies, advisories) models = plugin_models(all_plugins, root).extended(*advisory_sinks(affected)) + members = models.members_by_class() for manager in managers.values(): manager.provide(SecurityModelAnalysis, models) + manager.provide(MembersAnalysis, members) manager.provide(DependencyAnalysis, dependencies) imports: dict[str, ImportTable] = {} @@ -698,8 +702,10 @@ def _analyse_batch(batch: _Batch) -> dict[str, dict[str, Any]]: models = plugin_models(all_plugins, batch.root).extended(*advisory_sinks(affected)) routes = _decode_routes(batch.routes) escaped = frozenset(batch.escaped) + members = models.members_by_class() for manager in managers.values(): manager.provide(SecurityModelAnalysis, models) + manager.provide(MembersAnalysis, members) manager.provide(DependencyAnalysis, dependencies) manager.provide(RegisteredRoutes, routes) manager.provide(EscapedTemplates, escaped) diff --git a/src/coretrace_python/interprocedural/callgraph.py b/src/coretrace_python/interprocedural/callgraph.py index c93e810..de41d84 100644 --- a/src/coretrace_python/interprocedural/callgraph.py +++ b/src/coretrace_python/interprocedural/callgraph.py @@ -176,12 +176,12 @@ def callers(self, name: str) -> frozenset[str]: def derive_symbols( - function: FunctionIR, initial: Mapping[Value, SymbolId] | None = None + function: FunctionIR, table: SymbolTable, initial: Mapping[Value, SymbolId] | None = None ) -> dict[Value, SymbolId]: - """Symbols of values: ``Symbol`` results, attributes and items of symbol values, - results of calling a symbol (``sqlite3.connect(p)`` denotes ``python.sqlite3.connect``) - and the values a ``with`` on such a result binds. Known functions derive nothing; - parameters only through ``initial``, their annotated classes.""" + """Symbols of values: ``Symbol`` results, attributes and items of symbol values, as + ``table`` types them, results of calling a symbol (``sqlite3.connect(p)`` denotes + ``python.sqlite3.connect``) and the values a ``with`` on such a result binds. Known + functions derive nothing; parameters only through ``initial``, their annotated classes.""" symbols: dict[Value, SymbolId] = dict(initial or {}) changed = True @@ -195,11 +195,9 @@ def derive_symbols( if isinstance(instruction, Symbol): symbol = instruction.symbol_id elif isinstance(instruction, GetAttr) and instruction.object in symbols: - symbol = symbols[instruction.object].attribute(instruction.attribute) + symbol = table.attribute(symbols[instruction.object], instruction.attribute) elif isinstance(instruction, GetItem) and instruction.object in symbols: - # An item of a symbol-denoted container (``request.files['f']``) - # carries the container's symbol, so its methods resolve. - symbol = symbols[instruction.object] + symbol = table.item(symbols[instruction.object]) elif isinstance(instruction, Call | WithEnter): origin = ( instruction.callee if isinstance(instruction, Call) else instruction.context @@ -263,6 +261,8 @@ def resolve_targets( owner: str | None = None, typed: Mapping[Value, str] | None = None, bases: Mapping[str, SymbolId] | None = None, + *, + table: SymbolTable, ) -> tuple[dict[Value, Target], dict[Value, SymbolId]]: """Map every callee value of ``function`` to its target, and every symbol value. ``nested`` names the functions defined inside this one (``outer.inner``); @@ -271,7 +271,8 @@ def resolve_targets( call to ``App.__init__``, ``app.run()`` and ``self.run()`` calls to ``App.run``. ``bases`` maps a module class to the symbol of its base: an attribute the class does not define is inherited, so ``self.get_argument`` in a ``RequestHandler`` subclass - denotes ``tornado.web.RequestHandler.get_argument``.""" + denotes ``tornado.web.RequestHandler.get_argument``. ``table`` says what the + attributes and items of a class a ``Members`` model describes give.""" module = scopes.module_scope classes = classes or {} @@ -294,8 +295,8 @@ def resolve_targets( class_name = instance_of[instruction.object] base = (bases or {}).get(class_name) if base is not None and instruction.attribute not in classes.get(class_name, ()): - inherited[instruction.result] = base.attribute(instruction.attribute) - symbols = derive_symbols(function, inherited) + inherited[instruction.result] = table.attribute(base, instruction.attribute) + symbols = derive_symbols(function, table, inherited) targets: dict[Value, Target] = { value: ExternalSymbol(symbol) for value, symbol in symbols.items() } @@ -427,6 +428,7 @@ def compute(cls, ctx: AnalysisContext) -> CallGraph: owner, _typed_with_module_classes(function, ssa, classes), bases, + table=table, ) defs = {i.result: i for block in ssa.blocks for i in block.instructions if i.result is not None} found: list[CallSite] = [] diff --git a/src/coretrace_python/ir/lowering.py b/src/coretrace_python/ir/lowering.py index 3899fa9..0d6dde0 100644 --- a/src/coretrace_python/ir/lowering.py +++ b/src/coretrace_python/ir/lowering.py @@ -141,7 +141,7 @@ def imported_symbol(self, node: nodes.Expression) -> SymbolId | None: return self.symbols.resolve(self.scope.id, node.identifier) if isinstance(node, nodes.Attribute): parent = self.imported_symbol(node.value) - return parent.attribute(node.name) if parent is not None else None + return self.symbols.attribute(parent, node.name) if parent is not None else None return None def expression(self, node: nodes.Expression) -> Value: diff --git a/src/coretrace_python/semantic/__init__.py b/src/coretrace_python/semantic/__init__.py index 3806215..c651be2 100644 --- a/src/coretrace_python/semantic/__init__.py +++ b/src/coretrace_python/semantic/__init__.py @@ -2,8 +2,8 @@ from coretrace_python.semantic.imports import ImportAnalysis from coretrace_python.semantic.scopes import ScopeAnalysis -from coretrace_python.semantic.symbols import SymbolAnalysis +from coretrace_python.semantic.symbols import MembersAnalysis, SymbolAnalysis -SEMANTIC_ANALYSES = (ScopeAnalysis, ImportAnalysis, SymbolAnalysis) +SEMANTIC_ANALYSES = (ScopeAnalysis, ImportAnalysis, MembersAnalysis, SymbolAnalysis) -__all__ = ["SEMANTIC_ANALYSES", "ImportAnalysis", "ScopeAnalysis", "SymbolAnalysis"] +__all__ = ["SEMANTIC_ANALYSES", "ImportAnalysis", "MembersAnalysis", "ScopeAnalysis", "SymbolAnalysis"] diff --git a/src/coretrace_python/semantic/symbols.py b/src/coretrace_python/semantic/symbols.py index 20d3de4..3eec67b 100644 --- a/src/coretrace_python/semantic/symbols.py +++ b/src/coretrace_python/semantic/symbols.py @@ -4,6 +4,7 @@ import builtins from collections.abc import Mapping +from dataclasses import dataclass from types import MappingProxyType from typing import ClassVar @@ -16,19 +17,69 @@ BUILTIN_NAMES = frozenset(name for name in dir(builtins) if not name.startswith("_")) +@dataclass(frozen=True) +class Members: + """What the attributes and items of an instance of ``symbol`` give, for a class whose + instances give other objects by name, as a pymongo client gives the database it is + dotted or indexed with (``client.shop``, ``client['shop']``). + + ``typed`` maps an attribute to the class of what it gives, called or not, since a + call denotes its callee: ``get_database`` gives a database, as a ``db`` property does. + ``dynamic``, when set, is the class of every item and of every attribute outside + ``defined`` and ``typed``. Otherwise an attribute keeps its path and an item its + container's symbol, as for any other class.""" + + symbol: SymbolId + dynamic: SymbolId | None = None + defined: tuple[str, ...] = () + typed: tuple[tuple[str, SymbolId], ...] = () + + def __post_init__(self) -> None: + # Sorted: the models' repr is part of the cache key, the same in every process. + object.__setattr__(self, "defined", tuple(sorted(set(self.defined)))) + + def attribute(self, name: str) -> SymbolId: + for attribute, gives in self.typed: + if attribute == name: + return gives + if self.dynamic is not None and name not in self.defined: + return self.dynamic + return self.symbol.attribute(name) + + def item(self) -> SymbolId: + return self.symbol if self.dynamic is None else self.dynamic + + class SymbolTable: """Resolve names to canonical symbols through scopes, imports, builtins and the - module-level instances created by calling a symbol (``app = Flask(__name__)``).""" + module-level names bound to a value denoting a symbol (``app = Flask(__name__)``, + ``cursor = conn.cursor()``). ``members`` holds the ``Members`` models by class.""" def __init__( self, scopes: ScopeTable, imports: ImportTable, instances: Mapping[str, SymbolId] | None = None, + members: Mapping[SymbolId, Members] | None = None, ) -> None: self._scopes = scopes self._imports = imports self._instances = MappingProxyType(dict(instances or {})) + self._members = MappingProxyType(dict(members or {})) + + def attribute(self, symbol: SymbolId, name: str) -> SymbolId: + """The symbol of attribute ``name`` of what ``symbol`` denotes: its path, unless + a ``Members`` model describes the class.""" + + members = self._members.get(symbol) + return symbol.attribute(name) if members is None else members.attribute(name) + + def item(self, symbol: SymbolId) -> SymbolId: + """The symbol of an item of what ``symbol`` denotes: the container's own, so its + methods resolve (``request.files['f'].save``), unless a ``Members`` model says.""" + + members = self._members.get(symbol) + return symbol if members is None else members.item() def resolve(self, scope_id: ScopeId, name: str) -> SymbolId | None: resolution = self._scopes.resolve(scope_id, name) @@ -49,39 +100,78 @@ def resolve_expression(self, scope_id: ScopeId, node: nodes.Expression) -> Symbo return self.resolve(scope_id, node.identifier) if isinstance(node, nodes.Attribute): parent = self.resolve_expression(scope_id, node.value) - return parent.attribute(node.name) if parent is not None else None + return self.attribute(parent, node.name) if parent is not None else None if isinstance(node, nodes.Call): return self.resolve_expression(scope_id, node.callee) return None def analyze_symbols( - scopes: ScopeTable, imports: ImportTable, module: nodes.Module | None = None + scopes: ScopeTable, + imports: ImportTable, + module: nodes.Module | None = None, + members: Mapping[SymbolId, Members] | None = None, ) -> SymbolTable: - table = SymbolTable(scopes, imports) + table = SymbolTable(scopes, imports, members=members) if module is None: return table instances: dict[str, SymbolId] = {} module_scope = scopes.module_scope.id for statement in module.body: - if ( - isinstance(statement, nodes.Assign) - and isinstance(statement.target, nodes.Name) - and isinstance(statement.value, nodes.Call) - ): - symbol = table.resolve_expression(module_scope, statement.value.callee) + if isinstance(statement, nodes.Assign) and isinstance(statement.target, nodes.Name): + symbol = _denoted(table, module_scope, statement.value) if symbol is not None: instances[statement.target.identifier] = symbol - return SymbolTable(scopes, imports, instances) + # Later bindings resolve through this one, in statement order. + table = SymbolTable(scopes, imports, instances, members) + return table + + +def _denoted(table: SymbolTable, scope_id: ScopeId, node: nodes.Expression) -> SymbolId | None: + """What a value denotes, as a function body derives it (``derive_symbols``): a + resolved name or an attribute of one, the result of calling one, or an item of one, + which carries its container's symbol (``os.environ['HOME']``) unless a ``Members`` + model says what its items are.""" + + if isinstance(node, nodes.Subscript): + container = _denoted(table, scope_id, node.value) + return table.item(container) if container is not None else None + if isinstance(node, nodes.Attribute): + parent = _denoted(table, scope_id, node.value) + return table.attribute(parent, node.name) if parent is not None else None + if isinstance(node, nodes.Call): + return _denoted(table, scope_id, node.callee) + return table.resolve_expression(scope_id, node) + + +class MembersAnalysis(Analysis[Mapping[SymbolId, Members]]): + """The ``Members`` models by class, provided by the engine from the security models; + none on its own, so every attribute keeps its path.""" + + name: ClassVar[str] = "semantic.members" + + @classmethod + def compute(cls, ctx: AnalysisContext) -> Mapping[SymbolId, Members]: + return MappingProxyType({}) class SymbolAnalysis(Analysis[SymbolTable]): name: ClassVar[str] = "semantic.symbols" - requires: ClassVar[frozenset[AnyAnalysis]] = frozenset({ScopeAnalysis, ImportAnalysis}) + requires: ClassVar[frozenset[AnyAnalysis]] = frozenset({ScopeAnalysis, ImportAnalysis, MembersAnalysis}) @classmethod def compute(cls, ctx: AnalysisContext) -> SymbolTable: - return analyze_symbols(ctx.get(ScopeAnalysis), ctx.get(ImportAnalysis), ctx.module) - - -__all__ = ["BUILTIN_NAMES", "SymbolAnalysis", "SymbolId", "SymbolTable", "analyze_symbols"] + return analyze_symbols( + ctx.get(ScopeAnalysis), ctx.get(ImportAnalysis), ctx.module, ctx.get(MembersAnalysis) + ) + + +__all__ = [ + "BUILTIN_NAMES", + "Members", + "MembersAnalysis", + "SymbolAnalysis", + "SymbolId", + "SymbolTable", + "analyze_symbols", +] diff --git a/src/coretrace_python/taint/__init__.py b/src/coretrace_python/taint/__init__.py index c97f8ff..860dbbc 100644 --- a/src/coretrace_python/taint/__init__.py +++ b/src/coretrace_python/taint/__init__.py @@ -1,5 +1,6 @@ """Security models and the global multi-kind taint engine (architecture §16, §17).""" +from coretrace_python.semantic.symbols import Members from coretrace_python.taint.engine import ( EntryPointAnalysis, Taint, @@ -9,6 +10,7 @@ propagate_taint, ) from coretrace_python.taint.models import ( + TEXT_KINDS, AuthorizationGuard, EntryPoint, Model, @@ -32,10 +34,12 @@ from coretrace_python.taint.templates import EscapedTemplates, escaped_templates __all__ = [ + "TEXT_KINDS", "AuthorizationGuard", "EntryPoint", "EntryPointAnalysis", "EscapedTemplates", + "Members", "Model", "ModelError", "ModelTable", diff --git a/src/coretrace_python/taint/engine.py b/src/coretrace_python/taint/engine.py index 21f9a45..22bedcf 100644 --- a/src/coretrace_python/taint/engine.py +++ b/src/coretrace_python/taint/engine.py @@ -70,7 +70,7 @@ Value, ) from coretrace_python.ir.ssa import SSAAnalysis -from coretrace_python.semantic.scopes import ScopeAnalysis, ScopeTable +from coretrace_python.semantic.scopes import ScopeAnalysis, ScopeId, ScopeTable from coretrace_python.semantic.symbols import SymbolAnalysis, SymbolId, SymbolTable from coretrace_python.source import SourceSpan from coretrace_python.taint.models import ( @@ -551,7 +551,12 @@ def report( reaching = taint.kinds & sink.kinds_at(position, keyword) if not reaching: return - for source in sorted(taint.sources, key=lambda s: str(s.symbol)): + # One flow per origin: the parameters of one entry point share its source, with + # the kinds each can carry (a scalar one no NOSQL); the flow's kinds are the taint's. + origins: dict[tuple[SymbolId, str], Source] = {} + for source in sorted(taint.sources, key=lambda s: (str(s.symbol), s.label, s.kinds.value)): + origins.setdefault((source.symbol, source.label), source) + for source in origins.values(): flows.append( TaintFlow( source, @@ -770,17 +775,22 @@ def parameter_sources( ) -> Mapping[int, Source]: """The attacker-controlled parameters of ``function``, by index: every parameter of an entry point (``self`` excepted for a method), including one registered elsewhere - (``routes``), and every parameter annotated with a typed-parameter symbol.""" + (``routes``), and every parameter annotated with a typed-parameter symbol. An entry + point's parameter annotated with scalars is text: FastAPI validates it before the + handler runs, and a scalar cannot hold a query operator.""" owner = _owner(module, function) sources: dict[int, Source] = {} + scope = scopes.scope_for(function) + enclosing = scope.parent if scope.parent is not None else scope.id entry = function_entry_point(function, module, models, scopes, symbols, instances, routes) if entry is not None: first = 1 if owner is not None else 0 for index in range(first, len(function.parameters)): - sources[index] = Source(entry.symbol, entry.label, entry.kinds) - scope = scopes.scope_for(function) - enclosing = scope.parent if scope.parent is not None else scope.id + annotation = function.parameters[index].annotation + scalar = annotation is not None and _scalar(annotation, symbols, enclosing) + kinds = entry.kinds & ~TaintKind.NOSQL if scalar else entry.kinds + sources[index] = Source(entry.symbol, entry.label, kinds) for index, parameter in enumerate(function.parameters): if parameter.annotation is None: continue @@ -800,6 +810,59 @@ def parameter_sources( return sources +def _typing(*names: str) -> frozenset[SymbolId]: + return frozenset(SymbolId(f"python.{module}.{name}") for module in ("typing", "typing_extensions") for name in names) + + +_SCALARS = frozenset( + SymbolId(f"python.{name}") + for name in ( + "builtins.str", + "builtins.int", + "builtins.float", + "builtins.bool", + "builtins.bytes", + "uuid.UUID", + "datetime.date", + "datetime.datetime", + "datetime.time", + "datetime.timedelta", + "decimal.Decimal", + ) +) +_CONTAINERS = frozenset( + SymbolId(f"python.builtins.{name}") for name in ("list", "set", "frozenset", "tuple") +) | _typing("List", "Set", "FrozenSet", "Tuple", "Sequence") +_UNIONS = _typing("Optional", "Union") +_LITERAL = _typing("Literal") +_ANNOTATED = _typing("Annotated") + + +def _scalar(annotation: nodes.Expression, symbols: SymbolTable, scope: ScopeId) -> bool: + """Whether a value annotated with ``annotation`` holds scalars only: a scalar class, + a literal, a container of scalars, a union of them with ``None``, or one of them + ``Annotated`` with metadata. Anything unknown may hold a structure.""" + + if isinstance(annotation, nodes.Constant): + return annotation.value is None or annotation.value is Ellipsis + if isinstance(annotation, nodes.BinaryOp): + return annotation.operator == "bit_or" and all( + _scalar(side, symbols, scope) for side in (annotation.left, annotation.right) + ) + if isinstance(annotation, nodes.Subscript): + origin = symbols.resolve_expression(scope, annotation.value) + key = annotation.key + arguments = key.elements if isinstance(key, nodes.Tuple) else (key,) + if origin in _LITERAL: + return True + if origin in _ANNOTATED: + return _scalar(arguments[0], symbols, scope) + return (origin in _UNIONS or origin in _CONTAINERS) and all( + _scalar(argument, symbols, scope) for argument in arguments + ) + return symbols.resolve_expression(scope, annotation) in _SCALARS + + def propagate_taint( name: str, function: FunctionIR, diff --git a/src/coretrace_python/taint/models.py b/src/coretrace_python/taint/models.py index 77c9a6a..afa2670 100644 --- a/src/coretrace_python/taint/models.py +++ b/src/coretrace_python/taint/models.py @@ -10,6 +10,7 @@ import hashlib import re +from collections.abc import Mapping from dataclasses import dataclass, field from enum import Flag, auto from types import MappingProxyType @@ -17,7 +18,7 @@ from coretrace_python.analysis import Analysis, AnalysisContext, MissingInputError from coretrace_python.interprocedural import Clearing -from coretrace_python.semantic.symbols import SymbolId +from coretrace_python.semantic.symbols import Members, SymbolId class TaintKind(Flag): @@ -41,6 +42,11 @@ class TaintKind(Flag): PII = auto() +# What text input carries: a string cannot hold a query operator, only a structure can, +# so a form field, a header or a URL parameter carries every kind but NOSQL. +TEXT_KINDS = TaintKind.ALL & ~TaintKind.NOSQL + + class ModelError(Exception): """Two models of the same kind claim the same symbol.""" @@ -221,6 +227,7 @@ class AuthorizationGuard: | SuffixSink | SafeArgument | TemplateRender + | Members ) @@ -238,6 +245,7 @@ class ModelTable: suffix_sinks: tuple[SuffixSink, ...] = () safe_arguments: tuple[SafeArgument, ...] = () template_renders: tuple[TemplateRender, ...] = () + members: tuple[Members, ...] = () _by_symbol: dict[type[Model], dict[SymbolId, Model]] = field( init=False, repr=False, compare=False ) @@ -321,6 +329,7 @@ def extended(self, *sinks: Sink) -> ModelTable: self.suffix_sinks, self.safe_arguments, self.template_renders, + self.members, ) def sanitizer(self, symbol: SymbolId) -> Sanitizer | None: @@ -335,6 +344,11 @@ def template_render(self, symbol: SymbolId) -> TemplateRender | None: found = self._by_symbol[TemplateRender].get(symbol) return found if isinstance(found, TemplateRender) else None + def members_by_class(self) -> Mapping[SymbolId, Members]: + """The ``Members`` models by class, as symbol resolution reads them.""" + + return MappingProxyType({m.symbol: m for m in self.members}) + def clearing(self, escaped: frozenset[str] = frozenset()) -> Clearing: """What calls clear from the data they return: every sanitizer its kinds, every template render ``HTML`` when it names one of the ``escaped`` templates.""" @@ -393,6 +407,7 @@ def freeze(self) -> ModelTable: suffix_sinks=tuple(m for m in models if isinstance(m, SuffixSink)), safe_arguments=tuple(m for m in models if isinstance(m, SafeArgument)), template_renders=tuple(m for m in models if isinstance(m, TemplateRender)), + members=tuple(m for m in models if isinstance(m, Members)), ) diff --git a/tests/test_analysis_manager.py b/tests/test_analysis_manager.py index 2366df8..e16705e 100644 --- a/tests/test_analysis_manager.py +++ b/tests/test_analysis_manager.py @@ -45,7 +45,7 @@ from coretrace_python.semantic import SEMANTIC_ANALYSES from coretrace_python.semantic.imports import ImportAnalysis from coretrace_python.semantic.scopes import ScopeAnalysis - from coretrace_python.semantic.symbols import SymbolAnalysis + from coretrace_python.semantic.symbols import MembersAnalysis, SymbolAnalysis except ImportError as error: # pragma: no cover - red until the analysis manager lands MISSING = error else: @@ -225,7 +225,8 @@ def compute(cls, ctx: AnalysisContext) -> int: def test_transitive_dependencies_are_exposed() -> None: engine = manager(*SEMANTIC_ANALYSES) - assert engine.dependencies(SymbolAnalysis) == frozenset({ScopeAnalysis, ImportAnalysis}) + # Symbol resolution also reads the Members models the engine provides. + assert engine.dependencies(SymbolAnalysis) == frozenset({ScopeAnalysis, ImportAnalysis, MembersAnalysis}) assert engine.dependencies(ScopeAnalysis) == frozenset() diff --git a/tests/test_detectors.py b/tests/test_detectors.py index dc370ee..74b0b8d 100644 --- a/tests/test_detectors.py +++ b/tests/test_detectors.py @@ -182,6 +182,7 @@ def test_shipped_plugins_load_with_their_manifests() -> None: "insecure-temp-file", "insecure-tls", "missing-timeout", + "nosql-injection", "open-redirect", "path-traversal", "plaintext-credentials", @@ -199,7 +200,7 @@ def test_shipped_plugins_load_with_their_manifests() -> None: "weak-crypto", "xss", } - for rule in ("sql-injection", "command-injection", "code-injection", "path-traversal", "ssrf", "xss"): + for rule in ("sql-injection", "command-injection", "code-injection", "nosql-injection", "path-traversal", "ssrf", "xss"): assert by_name[rule].requires == ("taint.flows", "findings.refutation") assert by_name[rule].provides == (f"vulnerability.{rule}",) assert by_name["python-stdlib-models"].provides == ("model.python-stdlib",) diff --git a/tests/test_django_http_clients.py b/tests/test_django_http_clients.py index 644a1c8..f766abc 100644 --- a/tests/test_django_http_clients.py +++ b/tests/test_django_http_clients.py @@ -295,6 +295,7 @@ def test_detectors_stay_generic() -> None: "code_injection", "command_injection", "insecure_deserialization", + "nosql_injection", "open_redirect", "path_traversal", "plaintext_credentials", diff --git a/tests/test_docs.py b/tests/test_docs.py index 25979ae..d2c299a 100644 --- a/tests/test_docs.py +++ b/tests/test_docs.py @@ -90,7 +90,7 @@ def test_every_bundled_plugin_is_named_in_the_guides() -> None: re.search(r'^name = "([a-z-]+)"', manifest.read_text(encoding="utf-8"), re.MULTILINE).group(1) # type: ignore[union-attr] for manifest in engine.BUNDLED_PLUGINS.rglob("plugin.toml") ] - assert len(names) == 36 + assert len(names) == 37 assert all(f"`{name}`" in text for name in names), [n for n in names if f"`{n}`" not in text] diff --git a/tests/test_frameworks.py b/tests/test_frameworks.py index a1afdac..019fa4d 100644 --- a/tests/test_frameworks.py +++ b/tests/test_frameworks.py @@ -3,8 +3,9 @@ Framework plugins enrich the security model; detectors stay generic. Three engine mechanisms make that possible: -- a module-level ``name = Symbol(...)`` binds ``name`` to that symbol, so ``app.route`` - resolves to ``python.flask.Flask.route`` when ``app = Flask(__name__)``; +- a module-level name denotes what its value denotes in a function body, so ``app.route`` + resolves to ``python.flask.Flask.route`` when ``app = Flask(__name__)``, and + ``cursor = conn.cursor()`` goes through the ``conn`` bound before it; - inside functions, call results and ``with`` contexts carry their callee's symbol, so ``conn.cursor().execute`` resolves to ``python.sqlite3.connect.cursor.execute``; - an ``EntryPoint`` model taints every parameter of a function decorated by its symbol, @@ -89,6 +90,50 @@ def test_module_level_instances_resolve_to_their_class_symbol() -> None: assert symbols.resolve(f.id, "limit") is None +def test_module_level_names_bound_through_an_instance_denote_what_a_function_derives() -> None: + manager = manager_for( + "import os\nimport sqlite3\nfrom django.db import connections\n\n" + "early = conn.cursor()\n" + "conn = sqlite3.connect('app.db')\n" + "cursor = conn.cursor()\n" + "same = conn\n" + "run = os.system\n" + "home = os.environ['HOME']\n" + "raw = connections['default'].cursor()\n" + "limit = 3\n\n" + "def f():\n return cursor\n" + ) + symbols = manager.get(SymbolAnalysis) + scopes = manager.get(ScopeAnalysis) + f = next(s for s in scopes.children(scopes.module_scope.id) if s.name == "f") + + assert {name: symbols.resolve(f.id, name) for name in ("cursor", "same", "run", "home", "raw")} == { + "cursor": SymbolId("python.sqlite3.connect.cursor"), + "same": SymbolId("python.sqlite3.connect"), + "run": SymbolId("python.os.system"), + # An item carries its container's symbol, as in a function body. + "home": SymbolId("python.os.environ"), + "raw": SymbolId("python.django.db.connections.cursor"), + } + # Names resolve in statement order: ``conn`` is not bound yet when ``early`` is. + assert symbols.resolve(f.id, "early") is None + assert symbols.resolve(f.id, "limit") is None + + +def test_a_query_through_a_module_level_cursor_is_an_injection() -> None: + findings = check( + "import sqlite3\nfrom flask import Flask, request\n\n" + "app = Flask(__name__)\n" + "conn = sqlite3.connect('app.db')\n" + "cursor = conn.cursor()\n\n" + "@app.route('/users')\n" + "def users():\n" + " return str(cursor.execute(\"SELECT * FROM users WHERE name = '\" + request.args['q'] + \"'\").fetchall())\n" + ) + + assert [(f.rule_id, f.span.start_line) for f in findings if f.rule_id == "sql-injection"] == [("sql-injection", 10)] + + def test_attributes_of_module_level_instances_lower_to_symbols() -> None: module = module_for("from flask import Flask\napp = Flask(__name__)\n\ndef f():\n return app.route\n") output = format_module(lower_module(module)) diff --git a/tests/test_members.py b/tests/test_members.py new file mode 100644 index 0000000..444cc37 --- /dev/null +++ b/tests/test_members.py @@ -0,0 +1,206 @@ +"""A ``Members`` model says what the attributes and items of a class's instances give. + +Some libraries give objects by names the project chooses: a pymongo client gives the +database it is dotted or indexed with, a database gives a collection. Symbols follow +attribute paths, so those names used to end up in the symbol (``Client.shop.users.find``) +or, for an item, to vanish into the container's symbol (``Client.find``), and no model +could list them. A ``Members`` model describes the instances of a class: + +- ``typed`` maps an attribute to the class of what it gives, called or not + (``get_database``, or a ``db`` property); +- ``dynamic``, when set, is the class of every item and of every attribute outside + ``defined`` and ``typed``. + +Every path to a collection then denotes the collection class, the one an annotated +parameter denotes, and a single sink on ``Collection.find`` covers them all. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +from coretrace_python import engine +from coretrace_python.analysis import AnalysisManager +from coretrace_python.frontend import build_hir +from coretrace_python.hir import nodes +from coretrace_python.interprocedural import CallGraphAnalysis, ExternalSymbol +from coretrace_python.semantic.symbols import SymbolId +from coretrace_python.source import SourceManager +from coretrace_python.taint import ( + Members, + SecurityModelRegistry, + Sink, + Source, + TaintAnalysis, + TaintKind, +) + +CLIENT = SymbolId("python.docstore.Client") +DATABASE = SymbolId("python.docstore.database.Database") +COLLECTION = SymbolId("python.docstore.collection.Collection") +FIND = COLLECTION.attribute("find") +MEMBERS = ( + Members(CLIENT, DATABASE, defined=("close", "list_database_names"), typed=(("get_database", DATABASE),)), + Members( + DATABASE, + COLLECTION, + defined=("command", "list_collection_names"), + typed=(("get_collection", COLLECTION), ("client", CLIENT)), + ), + # A collection gives its sub-collections by name. + Members(COLLECTION, COLLECTION, defined=("delete_many", "find", "find_one", "insert_one", "name")), + # Attributes typed without dynamic members, like Flask-PyMongo's ``db``. + Members(SymbolId("python.docstore.flask.Store"), typed=(("db", DATABASE),)), +) + + +def manager_for(source_text: str, *models: object) -> AnalysisManager: + registry = SecurityModelRegistry() + registry.register(*MEMBERS, *models) # type: ignore[arg-type] + return engine.build_manager(build_hir(SourceManager().add_source("app.py", source_text)), registry) + + +def targets(source_text: str, function: str) -> list[SymbolId | None]: + graph = manager_for(source_text).get(CallGraphAnalysis) + return [s.target.symbol if isinstance(s.target, ExternalSymbol) else None for s in graph.sites(function)] + + +APP = ( + "import docstore\nfrom docstore.flask import Store\n\n" + "def f(q):\n" + " client = docstore.Client()\n" + " store = Store()\n" + " return {call}\n" +) + + +@pytest.mark.parametrize( + "call, symbol", + [ + ("client.shop.users.find(q)", FIND), + ("client['shop']['users'].find(q)", FIND), + ("client.shop['users'].find(q)", FIND), + ("client['shop'].users.find(q)", FIND), + ("client.get_database('shop').get_collection('users').find(q)", FIND), + ("client.shop.users.archive.find(q)", FIND), + ("store.db.users.find(q)", FIND), + ("client.shop.command(q)", DATABASE.attribute("command")), + ("client.shop.client.close()", CLIENT.attribute("close")), + ("client.close()", CLIENT.attribute("close")), + # What a member method returns is no member: a document's ``str.find``. + ("client.shop.users.find_one(q)['name'].find(q)", COLLECTION.attribute("find_one").attribute("find")), + ], +) +def test_attributes_and_items_give_the_class_the_model_names(call: str, symbol: SymbolId) -> None: + assert targets(APP.format(call=call), "f")[-1] == symbol + + +def test_module_level_names_give_the_same_classes() -> None: + source = ( + "import docstore\n\n" + "client = docstore.Client()\n" + "db = client.shop\n" + "users = db['users']\n" + "orders = client.get_database('shop').orders\n\n" + "def f(q):\n" + " users.find(q)\n" + " orders.find_one(q)\n" + " db.users.delete_many(q)\n" + ) + + assert targets(source, "f") == [FIND, COLLECTION.attribute("find_one"), COLLECTION.attribute("delete_many")] + + +def test_annotated_parameters_and_inherited_attributes_give_the_same_classes() -> None: + source = ( + "import docstore\nfrom docstore.database import Database\n\n" + "def f(db: Database, q):\n" + " return db.users.find(q)\n\n" + "class Shop(docstore.Client):\n" + " def orders(self, q):\n" + " return self.shop.orders.find(q)\n" + ) + + assert targets(source, "f") == [FIND] + assert targets(source, "Shop.orders") == [FIND] + + +def test_one_sink_on_the_member_class_covers_every_path() -> None: + source = ( + "import docstore\n\n" + "client = docstore.Client()\n" + "db = client['shop']\n\n" + "def by_attribute():\n client.shop.users.find(input())\n\n" + "def by_item():\n db['users'].find(input())\n\n" + "def by_getter():\n client.get_database('shop').get_collection('users').find(input())\n" + ) + manager = manager_for( + source, Source(SymbolId("python.builtins.input"), "stdin"), Sink(FIND, TaintKind.NOSQL) + ) + + sinks = [ + (function.name, str(flow.sink.symbol)) + for function in manager.module.body + if isinstance(function, nodes.Function) + for flow in manager.get(TaintAnalysis, function).flows + ] + assert sinks == [("by_attribute", str(FIND)), ("by_item", str(FIND)), ("by_getter", str(FIND))] + + +def test_defined_names_are_kept_sorted_so_the_models_repr_is_stable() -> None: + members = Members(CLIENT, DATABASE, defined=frozenset({"list_database_names", "close"})) # type: ignore[arg-type] + + assert members.defined == ("close", "list_database_names") + assert members == Members(CLIENT, DATABASE, defined=("list_database_names", "close")) + + +PLUGIN_MANIFEST = ( + 'name = "docstore-models"\nversion = "1.0.0"\nplugin_api = ">=1,<2"\nrequires = []\n' + 'provides = ["model.docstore"]\n\n[entrypoint]\nmodule = "docstore_models"\nclass = "DocstoreModels"\n' +) +PLUGIN = ''' +from typing import ClassVar + +from coretrace_python.plugins import ModelPlugin +from coretrace_python.semantic.symbols import SymbolId +from coretrace_python.taint import Members, Model, Sink, TaintKind + +CLIENT = SymbolId("python.docstore.Client") +DATABASE = SymbolId("python.docstore.database.Database") +COLLECTION = SymbolId("python.docstore.collection.Collection") + + +class DocstoreModels(ModelPlugin): + name: ClassVar[str] = "docstore-models" + models: ClassVar[tuple[Model, ...]] = ( + Members(CLIENT, DATABASE), + Members(DATABASE, COLLECTION, defined=("command",)), + Members(COLLECTION, COLLECTION, defined=("find",)), + # Any rule shows the flow reaching the sink; SQL has one. + Sink(COLLECTION.attribute("find"), TaintKind.SQL), + ) +''' + + +@pytest.mark.parametrize("jobs", [1, 2]) +def test_a_plugin_models_members_in_a_project_analysis(tmp_path: Path, jobs: int) -> None: + plugin = tmp_path / "plugins" / "docstore_models" + plugin.mkdir(parents=True) + (plugin / "plugin.toml").write_text(PLUGIN_MANIFEST, encoding="utf-8") + (plugin / "docstore_models.py").write_text(PLUGIN, encoding="utf-8") + root = tmp_path / "project" + root.mkdir() + for name in ("app.py", "admin.py"): + (root / name).write_text( + "import docstore\n\ndb = docstore.Client()['shop']\n\ndef run():\n return db.users.find(input())\n", + encoding="utf-8", + ) + + analysis = engine.analyze_project(root, [engine.BUNDLED_PLUGINS, tmp_path / "plugins"], jobs=jobs) + + found = sorted( + (Path(str(f.span.source_id)).name, f.span.start_line) for f in analysis.findings if f.rule_id == "sql-injection" + ) + assert found == [("admin.py", 6), ("app.py", 6)] diff --git a/tests/test_nosql_injection.py b/tests/test_nosql_injection.py new file mode 100644 index 0000000..dd29831 --- /dev/null +++ b/tests/test_nosql_injection.py @@ -0,0 +1,206 @@ +"""NoSQL injection: attacker input that may hold a structure reaching a NOSQL sink. + +MongoDB operator injection needs a structure: a JSON body giving +``{"password": {"$ne": null}}`` makes ``find_one({"name": name, "password": password})`` +match any password. A string cannot hold an operator, so only input that may hold a +structure carries ``NOSQL``: JSON and the raw bodies a ``json.loads`` decodes, files, +WebSocket messages, standard input. Text input carries ``TEXT_KINDS``, every kind but +``NOSQL``: form fields, query parameters, headers, cookies, URL parameters, the command +line and the environment. An entry-point parameter annotated with a scalar type is +text too, since FastAPI validates it before the handler runs. + +The engine ships no NOSQL sink; these tests declare one, ``docstore.find(filter)``. + +Limits: a Django or aiohttp request object is tainted as a whole, so ``request.GET`` +carries ``NOSQL`` like ``request.body``; the keys of a dict literal are not followed, so +a string concatenated into ``$where`` is not reported. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +from coretrace_python import engine +from coretrace_python.findings import Finding, Severity +from coretrace_python.source import SourceManager +from coretrace_python.taint import TEXT_KINDS, TaintKind + +MANIFEST = ( + 'name = "docstore-sinks"\nversion = "1.0.0"\nplugin_api = ">=1,<2"\nrequires = []\n' + 'provides = ["model.docstore"]\n\n[entrypoint]\nmodule = "docstore_sinks"\nclass = "DocstoreSinks"\n' +) +PLUGIN = ''' +from typing import ClassVar + +from coretrace_python.plugins import ModelPlugin +from coretrace_python.semantic.symbols import SymbolId +from coretrace_python.taint import Model, Sink, TaintKind + + +class DocstoreSinks(ModelPlugin): + name: ClassVar[str] = "docstore-sinks" + models: ClassVar[tuple[Model, ...]] = ( + Sink(SymbolId("python.docstore.find"), TaintKind.NOSQL, ((TaintKind.NOSQL, (0,)),)), + ) +''' + + +@pytest.fixture(scope="module") +def plugins(tmp_path_factory: pytest.TempPathFactory) -> list[Path]: + root = tmp_path_factory.mktemp("plugins") + plugin = root / "docstore_sinks" + plugin.mkdir() + (plugin / "plugin.toml").write_text(MANIFEST, encoding="utf-8") + (plugin / "docstore_sinks.py").write_text(PLUGIN, encoding="utf-8") + return [engine.BUNDLED_PLUGINS, root] + + +def check(plugins: list[Path], text: str) -> tuple[Finding, ...]: + return engine.check(SourceManager().add_source("app.py", text), plugins) + + +def injected(plugins: list[Path], text: str) -> list[int]: + return [f.span.start_line for f in check(plugins, text) if f.rule_id == "nosql-injection"] + + +def test_text_kinds_are_every_kind_but_nosql() -> None: + assert TEXT_KINDS == TaintKind.ALL & ~TaintKind.NOSQL + + +FLASK = ( + "import json\n\nimport docstore\nfrom flask import Flask, request\n\n" + "app = Flask(__name__)\n\n" + "@app.route('/users/')\n" + "def users(name):\n" + " return str(docstore.find({query}))\n" +) + + +@pytest.mark.parametrize( + "query", + [ + '{"name": request.json["name"], "password": request.json["password"]}', + "request.get_json()", + "json.loads(request.data)", + "json.loads(request.get_data())", + "json.load(request.files['filter'])", + ], +) +def test_a_structure_from_the_request_in_a_filter_is_a_nosql_injection(plugins: list[Path], query: str) -> None: + findings = [f for f in check(plugins, FLASK.format(query=query)) if f.rule_id == "nosql-injection"] + + assert [f.span.start_line for f in findings] == [10] + assert findings[0].severity is Severity.HIGH + assert findings[0].metadata["source_label"] == "http" + + +@pytest.mark.parametrize( + "query", + [ + '{"name": request.form["name"]}', + '{"name": request.args.get("name")}', + '{"name": request.values["name"], "tag": request.cookies["tag"]}', + '{"agent": request.headers["User-Agent"], "path": request.path}', + '{"name": name}', + ], +) +def test_text_from_the_request_cannot_hold_an_operator(plugins: list[Path], query: str) -> None: + assert injected(plugins, FLASK.format(query=query)) == [] + + +def test_text_sources_still_carry_the_other_kinds(plugins: list[Path]) -> None: + text = ( + "import sqlite3\nfrom flask import Flask, request\n\napp = Flask(__name__)\n\n" + "@app.route('/users/')\n" + "def users(name):\n" + " sqlite3.connect('app.db').execute(\"SELECT * FROM users WHERE name = '\" + request.form['q'] + \"'\")\n" + " sqlite3.connect('app.db').execute(\"SELECT * FROM users WHERE name = '\" + name + \"'\")\n" + ) + + assert sorted(f.span.start_line for f in check(plugins, text) if f.rule_id == "sql-injection") == [8, 9] + + +FASTAPI = ( + "from typing import Annotated, Optional, Union\nfrom uuid import UUID\n\n" + "import docstore\nfrom fastapi import FastAPI, Query\nfrom pydantic import BaseModel\n\n" + "class Filter(BaseModel):\n name: str\n\n" + "app = FastAPI()\n\n" + "@app.get('/users')\n" + "def users({parameter}):\n" + " return docstore.find({{'name': q}})\n" +) + + +@pytest.mark.parametrize( + "parameter", + ["q: dict", "q", "q: Filter", "q: list[dict]", "q: Optional[dict] = None"], +) +def test_a_parameter_that_may_hold_a_structure_is_structured_input(plugins: list[Path], parameter: str) -> None: + assert injected(plugins, FASTAPI.format(parameter=parameter)) == [15] + + +@pytest.mark.parametrize( + "parameter", + [ + "q: str", + "q: int = 0", + "q: Annotated[str, Query()]", + "q: Optional[int] = None", + "q: str | None = None", + "q: Union[str, None] = None", + "q: Annotated[Optional[str], Query(max_length=20)] = None", + "q: UUID", + # A container of scalars holds no operator either. + "q: list[str] = Query()", + ], +) +def test_a_parameter_annotated_with_a_scalar_type_is_text(plugins: list[Path], parameter: str) -> None: + assert injected(plugins, FASTAPI.format(parameter=parameter)) == [] + + +@pytest.mark.parametrize( + "body, reported", + [ + (" return docstore.find(json.loads(sys.stdin.read()))\n", True), + (" return docstore.find({'name': input()})\n", False), + (" return docstore.find({'name': sys.argv[1]})\n", False), + (" return docstore.find({'name': os.environ['USER']})\n", False), + ], +) +def test_standard_input_is_a_payload_and_the_command_line_text(plugins: list[Path], body: str, reported: bool) -> None: + text = f"import json\nimport os\nimport sys\n\nimport docstore\n\ndef run():\n{body}" + + assert injected(plugins, text) == ([8] if reported else []) + + +DJANGO = ( + "import json\n\nimport docstore\nfrom django.http import HttpRequest, JsonResponse\n\n" + "def users(request: HttpRequest):\n" + " return JsonResponse(docstore.find({query}))\n" +) + + +@pytest.mark.parametrize( + "query", + [ + "json.loads(request.body)", + # Limit: the request object is tainted as a whole, its text fields too. + '{"name": request.GET["name"]}', + ], +) +def test_a_django_request_carries_nosql_as_a_whole(plugins: list[Path], query: str) -> None: + assert injected(plugins, DJANGO.format(query=query)) == [7] + + +def test_parameters_of_one_entry_point_remain_one_origin(plugins: list[Path]) -> None: + text = ( + "from fastapi import FastAPI\nfrom fastapi.responses import HTMLResponse\n\n" + "app = FastAPI()\n\n" + "@app.get('/users')\n" + "def users(q, n: int):\n" + " return HTMLResponse(q + str(n))\n" + ) + + assert [f.span.start_line for f in check(plugins, text) if f.rule_id == "xss"] == [8] diff --git a/tests/test_operator_sources.py b/tests/test_operator_sources.py index a9cad30..1fb0a58 100644 --- a/tests/test_operator_sources.py +++ b/tests/test_operator_sources.py @@ -22,7 +22,7 @@ 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 +from coretrace_python.taint import TEXT_KINDS, TaintKind REPO = Path(__file__).resolve().parent.parent PLUGINS = REPO / "src" / "coretrace_python" / "bundled" @@ -52,10 +52,12 @@ def rules(findings: tuple[Finding, ...]) -> list[str]: def test_source_kinds_reflect_who_controls_them() -> None: - assert source_kinds("os.environ") == TaintKind.ALL & ~(TaintKind.COMMAND | TaintKind.PATH) + # The environment, argv and a line of input are text, which holds no query operator + # (#158); process output is a payload. + assert source_kinds("os.environ") == TEXT_KINDS & ~(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 + assert source_kinds("sys.argv") == TEXT_KINDS & ~TaintKind.PATH + assert source_kinds("builtins.input") == TEXT_KINDS def test_environment_does_not_inject_commands_or_paths_but_still_reaches_urls_and_statements() -> None: diff --git a/tests/test_packaging.py b/tests/test_packaging.py index 10143ae..faecb0a 100644 --- a/tests/test_packaging.py +++ b/tests/test_packaging.py @@ -64,7 +64,7 @@ def test_bundled_plugins_live_inside_the_package() -> None: assert bundled == Path(coretrace_python.__file__).resolve().parent / "bundled" assert bundled.is_dir() manifests = sorted(p.relative_to(bundled).as_posix() for p in bundled.rglob("plugin.toml")) - assert len(manifests) == 36 + assert len(manifests) == 37 assert "syntax/dangerous_eval/plugin.toml" in manifests assert not (REPO / "plugins").exists() @@ -108,7 +108,7 @@ def test_the_wheel_carries_the_bundled_plugins(tmp_path: Path) -> None: assert "coretrace_python/bundled/syntax/dangerous_eval/plugin.toml" in names assert "coretrace_python/bundled/syntax/dangerous_eval/dangerous_eval.py" in names - assert sum(1 for n in names if n.endswith("/plugin.toml")) == 36 + assert sum(1 for n in names if n.endswith("/plugin.toml")) == 37 assert not any(n.startswith("tests/") or "tests-project" in n for n in names)