Skip to content

Reject path traversal in multi-output filenames - #1342

Open
0xOverlordX wants to merge 1 commit into
google:masterfrom
0xOverlordX:fix-multi-output-path-traversal
Open

0xOverlordX wants to merge 1 commit into
google:masterfrom
0xOverlordX:fix-multi-output-path-traversal

Conversation

@0xOverlordX

Copy link
Copy Markdown

Problem

When using jsonnet -m <dir>, object keys become output filenames with no
validation. A key like "../escape.txt" writes outside the designated
output directory. This is unintended behavior — no file-producing tool
should silently write outside its output directory.

The function write_multi_output_files() in cmd/jsonnet.cpp constructs
the path as output_dir + pair.first with zero sanitization:

const std::string &filename = output_dir + pair.first;

A Jsonnet file like this writes outside the output dir:

{"../escape.txt": "data", "legit.json": "ok"}

Running jsonnet -m /tmp/outdir on this creates /tmp/escape.txt instead
of keeping all output inside /tmp/outdir/. The user gets no warning.

Security Impact

In CI/CD pipelines where jsonnet -m ./build/ processes untrusted Jsonnet
files from pull requests, an attacker can write to arbitrary locations
relative to the output directory. This can overwrite build configs,
dotfiles, or deployment scripts without any warning.

Combined with importstr (which reads arbitrary files by absolute path),
a single malicious .jsonnet file can read secrets and write them to
attacker-controlled locations outside the output directory.

Fix

Validate each output filename before writing. Reject any filename
containing ".." or starting with "/".

Before

jsonnet -m /tmp/outdir -e '{"../escape.txt": "pwned"}'
→ silently creates /tmp/escape.txt (outside output dir)

After

jsonnet -m /tmp/outdir -e '{"../escape.txt": "pwned"}'
→ ERROR: output filename contains '..': ../escape.txt

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request adds validation checks in cmd/jsonnet.cpp to prevent directory traversal and absolute path writes when generating multi-output files. The feedback highlights that the current check for absolute paths (starting with /) is insufficient on Windows systems, where paths can start with a backslash or a drive letter. A code suggestion is provided to address this security gap on Windows.

Comment thread cmd/jsonnet.cpp
Comment on lines +406 to +409
if (!pair.first.empty() && pair.first[0] == '/') {
std::cerr << "ERROR: output filename is an absolute path: " << pair.first << std::endl;
return false;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

On Windows systems, both backslash and forward slash are recognized as path separators. Additionally, drive letters (e.g., C:) can be used to specify absolute or drive-relative paths. The current check only prevents paths starting with /, which allows Windows-specific absolute paths (like \absolute\path or C:\absolute\path) to bypass the validation and potentially write files outside the output directory.\n\nTo ensure robust protection on Windows, we should also reject paths starting with backslash and paths containing a drive letter specifier (where the second character is :).

        if (!pair.first.empty() && (pair.first[0] == '/' || pair.first[0] == '\\')) {
            std::cerr << "ERROR: output filename is an absolute path: " << pair.first << std::endl;
            return false;
        }
        if (pair.first.size() >= 2 && pair.first[1] == ':') {
            std::cerr << "ERROR: output filename contains a drive letter: " << pair.first << std::endl;
            return false;
        }

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant