diff --git a/gprofiler/profilers/java.py b/gprofiler/profilers/java.py index 2e85486a1..f6db1e9e7 100644 --- a/gprofiler/profilers/java.py +++ b/gprofiler/profilers/java.py @@ -98,7 +98,7 @@ touch_path, wait_event, ) -from gprofiler.utils.fs import is_owned_by_root, is_rw_exec_dir, mkdir_owned_root, safe_copy +from gprofiler.utils.fs import is_owned_by_root, is_rw_exec_dir, mkdir_owned_root, safe_copy, safe_read_text from gprofiler.utils.perf import can_i_use_perf_events from gprofiler.utils.process import process_comm, search_proc_maps @@ -769,11 +769,10 @@ def _read_ap_log(self) -> str: if not os.path.exists(self._log_path_host): return "(log file doesn't exist)" - log = Path(self._log_path_host) - ap_log = log.read_text() + ap_log = safe_read_text(self._log_path_host) # clean immediately so we don't mix log messages from multiple invocations. # this is also what AP's profiler.sh does. - log.unlink() + Path(self._log_path_host).unlink() self._recreate_log() return ap_log diff --git a/gprofiler/utils/fs.py b/gprofiler/utils/fs.py index 1371e566f..6de0239cd 100644 --- a/gprofiler/utils/fs.py +++ b/gprofiler/utils/fs.py @@ -17,6 +17,7 @@ import errno import os import shutil +import stat from pathlib import Path from secrets import token_hex from typing import Union @@ -26,17 +27,108 @@ from gprofiler.platform import is_windows from gprofiler.utils import remove_path, run_process +# O_NOFOLLOW is always available on Linux (the target platform for this code). +# The getattr fallback to 0 covers non-Linux builds; on those platforms symlink +# protection in safe_read_text() is best-effort only. +_O_NOFOLLOW: int = getattr(os, "O_NOFOLLOW", 0) + + +def _is_symlink_lstat(path: str) -> bool: + """Check if path is a symlink without following it.""" + try: + return stat.S_ISLNK(os.lstat(path).st_mode) + except FileNotFoundError: + return False + def safe_copy(src: str, dst: str) -> None: """ Safely copies 'src' to 'dst'. Safely means that writing 'dst' is performed at a temporary location, and the file is then moved, making the filesystem-level change atomic. + + Security: Uses O_EXCL to atomically create the temp file, preventing symlink attacks where an + attacker plants a symlink to redirect writes to arbitrary locations. """ dst_tmp = f"{dst}.tmp" - shutil.copy(src, dst_tmp) + + # Remove any leftover tmp file from a previous interrupted copy. + # unlink() removes symlinks themselves (not their targets), so this is safe even if dst_tmp + # is a symlink; the subsequent O_EXCL open then creates the file fresh. + try: + os.unlink(dst_tmp) + except FileNotFoundError: + pass # Normal case: no leftover file + + # O_EXCL ensures atomic creation - fails if anything exists at dst_tmp (including symlinks). + # EEXIST means another process created the file after our delete - indicates a race or attack. + try: + fd = os.open(dst_tmp, os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0o644) + except FileExistsError: + raise Exception( + f"Refusing to copy: {dst_tmp} was created unexpectedly (possible race condition or symlink attack)" + ) + try: + dst_file = os.fdopen(fd, "wb") + except Exception: + os.close(fd) + try: + os.unlink(dst_tmp) + except OSError: + pass + raise + try: + with dst_file, open(src, "rb") as src_file: + shutil.copyfileobj(src_file, dst_file) + # Preserve source file permissions (e.g., executable bit) + shutil.copymode(src, dst_tmp) + except Exception: + try: + os.unlink(dst_tmp) + except OSError: + pass + raise + + # Best-effort check: refuse if dst is currently a symlink. + # os.rename() replaces the destination atomically (it does not follow dst symlinks), so even + # if an attacker races to plant a symlink between this check and the rename, the symlink itself + # would be replaced rather than its target being overwritten. This check adds defence-in-depth. + if _is_symlink_lstat(dst): + os.unlink(dst_tmp) + raise Exception(f"Refusing to copy: destination {dst} is a symlink (security restriction)") + os.rename(dst_tmp, dst) +def safe_read_text(path: str) -> str: + """ + Safely read text from a file, refusing to follow symlinks. + + Uses O_NOFOLLOW so the kernel rejects symlinks atomically at open time (Linux). + On platforms without O_NOFOLLOW the flag falls back to 0 and the protection + is best-effort; the target platform for this code is Linux where O_NOFOLLOW + is always available. + + Raises if path is a symlink. + """ + try: + # O_NOFOLLOW makes open() fail with ELOOP if the path is a symlink (Linux-specific behavior). + # On platforms without O_NOFOLLOW the flag is 0 and the call may follow symlinks; the target + # platform for this code is Linux, so O_NOFOLLOW is always available. + fd = os.open(path, os.O_RDONLY | _O_NOFOLLOW) + except OSError as e: + if e.errno == errno.ELOOP: + raise Exception(f"Refusing to read {path}: symlinks are not allowed for security reasons") + raise + + try: + f = os.fdopen(fd, "r") + except Exception: + os.close(fd) + raise + with f: + return f.read() + + def is_rw_exec_dir(path: Path) -> bool: """ Is 'path' rw and exec? diff --git a/tests/conftest.py b/tests/conftest.py index bb8264d97..2acd38ae8 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -427,7 +427,20 @@ def application_docker_image( ) if application_image_tag == "musl": pytest.xfail("This test does not work on aarch64 https://github.com/intel/gprofiler/issues/743") - image = build_image(docker_client, **application_docker_image_configs[image_name(runtime, application_image_tag)]) + try: + image = build_image( + docker_client, **application_docker_image_configs[image_name(runtime, application_image_tag)] + ) + except docker.errors.BuildError as e: + # Skip tests when Docker image build fails due to network issues (e.g., unavailable package + # repositories). The APT "Failed to fetch" / "Some index files failed to download" messages + # appear in the build log stream entries, not in the top-level error reason. + # This prevents transient network failures from turning into hard CI failures. + network_error_markers = ("Failed to fetch", "Some index files failed to download") + build_log_text = " ".join(str(entry) for entry in e.build_log) + if any(marker in build_log_text for marker in network_error_markers): + pytest.skip(f"Skipping: Docker image build failed due to unavailable package repository: {e}") + raise yield image # Clean up the test image after test completes to free disk space for subsequent tests