fix: [SNOW-3417278] contain read_file_content / procedure_from_js_file Jinja filters to project root#2983
Draft
sfc-gh-olorek wants to merge 1 commit into
Draft
Conversation
…e to project root The Jinja filters `read_file_content` and `procedure_from_js_file` are registered on every Jinja environment bootstrapped via `env_bootstrap()`, so they are always available during SQL template rendering. Both filters previously resolved any file path the template author requested and called `SecurePath(...).read_text(file_size_limit_mb=UNLIMITED)`, with no path containment and no size limit. A SQL template committed to a project repository could therefore embed arbitrary local files (e.g. `~/.ssh/id_rsa`, `~/.aws/credentials`, `~/.snowflake/config.toml`, `/etc/passwd`) into the rendered SQL, which is then executed against Snowflake and appears verbatim in QUERY_HISTORY. This change adds `_resolve_within_project_root()`, which expands `~` and resolves the requested path, then requires it to be a descendant of the active project root (`get_cli_context().project_root`). Paths that escape via `../` or absolute paths outside the project fail fast with a `ClickException` before any I/O happens. The `UNLIMITED` bypass is also replaced with `DEFAULT_SIZE_LIMIT_MB` (128 MB), aligning with the rest of the codebase. When the command runs outside a project (no project root available), the filter keeps its legacy behaviour to avoid breaking ad-hoc `snow sql -q` invocations — the attack requires a committed template, which requires a project root. Tests cover the containment rule in both directions, the UNLIMITED removal, parent-directory traversal, and the no-project-root fallback.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pre-review checklist
Changes description
Jira: SNOW-3417278 — SAST MEDIUM (CWE-73: External Control of File Name or Path).
General behaviour change. The
read_file_contentandprocedure_from_js_fileJinja filters, which are registered on every SQL-rendering Jinja environment viaenv_bootstrap(), now require the file they are asked to read to resolve inside the active project root. Paths that escape the root (absolute paths elsewhere on disk, or../-traversing relative paths) raise aClickExceptionbefore any I/O happens. TheUNLIMITEDsize bypass is also removed — these filters now read with the sameDEFAULT_SIZE_LIMIT_MB(128 MB) used elsewhere in the codebase.Motivating scenario. A
.sql.jinjacommitted to a project repository could previously use{{ '~/.ssh/id_rsa' | read_file_content }},{{ '/etc/passwd' | read_file_content }}or{{ '../../.aws/credentials' | read_file_content }}to pull arbitrary files readable by the developer process into the rendered SQL. That SQL is subsequently executed against Snowflake and appears verbatim inINFORMATION_SCHEMA.QUERY_HISTORY, allowing an attacker with access to the victim's account to exfiltrate local developer secrets.Implementation.
_resolve_within_project_root(file_name, filter_name)insrc/snowflake/cli/api/rendering/jinja.py:Path(file_name).expanduser().resolve()normalises the target.get_cli_context().project_rootvia a lazy import (thecli_global_contextmodule already importsCONTEXT_KEYfromjinja.py, so the lazy import avoids a circular dependency).Path.relative_to(...)to enforce containment; on failure raisesClickException(f"{filter_name}: path '{file_name}' is outside the project root.").snow sql -qrun outside a project), the containment check is skipped. The attack model requires a committed.sql.jinjain a project, which requires a project root, so this fallback preserves ad-hoc CLI usage without reopening the vulnerability.read_file_contentandprocedure_from_js_fileroute through the helper and then useDEFAULT_SIZE_LIMIT_MBinstead ofUNLIMITED.Tests (
tests/api/test_rendering.py) cover:ClickExceptionmentioning the filter name and "outside the project root".../-traversal back out of the project root is rejected.procedure_from_js_file, which still wraps the JS body correctly when the file is inside the root.UNLIMITEDremoval — a file that would trip the size limit now raisesFileTooLargeError.snow sql -qbehaviour.References.
src/snowflake/cli/api/rendering/jinja.py(read_file_content,procedure_from_js_file)src/snowflake/cli/api/rendering/jinja.py:env_bootstrapsrc/snowflake/cli/api/rendering/sql_templates.py:snowflake_sql_jinja_render