classification
Title: winreg.SetValueEx should check the returned value
Type: behavior Stage: patch review
Components: Windows Versions: Python 3.11, Python 3.10, Python 3.9
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: eryksun, paul.moore, r3pwnx, shreyanavigyan, steve.dower, stutzbach, tim.golden, zach.ware
Priority: normal Keywords: patch

Created on 2021-04-30 08:42 by r3pwnx, last changed 2021-05-01 09:48 by shreyanavigyan.

Files
File name Uploaded Description Edit
winreg_bug.py r3pwnx, 2021-04-30 08:42
Pull Requests
URL Status Linked Edit
PR 25775 open shreyanavigyan, 2021-05-01 09:48
Messages (5)
msg392392 - (view) Author: (r3pwnx) Date: 2021-04-30 08:42
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", `Py2Reg`is 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
msg392459 - (view) Author: Shreyan Avigyan (shreyanavigyan) * Date: 2021-04-30 16:03
+1 on my side.
msg392482 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2021-04-30 17:34
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
msg392485 - (view) Author: Shreyan Avigyan (shreyanavigyan) * Date: 2021-04-30 17:58
I have a patch ready for this issue. Should I convert it to a PR?
msg392578 - (view) Author: Shreyan Avigyan (shreyanavigyan) * Date: 2021-05-01 09:48
I'm attaching a PR to fix this issue.
History
Date User Action Args
2021-05-01 09:48:29shreyanavigyansetkeywords: + patch
stage: needs patch -> patch review
pull_requests: + pull_request24465
2021-05-01 09:48:16shreyanavigyansetmessages: + msg392578
2021-04-30 17:58:55shreyanavigyansetmessages: + msg392485
2021-04-30 17:34:05eryksunsettype: security -> behavior
title: [security] winreg.SetValueEx should check the returned value -> winreg.SetValueEx should check the returned value
components: + Windows, - Library (Lib)

nosy: + eryksun
versions: + Python 3.10, Python 3.11
messages: + msg392482
stage: needs patch
2021-04-30 16:05:13shreyanavigyansettitle: [security]winreg.SetValueEx shoule check the returned value -> [security] winreg.SetValueEx should check the returned value
2021-04-30 16:03:57shreyanavigyansetnosy: + paul.moore, tim.golden, stutzbach, zach.ware, steve.dower, shreyanavigyan
messages: + msg392459
2021-04-30 08:42:01r3pwnxcreate