classification
Title: Check path arguments of os functions for null character
Type: behavior Stage: resolved
Components: Extension Modules, Windows Versions: Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: benjamin.peterson, haypo, koobs, python-dev, serhiy.storchaka, steve.dower, tim.golden, zach.ware
Priority: normal Keywords: patch

Created on 2015-04-10 16:43 by serhiy.storchaka, last changed 2016-07-17 10:03 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
path_converter_null_char.patch serhiy.storchaka, 2015-04-10 16:43 review
path_converter_null_char_2.patch serhiy.storchaka, 2015-04-14 11:39 review
unicode_converter_null_char-2.7.patch serhiy.storchaka, 2016-06-20 21:52 review
Messages (17)
msg240435 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-04-10 16:43
Proposed patch adds checks for null character in unicode path arguments of os functions on Windows. Null character is already tested on Unix, in bytes paths on Windows, and in unicode argument of _io.FileIO.

Removed private function _PyUnicode_HasNULChars(), because it is used only in two places and inlined code is simpler and more efficient.

The patch doesn't contain tests because I can't test them. But they should be simple, just pass a path with null character to os function.
msg240594 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2015-04-13 07:29
> The patch doesn't contain tests because I can't test them. But they should be simple, just pass a path with null character to os function.

IMO this issue is a bug, so you must write an unit test to test for non regression. One test for FileIO, another for the os module should be enough, and you can test them on UNIX since the behaviour now must be the same.

Except of the test, path_converter_null_char.patch looks good to me.

Do you want to fix Python 2.7 and 3.4? A filename with an embedded null character can be a security vulnerability, so it's good to fix these versions too.
msg240849 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-04-14 06:27
I can't test the patch on UNIX, this branch of the code is executed only on Windows.
msg240857 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2015-04-14 08:06
> I can't test the patch on UNIX, this branch of the code is executed only on Windows.

If you write a test which ensures that a path with a null character raises a TypeError, it should pass on UNIX on your PC. Then builbots will ensure that it also pass on Windows, no?
msg240872 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-04-14 11:39
I afraid not that builbots will fail, but that that test could pass without the fix. Tests should be tested without applying the fix. Writing right tests can require several iterations with running tests on patched and unpatched the os module.

Here is a patch with tests.
msg241620 - (view) Author: Roundup Robot (python-dev) Date: 2015-04-20 07:14
New changeset cdadde8396a4 by Serhiy Storchaka in branch '3.4':
Issue #23908: os functions now reject paths with embedded null character
https://hg.python.org/cpython/rev/cdadde8396a4

New changeset bdf13c7dcf7f by Serhiy Storchaka in branch 'default':
Issue #23908: os functions now reject paths with embedded null character
https://hg.python.org/cpython/rev/bdf13c7dcf7f
msg241629 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-04-20 11:03
The patch for 2.7 should be different and more complex. First at all, "s" and "u" format units don't check for null character (but "et" and "es" used in Unix implementations of os functions do).
msg268942 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-06-20 21:52
Proposed patch fixes this issue and issue13848 in 2.7.
msg268943 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-06-20 21:53
Since the large part of code is Windows specific (and even is not compiled on Linux), the patch needs testing on Windows.
msg269647 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-07-01 06:25
Ping. Can anybody please test the patch on Windows?
msg269677 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2016-07-01 16:17
I get one test failure:

ERROR: test_u (__main__.Unicode_TestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "P:\ath\to\cpython\lib\test\test_getargs2.py", line 782, in test_u
    self.assertEqual(getargs_u(u'nul:\0'), u'nul:')
TypeError: argument 1 must be unicode without null characters, not unicode

All other tests appear to pass.
msg269694 - (view) Author: Roundup Robot (python-dev) Date: 2016-07-01 20:35
New changeset 30099abdb3a4 by Serhiy Storchaka in branch '2.7':
Issue #23908: os functions, open() and the io.FileIO constructor now reject
https://hg.python.org/cpython/rev/30099abdb3a4
msg269695 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-07-01 20:36
Thank you Zachary.
msg269752 - (view) Author: Kubilay Kocak (koobs) Date: 2016-07-03 05:09
koobs-freebsd{9,10,current) failing after 30099abdb3a46d0e306a4cf995b95fa8cfb8b78a merge to 2.7
msg269753 - (view) Author: Kubilay Kocak (koobs) Date: 2016-07-03 05:10
======================================================================
ERROR: test_path_with_null_unicode (test.test_posix.PosixTester)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/home/buildbot/python/2.7.koobs-freebsd9/build/Lib/test/test_posix.py", line 585, in test_path_with_null_unicode
    test_support.unlink(fn)
  File "/usr/home/buildbot/python/2.7.koobs-freebsd9/build/Lib/test/test_support.py", line 243, in unlink
    _unlink(filename)
UnicodeEncodeError: 'ascii' codec can't encode characters in position 6-7: ordinal not in range(128)

======================================================================
ERROR: test_path_with_null_unicode (test.test_posix.PosixTester)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/home/buildbot/python/2.7.koobs-freebsd9/build/Lib/test/test_support.py", line 243, in unlink
    _unlink(filename)
UnicodeEncodeError: 'ascii' codec can't encode characters in position 6-7: ordinal not in range(128)

----------------------------------------------------------------------
Ran 43 tests in 0.105s
msg269754 - (view) Author: Roundup Robot (python-dev) Date: 2016-07-03 07:55
New changeset b0e8a2e7c361 by Serhiy Storchaka in branch '2.7':
Fixed a test for issue23908 with C locale.
https://hg.python.org/cpython/rev/b0e8a2e7c361
msg269755 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-07-03 07:57
Thank you for noticing this.
History
Date User Action Args
2016-07-17 10:03:23serhiy.storchakasetstatus: open -> closed
resolution: fixed
2016-07-03 07:57:14serhiy.storchakasetmessages: + msg269755
2016-07-03 07:55:41python-devsetmessages: + msg269754
2016-07-03 05:10:18koobssetmessages: + msg269753
2016-07-03 05:09:26koobssetstatus: closed -> open

nosy: + koobs
messages: + msg269752

resolution: fixed -> (no value)
2016-07-01 20:36:25serhiy.storchakasetstatus: open -> closed
resolution: fixed
messages: + msg269695

stage: patch review -> resolved
2016-07-01 20:35:17python-devsetmessages: + msg269694
2016-07-01 16:17:45zach.waresetmessages: + msg269677
2016-07-01 06:25:01serhiy.storchakasetmessages: + msg269647
2016-06-20 21:53:16serhiy.storchakasetmessages: + msg268943
2016-06-20 21:52:07serhiy.storchakasetfiles: + unicode_converter_null_char-2.7.patch

nosy: + benjamin.peterson
messages: + msg268942

stage: needs patch -> patch review
2015-05-12 11:06:09serhiy.storchakaunlinkissue21859 dependencies
2015-04-20 11:03:51serhiy.storchakasetstage: patch review -> needs patch
messages: + msg241629
versions: + Python 2.7, - Python 3.4, Python 3.5
2015-04-20 07:14:33python-devsetnosy: + python-dev
messages: + msg241620
2015-04-14 11:39:47serhiy.storchakasetfiles: + path_converter_null_char_2.patch

messages: + msg240872
2015-04-14 08:06:58hayposetmessages: + msg240857
2015-04-14 06:27:57serhiy.storchakasetmessages: + msg240849
2015-04-13 07:29:38hayposetmessages: + msg240594
2015-04-12 05:20:44serhiy.storchakasetnosy: + tim.golden, zach.ware, steve.dower
components: + Windows
2015-04-10 16:44:48serhiy.storchakalinkissue21859 dependencies
2015-04-10 16:43:52serhiy.storchakacreate