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]

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,

[HorlogeSkynet]

Hello @nir0s, could we have your review/input on this ? I'd like to incorporate this (security) fix in v1.10 release.
Thanks, bye 👋

[adamjstewart]

^ @nir0s if you're back online...

[nir0s]

Will take a look promptly.

[nir0s]

Approach is good... so just a few comments.

[nir0s]

The logic here is correct as far as I can tell, but it's hard to audit for security-sensitive code: ~50 lines of relative_parts[:i] slicing, mutate-and-restart, for/else, and a lexical relative_to. I'd prefer a single-pass input-pop / output-push segment stack (mirroring os.path.realpath(strict=False) with a clamped root) — it's linear and much easier to reason about. Worth considering folding that style in here.

[nir0s]

Not critical, but would be nice.

[HorlogeSkynet]

I'd prefer not getting back to work on this matter to rewrite one more time this algorithm... But feel free to change implementation with your idea and I'd be pleased to review it.

[nir0s]

This resolves + stats the path, and then _os_release_info resolves + opens the same path again - two walks plus a stat→open TOCTOU window. The distro-release path uses "open and catch" instead; doing the same here would drop both the double resolution and the race.

[HorlogeSkynet]

If we (only) delegate this case handling to _os_release_info, we cannot accomplish the "backward-compatible way of setting self.os_release_file attribute (added in #262). We need to know whether the path actually resolve to a file or not, to properly address /etc or /usr/lib variant.
Or I missed your point here.