gh-118441: Limit `posixpath.realpath(..., strict=True)` symlinks #119172

nineteendo · 2024-05-19T09:39:31Z

Benchmark

Show script

echo relative-symlink
ln -nsfw . tmp/b
speedup-symlink-lookup/python.exe            -m timeit -s "import os; path = os.getcwd() + '/tmp' + '/b' * 40" "os.path.realpath(path, strict=True)"
main/python.exe                              -m timeit -s "import os; path = os.getcwd() + '/tmp' + '/b' * 40" "os.path.realpath(path, strict=True)"
limit-posixpath.realpath-symlinks/python.exe -m timeit -s "import os; path = os.getcwd() + '/tmp' + '/b' * 40" "os.path.realpath(path, strict=True)"

echo symlink
ln -nsfw `realpath tmp` tmp/c
speedup-symlink-lookup/python.exe            -m timeit -s "import os; path = os.getcwd() + '/tmp' + '/c' * 40" "os.path.realpath(path, strict=True)"
main/python.exe                              -m timeit -s "import os; path = os.getcwd() + '/tmp' + '/c' * 40" "os.path.realpath(path, strict=True)"
limit-posixpath.realpath-symlinks/python.exe -m timeit -s "import os; path = os.getcwd() + '/tmp' + '/c' * 40" "os.path.realpath(path, strict=True)"

relative-symlink
5000 loops, best of 5: 45.4 usec per loop # speedup-symlink-lookup
1000 loops, best of 5: 227  usec per loop # main
1000 loops, best of 5: 373  usec per loop # limit-posixpath.realpath-symlinks
# -> 8.22x slower when #120145 lands
symlink
5000 loops, best of 5: 72.2 usec per loop # speedup-symlink-lookup
1000 loops, best of 5: 251  usec per loop # main
200 loops,  best of 5: 1.38 msec per loop # limit-posixpath.realpath-symlinks
# -> 19.11x slower when #120145 lands

Issue: posixpath.realpath(..., strict=True) doesn't limit symlinks #118441

Lib/test/test_posixpath.py

nineteendo · 2024-05-19T10:46:23Z

@barneygale, I decided to exclude cached symlinks from the count as they don't matter. But 40 -> 39 -> ... -> 1 -> . is an error.

barneygale · 2024-05-19T14:41:00Z

Hah, we seem to be working on almost the same thing :-)

I've opened another PR (#119178) that adds internal support for limiting the number of symlink traversals, but doesn't (yet) enable or expose it in posixpath.realpath() as you've done here. Maybe we can land that first, and then worry about how to expose this to users in posixpath.realpath()?

barneygale · 2024-05-19T14:54:39Z

@barneygale, I decided to exclude cached symlinks from the count as they don't matter. But 40 -> 39 -> ... -> 1 -> . is an error.

I think they probably do matter, otherwise, when given a symlink that references a parent directory, realpath("link/link/link/[...]/link") will succeed no matter how many link/ tokens are in the path, whereas I think you'll find that Linux/macOS/coreutils realpath will bail out with "Too many symbolic links in path". If you try making the same change in #119178, you'll find that some pathlib ABC tests hang from infinitely expanding such a symlink tree.

barneygale · 2024-05-19T15:25:38Z

On my Linux system, man path_resolution says:

In order to protect the kernel against stack overflow, and also to protect against denial of service, there are limits on the maximum recursion depth, and on the maximum number of symbolic links followed. An ELOOP error is returned when the maximum is exceeded ("Too many levels of symbolic links").

Note that it doesn't say anything about the number of unique symbolic links followed, and I doubt Linux has something resembling our seen dict anyway - it's not strictly necessary if you limit the number of traversals.

nineteendo · 2024-05-19T16:39:22Z

Maybe we can land that first, and then worry about how to expose this to users in posixpath.realpath()?

Fine by me.

I think they probably do matter, otherwise, when given a symlink that references a parent directory, realpath("link/link/link/[...]/link") will succeed no matter how many link/ tokens are in the path, whereas I think you'll find that Linux/macOS/coreutils realpath will bail out with "Too many symbolic links in path".

Yeah, I'm very aware of that, that's the reason I created my bug report. But like I already outlined previously, we can't properly track the number of symlinks traversed. Take this symlink chain: 10->9->...->1->. 10/10/10/10 contains 40 symlinks, but we encounter only 13 of which we read 10.

I don't think there's much cost to looking up the already resolved symlinks compared to reading and resolving them.
Therefore, I think it would be silly to count cached symlinks.

If you try making the same change in #119178, you'll find that some pathlib ABC tests hang from infinitely expanding such a symlink tree.

Which tests hang from doing dictionary lookups? We have mostly the same cost from calling lstat() on regular files.

nineteendo · 2024-05-19T16:48:43Z

We can also re-use seen to count the number of unique symlinks. :)

barneygale · 2024-05-19T16:51:26Z

But like I already outlined previously, we can't properly track the number of symlinks traversed.

You're equating a "symlink traversal" with a readlink() call, but that's not correct. A lookup in seen is also a symlink traversal. A seen dict is not necessary to implement realpath() - it's an optimization that should not be user-visible.

Which tests hang from doing dictionary lookups?

test.test_pathlib.test_pathlib_abc.DummyPathWithSymlinksTest.test_glob_recurse_symlinks_common for example.

They hang from attempting to recursively walk an infinite-depth virtual filesystem, which is the denial-of-service issue mentioned in the Linux docs.

barneygale · 2024-05-19T16:56:20Z

We can also re-use seen to count the number of unique symlinks. :)

Which doesn't help you, because you need to guard against repeatedly traversing the same symlink.

nineteendo · 2024-05-19T17:08:09Z

You're equating a "symlink traversal" with a readlink() call, but that's not correct.

I never said that. On macOS the symlink limit is set to 32. If I call realpath 10/10/10/10 I get an error, but your implementation won't raise an error as it only encounters 13 symlinks.

A lookup in seen is also a symlink traversal. A seen dict is not necessary to implement realpath() - it's an optimization that should not be user-visible.

Sadly, the implementation is going to get a lot uglier if we want to make it an implementation detail as we need to store how many symlinks would be traversed without caching. Unless you want to disable caching in strict mode? Please don't.

barneygale · 2024-05-19T17:27:46Z

I never said that. On macOS the symlink limit is set to 32. If I call realpath 10/10/10/10 I get an error, but your implementation won't raise an error as it only encounters 13 symlinks.

Ah I see, because each seen lookup can save multiple readlink()s if the symlink target itself contains symlinks?

nineteendo · 2024-05-19T18:01:53Z

Exactly, so we can't use caching in strict mode (with maxlinks).

nineteendo · 2024-06-05T18:15:26Z

Sorry for the force push, I accidentally had my linter on during the merge.

nineteendo · 2024-06-05T18:59:05Z

@eryksun, how do we figure out the limit set by the OS? I don't like a hardcoded constant.

nineteendo · 2024-06-05T19:07:22Z

Does this count as a security issue? And do you think we should do a benchmark?

barneygale · 2024-06-05T19:26:39Z

Does this count as a security issue?

It might. See if you can find a simple-ish setup where os.path.realpath() takes a long time (~seconds) to return, whereas the OS errors out quickly?

nineteendo · 2024-06-05T20:40:20Z

@barneygale, precisely using the link/link/... spam you mentioned earlier:

ln -nsfw . tmp/b
main/python.exe                              -m timeit -s "import os; path = os.getcwd() + '/tmp' + '/b' * 200_000" "try: os.path.realpath(path, strict=True)" "except: pass"
limit-posixpath.realpath-symlinks/python.exe -m timeit -s "import os; path = os.getcwd() + '/tmp' + '/b' * 200_000" "try: os.path.realpath(path, strict=True)" "except: pass"

1   loop,  best of 5: 1.01 sec  per loop # before
100 loops, best of 5: 2.82 msec per loop # after
# -> 358.16x faster and gives an error that can be investigated.

This can be massively improved for non-strict mode: #120145, but that doesn't solve it entirely:

ln -nsfw . tmp/b
speedup-symlink-lookup/python.exe            -m timeit -s "import os; path = os.getcwd() + '/tmp' + '/b' * 4_000_000" "try: os.path.realpath(path, strict=True)" "except: pass"
limit-posixpath.realpath-symlinks/python.exe -m timeit -s "import os; path = os.getcwd() + '/tmp' + '/b' * 4_000_000" "try: os.path.realpath(path, strict=True)" "except: pass"

1 loop,  best of 5: 989  msec per loop # speedup-symlink-lookup
5 loops, best of 5: 51.5 msec per loop # limit-posixpath.realpath-symlinks
# -> 19.20x faster and gives an error that can be investigated.

eryksun · 2024-06-06T00:08:54Z

how do we figure out the limit set by the OS? I don't like a hardcoded constant.

On Linux, I think it's going to end up being a hard-coded constant equal to 40, which is what glibc returns from its internal __eloop_threshold() function.

#ifndef MIN_ELOOP_THRESHOLD
# define MIN_ELOOP_THRESHOLD    40
#endif

/* Return the maximum number of symlink traversals to permit
   before diagnosing ELOOP.  */
static inline unsigned int __attribute__ ((const))
__eloop_threshold (void)
{
#ifdef SYMLOOP_MAX
  const int symloop_max = SYMLOOP_MAX;
#else
  /* The function is marked 'const' even though we use memory and
     call a function, because sysconf is required to return the
     same value in every call and so it must always be safe to
     call __eloop_threshold exactly once and reuse the value.  */
  static long int sysconf_symloop_max;
  if (sysconf_symloop_max == 0)
    sysconf_symloop_max = __sysconf (_SC_SYMLOOP_MAX);
  const unsigned int symloop_max = (sysconf_symloop_max <= 0
                                    ? _POSIX_SYMLOOP_MAX
                                    : sysconf_symloop_max);
#endif

  return MAX (symloop_max, MIN_ELOOP_THRESHOLD);
}

For the glibc build on Linux, SYMLOOP_MAX isn't defined, and the configured value sysconf(_SC_SYMLOOP_MAX) is -1. So glibc sets symloop_max to the constant _POSIX_SYMLOOP_MAX (8), and __eloop_threshold() returns the constant MIN_ELOOP_THRESHOLD (40).

Note that Python hasn't updated its support for os.sysconf_names in a long while. It should be updated to define "SC_SYMLOOP_MAX" (the value is 173 on Linux), along with a few other new values specified for sysconf() in POSIX issues 6 and 7.

nineteendo · 2024-06-06T07:02:20Z

I also found a setup using 96 symlinks and a directory that >489 levels deep:

Show script

# limit-posixpath.realpath-symlinks.sh
mkdir -p "tmp/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/\
a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/\
a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/\
a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/\
a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/\
a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a"
ln -nsfw `realpath tmp` "tmp/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a\
/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a\
/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a\
/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a\
/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a\
/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a"
echo creating 96 symlinks ...
for i in {0..95};
    do ln -nsfw "a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a\
/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a\
/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a\
/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a\
/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a\
/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a" tmp/${i};
done
echo 96 symlinks created
main/python.exe                              -m timeit -s "import os; path = os.getcwd() + '/tmp/' + '/'.join(map(str, range(96)))" "try: os.path.realpath(path, strict=True)" "except: pass"
speedup-symlink-lookup/python.exe            -m timeit -s "import os; path = os.getcwd() + '/tmp/' + '/'.join(map(str, range(96)))" "try: os.path.realpath(path, strict=True)" "except: pass"
limit-posixpath.realpath-symlinks/python.exe -m timeit -s "import os; path = os.getcwd() + '/tmp/' + '/'.join(map(str, range(96)))" "try: os.path.realpath(path, strict=True)" "except: pass"

creating 96 symlinks ...
96 symlinks created
1 loop, best of 5: 1    sec per loop # main
1 loop, best of 5: 1.01 sec per loop # speedup-symlink-lookup
1 loop, best of 5: 210 msec per loop # limit-posixpath.realpath-symlinks
# -> 4.81x faster and gives an error that can be investigated.

Limit posixpath.realpath(..., strict=True) symlinks

3a22d93

bedevere-app bot mentioned this pull request May 19, 2024

posixpath.realpath(..., strict=True) doesn't limit symlinks #118441

Open

nineteendo and others added 2 commits May 19, 2024 12:08

Add test

dc6f7d3

📜🤖 Added by blurb_it.

2605bfc

nineteendo commented May 19, 2024

View reviewed changes

Lib/test/test_posixpath.py Outdated Show resolved Hide resolved

nineteendo added 2 commits May 19, 2024 12:22

Fix syntax error

d9475bf

import errno

b18e5d4

nineteendo marked this pull request as ready for review May 19, 2024 10:44

bedevere-app bot added the awaiting review label May 19, 2024

barneygale mentioned this pull request May 19, 2024

pathlib ABCs: remove duplicate realpath() implementation. #119178

Merged

Use len(seen)

628acd3

nineteendo marked this pull request as draft May 19, 2024 18:02

bedevere-app bot removed the awaiting review label May 19, 2024

Merge branch 'main' into limit-posixpath.realpath-symlinks

4c60431

nineteendo force-pushed the limit-posixpath.realpath-symlinks branch from a6f1869 to 4c60431 Compare June 5, 2024 18:14

nineteendo added 2 commits June 5, 2024 20:17

Restrict maxlinks when strict is set

3ada546

Simplify test

e502666

nineteendo marked this pull request as ready for review June 5, 2024 18:58

bedevere-app bot added the awaiting review label Jun 5, 2024

Fix error message

06c3853

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-118441: Limit `posixpath.realpath(..., strict=True)` symlinks #119172

gh-118441: Limit `posixpath.realpath(..., strict=True)` symlinks #119172

nineteendo commented May 19, 2024 •

edited

nineteendo commented May 19, 2024 •

edited

barneygale commented May 19, 2024

barneygale commented May 19, 2024

barneygale commented May 19, 2024

nineteendo commented May 19, 2024

nineteendo commented May 19, 2024

barneygale commented May 19, 2024 •

edited

barneygale commented May 19, 2024

nineteendo commented May 19, 2024

barneygale commented May 19, 2024 •

edited

nineteendo commented May 19, 2024 •

edited

nineteendo commented Jun 5, 2024 •

edited

nineteendo commented Jun 5, 2024

nineteendo commented Jun 5, 2024

barneygale commented Jun 5, 2024

nineteendo commented Jun 5, 2024 •

edited

eryksun commented Jun 6, 2024

nineteendo commented Jun 6, 2024 •

edited

gh-118441: Limit posixpath.realpath(..., strict=True) symlinks #119172

Are you sure you want to change the base?

gh-118441: Limit posixpath.realpath(..., strict=True) symlinks #119172

Conversation

nineteendo commented May 19, 2024 • edited

Benchmark

nineteendo commented May 19, 2024 • edited

barneygale commented May 19, 2024

barneygale commented May 19, 2024

barneygale commented May 19, 2024

nineteendo commented May 19, 2024

nineteendo commented May 19, 2024

barneygale commented May 19, 2024 • edited

barneygale commented May 19, 2024

nineteendo commented May 19, 2024

barneygale commented May 19, 2024 • edited

nineteendo commented May 19, 2024 • edited

nineteendo commented Jun 5, 2024 • edited

nineteendo commented Jun 5, 2024

nineteendo commented Jun 5, 2024

barneygale commented Jun 5, 2024

nineteendo commented Jun 5, 2024 • edited

eryksun commented Jun 6, 2024

nineteendo commented Jun 6, 2024 • edited

gh-118441: Limit `posixpath.realpath(..., strict=True)` symlinks #119172

gh-118441: Limit `posixpath.realpath(..., strict=True)` symlinks #119172

nineteendo commented May 19, 2024 •

edited

nineteendo commented May 19, 2024 •

edited

barneygale commented May 19, 2024 •

edited

barneygale commented May 19, 2024 •

edited

nineteendo commented May 19, 2024 •

edited

nineteendo commented Jun 5, 2024 •

edited

nineteendo commented Jun 5, 2024 •

edited

nineteendo commented Jun 6, 2024 •

edited