Skip to content

Fix symlink escape vulnerability in touch_path (CWE-59) - #1060

Merged
mlim19 merged 2 commits into
masterfrom
fix_touch_path_symlink_vulnerability
Oct 7, 2026
Merged

mlim19 merged 2 commits into
masterfrom
fix_touch_path_symlink_vulnerability

Conversation

@mlim19

@mlim19 mlim19 commented Oct 5, 2026

Copy link
Copy Markdown
Contributor

Description

touch_path() in gprofiler/utils/__init__.py used Path.touch() followed by os.chmod(), both of which follow a pre-existing final symlink. When gProfiler runs as root, a malicious profiled container could plant a symlink at a predictable async-profiler log/output path and cause gProfiler to chmod an attacker-chosen host file to 0o622.

This does not require winning a tight race: for a given Java PID the async-profiler filename is stable across profiling cycles, so the container can plant the symlink after one cycle removes the file and before the next cycle's touch_path() runs.

Fix

  • Open the path with os.open(..., O_NOFOLLOW) so the kernel atomically rejects a symlink final component (raises ELOOP).
  • Use os.fchmod(fd, mode) on the resulting file descriptor instead of path-based os.chmod(), eliminating the TOCTOU window and bypassing umask (preserving the exact mode the callers expect).

This mirrors the existing hardening already applied to safe_copy() and safe_read_text().

Related Issue

#1056

How Has This Been Tested?

Covered by the existing Java profiler tests in CI, which exercise touch_path() for async-profiler output/log files. Legitimate paths are regular files, not symlinks, so behavior is unchanged.

touch_path() used Path.touch() and os.chmod() which both follow symlinks.
A malicious container could place a symlink at a predictable async-profiler
log/output path and cause privileged gProfiler to chmod the symlink target
on the host to 0622.

Fix: Use os.open() with O_NOFOLLOW to reject symlinks atomically at kernel
level, and os.fchmod() on the fd to avoid TOCTOU races.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings October 5, 2026 18:03

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

The new open mode can block indefinitely on an attacker-created FIFO, and the security behavior lacks focused regression coverage.

Review effort: Balanced
Findings: 1 High severity · 1 Low severity

Open (2)
What changed in this PR

Hardens touch_path() against final-component symlink attacks.

Changes:

  • Opens files with O_NOFOLLOW.
  • Applies permissions through the file descriptor.
  • Converts ELOOP into a security-focused error.
File Description
gprofiler/​utils/​__init__.py Implements symlink-safe file creation and permission handling.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread gprofiler/utils/__init__.py Outdated
Comment thread gprofiler/utils/__init__.py Outdated
Comment on lines +390 to +393
fd = os.open(path, os.O_WRONLY | os.O_CREAT | _O_NOFOLLOW, mode)
except OSError as e:
if e.errno == errno.ELOOP:
raise Exception(f"Refusing to touch {path}: symlinks are not allowed for security reasons")
…acker-controlled FIFO paths'

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 6, 2026 02:56

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

The implementation drops touch timestamp semantics and lacks a focused security regression test.

Review effort: Balanced
Findings: 1 Low severity

Open (1)
Resolved since last review (1)
Previously missed (2)

In code that hasn't changed since last review

Medium severity Preserve touch timestamp updates for existing regular files

gprofiler/​utils/​__init__.py:398

This no longer performs the “touch” part for an existing regular file: opening it without writing or truncating and then calling fchmod() leaves its access and modification timestamps unchanged. That regresses the previous Path.touch() behavior (and the function’s contract). Update the timestamps through the already-validated descriptor before closing it.

Low severity Add regression test rejecting symlinked final output paths

gprofiler/​utils/​__init__.py:390

The existing Java symlink test only exercises symlinks in parent paths; it never places a symlink at the final async-profiler output/log path. Add a focused regression test that makes the final path a symlink, asserts touch_path() rejects it, and verifies the symlink target’s mode is unchanged. Otherwise the security property this PR introduces can regress while the cited tests still pass.

@mlim19
mlim19 merged commit f0a00ae into master Oct 7, 2026
79 of 81 checks passed
@mlim19
mlim19 deleted the fix_touch_path_symlink_vulnerability branch October 7, 2026 17:30
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.

3 participants