diff --git a/docs/usage.md b/docs/usage.md index cd50c84..12255d5 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -79,7 +79,7 @@ report's per-file detail tell "no findings" from "nothing analysed". Taint follows values between functions and across files through function summaries, 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 +closures. Flask, FastAPI, Django and aiohttp route handlers, class-based views and registered URL patterns receive HTTP input; click and Typer commands receive `argv` input; `input()`, `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 @@ -309,10 +309,10 @@ real 45 000-line project is exercised on every change. ## Plugins -The package ships 26 plugins, loaded by default. Security models for the standard library +The package ships 28 plugins, loaded by default. Security models for the standard library and the supported frameworks: `python-stdlib-models`, `flask-models`, `fastapi-models`, -`django-models`, `sqlalchemy-models`, `http-client-models`, `credential-models` and -`cli-models`. Taint detectors: `command-injection`, `sql-injection`, `path-traversal`, +`django-models`, `aiohttp-models`, `sqlalchemy-models`, `db-driver-models` (aiopg, +asyncpg, psycopg2, PyMySQL), `http-client-models`, `credential-models` and `cli-models`. Taint detectors: `command-injection`, `sql-injection`, `path-traversal`, `ssrf`, `xss`, `insecure-deserialization`, `open-redirect` and `plaintext-credentials`. Dangerous API detectors: `dangerous-eval`, `weak-crypto`, `flask-debug` and `missing-timeout`. Secret scanners: `hardcoded-secrets` for Python sources and diff --git a/src/coretrace_python/bundled/models/aiohttp/aiohttp_models.py b/src/coretrace_python/bundled/models/aiohttp/aiohttp_models.py new file mode 100644 index 0000000..466acc7 --- /dev/null +++ b/src/coretrace_python/bundled/models/aiohttp/aiohttp_models.py @@ -0,0 +1,61 @@ +"""aiohttp security models: handlers receive HTTP input whether they take an annotated +``web.Request``, are registered on the router or decorated by a ``RouteTableDef``; +responses, file responses and redirect exceptions are sinks; client sessions are SSRF +sinks whose responses are untrusted.""" + +from __future__ import annotations + +from typing import ClassVar + +from coretrace_python.plugins import ModelPlugin +from coretrace_python.semantic.symbols import SymbolId +from coretrace_python.taint import ( + EntryPoint, + Model, + RouteRegistrar, + Sink, + Source, + TaintKind, + TypedParameter, +) + +_METHODS = ("get", "post", "put", "patch", "delete", "head", "options") +_REQUEST_CLASSES = ("aiohttp.web.Request", "aiohttp.web_request.Request") +_ROUTERS = ("aiohttp.web.Application.router", "aiohttp.web.UrlDispatcher") +_REDIRECTS = ("HTTPFound", "HTTPMovedPermanently", "HTTPSeeOther", "HTTPTemporaryRedirect", "HTTPPermanentRedirect") +_CLIENT_FUNCTIONS = ( + *(f"aiohttp.ClientSession.{method}" for method in (*_METHODS, "request")), + *(f"aiohttp.client.ClientSession.{method}" for method in (*_METHODS, "request")), + "aiohttp.request", +) + + +def _sym(path: str) -> SymbolId: + return SymbolId(f"python.{path}") + + +class AiohttpModels(ModelPlugin): + name: ClassVar[str] = "aiohttp-models" + models: ClassVar[tuple[Model, ...]] = ( + # The whole request is attacker-controlled: its query, match info, headers, + # cookies and body all come from the client. + *(TypedParameter(_sym(cls), "http") for cls in _REQUEST_CLASSES), + # ``@routes.get('/')`` on a ``RouteTableDef``. + *(EntryPoint(_sym(f"aiohttp.web.RouteTableDef.{method}"), "http") for method in (*_METHODS, "route", "view")), + # ``app.router.add_route('GET', '/', handler)`` and ``app.router.add_get('/', handler)``. + *(RouteRegistrar(_sym(f"{router}.add_route"), 2, "http", keyword="handler") for router in _ROUTERS), + *( + RouteRegistrar(_sym(f"{router}.add_{method}"), 1, "http", keyword="handler") + for router in _ROUTERS + for method in (*_METHODS, "view") + ), + # ``app.add_routes([web.get('/', handler)])``: the route definitions name the handler. + RouteRegistrar(_sym("aiohttp.web.route"), 2, "http", keyword="handler"), + *(RouteRegistrar(_sym(f"aiohttp.web.{method}"), 1, "http", keyword="handler") for method in (*_METHODS, "view")), + Sink(_sym("aiohttp.web.Response"), TaintKind.HTML), + Sink(_sym("aiohttp.web.FileResponse"), TaintKind.PATH), + *(Sink(_sym(f"aiohttp.web.{exception}"), TaintKind.REDIRECT) for exception in _REDIRECTS), + *(Sink(_sym(f"aiohttp.web_exceptions.{exception}"), TaintKind.REDIRECT) for exception in _REDIRECTS), + *(Sink(_sym(function), TaintKind.SSRF) for function in _CLIENT_FUNCTIONS), + *(Source(_sym(function), "http-response") for function in _CLIENT_FUNCTIONS), + ) diff --git a/src/coretrace_python/bundled/models/aiohttp/plugin.toml b/src/coretrace_python/bundled/models/aiohttp/plugin.toml new file mode 100644 index 0000000..95ab8cf --- /dev/null +++ b/src/coretrace_python/bundled/models/aiohttp/plugin.toml @@ -0,0 +1,9 @@ +name = "aiohttp-models" +version = "1.0.0" +plugin_api = ">=1,<2" +requires = [] +provides = ["model.http-sources", "model.aiohttp-routes", "model.ssrf-sinks"] + +[entrypoint] +module = "aiohttp_models" +class = "AiohttpModels" diff --git a/src/coretrace_python/bundled/models/db_drivers/db_driver_models.py b/src/coretrace_python/bundled/models/db_drivers/db_driver_models.py new file mode 100644 index 0000000..dd79aa0 --- /dev/null +++ b/src/coretrace_python/bundled/models/db_drivers/db_driver_models.py @@ -0,0 +1,71 @@ +"""Database driver models: the statement argument of aiopg, asyncpg, psycopg2 and PyMySQL +query methods is a SQL sink; the parameter tuple or mapping is not a statement.""" + +from __future__ import annotations + +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 + +_STATEMENT_ONLY = ((TaintKind.SQL, (0,)),) +_CURSOR_METHODS = ("execute", "executemany", "mogrify") +_ASYNCPG_METHODS = ("execute", "executemany", "fetch", "fetchrow", "fetchval", "cursor") + +# Every way a cursor is reached: through a connection object, an annotated class or a +# pool; the symbol derivation follows call chains, so ``connect().cursor().execute`` is +# ``connect.cursor.execute``. +_CURSORS = ( + "aiopg.connect.cursor", + "aiopg.Connection.cursor", + "aiopg.connection.Connection.cursor", + "aiopg.Cursor", + "aiopg.cursor.Cursor", + "aiopg.create_pool.acquire.cursor", + "aiopg.Pool.acquire.cursor", + "aiopg.pool.Pool.acquire.cursor", + "aiopg.create_pool.cursor", + "aiopg.Pool.cursor", + "psycopg2.connect.cursor", + "psycopg2.extensions.connection.cursor", + "psycopg2.extensions.cursor", + "psycopg2.extras.DictCursor", + "psycopg2.extras.RealDictCursor", + "pymysql.connect.cursor", + "pymysql.connections.Connection.cursor", + "pymysql.Connection.cursor", + "pymysql.cursors.Cursor", + "pymysql.cursors.DictCursor", +) +_ASYNCPG_CONNECTIONS = ( + "asyncpg.connect", + "asyncpg.Connection", + "asyncpg.connection.Connection", + "asyncpg.create_pool", + "asyncpg.Pool", + "asyncpg.pool.Pool", + "asyncpg.create_pool.acquire", + "asyncpg.Pool.acquire", + "asyncpg.pool.Pool.acquire", +) + + +def _sym(path: str) -> SymbolId: + return SymbolId(f"python.{path}") + + +class DbDriverModels(ModelPlugin): + name: ClassVar[str] = "db-driver-models" + models: ClassVar[tuple[Model, ...]] = ( + *( + Sink(_sym(f"{cursor}.{method}"), TaintKind.SQL, _STATEMENT_ONLY) + for cursor in _CURSORS + for method in _CURSOR_METHODS + ), + *( + Sink(_sym(f"{connection}.{method}"), TaintKind.SQL, _STATEMENT_ONLY) + for connection in _ASYNCPG_CONNECTIONS + for method in _ASYNCPG_METHODS + ), + ) diff --git a/src/coretrace_python/bundled/models/db_drivers/plugin.toml b/src/coretrace_python/bundled/models/db_drivers/plugin.toml new file mode 100644 index 0000000..c325d8a --- /dev/null +++ b/src/coretrace_python/bundled/models/db_drivers/plugin.toml @@ -0,0 +1,9 @@ +name = "db-driver-models" +version = "1.0.0" +plugin_api = ">=1,<2" +requires = [] +provides = ["model.sql-sinks"] + +[entrypoint] +module = "db_driver_models" +class = "DbDriverModels" diff --git a/src/coretrace_python/interprocedural/callgraph.py b/src/coretrace_python/interprocedural/callgraph.py index 7573ba9..8a56880 100644 --- a/src/coretrace_python/interprocedural/callgraph.py +++ b/src/coretrace_python/interprocedural/callgraph.py @@ -18,6 +18,7 @@ from coretrace_python.hir import nodes from coretrace_python.ir.lowering import LoweringError, analyzable_functions, qualified_name from coretrace_python.ir.model import ( + Await, Call, FunctionIR, GetAttr, @@ -139,6 +140,9 @@ def derive_symbols( instruction.callee if isinstance(instruction, Call) else instruction.context ) symbol = symbols.get(origin) + elif isinstance(instruction, Await) and instruction.value in symbols: + # ``await asyncpg.connect()`` denotes what the awaited call denotes. + symbol = symbols[instruction.value] if symbol is not None: symbols[instruction.result] = symbol changed = True diff --git a/src/coretrace_python/taint/routes.py b/src/coretrace_python/taint/routes.py index e85b167..6e208c8 100644 --- a/src/coretrace_python/taint/routes.py +++ b/src/coretrace_python/taint/routes.py @@ -45,9 +45,8 @@ def registered_routes( s.name for s in module.body if isinstance(s, nodes.Function | nodes.Class) } found: dict[SymbolId, EntryPoint] = {} - for call in _calls(module.body): - callee = symbols.resolve_expression(scope, call.callee) - registrar = models.route_registrar(callee) if callee is not None else None + for call, callee in _registration_calls(module, scopes, symbols): + registrar = models.route_registrar(callee) if registrar is None: continue handler: nodes.Expression | None = None @@ -83,6 +82,58 @@ def _handler_symbol( return None +def _registration_calls( + module: nodes.Module, scopes: ScopeTable, symbols: SymbolTable +) -> Iterator[tuple[nodes.Call, SymbolId]]: + """Every call of the module with a resolvable callee: at module level, and inside + functions such as ``def setup_routes(app: Application)``, where a call on an + annotated parameter denotes the class's method (``app.router.add_route``).""" + + module_scope = scopes.module_scope.id + for call in _calls(module.body): + callee = symbols.resolve_expression(module_scope, call.callee) + if callee is not None: + yield call, callee + for function in _functions(module.body): + scope = scopes.scope_for(function) + enclosing = scope.parent if scope.parent is not None else scope.id + annotated: dict[str, SymbolId] = {} + for parameter in function.parameters: + if parameter.annotation is not None: + symbol = symbols.resolve_expression(enclosing, parameter.annotation) + if symbol is not None: + annotated[parameter.name] = symbol + for call in _calls(function.body): + callee = symbols.resolve_expression(scope.id, call.callee) + if callee is None: + callee = _through_annotation(call.callee, annotated) + if callee is not None: + yield call, callee + + +def _through_annotation(expression: nodes.Expression, annotated: Mapping[str, SymbolId]) -> SymbolId | None: + """``app.router.add_route`` with ``app: Application`` is ``Application.router.add_route``.""" + + names: list[str] = [] + while isinstance(expression, nodes.Attribute): + names.append(expression.name) + expression = expression.value + if not isinstance(expression, nodes.Name) or expression.identifier not in annotated: + return None + symbol = annotated[expression.identifier] + for name in reversed(names): + symbol = symbol.attribute(name) + return symbol + + +def _functions(body: tuple[nodes.Statement, ...]) -> Iterator[nodes.Function]: + for statement in body: + if isinstance(statement, nodes.Function): + yield statement + elif isinstance(statement, nodes.Class): + yield from (member for member in statement.body if isinstance(member, nodes.Function)) + + def _calls(body: tuple[nodes.Statement, ...]) -> Iterator[nodes.Call]: for statement in body: if isinstance(statement, nodes.Function | nodes.Class): diff --git a/tests/regression/expected/dvpwa.json b/tests/regression/expected/dvpwa.json index 20d9bb9..3b1fd77 100644 --- a/tests/regression/expected/dvpwa.json +++ b/tests/regression/expected/dvpwa.json @@ -29,6 +29,12 @@ "line": 41, "rule": "weak-crypto", "function": "check_password" + }, + { + "file": "sqli/views.py", + "line": 57, + "rule": "sql-injection", + "function": "students" } ] } diff --git a/tests/test_aiohttp_models.py b/tests/test_aiohttp_models.py new file mode 100644 index 0000000..15bd613 --- /dev/null +++ b/tests/test_aiohttp_models.py @@ -0,0 +1,154 @@ +"""Acceptance tests for the aiohttp and database-driver models (issue #70). + +dvpwa, the intentionally vulnerable aiohttp application of the regression corpus, was +silent: no model gave its handlers HTTP input nor its aiopg cursors a SQL sink. The +``aiohttp-models`` plugin makes ``web.Request`` parameters, registered handlers and +``RouteTableDef`` decorated functions HTTP entry points, ``web.Response``, +``FileResponse`` and the redirect exceptions sinks, and ``ClientSession`` requests SSRF +sinks whose responses are untrusted. The ``db-driver-models`` plugin gives aiopg, +asyncpg, psycopg2 and PyMySQL cursors SQL sinks on the statement argument only. + +Expected to remain red until both plugins exist. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +from coretrace_python import engine +from coretrace_python.findings import Finding +from coretrace_python.source import SourceManager + +REPO = Path(__file__).resolve().parent.parent +PLUGINS = REPO / "src" / "coretrace_python" / "bundled" +MISSING = [ + name + for name in ("aiohttp", "db_drivers") + if not (PLUGINS / "models" / name / "plugin.toml").is_file() +] or None + + +@pytest.fixture(autouse=True) +def require_models() -> None: + if MISSING is not None: + pytest.fail(f"model plugins are not written yet: {MISSING}") + + +def check(text: str) -> tuple[Finding, ...]: + return engine.check(SourceManager().add_source("views.py", text), [PLUGINS]) + + +def rules(findings: tuple[Finding, ...]) -> list[str]: + return sorted(f.rule_id for f in findings) + + +DAO = ( + "from aiopg.connection import Connection\n\n" + "async def create(conn: Connection, name):\n" + " q = \"INSERT INTO students (name) VALUES ('%(name)s')\" % {'name': name}\n" + " async with conn.cursor() as cur:\n" + " await cur.execute(q)\n\n" +) + + +def test_the_plugins_load_with_their_names() -> None: + manager = engine.build_manager(engine.build_hir(SourceManager().add_source("e.py", ""))) + names = {loaded.manifest.name for loaded in engine.load_plugins([PLUGINS], manager)} + + assert {"aiohttp-models", "db-driver-models"} <= names + + +def test_an_annotated_request_reaches_an_aiopg_statement() -> None: + text = DAO + ( + "from aiohttp.web import Request\n\n" + "async def students(request: Request):\n" + " data = await request.post()\n" + " await create(request.app['db'], data['name'])\n" + ) + assert rules(check(text)) == ["sql-injection"] + + +def test_parameterised_aiopg_queries_are_safe() -> None: + text = ( + "from aiopg.connection import Connection\nfrom aiohttp.web import Request\n\n" + "async def get(conn: Connection, request: Request):\n" + " async with conn.cursor() as cur:\n" + " await cur.execute('SELECT id FROM students WHERE id = %s', (request.match_info['id'],))\n" + ) + assert check(text) == () + + +def test_handlers_registered_on_the_router_receive_http_input() -> None: + text = ( + "import os\nfrom aiohttp.web import Application\n\n" + "async def run(request):\n os.system(request.query['cmd'])\n\n" + "def setup_routes(app: Application):\n app.router.add_route('GET', '/run', run)\n" + ) + assert rules(check(text)) == ["command-injection"] + text = ( + "import os\nfrom aiohttp import web\n\n" + "async def run(request):\n os.system(request.query['cmd'])\n\n" + "app = web.Application()\napp.add_routes([web.get('/run', run)])\n" + ) + assert rules(check(text)) == ["command-injection"] + + +def test_route_table_decorators_are_entry_points() -> None: + text = ( + "import os\nfrom aiohttp import web\n\nroutes = web.RouteTableDef()\n\n" + "@routes.post('/run')\nasync def run(request):\n os.system(request.query['cmd'])\n" + ) + assert rules(check(text)) == ["command-injection"] + + +def test_unregistered_functions_are_not_entry_points() -> None: + text = "import os\n\nasync def helper(request):\n os.system(request.query['cmd'])\n" + assert check(text) == () + + +def test_responses_files_and_redirects_are_sinks() -> None: + text = ( + "from aiohttp import web\n\n" + "async def page(request: web.Request):\n" + " return web.Response(text=request.query['name'], content_type='text/html')\n" + ) + assert rules(check(text)) == ["xss"] + text = "from aiohttp import web\n\nasync def download(request: web.Request):\n return web.FileResponse(request.query['path'])\n" + assert rules(check(text)) == ["path-traversal"] + text = "from aiohttp import web\n\nasync def go(request: web.Request):\n raise web.HTTPFound(request.query['next'])\n" + assert rules(check(text)) == ["open-redirect"] + text = "import html\nfrom aiohttp import web\n\nasync def page(request: web.Request):\n return web.Response(text=html.escape(request.query['name']))\n" + assert check(text) == () + + +def test_client_sessions_are_ssrf_sinks_with_untrusted_responses() -> None: + text = ( + "import os\nimport aiohttp\nfrom aiohttp import web\n\n" + "async def fetch(request: web.Request):\n" + " async with aiohttp.ClientSession() as session:\n" + " await session.get(request.query['url'])\n" + " async with session.get('https://example.com/feed') as response:\n" + " os.system(await response.text())\n" + ) + findings = check(text) + assert rules(findings) == ["command-injection", "ssrf"] + assert {f.metadata["source_label"] for f in findings} == {"http", "http-response"} + + +def test_other_database_drivers_have_statement_sinks() -> None: + for module, connect in (("psycopg2", "psycopg2.connect(dsn)"), ("pymysql", "pymysql.connect()")): + text = ( + f"import {module}\nfrom aiohttp.web import Request\n\n" + f"def run(request: Request, dsn):\n cur = {connect}.cursor()\n" + " cur.execute('SELECT * FROM t WHERE a = ' + request.query['a'])\n" + " cur.execute('SELECT * FROM t WHERE a = %s', (request.query['a'],))\n" + ) + assert rules(check(text)) == ["sql-injection"], module + text = ( + "import asyncpg\nfrom aiohttp.web import Request\n\n" + "async def run(request: Request):\n conn = await asyncpg.connect()\n" + " await conn.fetch('SELECT * FROM t WHERE a = ' + request.query['a'])\n" + ) + assert rules(check(text)) == ["sql-injection"] diff --git a/tests/test_detectors.py b/tests/test_detectors.py index 3ffd836..baaae6e 100644 --- a/tests/test_detectors.py +++ b/tests/test_detectors.py @@ -162,11 +162,13 @@ def test_shipped_plugins_load_with_their_manifests() -> None: by_name = {plugin.manifest.name: plugin.manifest for plugin in loaded} assert set(by_name) == { + "aiohttp-models", "cli-models", "command-injection", "config-secrets", "credential-models", "dangerous-eval", + "db-driver-models", "dependency-policy", "django-models", "fastapi-models", diff --git a/tests/test_docs.py b/tests/test_docs.py index 9a91e4d..6a67c88 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) == 26 + assert len(names) == 28 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_packaging.py b/tests/test_packaging.py index b9743aa..527f973 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) == 26 + assert len(manifests) == 28 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")) == 26 + assert sum(1 for n in names if n.endswith("/plugin.toml")) == 28 assert not any(n.startswith("tests/") or "tests-project" in n for n in names)