| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
Sorry, something went wrong.
| 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 |
There was a problem hiding this comment.
Too verbose:
| 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. |
Sorry, something went wrong.
There was a problem hiding this comment.
Please add the test repro as a regression test.
Sorry, something went wrong.
|
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 phrase I 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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
I checked the HMAC implementation in _hashopenssl.c. It uses the same state->hashtable that is initialized by py_hashentry_table_new(). There's no separate hashtable for HMAC. So this fix covers both hash functions and HMAC. |
Sorry, something went wrong.
|
I'm sorry, I meant the hmacmodule.c |
Sorry, something went wrong.
Yes, hmacmodule.c has the same issue in py_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_LINKShould I include this fix in the same PR or create a separate one? |
Sorry, something went wrong.
|
Yes, please do so, and put a comment as well. |
Sorry, something went wrong.
I investigated adding a regression test using _testcapi.set_nomemory, but found it's not feasible for this specific bug. The set_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? |
Sorry, something went wrong.
| 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() */ \ |
There was a problem hiding this comment.
| /* entry may already be in ht, will be freed by _Py_hashtable_destroy() */ \ | |
| /* entry may already be in ht, will be \ | |
| * freed by _Py_hashtable_destroy() */ \ |
Sorry, something went wrong.
You have an ASASN reproducer, why not use it? |
Sorry, something went wrong.
| import tempfile | ||
| import threading | ||
| import unittest | ||
| import subprocess |
There was a problem hiding this comment.
Put the imports alphabetically sorted.
Sorry, something went wrong.
| try: | ||
| import _testcapi | ||
| if not hasattr(_testcapi, 'set_nomemory'): | ||
| self.skipTest('requires _testcapi.set_nomemory') | ||
| except ImportError: | ||
| self.skipTest('requires _testcapi') |
There was a problem hiding this comment.
Don't use this, use import_helper for that.
Sorry, something went wrong.
| if '_hashlib' in sys.modules: | ||
| del sys.modules['_hashlib'] |
There was a problem hiding this comment.
I don't think this is necessary.
Sorry, something went wrong.
| _testcapi.remove_mem_hooks() | ||
| """) | ||
|
|
||
| rc = subprocess.call( |
There was a problem hiding this comment.
Use assert_python_ok instead.
Sorry, something went wrong.
| @@ -0,0 +1 @@ | |||
| Fix a crash when :mod:`hashlib` or :mod:`hmac` C extension module initialization fails. | |||
There was a problem hiding this comment.
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.
Sorry, something went wrong.
| /* entry may already be in ht, will be freed by \ | ||
| _Py_hashtable_destroy() */ \ |
There was a problem hiding this comment.
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").
Sorry, something went wrong.
| self.skipTest('requires _testcapi') | ||
|
|
||
| code = textwrap.dedent(""" | ||
| import sys |
There was a problem hiding this comment.
sys is no more necessary
Sorry, something went wrong.
|
|
||
| requires_sha3 = unittest.skipUnless(_sha3, 'requires _sha3') | ||
|
|
||
| _testcapi = import_helper.import_module('_testcapi') |
There was a problem hiding this comment.
Don't do it at at the module level, it will skip the entire test module. Do it inside the test directly.
Sorry, something went wrong.
| hash_type.value = False | ||
|
|
||
| @unittest.skipUnless(HASH is not None, 'need _hashlib') | ||
| @unittest.skipUnless(hasattr(_testcapi, 'set_nomemory'), |
There was a problem hiding this comment.
When is _testcapi lacking this function?
Sorry, something went wrong.
I think that testing using set_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 in py_hashentry_table_new on all platforms. I believe this is why the 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. |
Sorry, something went wrong.
| import_helper.import_module('_testcapi') | ||
| code = textwrap.dedent(""" | ||
| import _testcapi | ||
| _testcapi.set_nomemory(40, 41) |
There was a problem hiding this comment.
Yes, this seems flaky. It will change depending on the python's version and a platform.
Sorry, something went wrong.
| from test.support import _4G, bigmemtest | ||
| from test.support import hashlib_helper | ||
| from test.support.import_helper import import_fresh_module | ||
| from test.support import import_helper |
There was a problem hiding this comment.
nit: can we please revert this change if you decide to remove the test?
Sorry, something went wrong.
|
Oo for removig the test. |
Sorry, something went wrong.
|
Thanks for making the requested changes! @picnixz: please review the changes made to this pull request. |
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM.
But I'm not sure that the NEWS entries are worth it since it's unlikely to get a memory allocation failure on importing these C extensions.
Sorry, something went wrong.
|
Thanks @krylosov-aa for the PR, and @gpshead for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14. |
Sorry, something went wrong.
…ation (pythonGH-145321) (cherry picked from commit 6acaf65) Co-authored-by: krylosov-aa <krylosov.andrew@gmail.com> pythongh-145301: Fix double-free in hashlib and hmac initialization
|
Sorry, @krylosov-aa and @gpshead, I could not cleanly backport this to 3.13 due to a conflict. cherry_picker 6acaf659ef0fdee131bc02f0b58685da039b5855 3.13 |
Sorry, something went wrong.
|
GH-145523 is a backport of this pull request to the 3.14 branch. |
Sorry, something went wrong.
⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️Hi! The buildbot AMD64 FreeBSD14 3.14 (tier-3) has failed when building commit f426928. What do you need to do:
You can take a look at the buildbot page here: https://buildbot.python.org/#/builders/1740/builds/1059 Failed tests:
Failed subtests:
Summary of the results of the build (if available): == Click to see traceback logsTraceback (most recent call last):
File "/home/buildbot/buildarea/3.14.opsec-fbsd14/build/Lib/test/test_httpservers.py", line 1139, in test_large_content_length_truncated
res = self.request('/cgi-bin/file1.py', 'POST', b'x', headers)
File "/home/buildbot/buildarea/3.14.opsec-fbsd14/build/Lib/test/test_httpservers.py", line 132, in request
return self.connection.getresponse()
~~~~~~~~~~~~~~~~~~~~~~~~~~~^^
File "/home/buildbot/buildarea/3.14.opsec-fbsd14/build/Lib/http/client.py", line 1450, in getresponse
response.begin()
~~~~~~~~~~~~~~^^
File "/home/buildbot/buildarea/3.14.opsec-fbsd14/build/Lib/http/client.py", line 336, in begin
version, status, reason = self._read_status()
~~~~~~~~~~~~~~~~~^^
File "/home/buildbot/buildarea/3.14.opsec-fbsd14/build/Lib/http/client.py", line 305, in _read_status
raise RemoteDisconnected("Remote end closed connection without"
" response")
http.client.RemoteDisconnected: Remote end closed connection without response
|
Sorry, something went wrong.
|
@krylosov-aa Could you please take care of manually backporting to 3.13? |
Sorry, something went wrong.
Yes, I will do it. |
Sorry, something went wrong.
|
For 3.13, it is only hashopenssl that you need to change. HMAC HACL* did not exist yet. |
Sorry, something went wrong.
|
This is a nice tricky fix :-) Thanks @krylosov-aa for the fix and thanks @raminfp for the bug report! |
Sorry, something went wrong.
|
GH-145532 is a backport of this pull request to the 3.13 branch. |
Sorry, something went wrong.
|
I have created a manual backport for 3.13: see #145532 |
Sorry, something went wrong.
|
GH-145532 is a backport of this pull request to the 3.13 branch. |
Sorry, something went wrong.
…ation (pythonGH-145321) pythongh-145301: Fix double-free in hashlib and hmac initialization
| Back | FazBrowse Home | New Git URL |
Both hashlib and hmac modules have similar double-free bugs in their hashtable initialization code.
Problem
hashlib
In py_hashentry_table_new(), when _Py_hashtable_set() fails while adding an entry by its py_alias key (after successfully adding it by py_name), the code calls PyMem_Free(entry) before goto error. The error handler then calls _Py_hashtable_destroy() which frees the same entry again via py_hashentry_t_destroy_value().
hmac
In py_hmac_hinfo_ht_new(), the Py_HMAC_HINFO_LINK macro has the same issue. When py_hmac_hinfo_ht_add() fails for hashlib_name key (after successfully adding name), the code calls PyMem_Free(value) while the entry is already in the hashtable with refcnt=1.
Solution
hashlib
Remove the manual PyMem_Free(entry) call since the entry is already tracked by the hashtable under the py_name key and will be properly freed by _Py_hashtable_destroy().
hmac
Add a refcnt check before freeing: only call PyMem_Free(value) if value->refcnt == 0 (meaning it was never added to the table). If refcnt > 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:
Reproducer script (uaf_asan.py):
Before fix:
After fix: No crash.