diff --git a/.github/codeql/codeql-config.yml b/.github/codeql/codeql-config.yml new file mode 100644 index 00000000..73ac878f --- /dev/null +++ b/.github/codeql/codeql-config.yml @@ -0,0 +1,14 @@ +# Python analysis only - see the matrix in .github/workflows/codeql.yml. +# The other languages run with the default configuration. +name: "diffusers-workflow (python)" + +# The default suite, plus the local pack that models dw/security.py's +# validators. Both queries are the same analysis; the built-in one is +# excluded below so a sanitized path is not reported by it after the +# modeled query has cleared it. +queries: + - uses: ./.github/codeql/dw-security + +query-filters: + - exclude: + id: py/path-injection diff --git a/.github/codeql/dw-security/DwPathInjection.ql b/.github/codeql/dw-security/DwPathInjection.ql new file mode 100644 index 00000000..5a157a31 --- /dev/null +++ b/.github/codeql/dw-security/DwPathInjection.ql @@ -0,0 +1,31 @@ +/** + * @name Uncontrolled data used in path expression + * @description Accessing paths influenced by users can allow an attacker to + * access unexpected resources. + * @kind path-problem + * @problem.severity error + * @security-severity 7.5 + * @sub-severity high + * @precision high + * @id dw/path-injection + * @tags correctness + * security + * external/cwe/cwe-022 + * external/cwe/cwe-023 + * external/cwe/cwe-036 + * external/cwe/cwe-073 + * external/cwe/cwe-099 + */ + +// The standard py/path-injection query, with dw/security.py's validators +// modeled as sanitizers. The built-in one is filtered out in +// .github/codeql/codeql-config.yml so the two do not both report. +import python +import semmle.python.security.dataflow.PathInjectionQuery +import DwPathSanitizers +import PathInjectionFlow::PathGraph + +from PathInjectionFlow::PathNode source, PathInjectionFlow::PathNode sink +where PathInjectionFlow::flowPath(source, sink) +select sink.getNode(), source, sink, "This path depends on a $@.", source.getNode(), + "user-provided value" diff --git a/.github/codeql/dw-security/DwPathSanitizers.qll b/.github/codeql/dw-security/DwPathSanitizers.qll new file mode 100644 index 00000000..c15df645 --- /dev/null +++ b/.github/codeql/dw-security/DwPathSanitizers.qll @@ -0,0 +1,139 @@ +/** + * Models `dw/security.py`'s validators for the path-injection query. + * + * Every filesystem access in this project reaches the disk through + * `validate_path` (or a thin wrapper around it), which resolves the path + * with `os.path.realpath` and then raises unless the result is the base + * directory or a descendant of it; a name that is joined onto a library + * root goes through one of the reference validators, a full-match regex + * whose first character class precludes `..`, a leading separator and a + * null byte. That is exactly the normalize-then-check shape the standard + * `py/path-injection` query looks for - but it only recognizes the check as + * a *local* barrier guard, so a validator living in another module, + * returning the safe value rather than guarding a branch, is invisible to + * it. Without this file the query flags every route that touches a file, + * which buries the ones that skipped a validator. + * + * The trade this makes is that `dw/security.py` is now trusted by the + * query rather than checked by it: a bug in a validator would not be + * reported here. That file is the security layer and is reviewed as such - + * see tests/test_security.py, which is where a validator's containment is + * actually established. + * + * The model deliberately distinguishes the two ways `validate_path` is + * called: + * + * - with a base directory, it is a containment check and a full barrier + * - with `None` (or nothing) for the base, it only normalizes, so it is + * modeled as a `PathNormalization` and the path stays reportable until + * something checks it + * + * so a call that forgets the base directory is still a finding. + */ + +private import python +private import semmle.python.Concepts +private import semmle.python.dataflow.new.DataFlow +private import semmle.python.security.dataflow.PathInjectionCustomizations + +/** + * Holds if `name` is a `dw.security` function that returns a path confined + * to a base directory it was given. + */ +private predicate pathValidatorName(string name) { + name = + [ + "validate_path", "validate_workflow_path", "validate_output_path", + "validate_prompt_path", "safe_join_path" + ] +} + +/** + * Holds if `name` is a `dw.security` function that returns a name it has + * matched against a pattern admitting no traversal, for joining onto a + * library root. + */ +private predicate nameValidatorName(string name) { + name = + [ + "validate_workspace_name", "validate_prompt_reference", "validate_asset_reference", + "validate_output_reference", "validate_variable_name", "validate_commit_hash" + ] +} + +/** + * Gets the name of the function `call` invokes, for the flat and the + * qualified spelling. Matched by name rather than by resolved definition + * because the package imports these relatively (`from .security import + * validate_path`), which API graphs do not track, and no other function in + * the project carries one of these names. + */ +private string calledName(DataFlow::CallCfgNode call) { + result = call.getFunction().asExpr().(Name).getId() or + result = call.getFunction().asExpr().(Attribute).getName() +} + +/** A call to one of the validators. */ +private class ValidatorCall extends DataFlow::CallCfgNode { + ValidatorCall() { + pathValidatorName(calledName(this)) or + nameValidatorName(calledName(this)) + } + + /** Gets the value this call validates. */ + DataFlow::Node getPathArg() { result = this.getArg(0) } + + /** + * Holds if this call returns a value that cannot leave the directory it + * will be resolved against. A name validator always does. A path + * validator does when it was given a base directory to confine the path + * to - `safe_join_path` joins its parts under the first one, so it + * always confines; the others take the base as their second argument, + * and passing `None` there asks for normalization only. + */ + predicate confines() { + nameValidatorName(calledName(this)) + or + calledName(this) = "safe_join_path" + or + exists(DataFlow::Node base | + base = + [ + this.getArg(1), + this.getArgByName(["base_dir", "workflow_dir", "prompt_dir", "output_dir"]) + ] + | + not base.asExpr() instanceof None + ) + } +} + +/** A value that has been validated and confined. */ +private class ConfinedValue extends PathInjection::Sanitizer { + ConfinedValue() { this.(ValidatorCall).confines() } +} + +/** A path validator call given no base directory: normalization only. */ +private class UnconfinedPath extends Path::PathNormalization::Range { + UnconfinedPath() { this instanceof ValidatorCall and not this.(ValidatorCall).confines() } + + override DataFlow::Node getPathArg() { result = this.(ValidatorCall).getPathArg() } +} + +/** + * A parameter of a validator, so the query does not report the validator's + * own `os.path.realpath` / `os.path.exists` of the value it is in the + * middle of checking. This is the "trusted rather than checked" half of + * the trade described at the top of this file, and it is scoped to the + * definitions in `dw/security.py`. + */ +private class ValidatorParameter extends PathInjection::Sanitizer { + ValidatorParameter() { + exists(Function validator | + pathValidatorName(validator.getName()) or nameValidatorName(validator.getName()) + | + validator.getLocation().getFile().getRelativePath() = "dw/security.py" and + this.asExpr() = validator.getArg(_) + ) + } +} diff --git a/.github/codeql/dw-security/codeql-pack.lock.yml b/.github/codeql/dw-security/codeql-pack.lock.yml new file mode 100644 index 00000000..b31e5328 --- /dev/null +++ b/.github/codeql/dw-security/codeql-pack.lock.yml @@ -0,0 +1,30 @@ +--- +lockVersion: 1.0.0 +dependencies: + codeql/concepts: + version: 0.0.30 + codeql/controlflow: + version: 2.0.40 + codeql/dataflow: + version: 2.1.12 + codeql/mad: + version: 1.0.56 + codeql/python-all: + version: 7.2.4 + codeql/regex: + version: 1.0.56 + codeql/ssa: + version: 2.0.32 + codeql/threat-models: + version: 1.0.56 + codeql/tutorial: + version: 1.0.56 + codeql/typetracking: + version: 2.0.40 + codeql/util: + version: 2.0.43 + codeql/xml: + version: 1.0.56 + codeql/yaml: + version: 1.0.56 +compiled: false diff --git a/.github/codeql/dw-security/qlpack.yml b/.github/codeql/dw-security/qlpack.yml new file mode 100644 index 00000000..1ad4837a --- /dev/null +++ b/.github/codeql/dw-security/qlpack.yml @@ -0,0 +1,8 @@ +# A one-query pack whose only job is to teach CodeQL about the path +# validators in dw/security.py. See DwPathSanitizers.qll for why the +# built-in py/path-injection cannot recognize them on its own. +name: dw/security-queries +version: 0.0.1 +library: false +dependencies: + codeql/python-all: "*" diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml new file mode 100644 index 00000000..59c7afb9 --- /dev/null +++ b/.github/workflows/codeql.yml @@ -0,0 +1,59 @@ +# Advanced setup, replacing the repository's CodeQL default setup. The +# reason it is a workflow rather than default setup is the local query pack +# in .github/codeql/dw-security, which teaches the path-injection query +# about dw/security.py's validators - default setup cannot load a pack. +# +# The language list and the weekly schedule match what default setup ran, +# so nothing that was being scanned stopped being scanned. +name: CodeQL + +on: + push: + branches: [master] + pull_request: + schedule: + # Weekly, as default setup ran it + - cron: '23 4 * * 1' + workflow_dispatch: + +permissions: + contents: read + +jobs: + analyze: + name: Analyze (${{ matrix.language }}) + runs-on: ubuntu-latest + timeout-minutes: 60 + permissions: + security-events: write + # Both needed by the action to read workflow runs for the actions analysis + actions: read + contents: read + strategy: + fail-fast: false + matrix: + include: + - language: actions + build-mode: none + - language: javascript-typescript + build-mode: none + # Only python gets a config file, because the pack it adds is a + # python pack and would not compile against another language's + # database + - language: python + build-mode: none + config-file: .github/codeql/codeql-config.yml + steps: + - uses: actions/checkout@v7 + + - name: Initialize CodeQL + uses: github/codeql-action/init@v4 + with: + languages: ${{ matrix.language }} + build-mode: ${{ matrix.build-mode }} + config-file: ${{ matrix.config-file }} + + - name: Analyze + uses: github/codeql-action/analyze@v4 + with: + category: "/language:${{ matrix.language }}" diff --git a/CLAUDE.md b/CLAUDE.md index 1c79c906..c66480fb 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -191,6 +191,19 @@ All entry points use `dw/security.py`. When adding features: - **Never** use `eval()`, `exec()`, or `shell=True` - Path traversal (`../`) is blocked +CodeQL knows about these validators, which is why the scan is quiet: the local +query pack in `.github/codeql/dw-security/` models them as sanitizers for +`py/path-injection`, because the built-in query recognizes a +normalize-then-check only as a local barrier guard and so cannot see one that +lives in another module and returns the safe value. This is what makes code +scanning useful here rather than 26 identical false positives - but it only +holds while new filesystem access goes through a validator. Reaching the disk +some other way is a real alert, so treat one as a finding rather than as more +of the old noise. `validate_path(path, base)` is modeled as a barrier only when +`base` is not `None`; with `None` it is normalization only, and the path stays +reportable. Scanning is advanced setup (`.github/workflows/codeql.yml`) for the +same reason - default setup cannot load a pack. + ## Critical Gotchas - **Schema validation runs before variable substitution** — variable defaults must match expected JSON types (use `25` not `"25"` for numbers) diff --git a/dw/server/app.py b/dw/server/app.py index 89f45c7e..80856947 100644 --- a/dw/server/app.py +++ b/dw/server/app.py @@ -345,9 +345,19 @@ def resolve_workflow_reference(workflow_path, sources): if path is not None: return path, source candidate = os.path.abspath(workflow_path) - source = source_for_path(sources, candidate) if os.path.isfile(candidate) else None + source = source_for_path(sources, candidate) if source is not None: - return candidate, source + # The containment check re-applied to the path this returns, rather + # than trusted from source_for_path's answer about it - and applied + # before anything asks the filesystem about the path, so a + # workflow_path outside every source cannot be used to find out + # whether a file exists there + try: + confined = validate_path(candidate, source.root, allow_create=False) + except SecurityError: + confined = None + if confined is not None and os.path.isfile(confined): + return confined, source raise HTTPException( status_code=400, detail=f"workflow_path must name a workflow the server can reach: " @@ -1139,11 +1149,20 @@ def validate_workflow( ) try: errors = candidate.validation_errors() - except Exception as e: + except Exception: + # An error here is not the schema's verdict on the workflow - + # validation_errors() reports that by returning it. It is the + # validator itself failing, and its message could carry + # internals, so the log keeps the detail and the client is told + # the category, as above + logger.exception("Workflow could not be validated") + detail = ( + "The workflow could not be validated - the server log has the detail" + ) return { "valid": False, - "error": f"Validation error: {e}", - "errors": [{"path": None, "message": str(e)}], + "error": detail, + "errors": [{"path": None, "message": detail}], "warnings": [], } if errors: diff --git a/tests/test_server.py b/tests/test_server.py index 919bfd3e..5014aba0 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -3,6 +3,7 @@ import asyncio import json +import logging import os import queue import time @@ -552,7 +553,10 @@ def test_validate_endpoint_lists_every_schema_error(server): assert result["valid"] is True and result["errors"] == [] -def test_validate_endpoint_reports_non_schema_exception(server, monkeypatch): +def test_validate_endpoint_reports_non_schema_exception(server, monkeypatch, caplog): + """A validator that fails, rather than reporting a verdict, is still an + invalid answer to the client - but the exception's own message stays in + the log, since it is internal detail and not the schema's complaint.""" import dw.workflow def raise_boom(self): @@ -561,12 +565,16 @@ def raise_boom(self): monkeypatch.setattr(dw.workflow.Workflow, "validation_errors", raise_boom) with server(success_script) as client: - result = client.post("/api/validate", json={"workflow": valid_workflow()}) + with caplog.at_level(logging.ERROR): + result = client.post("/api/validate", json={"workflow": valid_workflow()}) assert result.status_code == 200 body = result.json() assert body["valid"] is False - assert "boom" in body["error"] + assert "could not be validated" in body["error"] + assert body["errors"] == [{"path": None, "message": body["error"]}] + assert "boom" not in json.dumps(body) + assert "boom" in caplog.text def test_workflow_browsing_and_confinement(server):