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

sys.getwindowsversion().platform_version is incorrect #87450

Closed
bugalebugale mannequin opened this issue Feb 21, 2021 · 35 comments
Closed

sys.getwindowsversion().platform_version is incorrect #87450

bugalebugale mannequin opened this issue Feb 21, 2021 · 35 comments
Labels
3.8 only security fixes 3.9 only security fixes 3.10 only security fixes OS-windows type-bug An unexpected behavior, bug, or error

Comments

@bugalebugale
Copy link
Mannequin

bugalebugale mannequin commented Feb 21, 2021

BPO 43284
Nosy @pfmoore, @tjguk, @zware, @eryksun, @zooba, @corona10, @miss-islington, @shreyanavigyan, @OrbitalHorizons
PRs
  • bpo-43284: Update platform.win32_ver to use platform._syscmd_ver instead of sys.getwindowsversion().platform_version for determining accurate Windows version #25500
  • [3.9] bpo-43284: Update platform.win32_ver to use _syscmd_ver instead of sys.getwindowsversion() (GH-25500) #25523
  • [3.8] bpo-43284: Update platform.win32_ver to use _syscmd_ver instead of sys.getwindowsversion() (GH-25500) #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:

    assignee = None
    closed_at = <Date 2021-04-22.16:47:28.307>
    created_at = <Date 2021-02-21.10:42:26.691>
    labels = ['3.10', 'type-bug', '3.8', '3.9', 'OS-windows']
    title = 'sys.getwindowsversion().platform_version is incorrect'
    updated_at = <Date 2021-04-23.18:09:24.687>
    user = 'https://bugs.python.org/bugalebugale'

    bugs.python.org fields:

    activity = <Date 2021-04-23.18:09:24.687>
    actor = 'steve.dower'
    assignee = 'none'
    closed = True
    closed_date = <Date 2021-04-22.16:47:28.307>
    closer = 'steve.dower'
    components = ['Windows']
    creation = <Date 2021-02-21.10:42:26.691>
    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']

    @bugalebugale
    Copy link
    Mannequin Author

    bugalebugale mannequin commented Feb 21, 2021

    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

    @bugalebugale bugalebugale mannequin added 3.9 only security fixes OS-windows type-bug An unexpected behavior, bug, or error labels Feb 21, 2021
    @OrbitalHorizons
    Copy link
    Mannequin

    OrbitalHorizons mannequin commented Mar 2, 2021

    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.

    @OrbitalHorizons OrbitalHorizons mannequin added the 3.8 only security fixes label Mar 2, 2021
    @OrbitalHorizons OrbitalHorizons mannequin changed the title Wrong windows build in 20H2 Wrong windows build post version 2004 Mar 2, 2021
    @OrbitalHorizons OrbitalHorizons mannequin added the 3.8 only security fixes label Mar 2, 2021
    @OrbitalHorizons OrbitalHorizons mannequin changed the title Wrong windows build in 20H2 Wrong windows build post version 2004 Mar 2, 2021
    @zooba
    Copy link
    Member

    zooba commented Mar 2, 2021

    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.

    @bugalebugale
    Copy link
    Mannequin Author

    bugalebugale mannequin commented Mar 2, 2021

    Is there a good reason to not use GetVersionEx?

    @OrbitalHorizons
    Copy link
    Mannequin

    OrbitalHorizons mannequin commented Mar 2, 2021

    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
    Copy link
    Member

    zooba commented Mar 2, 2021

    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.

    @OrbitalHorizons
    Copy link
    Mannequin

    OrbitalHorizons mannequin commented Mar 16, 2021

    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.

    @OrbitalHorizons OrbitalHorizons mannequin changed the title Wrong windows build post version 2004 Inability to fetch build 20H2 Mar 16, 2021
    @OrbitalHorizons OrbitalHorizons mannequin changed the title Wrong windows build post version 2004 Inability to fetch build 20H2 Mar 16, 2021
    @zooba
    Copy link
    Member

    zooba commented Mar 16, 2021

    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
    Copy link
    Contributor

    eryksun commented Mar 16, 2021

    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] MicrosoftDocs/win32#143
    [2] https://docs.microsoft.com/en-us/windows/win32/sysinfo/getting-the-system-version

    @zooba
    Copy link
    Member

    zooba commented Mar 17, 2021

    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
    Copy link
    Contributor

    eryksun commented Mar 17, 2021

    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
    Copy link
    Member

    zooba commented Mar 17, 2021

    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.

    @eryksun eryksun added the 3.10 only security fixes label Apr 16, 2021
    @eryksun eryksun changed the title Inability to fetch build 20H2 sys.getwindowsversion().platform_version is incorrect Apr 16, 2021
    @eryksun eryksun added the 3.10 only security fixes label Apr 16, 2021
    @eryksun eryksun changed the title Inability to fetch build 20H2 sys.getwindowsversion().platform_version is incorrect Apr 16, 2021
    @shreyanavigyan
    Copy link
    Mannequin

    shreyanavigyan mannequin commented Apr 16, 2021

    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
    Copy link
    Contributor

    eryksun commented Apr 16, 2021

    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.

    @shreyanavigyan
    Copy link
    Mannequin

    shreyanavigyan mannequin commented Apr 18, 2021

    @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.

    @shreyanavigyan
    Copy link
    Mannequin

    shreyanavigyan mannequin commented Apr 18, 2021

    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
    Copy link
    Contributor

    eryksun commented Apr 18, 2021

    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.

    @shreyanavigyan
    Copy link
    Mannequin

    shreyanavigyan mannequin commented Apr 18, 2021

    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
    Copy link
    Contributor

    eryksun commented Apr 18, 2021

    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.

    @shreyanavigyan
    Copy link
    Mannequin

    shreyanavigyan mannequin commented Apr 19, 2021

    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
    Copy link
    Contributor

    eryksun commented Apr 19, 2021

    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).

    @shreyanavigyan
    Copy link
    Mannequin

    shreyanavigyan mannequin commented Apr 19, 2021

    Ok. So are you planning to implement this fix?

    @zooba
    Copy link
    Member

    zooba commented Apr 19, 2021

    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
    Copy link
    Contributor

    eryksun commented Apr 19, 2021

    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
    Copy link
    Member

    zooba commented Apr 19, 2021

    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.)

    @shreyanavigyan
    Copy link
    Mannequin

    shreyanavigyan mannequin commented Apr 20, 2021

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

    @shreyanavigyan
    Copy link
    Mannequin

    shreyanavigyan mannequin commented Apr 20, 2021

    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")?

    @shreyanavigyan
    Copy link
    Mannequin

    shreyanavigyan mannequin commented Apr 20, 2021

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

    @zooba
    Copy link
    Member

    zooba commented Apr 20, 2021

    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.)

    @shreyanavigyan
    Copy link
    Mannequin

    shreyanavigyan mannequin commented Apr 21, 2021

    PR opened against master branch.

    @zooba
    Copy link
    Member

    zooba commented Apr 22, 2021

    New changeset 2a3f489 by Shreyan Avigyan in branch 'master':
    bpo-43284: Update platform.win32_ver to use _syscmd_ver instead of sys.getwindowsversion() (GH-25500)
    2a3f489

    @zooba
    Copy link
    Member

    zooba commented Apr 22, 2021

    Thanks for doing the patch!

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

    New changeset ef63328 by Miss Islington (bot) in branch '3.8':
    bpo-43284: Update platform.win32_ver to use _syscmd_ver instead of sys.getwindowsversion() (GH-25500)
    ef63328

    @shreyanavigyan
    Copy link
    Mannequin

    shreyanavigyan mannequin commented Apr 23, 2021

    @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
    Copy link
    Member

    zooba commented Apr 23, 2021

    New changeset 52e9031 by Miss Islington (bot) in branch '3.9':
    bpo-43284: Update platform.win32_ver to use _syscmd_ver instead of sys.getwindowsversion() (GH-25500)
    52e9031

    @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 OS-windows type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants