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

winreg.SetValueEx should check the returned value #88150

Closed
r3pwnx mannequin opened this issue Apr 30, 2021 · 5 comments
Closed

winreg.SetValueEx should check the returned value #88150

r3pwnx mannequin opened this issue Apr 30, 2021 · 5 comments
Labels
3.9 only security fixes 3.10 only security fixes 3.11 bug and security fixes OS-windows type-bug An unexpected behavior, bug, or error

Comments

@r3pwnx
Copy link
Mannequin

r3pwnx mannequin commented Apr 30, 2021

BPO 43984
Nosy @pfmoore, @tjguk, @zware, @eryksun, @zooba, @shreyanavigyan
PRs
  • bpo-43984: Update winreg.SetValueEx to make sure the value set is not -1 #25775
  • Files
  • winreg_bug.py
  • 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 = None
    created_at = <Date 2021-04-30.08:42:01.576>
    labels = ['3.10', 'type-bug', '3.9', 'OS-windows', '3.11']
    title = 'winreg.SetValueEx should check the returned value'
    updated_at = <Date 2021-05-01.09:48:29.376>
    user = 'https://bugs.python.org/r3pwnx'

    bugs.python.org fields:

    activity = <Date 2021-05-01.09:48:29.376>
    actor = 'shreyanavigyan'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['Windows']
    creation = <Date 2021-04-30.08:42:01.576>
    creator = 'r3pwnx'
    dependencies = []
    files = ['50001']
    hgrepos = []
    issue_num = 43984
    keywords = ['patch']
    message_count = 5.0
    messages = ['392392', '392459', '392482', '392485', '392578']
    nosy_count = 8.0
    nosy_names = ['paul.moore', 'tim.golden', 'stutzbach', 'zach.ware', 'eryksun', 'steve.dower', 'shreyanavigyan', 'r3pwnx']
    pr_nums = ['25775']
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue43984'
    versions = ['Python 3.9', 'Python 3.10', 'Python 3.11']

    @r3pwnx
    Copy link
    Mannequin Author

    r3pwnx mannequin commented Apr 30, 2021

    The library winreg[1] can be used to access registry on windows.

    reg.SetValueEx(key, 'X', 0, reg.REG_DWORD, -1337)
    SetValueEx could store data in the value field of an open registry key,

    In the source file of "PC/winreg.c", Py2Regis called first

    static PyObject *
    winreg_SetValueEx_impl(PyObject *module, HKEY key,
                           const Py_UNICODE *value_name, PyObject *reserved,
                           DWORD type, PyObject *value)
    /*[clinic end generated code: output=811b769a66ae11b7 input=900a9e3990bfb196]*/
    {
        BYTE *data;
        DWORD len;
    
        LONG rc;
    
        if (!Py2Reg(value, type, &data, &len))
        ...
    

    Py2Reg is implemented in the same file:

    Py2Reg(PyObject *value, DWORD typ, BYTE **retDataBuf, DWORD *retDataSize)
    {
        Py_ssize_t i,j;
        switch (typ) {
            case REG_DWORD:
                if (value != Py_None && !PyLong_Check(value))
                    return FALSE;
                *retDataBuf = (BYTE *)PyMem_NEW(DWORD, 1);
                if (*retDataBuf == NULL){
                    PyErr_NoMemory();
                    return FALSE;
                }
                *retDataSize = sizeof(DWORD);
                if (value == Py_None) {
                    DWORD zero = 0;
                    memcpy(*retDataBuf, &zero, sizeof(DWORD));
                }
                else {
                    DWORD d = PyLong_AsUnsignedLong(value);
                    memcpy(*retDataBuf, &d, sizeof(DWORD));
                }
                break;
    

    When the type is set with reg.REG_DWORD, PyLong_AsUnsignedLong(value) will change the value's type, and then memcpy the returned value directly, without any check.

    In the Objects/longobject.c, as the comment said:

    /* Get a C unsigned long int from an int object.
    Returns -1 and sets an error condition if overflow occurs. */

    If PyLong_AsUnsignedLong return -1, the -1 will be stored in the registry though the error occured

    PoC:

    import winreg as reg
    
    key = reg.CreateKey(reg.HKEY_CURRENT_USER, 'SOFTWARE\\Classes\\r3pwn')

    try:
    print("Set Subkey X: -1337")
    reg.SetValueEx(key, 'X', 0, reg.REG_DWORD, -1337)
    except Exception as e:
    print("Get Subkey: ", reg.QueryValueEx(key, "X")[0])

    try:
    print("Set Subkey Y: 2**33")
    reg.SetValueEx(key, 'Y', 0, reg.REG_DWORD, 2**33)
    except Exception as e:
    print("Get Subkey: ", reg.QueryValueEx(key, "Y")[0])

    The Test Environment
    OS: Windows
    Python Version: 3.9.0

    python winreg_bug.py
    Set Subkey X: -1337
    Get Subkey: 4294967295
    Set Subkey Y: 2**33
    Get Subkey: 4294967295

    the return value should be checked:

     DWORD d = PyLong_AsUnsignedLong(value);
    

    + if (d == (unsigned long)-1 && PyErr_Occurred())
    + return False;
    memcpy(*retDataBuf, &d, sizeof(DWORD));

    [1] https://docs.python.org/3.9/library/winreg.html#winreg.SetValueEx

    @r3pwnx r3pwnx mannequin added type-security A security issue stdlib Python modules in the Lib dir 3.9 only security fixes labels Apr 30, 2021
    @shreyanavigyan
    Copy link
    Mannequin

    shreyanavigyan mannequin commented Apr 30, 2021

    +1 on my side.

    @shreyanavigyan shreyanavigyan mannequin changed the title [security]winreg.SetValueEx shoule check the returned value [security] winreg.SetValueEx should check the returned value Apr 30, 2021
    @shreyanavigyan shreyanavigyan mannequin changed the title [security]winreg.SetValueEx shoule check the returned value [security] winreg.SetValueEx should check the returned value Apr 30, 2021
    @eryksun
    Copy link
    Contributor

    eryksun commented Apr 30, 2021

    I'm changing this issue to a behavior bug. Many issues have the potential to be exploited as a security vulnerability in some contrived scenario, but the security issue type is for cases that have provably significant security implications, such as privilege escalation, which should be reported using the documented procedure [1]. Registry keys are secured, and modifying system keys requires privileged access, so a bug that sets an incorrect value is not particularly exploitable. Anyone that can set a system value already has full control of the system.

    The suggested fix is correct, except the C macro is FALSE, not False. I would simply cast to DWORD instead of unsigned long. Also, the REG_QWORD conversion has the same problem with not checking for an overflow after calling PyLong_AsUnsignedLongLong().

    ---

    [1] https://www.python.org/dev/security

    @eryksun eryksun added 3.10 only security fixes 3.11 bug and security fixes OS-windows and removed stdlib Python modules in the Lib dir labels Apr 30, 2021
    @eryksun eryksun changed the title [security] winreg.SetValueEx should check the returned value winreg.SetValueEx should check the returned value Apr 30, 2021
    @eryksun eryksun added type-bug An unexpected behavior, bug, or error 3.10 only security fixes 3.11 bug and security fixes OS-windows and removed type-security A security issue stdlib Python modules in the Lib dir labels Apr 30, 2021
    @eryksun eryksun changed the title [security] winreg.SetValueEx should check the returned value winreg.SetValueEx should check the returned value Apr 30, 2021
    @eryksun eryksun added type-bug An unexpected behavior, bug, or error and removed type-security A security issue labels Apr 30, 2021
    @shreyanavigyan
    Copy link
    Mannequin

    shreyanavigyan mannequin commented Apr 30, 2021

    I have a patch ready for this issue. Should I convert it to a PR?

    @shreyanavigyan
    Copy link
    Mannequin

    shreyanavigyan mannequin commented May 1, 2021

    I'm attaching a PR to fix this issue.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @zooba zooba closed this as completed Dec 9, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.9 only security fixes 3.10 only security fixes 3.11 bug and security fixes OS-windows type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants