Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Windows] Inconsistent os.stat behavior for directory with Access Denied #83003

Closed
CrouZ mannequin opened this issue Nov 16, 2019 · 12 comments
Closed

[Windows] Inconsistent os.stat behavior for directory with Access Denied #83003

CrouZ mannequin opened this issue Nov 16, 2019 · 12 comments
Labels
3.8 only security fixes 3.9 only security fixes 3.10 only security fixes extension-modules C modules in the Modules dir OS-windows type-bug An unexpected behavior, bug, or error

Comments

@CrouZ
Copy link
Mannequin

CrouZ mannequin commented Nov 16, 2019

BPO 38822
Nosy @pfmoore, @tjguk, @zware, @eryksun, @zooba, @miss-islington, @nw0
PRs
  • bpo-38822: Fixed os.stat failing on inaccessible directories with a trailing slash, rather than falling back to the parent directory's metadata #25527
  • [3.9] bpo-38822: Fixed os.stat failing on inaccessible directories. (GH-25527) #25530
  • [3.8] bpo-38822: Fixed os.stat failing on inaccessible directories. (GH-25527) #25531
  • bpo-38822: Check specifically for a drive, not just a colon #25540
  • [3.8] bpo-38822: Check specifically for a drive, not just a colon (GH-25540) #25543
  • [3.9] bpo-38822: Check specifically for a drive, not just a colon (GH-25540) #25542
  • 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 = None
    closed_at = <Date 2021-04-22.19:48:08.500>
    created_at = <Date 2019-11-16.18:14:54.808>
    labels = ['type-bug', '3.8', '3.9', '3.10', 'extension-modules', 'OS-windows']
    title = '[Windows] Inconsistent os.stat behavior for directory with Access Denied'
    updated_at = <Date 2021-04-22.23:56:52.499>
    user = 'https://bugs.python.org/CrouZ'

    bugs.python.org fields:

    activity = <Date 2021-04-22.23:56:52.499>
    actor = 'steve.dower'
    assignee = 'none'
    closed = True
    closed_date = <Date 2021-04-22.19:48:08.500>
    closer = 'steve.dower'
    components = ['Extension Modules', 'Windows']
    creation = <Date 2019-11-16.18:14:54.808>
    creator = 'CrouZ'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 38822
    keywords = ['patch']
    message_count = 12.0
    messages = ['356763', '356885', '356915', '384729', '387445', '387504', '391625', '391628', '391629', '391632', '391641', '391642']
    nosy_count = 8.0
    nosy_names = ['paul.moore', 'tim.golden', 'CrouZ', 'zach.ware', 'eryksun', 'steve.dower', 'miss-islington', 'nw0']
    pr_nums = ['25527', '25530', '25531', '25540', '25543', '25542']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue38822'
    versions = ['Python 3.8', 'Python 3.9', 'Python 3.10']

    @CrouZ
    Copy link
    Mannequin Author

    CrouZ mannequin commented Nov 16, 2019

    After upgrading some scripts from Python 2.7 to 3.7 in Windows 10, I got different behavior that seems to be caused by inconsistent behavior for os.stat in Python 3.7.

    Python 2.7:
    >>> os.stat("D:\\System Volume Information")
    nt.stat_result ...
    >>> os.stat("D:\\System Volume Information\\")
    nt.stat_result ... (same as previous call)
    
    Python 3.7:
    >>> os.stat("D:\\System Volume Information")
    os.stat_result ...
    >>> os.stat("D:\\System Volume Information\\")
    Traceback ...
    PermissionError: [WinError 5] Access is denied: 'D:\\System Volume Information\\'
    
    
    
    What I really do is calling:
    >>> os.path.exists("D:\\System Volume Information\\")
    False (Unexpected and inconsistent. I expect the return value to be True.)
    
    Behavior for other calls:
    >>> os.path.exists("D:\\System Volume Information")
    True (OK)
    >>> os.path.isdir("D:\\System Volume Information\\")
    True (OK, but according to the documentation "Return True if path is an existing directory." where 'existing' links to os.path.exists, which returns False)

    The closest issue I could find was bpo-28075 which has already been fixed.

    @CrouZ CrouZ mannequin added 3.7 (EOL) end of life OS-windows type-bug An unexpected behavior, bug, or error labels Nov 16, 2019
    @zooba
    Copy link
    Member

    zooba commented Nov 18, 2019

    I haven't debugged it, but I'm guessing we're not handling the trailing slash properly when falling back to asking the parent directory for information.

    Looking at the actual stat result for "C:\System Volume Information" vs. "C:\Windows", most of the information is missing in the first case, which should mean that we can't access it but we know it's a directory because the entry in C:\ said so.

    @eryksun
    Copy link
    Contributor

    eryksun commented Nov 18, 2019

    In attributes_from_dir() in Modules/posixmodule.c, a trailing backslash or slash should be stripped from the lpFileName parameter of FindFirstFileW. Otherwise the call opens the directory via NtOpenFile, instead of opening its parent directory. Even if opening the directory is successful, which we don't expect in this case, FindFirstFileW forcibly fails the call with ERROR_FILE_NOT_FOUND (2) because it expects a filename filter (e.g. "*") for the internal NtQueryDirectoryFile[Ex] system call.

    Care needs to be taken to not strip the trailing slash of the root directory of a DOS drive because that creates a drive-relative path (e.g. "C:"). It is expected that FindFirstFileW will fail for the root of a DOS drive or UNC share, since there's no parent directory to open.

    ----

    "System Volume Information" explicitly grants access only to the SYSTEM account. Implicitly we have read-attributes access to this directory because we have read-data (i.e. list-directory) access to the root directory. Great, but even for 0 desired access, CreateFileW requests both read-attributes and synchronize access, even for overlapped I/O (i.e. kernel File objects created by CreateFileW can always be waited on). So even an elevated administrator normally can't open this directory to query information. However, backup and restore privileges are in effect when an open requests backup semantics, which we already do. We could extend os.stat to temporarily enable SeBackupPrivilege if the caller has it.

    It's also possible to open the directory with a native NtOpenFile or NtCreateFile system call, without the FILE_SYNCHRONOUS_IO_NONALERT option and without requesting SYNCHRONIZE access -- i.e. the File object will be asynchronous and not waitable.

    @CrouZ
    Copy link
    Mannequin Author

    CrouZ mannequin commented Jan 9, 2021

    The problem exists in Python 3.8 as well, with the difference that os.path.isdir("D:\\System Volume Information\\") also returns False.

    Tested with Python 3.8.2 and 3.8.7.

    @CrouZ CrouZ mannequin added the 3.8 only security fixes label Jan 9, 2021
    @CrouZ
    Copy link
    Mannequin Author

    CrouZ mannequin commented Feb 21, 2021

    The problem exists in Python 3.9 as well, and it behaves the same as Python 3.8.

    Tested with Python 3.9.1.

    @CrouZ CrouZ mannequin added the 3.9 only security fixes label Feb 21, 2021
    @eryksun
    Copy link
    Contributor

    eryksun commented Feb 22, 2021

    Here's an implementation of attributes_from_dir() that strips trailing slashes (e.g. "C:/spam///" -> "C:/spam"), which entails copying the const string up to the last character that's not a slash. Rooted paths and drive paths that reference a root directory, such as "/" and "C:////", are special cased to avoid creating an empty path or a drive-relative path (e.g. "C:", which expands to the current working directory on drive C:).

    static BOOL
    attributes_from_dir(LPCWSTR pszFile, BY_HANDLE_FILE_INFORMATION *info,
                        ULONG *reparse_tag)
    {
        BOOL result = TRUE;
        HANDLE hFind;
        WIN32_FIND_DATAW fileData;
        LPWSTR filename = (LPWSTR)pszFile;
        size_t len, n;
    
        /* Issue 38822: trailing slashes must be removed. */
        /* Get the length without trailing slashes. */
        n = len = wcslen(filename);
        while (n && (filename[n - 1] == L'\\' || filename[n - 1] == L'/')) {
            n--;
        }
    
        if (n != len) {
            if (n == 0 || (n == 2 && filename[1] == L':')) {
                /* A root directory such as \ or C:\ has no parent to query. */
                result = FALSE;
                goto cleanup;
            }
            /* Copy the string without trailing slashes. */
            filename = malloc((n + 1) * sizeof(WCHAR));
            if (!filename || wcsncpy_s(filename, n + 1, pszFile, n)) {
                result = FALSE;
                goto cleanup;
            }
        }
    
        hFind = FindFirstFileW(filename, &fileData);
        if (hFind == INVALID_HANDLE_VALUE) {
            result = FALSE;
            goto cleanup;
        }
        FindClose(hFind);
        find_data_to_file_info(&fileData, info, reparse_tag);
    
    cleanup:
        if (filename && filename != pszFile) {
            free(filename);
        }
        return result;
    }

    @eryksun eryksun added 3.10 only security fixes extension-modules C modules in the Modules dir and removed 3.7 (EOL) end of life labels Mar 30, 2021
    @eryksun eryksun changed the title Inconsistent os.stat behavior for directory with Access Denied [Windows] Inconsistent os.stat behavior for directory with Access Denied Mar 30, 2021
    @zooba
    Copy link
    Member

    zooba commented Apr 22, 2021

    New changeset fe63a40 by Steve Dower in branch 'master':
    bpo-38822: Fixed os.stat failing on inaccessible directories. (GH-25527)
    fe63a40

    @zooba zooba closed this as completed Apr 22, 2021
    @miss-islington
    Copy link
    Contributor

    New changeset 400bd9a by Miss Islington (bot) in branch '3.8':
    bpo-38822: Fixed os.stat failing on inaccessible directories. (GH-25527)
    400bd9a

    @miss-islington
    Copy link
    Contributor

    New changeset 8e7cebb by Miss Islington (bot) in branch '3.9':
    bpo-38822: Fixed os.stat failing on inaccessible directories. (GH-25527)
    8e7cebb

    @eryksun
    Copy link
    Contributor

    eryksun commented Apr 22, 2021

    Steve, your PR checks filename[n] == L':'. I think it should check for a drive name, i.e. n == 1 && filename[1] == L':'. For example, the VirtualBox shared-folder filesystem allows creating and accessing filenames that contain and end with colons (e.g. "spam:" or even "./X:"), and such names can be queried with FindFirstFileW(). As long as a filename can be queried, I think it should be supported by attributes_from_dir().

    @zooba
    Copy link
    Member

    zooba commented Apr 22, 2021

    New changeset 9f0b3a9 by Miss Islington (bot) in branch '3.8':
    bpo-38822: Check specifically for a drive, not just a colon (GH-25540)
    9f0b3a9

    @zooba
    Copy link
    Member

    zooba commented Apr 22, 2021

    New changeset 2857592 by Miss Islington (bot) in branch '3.9':
    bpo-38822: Check specifically for a drive, not just a colon (GH-25540)
    2857592

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.8 only security fixes 3.9 only security fixes 3.10 only security fixes extension-modules C modules in the Modules dir OS-windows type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants