Manually resolve path relatively to root_dir to prevent escape

[HorlogeSkynet]

Dear Sebastian, dear maintainers,

This PR drafts a first implementation to address concerns raised in #311 review.

Currently, paths are not manually resolved, and by following symbolic links, this may lead to outside specified root_dir.

By using os.path.realpath and os.path.commonprefix, we can verify that important paths are still prefixed by root_dir, once they have been resolved.

---

It turned out enforcing those checks was not as easy as I thought :

• some paths are provided on purpose as parameters, others automatically derived
• some paths are known during instantiation, others will be "later"
• I'm not sure PermissionError actually fits, but I was out of option
• I'm not sure the error message itself is relevant either
• New __prevent_root_dir_escape function might require a renaming too

At least, the new functional test actually enlightens the issue.

--> A deep review is required, ideas are welcome and commits on this branch too 🙃

---

Have a nice WE all 🙇

[hartwork]

Hi @HorlogeSkynet , maybe I'm misunderstanding something, but right now I only see the part that prevents "unjust" escapes from the chroot (from absolute links or from going above the root) but I don't see any code that would "bend" "legit" absolute symlinks back into the chroot.

Let me demo what I think we'd need in some form:

def resolve_chroot_symlink(link_location, root_dir):
    assert os.path.commonprefix([link_location, root_dir]) == root_dir

    while True:
        try:
            resolved = os.readlink(link_location)
        except FileNotFoundError:  # includes case "not a symlink"
            return link_location

        if resolved.startswith('/'):
            # i.e. absolute path (with regard to the chroot)
            # that we need to move back inside the chroot
            resolved = os.path.join(root_dir, resolved.lstrip('/'))
        else:
            # i.e. relative path that we make absolute
            resolved = os.path.join(os.path.dirname(link_location), resolved)

        if os.path.commonprefix([resolved, root_dir]) != root_dir:
            raise Value('ESCAPE DETECTED')  # TODO

        link_location = resolved

What do you think?

[HorlogeSkynet]

Dear Sebastian, sorry for the delay.
I got your point and understood what you were trying to achieve : I did miss the part about resolving symbolic links relatively to root_dir.

So I tweaked your function a bit in order to :

• ease any future Windows integration task (see Support for Windows and Mac OS. #177)
• detect symbolic links loop (as os.path.realpath does)
• fix os.readlink usage on regular files (it actually raises OSError: "Invalid argument")
• fix os.path.commonprefix usage by removing redundant "up-level references"

You will see that I opted for links resolution on the verge of calling open. This definitely simplifies the constructor.

I've implemented three short test cases to check our new logic (note : relative symbolic links resolving is already tested by regular rootfs under tests/resources/distros).

I also took the liberty to update our .gitignore file, as my working directory used not to be clean since #315.

Bye ! Thanks for your time 👋

[hartwork]

Hi @HorlogeSkynet kudos for the new tests and the loop detection! 👍 I believe a few details need more work, please see my comments below

[HorlogeSkynet]

Hi @HorlogeSkynet kudos for the new tests and the loop detection! +1 I believe a few details need more work, please see my comments below

🙏
I've answered/resolved your comments Sebastian.
Although, I'm concerned about the CI failure, as tests pass "on my machine".
I think this is due to an issue related to GitHub Actions and symbolic links resolving to directories (I might be wrong).

My mistake ! There was an empty directory that Git didn't track, I've just fixed this 😉

Waiting for your inputs, thanks for your time 👋

---

EDIT : If you are tired of these round trips, feel free to directly amend the branch !

[HorlogeSkynet]

PR's (finally) ready @hartwork !

Branch reworked

[hartwork]

Hello @HorlogeSkynet, sorry for the big delay. I found some remaining issues. Please see my four comments below for details:

[HorlogeSkynet]

Dear @python-distro/maintainers, Dear @hartwork (if you haven't muted notifications from this repository 🙃),

I've finally taken the time to rework this feature (fix ?).

What has been done :

• os.path.commonpath has been preferred over commonprefix (first relative path normalization has been added to prevent standard library exception ;
• 100% (99,99%) code coverage (for newly added code) ;
• os.path.realpath have been removed from iteration to prevent OS links resolution, but there is one left for final path to detect some escape cases ;
• Three new tests (two for os_release_file as well as another one for above chroot escape case) ;
• root_dir is resolved, mainly to deal with .. (as you correctly pointed out Sebastian) ;
• link check before resolution instead of try..except (indeed simpler) ;
• OSError has been replaced by FileNotFoundError (symlinks loop corresponds to a missing file, at least from the caller PoV) ;
• a "normalization" for relative paths has been added, to strip ../ that would legitimately resolve to chroot in a real-world situation.

This branch as also been rebased, and hence now "ready".

Thanks for your time,
Bye 👋

[hartwork]

Dear @python-distro/maintainers, Dear @hartwork (if you haven't muted notifications from this repository 🙃),

Hi @HorlogeSkynet, back in 2022-08 — soon two years ago — I implemented a solution on branch security/prevent_rootfs_escape_2 that still exists today. I remember you were unsure if the approach was the best way forward and called out to all maintainers for feedback (it's archived in https://github.com/orgs/python-distro/teams/maintainers/members/archived_team_posts.json for everyone with read permissions); the topic and my branch has since been ignored altogether, until your new commits here 5 days ago. 2022 is a long time ago, and I'm not interested in getting back into this topic again in 2024, myself. Thanks for your understanding, I wish this pull request the best.

[HorlogeSkynet]

Hey @hartwork, and thanks for your answer. Indeed, I completely forgot about this (these) discussions, since GitHub archived them...

I took a quick glimpse of your branch which I find very complex (but this bug/feature is not trivial at all, and my new algorithm is possibly sill broken).
Although, I've updated a test case according to your commit 238925a.

So I'll sum up the situation here again, so @python-distro/maintainers will have the full context to properly review this (these) branches :

---

The idea is to "fix" the --root-dir option (brought in #161), which allows a separate rootfs to be specified (from CLI, or through LinuxDistribution API). As noticed first during #241, and then #311, multiple issues are related to this parameter in it's current implementation :

• [fixed during Add support for AIX (closes #241) #311] this led to "unknown" binary executions, which possibly depend on host information (like the running kernel with uname) or even unsupported architecture (e.g. an ARM rootfs on an x86_64 host)
• paths under root_dir are mis-resolved to paths outside root_dir : this means that opening $root_dir/etc/os-release which links to ../../../../../etc/os-release (or even to /etc/os-release) would result in host' own OS-RELEASE file being read (and distro info inferred from there)
• as a consequence, symbolic link loops are not detected (a kind of DoS could happen in some cases)

So we (@hartwork and I) worked on these issues without opting for solutions as requiring system privileges or using a third-party dependency.
There are two "final" implementations :

• The one proposed in this MR, which eventually addresses Sebastian's concerns, and which :

  1. Recursively resolves links relatively to root_dir until a non-link file is found (in a best-effort manner) ;
  2. Assert this file is actually under root_dir before returning it ;
  3. Any directory symlink encountered is not supported (they won't be "bent" in chroot).

• Sebastian's (not rebased onto master, which manually resolves file paths relatively to root_dir (with full NT paths support ?)

From here, we see multiple options that I'd like to propose/discuss with you :

1. Choose and keep one of these implementations, as is ;
2. Implement one of these algorithms internally, as a separate module in the distro package, and fully-optional (try..except ModuleNotFoundError for projects that vendor distro.py script, disabling --root-dir option when missing) ;
3. Implement this algorithm internally, as a separate module in the distro package, and required (import [...]) ;
4. Implement this algorithm externally (a third package that we [both of us] would maintain on PyPI, this would adding a [extra ?] dependency to this project) ;
5. Drop the --root-dir parameter completely, close this MR and release distro v2 ;
6. Use a feature/trick that we do not know about, safely resolving links relatively to a specified directory (and cross platform ?).

---

Thanks for your time, see you all 👋

Implementation reworked and rebased

[HorlogeSkynet]

@adamjstewart As #369 is now closed and we eventually don't "open" another file, I take the liberty to re-propose this patch to address concerns raised in #311. I agree it's a big piece, but as it's safely guarded by if self.root_dir is None condition, it'd be harmless for 99,99% of cases.

[adamjstewart]

@HorlogeSkynet this one's a bit outside my area of expertise. I think someone else would be better to review this, as I barely understand the problem it's trying to solve.

[HorlogeSkynet]

@HorlogeSkynet this one's a bit outside my area of expertise. I think someone else would be better to review this, as I barely understand the problem it's trying to solve.

Two years and a half, except for Sebastian, no one really gave a damn about this issue.
TL; DR : if you specify -r /mnt/chroot with /mnt/chroot being a rootfs containing, let's say, a symbolic link resolving to /etc/os-release, distro will parse data from your system.
Before 2a89f76 it was definitely a security issue as any program could be executed when distro tried uname or lsb_release data sources.
Since 2a89f76, it's still a (minor ?) security issue as a rootfs could be misidentified as host OS, or worst : a malicious rootfs may lead to arbitrary host files parsing (e.g. /mnt/chroot/etc/os-release resolving to /etc/shadow).

This PR resolves rootfs symbolic links relatively to chroot to prevent this behavior, but is still vulnerable to TOCTOU,