python / cpython

The Python programming language
https://www.python.org
Other
63.31k stars 30.31k forks source link

sys.getwindowsversion().platform_version is incorrect #87450

Closed 89f4240f-251b-48e9-858b-0ad5ad4ede03 closed 3 years ago

89f4240f-251b-48e9-858b-0ad5ad4ede03 commented 3 years ago
BPO 43284
Nosy @pfmoore, @tjguk, @zware, @eryksun, @zooba, @corona10, @miss-islington, @shreyanavigyan, @OrbitalHorizons
PRs
  • python/cpython#25500
  • python/cpython#25523
  • python/cpython#25524
  • 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: ```python assignee = None closed_at = created_at = labels = ['3.10', 'type-bug', '3.8', '3.9', 'OS-windows'] title = 'sys.getwindowsversion().platform_version is incorrect' updated_at = user = 'https://bugs.python.org/bugalebugale' ``` bugs.python.org fields: ```python activity = actor = 'steve.dower' assignee = 'none' closed = True closed_date = closer = 'steve.dower' components = ['Windows'] creation = creator = 'bugale bugale' dependencies = [] files = [] hgrepos = [] issue_num = 43284 keywords = ['patch'] message_count = 35.0 messages = ['387450', '387931', '387979', '387980', '387981', '387984', '388814', '388833', '388868', '388925', '388937', '388945', '391221', '391234', '391329', '391333', '391334', '391338', '391346', '391368', '391390', '391392', '391402', '391409', '391410', '391424', '391425', '391461', '391476', '391528', '391611', '391613', '391617', '391706', '391719'] nosy_count = 10.0 nosy_names = ['paul.moore', 'tim.golden', 'zach.ware', 'eryksun', 'steve.dower', 'corona10', 'bugale bugale', 'miss-islington', 'shreyanavigyan', 'Orbital'] pr_nums = ['25500', '25523', '25524'] priority = 'normal' resolution = 'fixed' stage = 'resolved' status = 'closed' superseder = None type = 'behavior' url = 'https://bugs.python.org/issue43284' versions = ['Python 3.8', 'Python 3.9', 'Python 3.10'] ```

    89f4240f-251b-48e9-858b-0ad5ad4ede03 commented 3 years ago

    Running platform.platform() on Windows 10 20H2 results in the build number 19041:

    Python 3.9.0 (tags/v3.9.0:9cf6752, Oct  5 2020, 15:34:40) [MSC v.1927 64 bit (AMD64)] on win32
    Type "help", "copyright", "credits" or "license" for more information.
    >>> import platform
    >>> platform.platform()
    'Windows-10-10.0.19041-SP0'

    This is incorrect, the build number is 19042. Using ctypes like in the answer here produces a correct result: https://stackoverflow.com/questions/32300004/python-ctypes-getting-0-with-getversionex-function

    75e98f1b-5cf5-47d0-86a5-16473892fab6 commented 3 years ago

    Running platform.platform() on Windows 10 21H1 results in the build number 19041:

    Python 3.8.8 (tags/v3.8.8:024d805, Feb 19 2021, 13:18:16) [MSC v.1928 64 bit (AMD64)] on win32
    Type "help", "copyright", "credits" or "license" for more information.
    >>> import platform
    >>> platform.platform()
    'Windows-10-10.0.19041-SP0'
    This is incorrect, the correct build number is 19043.
    zooba commented 3 years ago

    So somehow Windows made a rebuild *without* having to touch kernel32.dll, which is fairly impressive.

    That version number comes from kernel32.dll, because there's no good way to get the build number via an API and also avoid compatibility settings lying about it.

    I *guess* we need to find a better API for querying the current build number, hopefully without having to set up a whole WMI query.

    89f4240f-251b-48e9-858b-0ad5ad4ede03 commented 3 years ago

    Is there a good reason to not use GetVersionEx?

    75e98f1b-5cf5-47d0-86a5-16473892fab6 commented 3 years ago

    Node Js version of os wasnt affected. If this is having an issue then they used some bootleg method to fetch the build number in the first place.

    zooba commented 3 years ago

    The reason to avoid the GetVersion API is that certain automatic compatibility modes will lie about what the version number is, and people complained that it was wrong (kind of like this complaint ;) ). Reading the version from a system DLL bypasses that risk.

    If your aim is to check the version for API compatibility/behaviour, then GetVersion is exactly the right thing to use, because it'll be adapted to match any compatibility options that are in effect. However, the platform module is not for this purpose, but is for logging system information. So we read from kernel32.dll instead (which is, yes, a bootleg way of doing it).

    Node.js has some fairly horrible ways of doing this stuff on Windows. I haven't looked into this one in particular, but if they're not using GetVersion it wouldn't surprise me if they're running "cmd /C ver" and reading the output (don't even get me started on the security risks of that).

    The most correct way is to query the system information service via WMI (which I'm 99% sure is what msinfo32.exe does). However, that's a _big_ step up in complexity, which is why we've avoided doing it to date. It might be the only viable option here if Windows is getting this good at shipping incremental rebuilds.

    75e98f1b-5cf5-47d0-86a5-16473892fab6 commented 3 years ago

    Platform seems to have been fixed as all pre release builds were fetched as intended shortly after the post was created here, however Windows 10 Version 10.0.19042 is still unable to be displayed by the Python platform module in my Python 3.8.8 system.

    zooba commented 3 years ago

    Nothing has changed in platform, and all current releases return the same version number for me (which matches the original report).

    As I said, we need to find a versioned DLL that _always_ rebuilds to extract the version number from, because that's the most reliable way I know of to avoid the compatibility shims.

    eryksun commented 3 years ago

    Note that the following recommendation for getting the system version was removed in late 2019 [1][2]:

    To obtain the full version number for the operating system,
    call the GetFileVersionInfo function on one of the system
    DLLs, such as Kernel32.dll, then call VerQueryValue to obtain
    the \\StringFileInfo\\\\ProductVersion subblock of the file 
    version information.

    The first commit added advice to check the "CurrentBuildNumber" registry value in Windows 10 1909+, but an engineer decided to just remove the paragraph. Apparently the developers do not want to guarantee that the version information on any particular system DLL can be used to get the system version. Apparently they also do not want to officially sanction using the "CurrentMajorVersionNumber", "CurrentMinorVersionNumber", and "CurrentBuildNumber" values in the registry.

    ---

    [1] https://github.com/MicrosoftDocs/win32/pull/143 [2] https://docs.microsoft.com/en-us/windows/win32/sysinfo/getting-the-system-version

    zooba commented 3 years ago

    Yeah, that all makes sense. 90%+ of the time, checking the version number is a compatibility issue waiting to happen.

    For the other 10% (diagnostic logging), it would be nice to have a better option than running "cmd /c ver", but that might be the easiest thing to do. I'd want to remove any pretence that the returned version is a comparable number, and I'm not sure how easily we can do that without hurting compatibility...

    eryksun commented 3 years ago

    For the other 10% (diagnostic logging), it would be nice to have a better option than running "cmd /c ver"

    CMD's VER command (cmd!eVersion) calls GetVersion(), for which, in its case, the API calls the internal function GetVersion_Current(). The VER command also reads the UBR value (update build revision) directly from the registry.

    Other than using CMD, I suppose there's the option of creating an extension module in C++ that gets the Win32_OperatingSystem WMI data, which includes the "major.minor.build" version string. But that's much more complicated.

    zooba commented 3 years ago

    CMD is not going to be subjected to compatibility shims that hide the version number, and I *think* those are not inherited by child processes. So it can use the basic APIs. (It's also likely to be the comparison that most people will check against.)

    I agree that using WMI is probably overkill. PyWin32 or a new extension module could handle it for people who have a more urgent need.

    1e2b3793-5146-46c5-b55f-27b9e782b291 commented 3 years ago

    When we're talking about version are we talking about the kernel32.dll version or the Windows version? If we're talking about Windows version then why not use the return value of sys.getwindowsversion() itself? How is that function getting the Windows version? Moreover shouldn't the documentation provide the information that sys.getwindowsversion().platform_version returns the kernel32.dll version?

    eryksun commented 3 years ago

    shouldn't the documentation provide the information that sys.getwindowsversion().platform_version returns the kernel32.dll version?

    platform_version is documented as an "accurate major version, minor version and build number of the current operating system, rather than the version that is being emulated for the process". That's it's currently using the file version of "kernel32.dll" is an implementation detail, one that needs to be changed to something more reliable.

    Keep in mind that the Python DLL can be embedded in any application, which may not be manifested to get the true OS version via GetVersionExW(). For example, when running in Windows 10, the default with no manifest reports Windows 8 (NT 6.2):

        >>> sys.getwindowsversion()
        sys.getwindowsversion(major=6, minor=2, build=9200, platform=2, service_pack='')

    Here it is with a manifest that supports Windows 8.1 (NT 6.3):

        >>> sys.getwindowsversion()
        sys.getwindowsversion(major=6, minor=3, build=9600, platform=2, service_pack='')

    This is the OS version that's relevant to the application, but for logging and other diagnostics, platform_version is provided to get the real version, at least if all goes according to plan.

    1e2b3793-5146-46c5-b55f-27b9e782b291 commented 3 years ago

    @eryksun described "Apparently the developers do not want to guarantee that the version information on any particular system DLL can be used to get the system version. Apparently they also do not want to officially sanction using the "CurrentMajorVersionNumber", "CurrentMinorVersionNumber", and "CurrentBuildNumber" values in the registry."

    If that's the case why not get the version from CurrentBuild registry key present in HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows NT\CurrentVersion. This key at least goes back to Windows 7 and this is the key that's currently being used by winver command. And I don't think anyone at all uses windows version older than Windows 7. And chances are that the key maybe present even before Windows 7. At least the CurrentBuild is accurate.

    1e2b3793-5146-46c5-b55f-27b9e782b291 commented 3 years ago

    I researched a little more and found that before Vista the winver command used the CurrentBuildNumber key instead of CurrentBuild key. In fact before Vista CurrentBuild was marked as obsolete by Microsoft. But that changed in Vista, when Microsoft started using CurrentBuild key instead of the CurrentBuildNumber key. Though still today CurrentBuildNumber key actually gives the exact version (for backward compatibilities maybe?). Therefore why not use CurrentBuildNumber? At least CurrentBuildNumber gives the right info. The only problem is that we have to only use that key to determine the windows version (Eg - Windows 10, 8.1, 8, 7, Vista, etc.) and to do that we have to use a very long switch or if-else statement.

    https://en.wikipedia.org/wiki/Comparison_of_Microsoft_Windows_versions#Windows_NT has the whole list with the column "RTM build" as the CurrentBuildNumber value.

    eryksun commented 3 years ago

    The "CurrentBuild" and "CurrentVersion" values go back to the first release of Windows NT 3.1 in 1993 (build 511, which was quickly replaced by build 528). In NT 3.1 (build 528), the "CurrentBuild" value was "1.528.1 () (July 1993)". In NT 3.5, this awkward string was replaced by the "CurrentBuildNumber" value, and up to NT 5.2 the old "CurrentBuild" value was set to the string "1.511.1 () (Obsolete data - do not use)". It was brought back as the build number starting with Windows Vista (NT 6.0). However, in Windows 10 the related "CurrentVersion" value is now obsolete and frozen at "6.3". The true value is now split into "CurrentMajorVersionNumber" and "CurrentMinorVersionNumber".

    These registry values aren't a reliable source source of truth. The reliable values are the kernel global variables NtMajorVersion, NtMinorVersion, and NtBuildNumber, which are compiled into the kernel image. They get copied into the process environment block (PEB) of each process as OSMajorVersion, OSMinorVersion, and OSBuildNumber. If the application manifest supports the current OS version, then GetVersion() and GetVersionExW() will simply return these values from the PEB. That's why it was suggested to spawn an instance of the system CMD shell and parse the output of its VER command.

    1e2b3793-5146-46c5-b55f-27b9e782b291 commented 3 years ago

    But kernel32.dll (since it's of a different version) isn't accurate at all right? To find the accurate result we have to use the CurrentBuildNumber registry key. I think this key will not be removed so easily by Microsoft. This key have been in existence from NT 3.5 till today. I think it's the safest and reliable way to get the version number other than WMI query. I know the key can be removed in the future but if it that happens what about reimplementing the kernel32.dll then. We need a way to get the accurate version right?

    eryksun commented 3 years ago

    But kernel32.dll (since it's of a different version) isn't accurate at all right?

    To clarify, CMD's VER command calls GetVersion(). It has nothing to do with the file version of any system DLL. Because CMD is a system component, the GetVersion() call returns the true OS version number and build number, directly from its PEB OSMajorVersion, OSMinorVersion, and OSBuildNumber values. When the PEB for a process is created, these values are copied from the kernel's NtMajorVersion, NtMinorVersion, and NtBuildNumber values.

    While the latter may be and most likely are the same as the "CurrentMajorVersionNumber", "CurrentMinorVersionNumber", and "CurrentBuildNumber" values in the registry, the kernel values are not based on the registry. They're compiled into the kernel image when it's built. At boot, the configuration manager sets the registry values to the current values, so that's all fine -- unless someone with admin or system access changes the values (but if a hacker has admin or system access; they own the OS, so such a prank would be the least of one's problems).

    My concern, if you read my previous comments, is that an engineer at Microsoft rejected someone's attempt to recommend using "CurrentBuildNumber" as a replacement for the old advice to use the file version of "kernel32.dll". That's enough to make me worry about what's planned for a future release. So I'd rather just parse the output from CMD.

    1e2b3793-5146-46c5-b55f-27b9e782b291 commented 3 years ago

    But isn't calling CMD's VER command risky? A process can overwrite its PEB OSMajorVersion, OSMinorVersion, and OSBuildNumber. If the cmd has been set to compatibility mode somehow then the same problem will occur since it will then overwrite its PEB OSMajorVersion, OSMinorVersion, and OSBuildNumber. Using a DLL or registry key bypasses this risk.

    eryksun commented 3 years ago

    But isn't calling CMD's VER command risky? A process can overwrite its PEB OSMajorVersion, OSMinorVersion, and OSBuildNumber.

    As long as VER is executed without quotes, the shell will not search for an external command. CMD is not going to intentionally overwrite the OS version and build number in the PEB, so that would be due to some kind of weird, unlikely bug. (CMD's code base is stable. It hasn't seen a new feature since MKLINK was added 15 years ago.) If malware messes with this, for some strange reason, it's not Python's problem. For example, I'll attach a debugger and do just that:

    0:000> ?? @$peb->OSMajorVersion = 11
    unsigned long 0xb
    0:000> ?? @$peb->OSMinorVersion = 0
    unsigned long 0
    0:000> ?? @$peb->OSBuildNumber = 0x8000
    unsigned short 0x8000
    
    C:\>ver
    Microsoft Windows [Version 11.0.32768.0]

    Hi from the future. :)

    If the cmd has been set to compatibility mode

    "%SystemRoot%\System32\cmd.exe" is exempt from compatibility mode (e.g. the "__COMPAT_LAYER" environment variable).

    1e2b3793-5146-46c5-b55f-27b9e782b291 commented 3 years ago

    Ok. So are you planning to implement this fix?

    zooba commented 3 years ago

    Python is a volunteer built project, so someone will need to volunteer to write the fix. Until there is a volunteer, there is no plan. (One of the core devs might volunteer, but there's no guarantee of that.)

    If we're going to launch cmd.exe, I'd prefer to only do that in the platform module and not the sys function. Nothing in sys should start a subprocess (if we can at all avoid it).

    eryksun commented 3 years ago

    If we're going to launch cmd.exe, I'd prefer to only do that in the platform module and not the sys function. Nothing in sys should start a subprocess (if we can at all avoid it).

    In that case, would you want to deprecate sys.getwindowsversion().platform_version?

    platform._syscmd_ver() is already implemented to parse the output of CMD's VER command. The result has to be post-processed because the regex isn't as exact as it could be. It supports versions back to Windows 2000, which returned "Microsoft Windows 2000 [Version maj.min.build]". Starting with Windows Vista up to early versions of Windows 10, the format is "Microsoft Windows [Version maj.min.build]". In more recent versions of Windows 10, it includes the update build revision number -- "Microsoft Windows [Version maj.min.build.ubr]".

    zooba commented 3 years ago

    In that case, would you want to deprecate sys.getwindowsversion().platform_version?

    Yeah, but I'm not so concerned about raising a warning on use. Just in the docs will be fine. We should also add a mention that it is extracting the value from efficient but unstable system locations that may change in future releases. (That is not my suggested wording btw, just the gist of what we should mention.)

    1e2b3793-5146-46c5-b55f-27b9e782b291 commented 3 years ago

    I would like to help to write the fix for this issue.

    1e2b3793-5146-46c5-b55f-27b9e782b291 commented 3 years ago

    For Windows 10.0.17134 (1803) or above why don't we use the format "maj.min.build.ubr" in platform._norm_version (the function that post-processes the version string and then returns the version string in the format "maj.min.build")?

    1e2b3793-5146-46c5-b55f-27b9e782b291 commented 3 years ago

    I have a patch for this issue. Should I attach it or should I directly open a PR?

    zooba commented 3 years ago

    Opening a PR against the master branch would be ideal. (Make sure you forked from that branch too, or you may see lots of irrelevant changes, which will make it impossible to review.)

    1e2b3793-5146-46c5-b55f-27b9e782b291 commented 3 years ago

    PR opened against master branch.

    zooba commented 3 years ago

    New changeset 2a3f4899c63806439e5bcea0c30f7e6a6295a763 by Shreyan Avigyan in branch 'master': bpo-43284: Update platform.win32_ver to use _syscmd_ver instead of sys.getwindowsversion() (GH-25500) https://github.com/python/cpython/commit/2a3f4899c63806439e5bcea0c30f7e6a6295a763

    zooba commented 3 years ago

    Thanks for doing the patch!

    miss-islington commented 3 years ago

    New changeset ef63328b46fe7402794cde51008a47e79f37b153 by Miss Islington (bot) in branch '3.8': bpo-43284: Update platform.win32_ver to use _syscmd_ver instead of sys.getwindowsversion() (GH-25500) https://github.com/python/cpython/commit/ef63328b46fe7402794cde51008a47e79f37b153

    1e2b3793-5146-46c5-b55f-27b9e782b291 commented 3 years ago

    @steve.dower Would you mind merging the backport of the PR to the 3.9 branch manually? Apparently there seems to a test failure. I'm not sure why. But it reports that test has been failing recently.

    zooba commented 3 years ago

    New changeset 52e9031fbd23c10668badc2a72ee5c203d6902c7 by Miss Islington (bot) in branch '3.9': bpo-43284: Update platform.win32_ver to use _syscmd_ver instead of sys.getwindowsversion() (GH-25500) https://github.com/python/cpython/commit/52e9031fbd23c10668badc2a72ee5c203d6902c7