Resolve paths with realpath to prevent symlink traversal during archive extraction

[LinZiyuu]

Summary

keras.utils.get_file(extract=True) and keras.utils.extract_archive validate each
TAR member against the extraction directory using resolve_sub_path, which relies on
resolve_path:

def resolve_path(path):
    return os.path.realpath(os.path.abspath(path))

os.path.abspath collapses .. segments lexically (as text) before any symlink
is resolved. For a member named link/../x, the .. cancels the link component
textually, so the subsequent realpath never traverses the symlink. The containment
check then concludes the member stays inside the extraction directory, while
tarfile.extractall writes it to the real location reached by following the
symlink — outside the directory.

As a result, an archive containing an in-bounds symlink link -> . plus a regular
file link/../<name> writes <name> into the parent of the extraction directory
(and chaining symlinks reaches deeper paths). Writing outside the extraction directory
during archive extraction is the issue this validation is meant to prevent.

Fix

Use os.path.realpath(path) directly. realpath already returns an absolute path and
resolves symlinks component by component, so .. is applied after the symlink is
resolved and resolve_sub_path correctly rejects members that escape the base
directory. For paths without a symlink-then-.. sequence the result is unchanged.

Reproduction (before this PR)

import io, os, tarfile, tempfile, keras
work = tempfile.mkdtemp(); cache = os.path.join(work, "cache"); os.makedirs(cache)
tp = os.path.join(work, "evil.tar.gz")
with tarfile.open(tp, "w:gz") as tf:
    sl = tarfile.TarInfo("ln"); sl.type = tarfile.SYMTYPE; sl.linkname = "."
    tf.addfile(sl)
    data = b"escaped\n"
    fi = tarfile.TarInfo("ln/../ESCAPED"); fi.size = len(data)
    tf.addfile(fi, io.BytesIO(data))
keras.utils.get_file(fname="a", origin="file://"+tp, extract=True,
                     cache_dir=cache, cache_subdir="datasets")
# before: cache/datasets/ESCAPED exists (outside cache/datasets/a)
# after:  the member is skipped; nothing is written outside the extraction dir

Tests

Added FilterSafePathsTest.test_file_routed_through_symlink_is_rejected. It fails on the
previous implementation and passes with this change. All existing file_utils tests
continue to pass.

Contributor agreement

• I am a human, and not a bot.
• I will be responsible for responding to review comments in a timely manner.
• I will work with the maintainers to push this PR forward until submission.

[gemini-code-assist]

Code Review

This pull request updates resolve_path in file_utils.py to use os.path.realpath(path) directly, avoiding os.path.abspath which lexically collapses .. before symlinks are resolved. It also adds a test to verify that files routed through symlinks are rejected. The reviewer identified a critical security vulnerability where this check can still be bypassed if the generator is eagerly evaluated before symlinks are written to disk. The reviewer recommended tracking symlinks in-memory during filtering to ensure robust path traversal protection.

[gemini-code-assist]

Security Vulnerability: Fragile Symlink Resolution during Eager Evaluation

While using os.path.realpath(path) directly instead of wrapping it in os.path.abspath resolves the lexical collapse issue when the symlink exists on disk, this approach remains highly fragile and vulnerable to path traversal under common usage patterns.

The Problem

The containment check relies on the physical filesystem state to resolve symlinks. However, during archive extraction, filter_safe_tarinfos is a generator. If a user or wrapper eagerly evaluates this generator (e.g., by calling list(filter_safe_tarinfos(...)) or pre-filtering members before extraction), none of the symlinks have been written to disk yet.

In this scenario:

1. os.path.realpath is called on base_dir/link/../escaped.txt.
2. Since link does not exist on disk yet, os.path.realpath falls back to lexical resolution and collapses link/...
3. The path resolves to base_dir/escaped.txt, which is deemed safe and accepted.
4. When extraction actually runs, the symlink link is created first, and the subsequent extraction of link/../escaped.txt traverses the symlink and writes outside the extraction directory.

The Solution

To make this completely robust and immune to eager evaluation or out-of-order processing, we should track the symlinks defined in the archive in-memory and resolve the member paths against these tracked symlinks before performing the containment check.

Here is how filter_safe_tarinfos can be securely implemented:

def filter_safe_tarinfos(members, base_dir):
    base_dir = resolve_path(base_dir)
    resolved_symlinks = {}
    for finfo in members:
        # Resolve the member's path against any symlinks we've seen so far in the archive
        name = finfo.name
        components = name.split("/")
        resolved = []
        for c in components:
            if not c or c == ".":
                continue
            if c == "..":
                if resolved and resolved[-1] != "..":
                    resolved.pop()
                else:
                    resolved.append("..")
                continue
            resolved.append(c)
            current_path = "/".join(resolved)
            if current_path in resolved_symlinks:
                target = resolved_symlinks[current_path]
                resolved.pop()
                for tc in target.split("/"):
                    if not tc or tc == ".":
                        continue
                    if tc == "..":
                        if resolved and resolved[-1] != "..":
                            resolved.pop()
                        else:
                            resolved.append("..")
                        continue
                    resolved.append(tc)
        resolved_name = "/".join(resolved)

        valid_path = False
        if finfo.issym() or finfo.islnk():
            if is_link_in_dir(finfo, base_dir):
                valid_path = True
                if finfo.issym():
                    resolved_symlinks[name] = finfo.linkname
                yield finfo
        elif resolve_sub_path(base_dir, resolved_name) is not None:
            valid_path = True
            yield finfo
        if not valid_path:
            warnings.warn(
                "Skipping invalid path during archive extraction: "
                f"'{finfo.name}'.",
                stacklevel=2,
            )

[LinZiyuu]

In Keras this generator is consumed lazily, not via list(...). extract_open_archive passes it straight to tarfile.extractall, which iterates members and extracts each one before pulling the next. So by the time the generator checks a member routed through an in-bounds symlink (the symlink necessarily appears earlier in the archive), that symlink is already on disk and os.path.realpath follows it — the escape is rejected. That is exactly what the added test covers.

The bypass you describe requires the generator to be fully materialized before any extraction begins, which Keras never does. If you'd prefer defense-in-depth against external callers that might eagerly evaluate it, in-memory symlink tracking could be added as a follow-up; I've kept this PR to the minimal root-cause fix.

[gemini-code-assist]

Test Setup Limitation

Notice that in this test, you had to manually create the symlink on disk before calling filter_safe_tarinfos:

os.symlink(".", os.path.join(self.base_dir, "link"))

This is because filter_safe_tarinfos currently relies on the physical filesystem to resolve symlinks. In a real-world scenario, when a user or library eagerly evaluates the generator (e.g., list(filter_safe_tarinfos(...))) before extraction begins, the symlink does not exist on disk yet, and the containment check would be bypassed.

If we implement the in-memory symlink tracking solution suggested in file_utils.py, this test can be run without manually creating the symlink on disk beforehand, making it a true representation of the extraction process.

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.68%. Comparing base (19796fa) to head (4ab48b1).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #23015   +/-   ##
=======================================
  Coverage   84.68%   84.68%           
=======================================
  Files         464      464           
  Lines       68627    68627           
  Branches    11272    11272           
=======================================
  Hits        58117    58117           
  Misses       7572     7572           
  Partials     2938     2938           

Flag	Coverage Δ
keras	84.48% <100.00%> (ø)
keras-cpu	83.76% <100.00%> (ø)
keras-gpu	69.37% <100.00%> (ø)
keras-jax	58.31% <100.00%> (ø)
keras-numpy	53.66% <100.00%> (ø)
keras-openvino	59.44% <100.00%> (ø)
keras-tensorflow	59.65% <100.00%> (ø)
keras-torch	59.00% <100.00%> (ø)
keras-tpu	57.12% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[hertschuh]

Thanks for the fix!

[hertschuh]

Remove comment. This won't really make sense outside of this PR and the context can be found by using "Blame" and looking at the PR.

[LinZiyuu]

Done — removed the comment. The rationale now lives in the commit message (findable via Blame).

[hertschuh]

Remove comment. This won't really make sense outside of this PR and the context can be found by using "Blame" and looking at the PR.

[LinZiyuu]

Done — collapsed it to a single-line docstring matching the other tests in this class.

[LinZiyuu]

Done — removed.

[hertschuh]

Thanks!

[LinZiyuu]

Hi @hertschuh,
Thanks for merging this! This PR fixes the vulnerability I reported on huntr. Whenever you have a moment, could you validate the report on the platform? Link: https://huntr.com/bounties/4315b670-239b-40ce-b6fa-5d78e95d7b1c

Happy to provide any additional details if needed.