classification
Title: Reject embedded null characters in wchar* strings
Type: security Stage: resolved
Components: Library (Lib), Unicode Versions: Python 3.7, Python 3.6, Python 3.5
process
Status: closed Resolution: fixed
Dependencies: 30708 Superseder:
Assigned To: serhiy.storchaka Nosy List: arnaudc, benhoyt, ezio.melotti, python-dev, serhiy.storchaka, vstinner
Priority: normal Keywords: patch

Created on 2011-12-17 03:53 by vstinner, last changed 2017-06-28 09:24 by vstinner. This issue is now closed.

Files
File name Uploaded Description Edit
embedded_nul.patch vstinner, 2011-12-17 03:53 review
doc_unicode.patch arnaudc, 2011-12-18 17:04 added warnings to concerned functions
doc_unicode-2.patch arnaudc, 2011-12-18 18:08
embedded_nul-2.patch vstinner, 2011-12-18 20:11 review
Pull Requests
URL Status Linked Edit
PR 2302 merged serhiy.storchaka, 2017-06-20 15:39
PR 2462 merged serhiy.storchaka, 2017-06-28 05:35
PR 2463 merged serhiy.storchaka, 2017-06-28 05:46
PR 2464 merged serhiy.storchaka, 2017-06-28 06:26
Messages (22)
msg149656 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2011-12-17 03:53
The curses module (only since Python 3.3), locale.strcoll(), locale.strxfrm(), time.strftime() and imp.NullImporter() (only on Windows) accept embedded null characters, whereas they convert the Unicode string to a wide character (wchar_t*) string.

The problem is that the null character truncates the string. Example:


>>> locale.strxfrm('a')
'a'
>>> locale.strxfrm('a\0b')
'a'

Attached patch fixes these functions. I wrote the patch for Python 3.3.
msg149657 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2011-12-17 03:56
PyUnicode_AsWideCharString() documentation should also warn about this issue.
msg149783 - (view) Author: Arnaud Calmettes (arnaudc) Date: 2011-12-18 17:04
Here is a patch for the documentation. 

I added warnings for, PyUnicode_AsWideChar*, PyUnicode_EncodeFSDefault and PyUnicode_AsUnicode*, since they're all concerned by this issue.
msg149787 - (view) Author: Arnaud Calmettes (arnaudc) Date: 2011-12-18 18:08
I removed the hints "using wcslen on the result of PyUnicode_AsWideChar*", since the resulting wchar_t strings may not be null-terminated
msg149791 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2011-12-18 18:29
New changeset fa5c8cf29963 by Victor Stinner in branch '3.2':
Issue #13617: Document that the result of the conversion of a Unicode object to
http://hg.python.org/cpython/rev/fa5c8cf29963

New changeset f30ac7729f2b by Victor Stinner in branch 'default':
Issue #13617: Document that the result of the conversion of a Unicode object to
http://hg.python.org/cpython/rev/f30ac7729f2b
msg149794 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2011-12-18 18:38
New changeset 1c4d9534263e by Victor Stinner in branch '2.7':
Issue #13617: Document that the result PyUnicode_AsUnicode() and
http://hg.python.org/cpython/rev/1c4d9534263e
msg149802 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2011-12-18 20:11
embedded_nul-2.patch: a more complete patch check also null byte in functions calling PyUnicode_EncodeFSDefault().
msg173147 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-10-17 09:11
I added some comments in Rietveld.

I see other instances of the use of non-checked PyUnicode_AsWideCharString() and PyUnicode_AsUnicode().
msg221844 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2014-06-29 13:45
@Victor can you pick this up again please.
msg226414 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-09-05 09:51
Could you please answer my comments Victor?
msg236699 - (view) Author: Ben Hoyt (benhoyt) * Date: 2015-02-26 19:21
Note that this (or a very similar issue) also affects os.listdir() on Windows: os.listdir(bytes_path_with_nul) raises ValueError as expected, but os.listdir(unicode_path_with_nul) does not. Test case:

>>> import os
>>> os.mkdir('foo')
>>> os.listdir(b'foo\x00zzz')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: listdir: embedded null character in path
>>> os.listdir('foo\x00zzz')
[]

However, this is not the case on Linux, as there both calls raise an appropriate ValueError.

This needs to be fixed in posixmodule.c's path_converter() function.

I'm in the middle of implementing PEP 471 (os.scandir), so don't want to create a proper patch right now, but the fix is to add these lines in posixmodule.c path_converter() after the if (length > 32767) {...} block:

        if ((size_t)length != wcslen(wide)) {
            FORMAT_EXCEPTION(PyExc_ValueError, "embedded null character in %s");
            Py_DECREF(unicode);
            return 0;
        }

We should also add test to test_os.py like the following:

    def test_listdir_nul_in_path(self):
        self.assertRaises(ValueError, os.listdir, 'y\x00z')
        self.assertRaises(ValueError, os.listdir, b'y\x00z')
msg296241 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-06-17 14:59
Could you update your patch Victor?
msg296332 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-19 13:30
Sorry, I lost track of this issue. Feel free to update and complete my patch :-)
msg296461 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-06-20 15:42
PR 2302 doesn't fix all issues with PyUnicode_AsWideCharString(). Issue30708 should fix them.
msg296512 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-06-21 04:33
While working on this issue I found a way to inject environment variables for a subprocess on Windows. Reclassified this issue as a security issue. PR 2302 fixes this. May be there are other security vulnerabilities fixed by it.
msg297132 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-28 01:44
Wow, it's nice to see activity on this issue that I opened 6 years ago :-)

Sorry Serhiy, I don't have the bandwidth right now to review your change :-( In lack of review, I suggest you to just push it.
msg297155 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-06-28 05:30
New changeset f7eae0adfcd4c50034281b2c69f461b43b68db84 by Serhiy Storchaka in branch 'master':
[security] bpo-13617: Reject embedded null characters in wchar* strings. (#2302)
https://github.com/python/cpython/commit/f7eae0adfcd4c50034281b2c69f461b43b68db84
msg297156 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-06-28 06:27
New changeset 0834905d9b61291b1fc5e05a1ffbc69de9c9379f by Serhiy Storchaka in branch '3.6':
[3.6] bpo-13617: Reject embedded null characters in wchar* strings. (GH-2302) (#2462)
https://github.com/python/cpython/commit/0834905d9b61291b1fc5e05a1ffbc69de9c9379f
msg297158 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-06-28 06:55
New changeset ccdc09ed1ebea7d7c6b41548132aa08bd797bfe8 by Serhiy Storchaka in branch 'master':
Fix compiler warnings on Windows introduced in bpo-13617. (#2464)
https://github.com/python/cpython/commit/ccdc09ed1ebea7d7c6b41548132aa08bd797bfe8
msg297161 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-06-28 07:31
New changeset 54ba940abc2fabb94fede46dfad80f8ac15632a3 by Serhiy Storchaka in branch '3.5':
[3.5] bpo-13617: Reject embedded null characters in wchar* strings. (GH-2302) (#2463)
https://github.com/python/cpython/commit/54ba940abc2fabb94fede46dfad80f8ac15632a3
msg297162 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-06-28 08:22
Backporting this to 2.7 requires too much work taking to account that PyArg_Parse and other argument parsing functions don't check for null characters in 2.7. The most serious security issue is fixed in issue30730, other cases unlikely can be used for attacks.
msg297165 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-28 09:24
Thank you very much Serhiy of taking care of this bug!
History
Date User Action Args
2017-06-28 09:24:05vstinnersetmessages: + msg297165
2017-06-28 08:22:41serhiy.storchakasetstatus: open -> closed
resolution: fixed
messages: + msg297162

stage: patch review -> resolved
2017-06-28 07:31:02serhiy.storchakasetmessages: + msg297161
2017-06-28 06:55:24serhiy.storchakasetmessages: + msg297158
2017-06-28 06:27:37serhiy.storchakasetmessages: + msg297156
2017-06-28 06:26:37serhiy.storchakasetpull_requests: + pull_request2519
2017-06-28 05:46:19serhiy.storchakasetpull_requests: + pull_request2518
2017-06-28 05:35:27serhiy.storchakasetpull_requests: + pull_request2517
2017-06-28 05:30:09serhiy.storchakasetmessages: + msg297155
2017-06-28 01:44:08vstinnersetmessages: + msg297132
2017-06-21 04:33:30serhiy.storchakasettype: behavior -> security
messages: + msg296512
2017-06-20 15:42:42serhiy.storchakasetstage: needs patch -> patch review
2017-06-20 15:42:20serhiy.storchakasetdependencies: + Ensure that the result of PyUnicode_AsWideCharString() doesn't contain null characters if size is not returned
messages: + msg296461
2017-06-20 15:39:29serhiy.storchakasetpull_requests: + pull_request2349
2017-06-20 04:33:46serhiy.storchakasetassignee: serhiy.storchaka
2017-06-19 13:30:16vstinnersetmessages: + msg296332
2017-06-17 17:51:03BreamoreBoysetnosy: - BreamoreBoy
2017-06-17 15:00:23serhiy.storchakasettype: behavior
versions: + Python 3.6, Python 3.7, - Python 3.4
2017-06-17 14:59:12serhiy.storchakasetmessages: + msg296241
2015-02-26 19:21:59benhoytsetnosy: + benhoyt
messages: + msg236699
2014-09-05 09:51:09serhiy.storchakasetmessages: + msg226414
versions: + Python 3.4, Python 3.5, - Python 3.2, Python 3.3
2014-06-29 13:45:18BreamoreBoysetnosy: + BreamoreBoy
messages: + msg221844
2012-10-24 09:15:18serhiy.storchakasetstage: needs patch
2012-10-17 09:11:34serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg173147
2011-12-18 20:11:32vstinnersetfiles: + embedded_nul-2.patch

messages: + msg149802
2011-12-18 18:38:02python-devsetmessages: + msg149794
2011-12-18 18:29:04python-devsetnosy: + python-dev
messages: + msg149791
2011-12-18 18:08:30arnaudcsetfiles: + doc_unicode-2.patch

messages: + msg149787
2011-12-18 17:04:46arnaudcsetfiles: + doc_unicode.patch
nosy: + arnaudc
messages: + msg149783

2011-12-17 03:56:00vstinnersetmessages: + msg149657
2011-12-17 03:53:41vstinnercreate