Check path arguments of os functions for null character

[serhiy-storchaka]

BPO	23908
Nosy	@vstinner, @tjguk, @benjaminp, @zware, @serhiy-storchaka, @koobs, @zooba
Files	path_converter_null_char.patch path_converter_null_char_2.patch unicode_converter_null_char-2.7.patch

^{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 2016-07-17.10:03:23.867>
created_at = <Date 2015-04-10.16:43:52.741>
labels = ['extension-modules', 'type-bug', 'OS-windows']
title = 'Check path arguments of os functions for null character'
updated_at = <Date 2016-07-17.10:03:23.866>
user = 'https://github.com/serhiy-storchaka'

bugs.python.org fields:

activity = <Date 2016-07-17.10:03:23.866>
actor = 'serhiy.storchaka'
assignee = 'none'
closed = True
closed_date = <Date 2016-07-17.10:03:23.867>
closer = 'serhiy.storchaka'
components = ['Extension Modules', 'Windows']
creation = <Date 2015-04-10.16:43:52.741>
creator = 'serhiy.storchaka'
dependencies = []
files = ['38890', '38984', '43492']
hgrepos = []
issue_num = 23908
keywords = ['patch']
message_count = 17.0
messages = ['240435', '240594', '240849', '240857', '240872', '241620', '241629', '268942', '268943', '269647', '269677', '269694', '269695', '269752', '269753', '269754', '269755']
nosy_count = 8.0
nosy_names = ['vstinner', 'tim.golden', 'benjamin.peterson', 'python-dev', 'zach.ware', 'serhiy.storchaka', 'koobs', 'steve.dower']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue23908'
versions = ['Python 2.7']

[serhiy-storchaka]

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.

[vstinner]

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.

[serhiy-storchaka]

I can't test the patch on UNIX, this branch of the code is executed only on Windows.

[vstinner]

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?

[serhiy-storchaka]

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.

[python-dev]

New changeset cdadde8396a4 by Serhiy Storchaka in branch '3.4':
Issue bpo-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 bpo-23908: os functions now reject paths with embedded null character
https://hg.python.org/cpython/rev/bdf13c7dcf7f

[serhiy-storchaka]

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).

[serhiy-storchaka]

Proposed patch fixes this issue and bpo-13848 in 2.7.

[serhiy-storchaka]

Since the large part of code is Windows specific (and even is not compiled on Linux), the patch needs testing on Windows.

[serhiy-storchaka]

Ping. Can anybody please test the patch on Windows?

[zware]

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.

[python-dev]

New changeset 30099abdb3a4 by Serhiy Storchaka in branch '2.7':
Issue bpo-23908: os functions, open() and the io.FileIO constructor now reject
https://hg.python.org/cpython/rev/30099abdb3a4