Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check path arguments of os functions for null character #68096

Closed
serhiy-storchaka opened this issue Apr 10, 2015 · 17 comments
Closed

Check path arguments of os functions for null character #68096

serhiy-storchaka opened this issue Apr 10, 2015 · 17 comments
Labels
extension-modules C modules in the Modules dir OS-windows type-bug An unexpected behavior, bug, or error

Comments

@serhiy-storchaka
Copy link
Member

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
    Copy link
    Member Author

    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.

    @serhiy-storchaka serhiy-storchaka added extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error OS-windows labels Apr 10, 2015
    @vstinner
    Copy link
    Member

    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
    Copy link
    Member Author

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

    @vstinner
    Copy link
    Member

    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
    Copy link
    Member Author

    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
    Copy link
    Mannequin

    python-dev mannequin commented Apr 20, 2015

    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
    Copy link
    Member Author

    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
    Copy link
    Member Author

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

    @serhiy-storchaka
    Copy link
    Member Author

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

    @serhiy-storchaka
    Copy link
    Member Author

    Ping. Can anybody please test the patch on Windows?

    @zware
    Copy link
    Member

    zware commented Jul 1, 2016

    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
    Copy link
    Mannequin

    python-dev mannequin commented Jul 1, 2016

    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

    @serhiy-storchaka
    Copy link
    Member Author

    Thank you Zachary.

    @koobs
    Copy link

    koobs commented Jul 3, 2016

    koobs-freebsd{9,10,current) failing after 30099abdb3a46d0e306a4cf995b95fa8cfb8b78a merge to 2.7

    @koobs koobs reopened this Jul 3, 2016
    @koobs
    Copy link

    koobs commented Jul 3, 2016

    ======================================================================
    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

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jul 3, 2016

    New changeset b0e8a2e7c361 by Serhiy Storchaka in branch '2.7':
    Fixed a test for bpo-23908 with C locale.
    https://hg.python.org/cpython/rev/b0e8a2e7c361

    @serhiy-storchaka
    Copy link
    Member Author

    Thank you for noticing this.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    extension-modules C modules in the Modules dir OS-windows type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants