python/cpythonPublic

NotificationsYou must be signed in to change notification settings
Fork34.2k
Star71.8k

gh-145301: Fix double-free in hashlib and hmac module initialization#145321

Open

krylosov-aa wants to merge 1 commit intopython:mainfrom

krylosov-aa:fix-issue-145301

Open

gh-145301: Fix double-free in hashlib and hmac module initialization#145321
krylosov-aa wants to merge 1 commit intopython:mainfrom
krylosov-aa:fix-issue-145301

Conversation

Copy link

krylosov-aa commentedFeb 27, 2026•
edited
Loading

Bothhashlib andhmac modules have similar double-free bugs in their hashtable initialization code.

Problem

`hashlib`

Inpy_hashentry_table_new(), when_Py_hashtable_set() fails while adding an entry by itspy_alias key (after successfully adding it bypy_name), the code callsPyMem_Free(entry) beforegoto error. The error handler then calls_Py_hashtable_destroy() which frees the same entry again viapy_hashentry_t_destroy_value().

`hmac`

Inpy_hmac_hinfo_ht_new(), thePy_HMAC_HINFO_LINK macro has the same issue. Whenpy_hmac_hinfo_ht_add() fails forhashlib_name key (after successfully addingname), the code callsPyMem_Free(value) while the entry is already in the hashtable withrefcnt=1.

Solution

`hashlib`

Remove the manualPyMem_Free(entry) call since the entry is already tracked by the hashtable under thepy_name key and will be properly freed by_Py_hashtable_destroy().

`hmac`

Add arefcnt check before freeing: only callPyMem_Free(value) ifvalue->refcnt == 0 (meaning it was never added to the table). Ifrefcnt > 0, the entry is already in the table and will be freed by_Py_hashtable_destroy().

Tests

No test added. I couldn't find a practical way to trigger this bug since it requires_Py_hashtable_set() to fail during module initialization. This type of bug is best caught by sanitizers rather than unit tests.

The fix can be verified by running the reproducer provided by the issue author:

Build with ASan:

mkdir build-asan&&cd build-asan../configure --with-pydebug --with-address-sanitizer --without-pymallocmake -j$(nproc)

Reproducer script (uaf_asan.py):

importsubprocessimportsyscode= ("import sys, _testcapi\n""if '_hashlib' in sys.modules:\n""    del sys.modules['_hashlib']\n""_testcapi.set_nomemory(40, 41)\n""try:\n""    import _hashlib\n""except (MemoryError, ImportError):\n""    pass\n""finally:\n""    _testcapi.remove_mem_hooks()\n")result=subprocess.run(    [sys.executable,'-c',code],capture_output=True,text=True,timeout=10)ifresult.returncode!=0:print(f"[*] CRASH confirmed (rc={result.returncode})")print(f"[*]{result.stderr.strip().split(chr(10))[-1]}")else:print("[*] No crash (try different start values)")

Before fix:

$ ./build-asan/python uaf_asan.py[*] CRASH confirmed (rc=-6)[*] python: ../Include/internal/pycore_stackref.h:554: PyStackRef_FromPyObjectSteal: Assertion `obj != NULL' failed.

After fix: No crash.

Issue:_hashlib: Use-After-Free + Double Free in_hashopenssl.cpy_hashentry_table_new() #145301

krylosov-aa requested review fromgpshead andpicnixz ascode owners

February 27, 2026 16:10

Copy link

python-cla-botbot commentedFeb 27, 2026•
edited
Loading

All commit authors signed the Contributor License Agreement.

bedevere-appbot added the awaiting review label

Feb 27, 2026

bedevere-appbot mentioned this pull request

Feb 27, 2026

_hashlib: Use-After-Free + Double Free in_hashopenssl.cpy_hashentry_table_new()#145301

Open

picnixz reviewed

Feb 27, 2026

View reviewed changes

Misc/NEWS.d/next/Library/2026-02-27-19-00-26.gh-issue-145301.2Wih4b.rst Outdated

Comment on lines +1 to +3

		Fix a double-free bug in :mod:`_hashlib` module initialization when
		``_Py_hashtable_set()`` fails while adding an algorithm alias to the hash
		table after the primary name was already added

Copy link

Member

picnixzFeb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Too verbose:

Suggested change

	Fix a double-free bug in:mod:`_hashlib` module initialization when
	``_Py_hashtable_set()`` fails while adding an algorithm alias to the hash
	table after the primary name was already added
	:mod:`hashlib`: fix a crash when the C extension module initialization fails.

picnixz requested changes

Feb 27, 2026

View reviewed changes

Copy link

Member

picnixz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Please add the test repro as a regression test.

bedevere-appbot added awaiting changes and removed awaiting review labels

Feb 27, 2026

Copy link

bedevere-appbot commentedFeb 27, 2026

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phraseI have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

picnixz added needs backport to 3.13

bugs and security fixes

needs backport to 3.14bugs and security fixes labels

Feb 27, 2026

Copy link

Member

picnixz commentedFeb 27, 2026

Can you also check whether_hmac suffers from that? I don't remember whether I actually C/Ced this code or if I used another refcounting approach.

Copy link

Author

krylosov-aa commentedFeb 27, 2026

Can you also check whether_hmac suffers from that? I don't remember whether I actually C/Ced this code or if I used another refcounting approach.

I checked the HMAC implementation in_hashopenssl.c. It uses the samestate->hashtable that is initialized bypy_hashentry_table_new(). There's no separate hashtable for HMAC. So this fix covers both hash functions and HMAC.

Copy link

Member

picnixz commentedFeb 27, 2026

I'm sorry, I meant thehmacmodule.c

Copy link

Author

krylosov-aa commentedFeb 27, 2026•
edited
Loading

I'm sorry, I meant thehmacmodule.c

Yes,hmacmodule.c has the same issue inpy_hmac_hinfo_ht_new().

I can suggest the following solution:

#define Py_HMAC_HINFO_LINK(KEY)                                 \        do {                                                    \            int rc = py_hmac_hinfo_ht_add(table, KEY, value);   \            if (rc < 0) {                                       \+                if (value->refcnt == 0) {                      \                    PyMem_Free(value);                          \+                }                                              \                goto error;                                     \            }                                                   \            else if (rc == 1) {                                 \                value->refcnt++;                                \            }                                                   \        } while (0)        Py_HMAC_HINFO_LINK(e->name);        Py_HMAC_HINFO_LINK(e->hashlib_name);#undef Py_HMAC_HINFO_LINK

Should I include this fix in the same PR or create a separate one?

Copy link

Member

picnixz commentedFeb 27, 2026

Yes, please do so, and put a comment as well.

Copy link

Author

krylosov-aa commentedFeb 27, 2026•
edited
Loading

Please add the test repro as a regression test.

I investigated adding a regression test using_testcapi.set_nomemory, but found it's not feasible for this specific bug.

Theset_nomemory() approach from the issue report triggers allocation failures globally, and the exact allocation number that hits our specific code path varies by platform/configuration.

Do you have any suggestions for how to reliably test this?

krylosov-aa changed the title~~gh-145301: Fix double-free in _hashlib module initialization~~gh-145301: Fix double-free in hashlib and hmac module initialization

Feb 27, 2026

picnixz reviewed

Feb 27, 2026

View reviewed changes

Modules/hmacmodule.c Outdated

		int rc = py_hmac_hinfo_ht_add(table, KEY, value); \
		if (rc < 0) { \
		PyMem_Free(value); \
		/* entry may already be in ht, will be freed by _Py_hashtable_destroy() */ \

Copy link

Member

picnixzFeb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Suggested change

	/* entry may already be in ht, will befreed by _Py_hashtable_destroy() */ \
	/*entrymayalreadybeinht,willbe\
	freedby_Py_hashtable_destroy()/ \

correctly align the\. You can augment the column if it does not exceed 80 chars. If it does, properly wrap the comment. Tia.

Copy link

Member

picnixz commentedFeb 27, 2026

Do you have any suggestions for how to reliably test this?

You have an ASASN reproducer, why not use it?

picnixz reviewed

Feb 27, 2026

View reviewed changes

Lib/test/test_hashlib.py Outdated

		import tempfile
		import threading
		import unittest
		import subprocess

Copy link

Member

picnixzFeb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Put the imports alphabetically sorted.

Lib/test/test_hashlib.py Outdated

Comment on lines +1210 to +1215

		try:
		import _testcapi
		if not hasattr(_testcapi, 'set_nomemory'):
		self.skipTest('requires _testcapi.set_nomemory')
		except ImportError:
		self.skipTest('requires _testcapi')

Copy link

Member

picnixzFeb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Don't use this, useimport_helper for that.

Lib/test/test_hashlib.py Outdated

Comment on lines +1221 to +1222

		if '_hashlib' in sys.modules:
		del sys.modules['_hashlib']

Copy link

Member

picnixzFeb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I don't think this is necessary.

Lib/test/test_hashlib.py Outdated

		_testcapi.remove_mem_hooks()
		""")

		rc = subprocess.call(

Copy link

Member

picnixzFeb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Useassert_python_ok instead.

Misc/NEWS.d/next/Library/2026-02-27-19-00-26.gh-issue-145301.2Wih4b.rst Outdated

		@@ -0,0 +1 @@
		Fix a crash when :mod:`hashlib` or :mod:`hmac` C extension module initialization fails.

Copy link

Member

picnixzFeb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Can you create two NEWS entries, one for hashlib and for hmac please with the same sentence:

:mod:`<name>`: fix a crash when the initialization of the underlying C extension module fails.

Modules/hmacmodule.c Outdated

Comment on lines +1460 to +1461

		/* entry may already be in ht, will be freed by \
		_Py_hashtable_destroy() */ \

Copy link

Member

picnixzFeb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Just put re-align the\ directly and put the comment on one line (you can remove the "will be freed by _Py_hashtable_destroy() and mention that it will be freed "upon exit").

picnixz reviewed

Feb 27, 2026

View reviewed changes

Lib/test/test_hashlib.py Outdated

		self.skipTest('requires _testcapi')

		code = textwrap.dedent("""
		import sys

Copy link

Member

picnixzFeb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

sys is no more necessary

picnixz reviewed

Feb 27, 2026

View reviewed changes

Lib/test/test_hashlib.py Outdated


		requires_sha3 = unittest.skipUnless(_sha3, 'requires _sha3')

		_testcapi = import_helper.import_module('_testcapi')

Copy link

Member

picnixzFeb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Don't do it at at the module level, it will skip the entire test module. Do it inside the test directly.

picnixz reviewed

Feb 27, 2026

View reviewed changes

Lib/test/test_hashlib.py Outdated

		hash_type.value = False

		@unittest.skipUnless(HASH is not None, 'need _hashlib')
		@unittest.skipUnless(hasattr(_testcapi, 'set_nomemory'),

Copy link

Member

picnixzFeb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

When is _testcapi lacking this function?

Copy link

Author

krylosov-aa commentedFeb 28, 2026

Do you have any suggestions for how to reliably test this?
You have an ASASN reproducer, why not use it?

I think that testing usingset_nomemory(40, 41) is a bad idea because the number of memory allocations differs across platforms, and we cannot guarantee that allocations 40-41 will always occur inpy_hashentry_table_new on all platforms.

I believe this is whythe macOS Intel test crashes.

I don't know how to write a regression test for this scenario. I would suggest removing the test, since the fix is simple, comments have been left, and a regression seems unlikely to happen.

sobolevn reviewed

Mar 2, 2026

View reviewed changes

Lib/test/test_hashlib.py Outdated

		import_helper.import_module('_testcapi')
		code = textwrap.dedent("""
		import _testcapi
		_testcapi.set_nomemory(40, 41)

Copy link

Member

sobolevnMar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Yes, this seems flaky. It will change depending on the python's version and a platform.

Lib/test/test_hashlib.py Outdated

		from test.support import _4G, bigmemtest
		from test.support import hashlib_helper
		from test.support.import_helper importimport_fresh_module
		from test.support importimport_helper