Repository navigation
Description
Bug report
Looks like posix._path_normpath has slightly different behaviour to the python implementation of normpath defined in posixpath, as such os.path.normpath behaves differently on Python 3.11 (where posix._path_normpath is used if it exists) vs 3.10 on posix systems:
Python 3.10:
>>> import os.path
>>> os.path.normpath('hello\x00world')
'hello\x00world'
>>> os.path.normpath('\x00hello')
'\x00hello'Python 3.11:
>>> import os.path
>>> os.path.normpath('hello\x00world')
'hello'
>>> os.path.normpath('\x00hello')
'.'Obviously filepaths shouldn't have nulls in them, but the above means invalid input to a program could result in the wrong files or directories being used, rather than an error about embedded nulls once the filepaths are actually used for a system call. And I'm guessing the inconsistency between Python3.10 and 3.11, or between the Python and C implementations of normpath was not intended in any case.
Your environment
CPython 3.11.3, running on Arch Linux
Python 3.11.3 (main, Jun 5 2023, 09:32:32) [GCC 13.1.1 20230429] on linux
Linked PRs
- gh-106242: Fix path truncation in normpath #106816
- [3.12] gh-106242: Fix path truncation in os.path.normpath (GH-106816) #107981
- [3.11] gh-106242: Fix path truncation in os.path.normpath (GH-106816) #107982
- gh-106242: Minor fixup to avoid compiler warnings #107983
- gh-106242: Make ntpath.realpath errors consistent with abspath when there are embedded nulls #108248
- [3.12] gh-106242: Make ntpath.realpath errors consistent with abspath when there are embedded nulls (GH-108248) #108251
- [3.11] gh-106242: Make ntpath.realpath errors consistent with abspath when there are embedded nulls (GH-108248) #108252
Activity
eryksun commentedon Jun 29, 2023
I think this is a matter of documentation. I don't recall whether Steve Dower (@zooba), the author of the C implementation of
normpath(), has commented before in regard to this behavior change.os.path.normpath()is allowed to modify a pathname if doing so doesn't change how the operating system would parse it, with the exception that path parsing is allowed to differ if a symlink is removed by normalizing a ".." component (e.g. "symlink/.." -> ".')1. The documentation doesn't mention embedded null characters, but all supported platforms do use null-terminated strings.Since the
ValueErrorthat gets raised in some cases if a pathname contains embedded null characters is from Python itself, not the operating system, thennormpath()shouldn't be obligated to preserve embedded null characters. But the behavior change sneaked in under the radar. It should be documented in "Doc/library/os.path.rst". For example, add a version-changed notification.Footnotes
Another exceptional case, which unfortunately is undocumented, is the removal of a leading dot component (i.e.
os.curdir) of a relative path, such as "./spam" -> "spam". This can change how the operating system handles the path in a search context. The pathname "./filename" is always resolved relative to the current working directory. On the other hand in a search context, "filename" without a leading "." component is resolved against the set of directories to be searched, which is typically from thePATHenvironment variable. On Windows, even "dirname/filename" is resolved by searching, in contrast to "./dirname/filename". ↩zooba commentedon Jun 29, 2023
It's probably easiest just to make
_path_normpathnot truncate. Looking at the current code:normpathis actually fine, but it needs to return the new length so that we can pass it toFromWideChar._Py_normpathhas the last character when it returns, so can easily calculate the size. It can be passed back through an (optional) pointer to aPy_ssize_t._Py_normpathis exported (not sure why?), so we really ought to add a new name (_Py_normpathAndSizeseems appropriate).I think that fix can be backported. As noted, the path is invalid anyway, but probably best to preserve behaviour rather than double-down on changing it.
chrisjbillington commentedon Jun 30, 2023
Thanks!
I think maintaining the 3.10 behaviour makes sense, but that if one wanted to double down on a change instead, then a
ValueErrorwould be preferred to truncation.FWIW I notice on Windows, we get truncation on Python 3.11, no truncation on Python3.10, and a ValueError on Python 3.8 (didn't test 3.9). So there is some precedent for a
ValueErrorMy use case is that I'm passing filepaths over a socket to another process, and since null bytes can't appear in filepaths, I'm using nulls as the delimiter. I was converting the paths to absolute paths with
os.path.abspath()and then checking for nulls before writing them to the socket. Nulls would result in truncation and my code would write invalid filepaths to the socket instead of raising its ownValueErrorupon finding the null. I'm now checking for nulls before callingabspath.eryksun commentedon Jun 30, 2023
Raising
ValueErroris the last thing I'd wantnormpath()to do for embedded null characters. It wouldn't be consistently handled across all functions inos.pathwithout introducing new behavior.On Windows,
os.path.normpath()in Python 3.8 doesn't raiseValueErrorfor embedded null characters. Which input pathnames did you test in which version (e.g. CPython 3.8.10)?os.path.abspath()is partially based onnt._getfullpathname()on Windows. The latter does raiseValueErrorif a pathname has embedded null characters because WinAPIGetFullPathNameW()takes a null-terminated string. Prior to 3.11, in this caseabspath()has to fall back on an inferior implementation. In 3.11,abspath()callsnormpath()first, which avoids having to use the fallback implementation. For example, the fallback implementation lacks support for per-drive working directories.Python 3.10:
The result
"E:spam\0"is wrong because it's a relative path that depends on the working directory on drive "E:". The result should never be a relative path. That's a bug in the fallback implementation, but fixing it isn't on the top of anyone's to-do list.Python 3.11:
The new behavior of
normpath()is better in this case, though such a weird edge case isn't a strong argument in its favor.Your workaround is how I would have implemented it from the start.
abspath()can't add null characters to the result, so it's more efficient to check the path beforehand.chrisjbillington commentedon Jul 1, 2023
Apologies, I'm an idiot - for some reason my fingers typed
realpath()for some of my tests. The path checked wasb'asd\x00asd'. This is CPython 3.8.10 from the Windows app store, and CPythons 3.10.11 and 3.11.4 downloaded from Python.org, all 64-bit.Being more careful with my typing:
I see
realpath()raisingValueErrorfor embedded nulls on Python 3.8 and 3.10, and truncating on Python 3.11.I see
abspath()andnormpath()preserving nulls on Python 3.8 and 3.10, and truncating on 3.11.I can see that as you said, on Python 3.8 and 3.10,
abspath()is catching theValueErrorfrom_getfullpathname()and using the fallback, which preserves nulls. In 3.11 the use ofnormpath()before calling_getfullpathname()truncates the nulls and so there is noValueError. Makes sense.Similarly, with
realpath, theValueErroris coming from a call to_getfinalpathname(), but an earlier call tonormpath()truncates the nulls on Python 3.11, and so the result is truncation rather than aValueError.So it's
realpath()where there is precedent for aValueError. But fair enough if that was bad and raising moreValueErrors would be the more chaotic option.eryksun commentedon Jul 1, 2023
The behavior of
os.path.realpath()with embedded null characters looks like an oversight that was introduced on Windows in Python 3.8 and on POSIX in Python 3.10. Prior to 3.8,realpath()on Windows was an alias forabspath(). The implementation in 3.8+ doesn't handle aValueErrorfromnt._getfinalpathname(). Prior to 3.10,realpath()on POSIX usedislink(), which handles aValueErrorif it's raised byos.lstat()by returningFalse. In 3.10+, the newstrictparameter ofrealpath()was implemented on POSIX by callingos.lstat()directly, butValueErroris no longer handled in non-strict mode. These behavior changes crept in because there are no regression tests forrealpath()that check pathnames that contain null characters.gh-106242: Fix path truncation in os.path.normpath (GH-106816)
pythongh-106242: Fix path truncation in os.path.normpath (pythonGH-10…
pythongh-106242: Fix path truncation in os.path.normpath (pythonGH-10…
4 remaining items
[3.11] gh-106242: Fix path truncation in os.path.normpath (GH-106816) (…
pythongh-106242: Fix path truncation in os.path.normpath (pythonGH-10…
Merge branch '3.12' into pythongh-106242-3.12
[3.12] gh-106242: Fix path truncation in os.path.normpath (GH-106816) (…
gpshead commentedon Aug 21, 2023
is there anything left to be done here? additional testing and possible other API corner case fixes based on Eryksun's comment?
pythongh-106242: Make ntpath.realpath errors consistent with abspath …
zooba commentedon Aug 21, 2023
Yes, I think
ntpath.realpathneeds some error handling updates to be consistent withabspath. Since this fix is already changing its error behaviour (back), we may as well take the opportunity to change it to match documentation.gh-106242: Make ntpath.realpath errors consistent with abspath when t…
pythongh-106242: Make ntpath.realpath errors consistent with abspath …
pythongh-106242: Make ntpath.realpath errors consistent with abspath …
[3.12] gh-106242: Make ntpath.realpath errors consistent with abspath…
[3.11] gh-106242: Make ntpath.realpath errors consistent with abspath…
samueloph commentedon Aug 28, 2023
CVE-2023-41105 was assigned to this.
There's also CVE-2023-40587 which is a CVE against pyramid, but it's about triggering this same issue.
I wasn't involved in these assignments, posting here so it's logged in the GitHub issue.
(CVE-2023-41105 is already tracked at https://github.com/psf/advisory-database)