integer overflow in encoding unicode

[pkt]

BPO	22518
Nosy	@birkenfeld, @vstinner, @benjaminp, @serhiy-storchaka
Files	poc_encode_latin1.py codecs_error_handlers_overflow_tests.patch: Attempt to test integer overflow

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2014-12-10.19:03:11.179>
created_at = <Date 2014-09-29.21:01:25.365>
labels = ['type-security']
title = 'integer overflow in encoding unicode'
updated_at = <Date 2014-12-10.19:03:11.179>
user = 'https://bugs.python.org/pkt'

bugs.python.org fields:

activity = <Date 2014-12-10.19:03:11.179>
actor = 'benjamin.peterson'
assignee = 'none'
closed = True
closed_date = <Date 2014-12-10.19:03:11.179>
closer = 'benjamin.peterson'
components = []
creation = <Date 2014-09-29.21:01:25.365>
creator = 'pkt'
dependencies = []
files = ['36754', '37285']
hgrepos = []
issue_num = 22518
keywords = ['patch']
message_count = 17.0
messages = ['227837', '227843', '227853', '227908', '227910', '227912', '227915', '227916', '227917', '227923', '228447', '228448', '228455', '228456', '231679', '231681', '231704']
nosy_count = 7.0
nosy_names = ['georg.brandl', 'vstinner', 'benjamin.peterson', 'Arfrever', 'python-dev', 'serhiy.storchaka', 'pkt']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'security'
url = 'https://bugs.python.org/issue22518'
versions = ['Python 2.7', 'Python 3.3', 'Python 3.4', 'Python 3.5']

[pkt]

# static PyObject *
# unicode_encode_ucs1(PyObject *unicode,
# const char *errors,
# unsigned int limit)
# {
# ...
# while (pos < size) {
# ...
# case 4: /* xmlcharrefreplace */
# /* determine replacement size */
# for (i = collstart, repsize = 0; i < collend; ++i) {
# Py_UCS4 ch = PyUnicode_READ(kind, data, i);
# ...
# else if (ch < 100000)
# 1 repsize += 2+5+1;
# ...
# }
# 2 requiredsize = respos+repsize+(size-collend);
# if (requiredsize > ressize) {
# ...
# if (_PyBytes_Resize(&res, requiredsize))
# ...
# }
# /* generate replacement */
# for (i = collstart; i < collend; ++i) {
# 3 str += sprintf(str, "&#%d;", PyUnicode_READ(kind, data, i));
# }
#
# 1. ch=0xffff<100000, so repsize = (number of unicode chars in string)*8
# =2^29*2^3=2^32 == 0 (mod 2^32)
# 2. respos==0, collend==0, so requiredsize=repsize==0, so the destination buffer
# isn't resized
# 3. overwrite

[serhiy-storchaka]

Looks very similar to bpo-22470.

[python-dev]

New changeset b2e68274aa8e by Benjamin Peterson in branch '2.7':
cleanup overflowing handling in unicode_decode_call_errorhandler and unicode_encode_ucs1 (closes bpo-22518)
https://hg.python.org/cpython/rev/b2e68274aa8e

New changeset 3b7e93249700 by Benjamin Peterson in branch '2.7':
add NEWS note for bpo-22518
https://hg.python.org/cpython/rev/3b7e93249700

New changeset 3c67d19c624f by Benjamin Peterson in branch '3.3':
cleanup overflowing handling in unicode_decode_call_errorhandler and unicode_encode_ucs1 (closes bpo-22518)
https://hg.python.org/cpython/rev/3c67d19c624f

New changeset 88332ea4c140 by Benjamin Peterson in branch '3.3':
NEWS issue for bpo-22518
https://hg.python.org/cpython/rev/88332ea4c140

New changeset 7dab27fffff2 by Benjamin Peterson in branch '3.4':
merge 3.3 (closes bpo-22518)
https://hg.python.org/cpython/rev/7dab27fffff2

New changeset f86fde20e9ce by Benjamin Peterson in branch 'default':
merge 3.4 (closes bpo-22518)
https://hg.python.org/cpython/rev/f86fde20e9ce

[vstinner]

New changeset f86fde20e9ce by Benjamin Peterson in branch 'default':
merge 3.4 (closes bpo-22518)
https://hg.python.org/cpython/rev/f86fde20e9ce

This changeset added ">>>> other". It looks like you commited a conflict.

•    if (requiredsize\<2\*outsize)
  

+ if (outsize <= PY_SSIZE_T_MAX/2 && requiredsize < 2*outsize)
requiredsize = 2*outsize;

I'm not sure that this change is correct. Why not raising an exception on overflow?

[vstinner]

It would be nice to add a bigmem test to check that repr('\x00'*(2**30+1)) doesn't crash anymore.

[vstinner]

It would be nice to add a bigmem test to check that repr('\x00'*(2**30+1)) doesn't crash anymore.

Ooops, wrong issue, the test is : ("\uffff" * (2**29)).encode("latin1", errors="xmlcharrefreplace").

[serhiy-storchaka]

I'm not sure that this change is correct. Why not raising an exception on
overflow?

This is correct. This check prevents overflow.

[vstinner]

This is correct. This check prevents overflow.

Oh, I didn't understand that "requiredsize = 2*outsize;" is only used for performances, to overallocate the buffer. So I agree that it's fine to not overallocate if it would overflow.

[birkenfeld]

Benjamin, could you make a patch for 3.2 as well?

[serhiy-storchaka]

Ooops, wrong issue, the test is : ("\uffff" * (2**29)).encode("latin1",
errors="xmlcharrefreplace").

("\uffff" * (sys.maxsize//8+1)).encode("latin1", errors="xmlcharrefreplace")

or

("\xff" * (sys.maxsize//6+1)).encode("ascii", errors="xmlcharrefreplace")