winreg.SetValueEx should check the returned value

[r3pwnx]

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]

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

[shreyanavigyan]

+1 on my side.

[eryksun]

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