This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: Use after free in siftdown (2)
Type: crash Stage: patch review
Components: Versions: Python 3.4, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: rhettinger Nosy List: Arfrever, pkt, python-dev, rhettinger, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2015-05-01 14:12 by pkt, last changed 2022-04-11 14:58 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
poc_siftdown2.py pkt, 2015-05-01 14:12
sc2.diff rhettinger, 2015-05-01 16:55 Update to Py3.5 version without DECREFs
Messages (7)
msg242317 - (view) Author: paul (pkt) Date: 2015-05-01 14:12
# _siftdown(PyListObject *heap, Py_ssize_t startpos, Py_ssize_t pos)
#     ...
#     while (pos > startpos){
#         parentpos = (pos - 1) >> 1;
#         parent = PyList_GET_ITEM(heap, parentpos);
# 1       cmp = PyObject_RichCompareBool(newitem, parent, Py_LT);
#         ...
# 2       if (size != PyList_GET_SIZE(heap)) {
#             Py_DECREF(newitem);
#             PyErr_SetString(PyExc_RuntimeError,
#                             "list changed size during iteration");
#             return -1;
#         }
#         if (cmp == 0)
# 3           break;
#         ...
#     }
# 4   Py_DECREF(PyList_GET_ITEM(heap, pos));
# 5   PyList_SET_ITEM(heap, pos, newitem);
# 
# 1. custom compare function replaces object at index "pos" with a fresh 
#    instance with refcnt==1
# 2. check is ineffective, since mutation was done without altering size
# 3. break out of the loop
# 4. refcnt drops to 0 and __del__ method is called. Destructed clears the heap
# 5. SET_ITEM doesn't do any bounds checking and does a wild write. 
#
# "pos" is under our control and is restricted only by the amount of free 
# memory. pos==X requires heap of size X-1.
# 
# gX global var is necessary. Without it, python crashes in debug checks inside
# Py_ForgetReference. Seems like clearing L puts objects in a bad state.
#
# GDB
# ---
# Program received signal SIGSEGV, Segmentation fault.
# 0x4002ed73 in _siftdown (heap=0x4058edfc, startpos=0, pos=112233) at /home/p/Python-3.4.1/Modules/_heapqmodule.c:58
# 58          PyList_SET_ITEM(heap, pos, newitem);
# (gdb) print *heap
# $1 = {ob_base = {ob_base = {_ob_next = 0x405913f4, _ob_prev = 0x4058ee6c, ob_refcnt = 2, ob_type = 0x830e1c0 <PyList_Type>}, 
#       ob_size = 0}, ob_item = 0x0, allocated = 0}
# (gdb) print pos
# $2 = 112233
msg242333 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2015-05-01 16:55
Paul, this was nice work.  Thanks.

Attaching a patch to make 3.4 match the Python 3.5 version of the code which rearranges the object pointers without changing their reference counts.

With that patch, your crasher no longer seg-faults, but gives this instead:

len(L): 112234
__del__
__del__
Exception ignored in: <bound method X.__del__ of <__main__.X object at 0x10efc9080>>
Traceback (most recent call last):
  File "heap_crasher.py", line 18, in __del__
IndexError: list index out of range
msg242409 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-05-02 17:07
New changeset 813854f49f9d by Raymond Hettinger in branch '3.4':
Issues #24099, #24100, and #24101: Fix free-after-use bug in heapq.
https://hg.python.org/cpython/rev/813854f49f9d
msg242412 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-05-02 17:19
It would be nice to add tests based on provided demo scripts.
msg242414 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-05-02 17:27
New changeset d356e68de236 by Raymond Hettinger in branch '2.7':
Issues #24099, #24100, and #24101: Fix free-after-use bug in heapq.
https://hg.python.org/cpython/rev/d356e68de236
msg242436 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2015-05-02 21:42
Go ahead and add tests if you like.  I don't see the point.
The fix was to simply backport code that was already in Py3.5.
and the code exercised by the exploit scripts is no longer there.
msg242443 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2015-05-03 01:02
I should add that the crasher scripts share common features and could perhaps be built into a unified test tool.  Also, some care should be taken to make the tool mostly independent of non-guaranteed "del" behavior or CPython specific ref-counting logic to trigger a particular sequence of events.  For example, pypy gives a different result than CPython 3.5 for for the various poc_sift* scripts.  The reason is that the "__del__" method isn't reliably triggered in a gc-only environment.
History
Date User Action Args
2022-04-11 14:58:16adminsetgithub: 68288
2015-05-04 11:22:45rhettingersetstatus: open -> closed
resolution: fixed
2015-05-03 06:51:12Arfreversetnosy: + Arfrever
2015-05-03 01:02:17rhettingersetmessages: + msg242443
2015-05-02 21:42:11rhettingersetmessages: + msg242436
2015-05-02 17:27:06python-devsetmessages: + msg242414
2015-05-02 17:19:02serhiy.storchakasetmessages: + msg242412
2015-05-02 17:07:44python-devsetnosy: + python-dev
messages: + msg242409
2015-05-02 04:51:10serhiy.storchakasetstage: patch review
2015-05-02 04:50:40serhiy.storchakasetnosy: + serhiy.storchaka
2015-05-01 16:55:09rhettingersetfiles: + sc2.diff
keywords: + patch
messages: + msg242333

versions: + Python 2.7
2015-05-01 16:30:23rhettingersetassignee: rhettinger

nosy: + rhettinger
2015-05-01 14:12:21pktcreate