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: bytearray pop and remove Buffer Over-read
Type: security Stage: resolved
Components: Interpreter Core Versions: Python 3.6, Python 3.4, Python 3.5, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: Arfrever, JohnLeitch, benjamin.peterson, dev_zzo, python-dev, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2015-06-18 21:04 by JohnLeitch, last changed 2022-04-11 14:58 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
bytearray.pop_Buffer_Over-read.py JohnLeitch, 2015-06-18 21:04
issue24467-2.7.patch dev_zzo, 2015-06-23 08:16 review
issue24467-3.2.patch dev_zzo, 2015-06-23 08:16 review
issue24467-3.3.patch dev_zzo, 2015-06-23 08:16 review
issue24467-3.4.patch dev_zzo, 2015-06-23 08:16 review
issue24467-3.5.patch dev_zzo, 2015-06-23 08:16 review
bytearray_init_trailing_null.patch serhiy.storchaka, 2015-06-28 19:26 review
Messages (6)
msg245483 - (view) Author: John Leitch (JohnLeitch) * Date: 2015-06-18 21:04
The bytearray pop and remove methods suffer from buffer over-reads caused by memmove use under the assumption that PyByteArrayObject ob_size is less than ob_alloc, leading to a single byte over-read. This condition can be triggered by creating a bytearray from a range of length 0x10, then calling pop with a valid index:

bytearray(range(0x10)).pop(0)

The result is a memmove that reads off the end of src:

0:000> r
eax=071aeff0 ebx=00000000 ecx=071aeff1 edx=00000010 esi=06ff80c8 edi=00000010
eip=6234b315 esp=0027fc98 ebp=0027fca0 iopl=0         nv up ei pl nz na po nc
cs=0023  ss=002b  ds=002b  es=002b  fs=0053  gs=002b             efl=00000202
MSVCR90!memmove+0x5:
6234b315 8b750c          mov     esi,dword ptr [ebp+0Ch] ss:002b:0027fcac=071aeff1
0:000> dV
            dst = 0x071aeff0 ""
            src = 0x071aeff1 "???"
          count = 0x10
0:000> db poi(dst)
071aeff0  00 01 02 03 04 05 06 07-08 09 0a 0b 0c 0d 0e 0f  ................
071af000  ?? ?? ?? ?? ?? ?? ?? ??-?? ?? ?? ?? ?? ?? ?? ??  ????????????????
071af010  ?? ?? ?? ?? ?? ?? ?? ??-?? ?? ?? ?? ?? ?? ?? ??  ????????????????
071af020  ?? ?? ?? ?? ?? ?? ?? ??-?? ?? ?? ?? ?? ?? ?? ??  ????????????????
071af030  ?? ?? ?? ?? ?? ?? ?? ??-?? ?? ?? ?? ?? ?? ?? ??  ????????????????
071af040  ?? ?? ?? ?? ?? ?? ?? ??-?? ?? ?? ?? ?? ?? ?? ??  ????????????????
071af050  ?? ?? ?? ?? ?? ?? ?? ??-?? ?? ?? ?? ?? ?? ?? ??  ????????????????
071af060  ?? ?? ?? ?? ?? ?? ?? ??-?? ?? ?? ?? ?? ?? ?? ??  ????????????????
0:000> db poi(src)
071aeff1  01 02 03 04 05 06 07 08-09 0a 0b 0c 0d 0e 0f ??  ...............?
071af001  ?? ?? ?? ?? ?? ?? ?? ??-?? ?? ?? ?? ?? ?? ?? ??  ????????????????
071af011  ?? ?? ?? ?? ?? ?? ?? ??-?? ?? ?? ?? ?? ?? ?? ??  ????????????????
071af021  ?? ?? ?? ?? ?? ?? ?? ??-?? ?? ?? ?? ?? ?? ?? ??  ????????????????
071af031  ?? ?? ?? ?? ?? ?? ?? ??-?? ?? ?? ?? ?? ?? ?? ??  ????????????????
071af041  ?? ?? ?? ?? ?? ?? ?? ??-?? ?? ?? ?? ?? ?? ?? ??  ????????????????
071af051  ?? ?? ?? ?? ?? ?? ?? ??-?? ?? ?? ?? ?? ?? ?? ??  ????????????????
071af061  ?? ?? ?? ?? ?? ?? ?? ??-?? ?? ?? ?? ?? ?? ?? ??  ????????????????
0:000> g
(1968.1a88): Access violation - code c0000005 (first chance)
First chance exceptions are reported before any exception handling.
This exception may be expected and handled.
eax=0c0b0a09 ebx=00000000 ecx=00000004 edx=00000000 esi=071aeff1 edi=071aeff0
eip=6234b468 esp=0027fc98 ebp=0027fca0 iopl=0         nv up ei ng nz ac pe cy
cs=0023  ss=002b  ds=002b  es=002b  fs=0053  gs=002b             efl=00010297
MSVCR90!UnwindUpVec+0x50:
6234b468 8b448efc        mov     eax,dword ptr [esi+ecx*4-4] ds:002b:071aeffd=????????
0:000> k
ChildEBP RetAddr  
0027fca0 1e0856aa MSVCR90!UnwindUpVec+0x50 [f:\dd\vctools\crt_bld\SELF_X86\crt\src\Intel\MEMCPY.ASM @ 322]
0027fcc0 1e0aafd7 python27!bytearray_pop+0x8a [c:\build27\cpython\objects\bytearrayobject.c @ 2378]
0027fcd8 1e0edd10 python27!PyCFunction_Call+0x47 [c:\build27\cpython\objects\methodobject.c @ 81]
0027fd04 1e0f017a python27!call_function+0x2b0 [c:\build27\cpython\python\ceval.c @ 4033]
0027fd74 1e0f1150 python27!PyEval_EvalFrameEx+0x239a [c:\build27\cpython\python\ceval.c @ 2682]
0027fda8 1e0f11b2 python27!PyEval_EvalCodeEx+0x690 [c:\build27\cpython\python\ceval.c @ 3265]
0027fdd4 1e11707a python27!PyEval_EvalCode+0x22 [c:\build27\cpython\python\ceval.c @ 672]
0027fdec 1e1181c5 python27!run_mod+0x2a [c:\build27\cpython\python\pythonrun.c @ 1371]
0027fe0c 1e118760 python27!PyRun_FileExFlags+0x75 [c:\build27\cpython\python\pythonrun.c @ 1358]
0027fe4c 1e1190d9 python27!PyRun_SimpleFileExFlags+0x190 [c:\build27\cpython\python\pythonrun.c @ 950]
0027fe68 1e038d35 python27!PyRun_AnyFileExFlags+0x59 [c:\build27\cpython\python\pythonrun.c @ 753]
0027fee4 1d001017 python27!Py_Main+0x965 [c:\build27\cpython\modules\main.c @ 643]
0027fef0 1d0011b6 pythonw!WinMain+0x17 [c:\build27\cpython\pc\winmain.c @ 15]
0027ff80 76477c04 pythonw!__tmainCRTStartup+0x140 [f:\dd\vctools\crt_bld\self_x86\crt\src\crtexe.c @ 578]
0027ff94 7799ad1f KERNEL32!BaseThreadInitThunk+0x24
0027ffdc 7799acea ntdll!__RtlUserThreadStart+0x2f
0027ffec 00000000 ntdll!_RtlUserThreadStart+0x1b

If the over-read is allowed to succeed, a byte adjacent to the buffer is copied:

0:000> r
eax=01d8e978 ebx=00000000 ecx=00000000 edx=0000003a esi=01dc80c8 edi=00000010
eip=1e08569a esp=0027fd0c ebp=01d5aa10 iopl=0         nv up ei pl nz na pe nc
cs=0023  ss=002b  ds=002b  es=002b  fs=0053  gs=002b             efl=00000206
python27!bytearray_pop+0x7a:
1e08569a 8bd7            mov     edx,edi
0:000> dt self
Local var @ 0x27fd20 Type PyByteArrayObject*
0x01dc80c8 
   +0x000 ob_refcnt        : 0n2
   +0x004 ob_type          : 0x1e21a6d0 _typeobject
   +0x008 ob_size          : 0n16
   +0x00c ob_exports       : 0n0
   +0x010 ob_alloc         : 0n16
   +0x014 ob_bytes         : 0x01d8e978  ""
0:000> db 0x01d8e978
01d8e978  00 01 02 03 04 05 06 07-08 09 0a 0b 0c 0d 0e 0f  ................
01d8e988  ab ab ab ab ab ab ab ab-00 00 00 00 00 00 00 00  ................
01d8e998  da 49 7a 0e b6 ac 10 00-b0 af d8 01 78 1c ce 01  .Iz.........x...
01d8e9a8  ee fe ee fe ee fe ee fe-ee fe ee fe ee fe ee fe  ................
01d8e9b8  5f 49 79 88 b7 ac 10 1d-02 00 00 00 f8 8b 22 1e  _Iy...........".
01d8e9c8  d6 03 00 00 ff ff ff ff-00 00 00 00 20 54 72 69  ............ Tri
01d8e9d8  65 73 20 74 6f 20 64 65-74 65 72 6d 69 6e 65 20  es to determine 
01d8e9e8  74 68 65 20 64 65 66 61-75 6c 74 20 6c 6f 63 61  the default loca
0:000> p
eax=01d8e978 ebx=00000000 ecx=00000004 edx=00000000 esi=01dc80c8 edi=00000010
eip=1e0856aa esp=0027fd00 ebp=01d5aa10 iopl=0         nv up ei pl nz na pe nc
cs=0023  ss=002b  ds=002b  es=002b  fs=0053  gs=002b             efl=00000206
python27!bytearray_pop+0x8a:
1e0856aa 4f              dec     edi
0:000> db 0x01d8e978
01d8e978  01 02 03 04 05 06 07 08-09 0a 0b 0c 0d 0e 0f ab  ................
01d8e988  ab ab ab ab ab ab ab ab-00 00 00 00 00 00 00 00  ................
01d8e998  da 49 7a 0e b6 ac 10 00-b0 af d8 01 78 1c ce 01  .Iz.........x...
01d8e9a8  ee fe ee fe ee fe ee fe-ee fe ee fe ee fe ee fe  ................
01d8e9b8  5f 49 79 88 b7 ac 10 1d-02 00 00 00 f8 8b 22 1e  _Iy...........".
01d8e9c8  d6 03 00 00 ff ff ff ff-00 00 00 00 20 54 72 69  ............ Tri
01d8e9d8  65 73 20 74 6f 20 64 65-74 65 72 6d 69 6e 65 20  es to determine 
01d8e9e8  74 68 65 20 64 65 66 61-75 6c 74 20 6c 6f 63 61  the default loca

However, a subsequent call to PyByteArray_Resize overwrites the copied byte with a null terminator:

0:000> p
eax=00000000 ebx=00000000 ecx=00000004 edx=00000000 esi=01dc80c8 edi=0000000f
eip=1e0856c0 esp=0027fd0c ebp=01d5aa10 iopl=0         nv up ei pl zr na pe nc
cs=0023  ss=002b  ds=002b  es=002b  fs=0053  gs=002b             efl=00000246
python27!bytearray_pop+0xa0:
1e0856c0 0fb6d3          movzx   edx,bl
0:000> db 0x01d8e978
01d8e978  01 02 03 04 05 06 07 08-09 0a 0b 0c 0d 0e 0f 00  ................
01d8e988  ab ab ab ab ab ab ab ab-00 00 00 00 00 00 00 00  ................
01d8e998  da 49 7a 0e b6 ac 10 00-b0 af d8 01 78 1c ce 01  .Iz.........x...
01d8e9a8  ee fe ee fe ee fe ee fe-ee fe ee fe ee fe ee fe  ................
01d8e9b8  5f 49 79 88 b7 ac 10 1d-02 00 00 00 f8 8b 22 1e  _Iy...........".
01d8e9c8  d6 03 00 00 ff ff ff ff-00 00 00 00 20 54 72 69  ............ Tri
01d8e9d8  65 73 20 74 6f 20 64 65-74 65 72 6d 69 6e 65 20  es to determine 
01d8e9e8  74 68 65 20 64 65 66 61-75 6c 74 20 6c 6f 63 61  the default loca

Because of this, these vulnerabilities should be classified as defense-in-depth. If PyByteArray_Resize could be forced to fail, or its behavior changes at a future date, it may become possible to exploit these issues to read adjacent memory.
msg245670 - (view) Author: DmitryJ (dev_zzo) * Date: 2015-06-23 07:46
Offending code in 2.7:

https://hg.python.org/cpython/file/20c9290a5de4/Objects/bytearrayobject.c#l2381
https://hg.python.org/cpython/file/20c9290a5de4/Objects/bytearrayobject.c#l2412

Let n = 16, where = 0; memmove() then attempts to copy (n - where) = 16 bytes where it should have copied 15, since we drop one. This appears to be a typical case of off-by-one. Changing (n - where) to (n - where - 1) should fix the issue. This underfows when (where + 1) > n, but this case is guarded against in bytearray_pop() and cannot occur in bytearray_remove().

The exact same memmove() invocation code is found in all 3.x branches as well.
msg245671 - (view) Author: DmitryJ (dev_zzo) * Date: 2015-06-23 08:16
Attached is a patch that fixes the reported issue.

Since there are no visible side effects in Python, I could not write a test for this.
msg245916 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-06-28 19:26
The bytearray object allocates one byte more for trailing null byte. ob_size always should be less than ob_alloc if ob_alloc != 0. But in rare cases when the bytearray is initialized with an iterator, this rule can be violated. Following patch restores this property. PyByteArray_AS_STRING() now always returns null-terminated string.
msg245918 - (view) Author: DmitryJ (dev_zzo) * Date: 2015-06-28 22:09
If this is the case, then issue24462 should be fixed by this patch as well.

I'm sorry about missing the root cause here.
msg245954 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-06-29 18:19
New changeset a887ce8611d2 by Serhiy Storchaka in branch '2.7':
Issue #24467: Fixed possible buffer over-read in bytearray. The bytearray
https://hg.python.org/cpython/rev/a887ce8611d2

New changeset c95d7ffa492e by Serhiy Storchaka in branch '3.4':
Issue #24467: Fixed possible buffer over-read in bytearray. The bytearray
https://hg.python.org/cpython/rev/c95d7ffa492e

New changeset 942860bada14 by Serhiy Storchaka in branch '3.5':
Issue #24467: Fixed possible buffer over-read in bytearray. The bytearray
https://hg.python.org/cpython/rev/942860bada14

New changeset 97a24bc714ec by Serhiy Storchaka in branch 'default':
Issue #24467: Fixed possible buffer over-read in bytearray. The bytearray
https://hg.python.org/cpython/rev/97a24bc714ec
History
Date User Action Args
2022-04-11 14:58:18adminsetgithub: 68655
2015-07-05 21:20:05Arfreversetnosy: + Arfrever
2015-06-29 18:25:33serhiy.storchakalinkissue24462 superseder
2015-06-29 18:22:24serhiy.storchakasetstatus: open -> closed
resolution: fixed
components: + Interpreter Core
stage: patch review -> resolved
2015-06-29 18:19:27python-devsetnosy: + python-dev
messages: + msg245954
2015-06-28 22:09:16dev_zzosetmessages: + msg245918
2015-06-28 19:26:46serhiy.storchakasetfiles: + bytearray_init_trailing_null.patch

messages: + msg245916
2015-06-28 17:11:18serhiy.storchakasetassignee: serhiy.storchaka
stage: patch review
versions: + Python 3.4, Python 3.5, Python 3.6
2015-06-23 08:16:56dev_zzosetfiles: + issue24467-3.5.patch
2015-06-23 08:16:52dev_zzosetfiles: + issue24467-3.4.patch
2015-06-23 08:16:48dev_zzosetfiles: + issue24467-3.3.patch
2015-06-23 08:16:43dev_zzosetfiles: + issue24467-3.2.patch
2015-06-23 08:16:21dev_zzosetfiles: + issue24467-2.7.patch
keywords: + patch
messages: + msg245671
2015-06-23 07:52:31serhiy.storchakasetnosy: + serhiy.storchaka
2015-06-23 07:46:33dev_zzosetnosy: + dev_zzo
messages: + msg245670
2015-06-19 20:10:20ned.deilysetnosy: + benjamin.peterson
2015-06-18 21:04:35JohnLeitchcreate