nt._getfinalpathname may use uninitialized memory

[izbyshev]

BPO	33016
Nosy	@pfmoore, @tjguk, @zware, @eryksun, @zooba, @izbyshev, @miss-islington
PRs	bpo-33016: Fix potential use of uninitialized memory in nt._getfinalpathname #6010 [3.7] bpo-33016: Fix potential use of uninitialized memory in nt._getfinalpathname (GH-6010) #6031 [3.6] bpo-33016: Fix potential use of uninitialized memory in nt._getfinalpathname (GH-6010) #6032

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/zooba'
closed_at = <Date 2018-09-21.20:47:00.142>
created_at = <Date 2018-03-06.22:31:37.624>
labels = ['extension-modules', '3.8', 'type-bug', '3.7', 'OS-windows']
title = 'nt._getfinalpathname may use uninitialized memory'
updated_at = <Date 2018-09-21.20:47:00.141>
user = 'https://github.com/izbyshev'

bugs.python.org fields:

activity = <Date 2018-09-21.20:47:00.141>
actor = 'steve.dower'
assignee = 'steve.dower'
closed = True
closed_date = <Date 2018-09-21.20:47:00.142>
closer = 'steve.dower'
components = ['Extension Modules', 'Windows']
creation = <Date 2018-03-06.22:31:37.624>
creator = 'izbyshev'
dependencies = []
files = []
hgrepos = []
issue_num = 33016
keywords = ['patch']
message_count = 11.0
messages = ['313366', '313367', '313445', '313448', '313463', '313464', '313465', '313466', '313467', '313469', '313593']
nosy_count = 7.0
nosy_names = ['paul.moore', 'tim.golden', 'zach.ware', 'eryksun', 'steve.dower', 'izbyshev', 'miss-islington']
pr_nums = ['6010', '6031', '6032']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue33016'
versions = ['Python 3.6', 'Python 3.7', 'Python 3.8']

[izbyshev]

The first call of GetFinalPathNameByHandleW requests the required buffer size for the NT path (VOLUME_NAME_NT), while the second call receives the DOS path (VOLUME_NAME_DOS) in the allocated buffer. Usually, NT paths are longer than DOS ones, for example:

NT path: \Device\HarddiskVolume2\foo
DOS path: \\?\C:\foo

Or, for UNC paths:

NT path: \Device\Mup\server\share\foo
DOS path: \\?\UNC\server\share\foo

However, it is not always the case. A volume can be mounted to an arbitrary path, and if a drive letter is not assigned to such a volume,
GetFinalPathNameByHandle will use the mount point path instead of C: above. This way, a DOS path can be longer than an NT path. Since the result of the second call is not checked properly, this condition won't be detected, resulting in an out-of-bounds access and use of uninitialized memory later.

Moreover, the path returned by GetFinalPathNameByHandle may change between the first and the second call, for example, because an intermediate directory was renamed. If the path becomes longer than buf_size, the same issue will occur.

[eryksun]

The inconsistent use of VOLUME_NAME_NT and VOLUME_NAME_DOS was addressed incidentally in bpo-29734, but that issue is primarily about the handle leaked when GetFinalPathNameByHandleW fails. That said, you've cleanly addressed the handle leak in your PR also. I prefer the common cleanup approach that you've used here, rather than spreading the cleanup tasks around in blocks, where it can easily be overlooked.

The PR for bpo-29734 also fixes some handle leaks related to win32_xstat_impl. Those leaks still need to be addressed for 3.7, and backported to 3.6.

[zooba]

New changeset 3b20d34 by Steve Dower (Alexey Izbyshev) in branch 'master':
bpo-33016: Fix potential use of uninitialized memory in nt._getfinalpathname (bpo-6010)
3b20d34

[miss-islington]

New changeset 8c163bb by Miss Islington (bot) in branch '3.7':
bpo-33016: Fix potential use of uninitialized memory in nt._getfinalpathname (GH-6010)
8c163bb

[izbyshev]

Copying the comment of @eryksun from PR 6010 for reference.

Because we only try VOLUME_NAME_DOS, this function always fails for a volume that's not mounted as either a drive letter or a junction mount point. The error in this case is ERROR_PATH_NOT_FOUND. We know that the path is valid because we have a handle (in this case the file system ensures that no parent directory in the path can be unlinked or renamed). To address this, if the flags parameter isn't already VOLUME_NAME_GUID, it could switch to it and continue the while loop (no need to realloc). Otherwise if it's already VOLUME_NAME_GUID or for any other error, the call should fail.

For DOS devices in paths (e.g. "C:\Temp\NUL"), CreateFile commonly succeeds, but GetFinalPathNameByHandle commonly fails with either ERROR_INVALID_FUNCTION or ERROR_INVALID_PARAMETER. What do you think about handling this failure by calling GetFullPathName instead (e.g. "C:\Temp\NUL" => "\\.\NUL")? That way most DOS device paths won't cause this function to fail (e.g. when called by pathlib's resolve method). We could do the same if CreateFile fails with ERROR_INVALID_PARAMETER, which should only occur for CON (e.g. "C:\Temp\CON") because it needs to be opened with either generic read or generic write access.

CreatFile also commonly fails here with either ERROR_SHARING_VIOLATION (from a paging file) or ERROR_ACCESS_DENIED. But I think those are best handled by the caller, with a PermissionError exception handler. Currently pathlib's resolve method doesn't handle PermissionError like I think it should in non-strict mode. It only handles FileNotFoundError

[zooba]

New changeset 32efcd1 by Steve Dower in branch '3.6':
bpo-33016: Fix potential use of uninitialized memory in nt._getfinalpathname (GH-6032)
32efcd1

[zooba]

Leaving this open for commit review (and buildbots) for a little while, but then I'll close it if we're all good.

[izbyshev]

Because we only try VOLUME_NAME_DOS, this function always fails for a volume that's not mounted as either a drive letter or a junction mount point.

If a volume is not mounted, users have to use \\?\ or \\.\ either directly or indirectly via a symlink or a junction to get to it, right? Do you think such uses are common enough to warrant dealing with VOLUME_NAME_GUID? Also note that pathlib currently expects DOS paths only: it will strip '\\?\' prefix in resolve(), making GUID path "invalid" (and, also, precluding direct usage of '\\?\' prefix with resolve() in other cases unless users are not prepared to deal with it).

[izbyshev]

unless users are not prepared to deal with it
ARE prepared

What do you think about handling this failure by calling GetFullPathName instead (e.g. "C:\Temp\NUL" => "\\.\NUL")?

I think it would indeed be nice if pathlib handled such paths in its resolve(), especially since os.path.abspath() does handle them, and it looks weird that even resolve(strict=False) fails. That could be an enhancement, but note that it'll expose users to '\\.\'-prefixed paths which can't be returned from resolve() now. It is not necessary a problem because users should be prepared to handle UNC-like paths anyway.

Currently pathlib's resolve method doesn't handle PermissionError like I think it should in non-strict mode. It only handles FileNotFoundError

That behavior doesn't look good, and it's inconsistent with POSIX resolve() which doesn't propagate any OSError in non-strict mode. I think this warrants an issue report.