classification
Title: allocation (and overwrite) of a 0 byte buffer
Type: security Stage: resolved
Components: Interpreter Core Versions: Python 3.4, Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: Arfrever, benjamin.peterson, pkt, python-dev, serhiy.storchaka, vstinner
Priority: normal Keywords: patch

Created on 2015-02-20 12:06 by pkt, last changed 2015-02-28 15:27 by Arfrever. This issue is now closed.

Files
File name Uploaded Description Edit
poc_strxfrm.py pkt, 2015-02-20 12:06
issue23490.patch serhiy.storchaka, 2015-02-20 15:35 review
Messages (9)
msg236275 - (view) Author: paul (pkt) Date: 2015-02-20 12:06
# Bug
# ---
# 
# Py_UNICODE *
# PyUnicode_AsUnicodeAndSize(PyObject *unicode, Py_ssize_t *size)
# {
#     ...
# #endif
#     wchar_t *w;
#     wchar_t *wchar_end;
# 
#     ...
# 1           _PyUnicode_WSTR(unicode) = (wchar_t *) PyObject_MALLOC(sizeof(wchar_t) *
#                                                   (_PyUnicode_LENGTH(unicode) + 1));
#             ...
#             w = _PyUnicode_WSTR(unicode);
# 2           wchar_end = w + _PyUnicode_LENGTH(unicode);
# 
#             if (PyUnicode_KIND(unicode) == PyUnicode_1BYTE_KIND) {
#                 one_byte = PyUnicode_1BYTE_DATA(unicode);
# 3               for (; w < wchar_end; ++one_byte, ++w)
#                     *w = *one_byte;
#                 /* null-terminate the wstr */
# 4               *w = 0;
#             }
# 
# 1. if length(unicode)==2**30-1, then malloced buffer has size equal to 
#    4*(2^30-1+1)=2^32 == 0 (modulo 2^32)
# 2. wchar_end is equal to w-4 because of pointer arithmetic (nonexplicit 
#    multiplication by 4)
# 3. w > wchar_end, so we don't enter the loop
# 4. 4 byte write to a 0 size buffer
# 
# GDB output
# ----------
# 
# 3860                _PyUnicode_WSTR(unicode) = (wchar_t *) PyObject_MALLOC(sizeof(wchar_t) *
# ...
# (gdb) print sizeof(wchar_t)*(((PyASCIIObject*)(unicode))->length+1)
# $21 = 0
# ...
# (gdb) n
# 3868                w = _PyUnicode_WSTR(unicode);
# (gdb) n
# 3869                wchar_end = w + _PyUnicode_LENGTH(unicode);
# (gdb) n
# 3871                if (PyUnicode_KIND(unicode) == PyUnicode_1BYTE_KIND) {
# (gdb) print w
# $22 = 0x805fc028 L"\xfbfbfbfb\xced00000"
# (gdb) print wchar_end
# $23 = 0x805fc024 L"\xfbfbfb6f\xfbfbfbfb\xced00000"
# ...
# 3876                    *w = 0;
#  
# )
# OS info
# -------
# 
# % ./python -V
# Python 3.4.1
#  
# % uname -a
# Linux ubuntu 3.8.0-29-generic #42~precise1-Ubuntu SMP Wed Aug 14 15:31:16 UTC 2013 i686 i686 i386 GNU/Linux
 
import locale
s='a'*(2**30-1)
locale.strxfrm(s)
msg236280 - (view) Author: paul (pkt) Date: 2015-02-20 13:10
And a nice error:

Debug memory block at address p=0x805fc028: API 'o'
    0 bytes originally requested
    The 3 pad bytes at p-3 are FORBIDDENBYTE, as expected.
    The 4 pad bytes at tail=0x805fc028 are not all FORBIDDENBYTE (0xfb):
        at tail+0: 0x00 *** OUCH
        at tail+1: 0x00 *** OUCH
        at tail+2: 0x00 *** OUCH
        at tail+3: 0x00 *** OUCH
    The block was made by call #53454 to debug malloc/realloc.
Fatal Python error: bad trailing pad byte
msg236297 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-02-20 15:35
Here is a patch. There is yet one similar bug in unicodeobject.c.
msg236312 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2015-02-20 17:35
I think it looks fine except why do you cast PyUnicode_LENGTH to size_t in the comparison?

I also wonder if we should have PyObject_NEW now.
msg236314 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-02-20 17:39
> I think it looks fine except why do you cast PyUnicode_LENGTH to size_t in
> the comparison?

To silence compiler warning. PyUnicode_LENGTH is signed, right hand is 
unsigned.

> I also wonder if we should have PyObject_NEW now.

We have PyObject_NEW.
msg236316 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2015-02-20 18:07
On Fri, Feb 20, 2015, at 12:39, Serhiy Storchaka wrote:
> 
> Serhiy Storchaka added the comment:
> 
> > I think it looks fine except why do you cast PyUnicode_LENGTH to size_t in
> > the comparison?
> 
> To silence compiler warning. PyUnicode_LENGTH is signed, right hand is 
> unsigned.

Okay.

> 
> > I also wonder if we should have PyObject_NEW now.
> 
> We have PyObject_NEW.

Can we use here then?
msg236323 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-02-20 19:35
New changeset 038297948389 by Serhiy Storchaka in branch '3.4':
Issue #23490: Fixed possible crashes related to interoperability between
https://hg.python.org/cpython/rev/038297948389

New changeset 56c6a4bce996 by Serhiy Storchaka in branch 'default':
Issue #23490: Fixed possible crashes related to interoperability between
https://hg.python.org/cpython/rev/56c6a4bce996
msg236324 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-02-20 19:38
> Can we use here then?

It is for PyObjects.
msg236325 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-02-20 19:39
Thank you for your report paul.
History
Date User Action Args
2015-02-28 15:27:54Arfreversetnosy: + Arfrever
2015-02-20 19:39:53serhiy.storchakasetstatus: open -> closed

assignee: serhiy.storchaka
components: + Interpreter Core
versions: + Python 3.5
resolution: fixed
messages: + msg236325
stage: resolved
2015-02-20 19:38:45serhiy.storchakasetmessages: + msg236324
2015-02-20 19:35:30python-devsetnosy: + python-dev
messages: + msg236323
2015-02-20 18:07:23benjamin.petersonsetmessages: + msg236316
2015-02-20 17:39:56serhiy.storchakasetmessages: + msg236314
2015-02-20 17:35:19benjamin.petersonsetstage: patch review -> (no value)
messages: + msg236312
versions: - Python 3.5
2015-02-20 15:35:05serhiy.storchakasetfiles: + issue23490.patch
versions: + Python 3.5
messages: + msg236297

keywords: + patch
stage: patch review
2015-02-20 13:13:16vstinnersetnosy: + vstinner, benjamin.peterson, serhiy.storchaka
type: crash -> security
2015-02-20 13:10:01pktsetmessages: + msg236280
2015-02-20 12:06:49pktcreate