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

avoid using a shell in ctypes.util: replace os.popen with subprocess #66826

Closed
vstinner opened this issue Oct 14, 2014 · 25 comments
Closed

avoid using a shell in ctypes.util: replace os.popen with subprocess #66826

vstinner opened this issue Oct 14, 2014 · 25 comments
Labels
topic-ctypes type-bug An unexpected behavior, bug, or error

Comments

@vstinner
Copy link
Member

BPO 22636
Nosy @warsaw, @vstinner, @vadmium, @serhiy-storchaka, @koobs
Files
  • ctypes_util_popen.patch
  • ctypes_util_popen-2.patch
  • ctypes_util_popen-3.py3.patch
  • ctypes_util_popen-3.py2.patch
  • dump: /usr/ccs/bin/dump
  • ldconfig: /sbin/ldconfig -r (BSD)
  • crle: /usr/bin/crle
  • ctypes_util_popen-4.py2.patch
  • ctypes_util_popen-5.py3.patch
  • ctypes_util_popen-5.py2.patch
  • ctypes_util_popen-6.py2.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-06-18.03:19:15.452>
    created_at = <Date 2014-10-14.22:32:55.866>
    labels = ['ctypes', 'type-bug']
    title = 'avoid using a shell in ctypes.util: replace os.popen with subprocess'
    updated_at = <Date 2016-06-18.06:05:44.654>
    user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2016-06-18.06:05:44.654>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2016-06-18.03:19:15.452>
    closer = 'martin.panter'
    components = ['ctypes']
    creation = <Date 2014-10-14.22:32:55.866>
    creator = 'vstinner'
    dependencies = []
    files = ['36923', '36946', '42796', '42797', '42798', '42799', '42800', '43326', '43329', '43383', '43387']
    hgrepos = []
    issue_num = 22636
    keywords = ['patch']
    message_count = 25.0
    messages = ['229363', '229520', '255522', '255552', '255635', '265111', '265242', '265243', '265244', '266051', '268081', '268085', '268087', '268092', '268094', '268098', '268505', '268508', '268510', '268516', '268548', '268668', '268754', '268755', '268766']
    nosy_count = 7.0
    nosy_names = ['barry', 'vstinner', 'Arfrever', 'python-dev', 'martin.panter', 'serhiy.storchaka', 'koobs']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue22636'
    versions = ['Python 2.7', 'Python 3.5', 'Python 3.6']

    @vstinner
    Copy link
    Member Author

    Attached patch modifies the ctypes.util module to not use a shell: it replaces os.open() with subprocess.Popen on Linux.

    Running a shell is slower and is more vulnerable to code injection.

    I only modified code path on Linux right now. They are still calls to os.popen() on sunos5, freebsd, openbsd and dragonfly.

    @vstinner vstinner added the type-feature A feature request or enhancement label Oct 14, 2014
    @vstinner
    Copy link
    Member Author

    Updated patch which address also BSD and Solaris systems.

    I also changed the behaviour when a required command is missing: return None instead of raising an OSError.

    In the current code, when a command is missing, the shell scripts return the exit code 10. The Python codes checks for the exit code 10, but in fact os.popen() returns a status, not directly the exit code. So the OSError was never raised.

    I don't know if it's better to return None instead of raising an error? It changes the behaviour, can it break backward compatibility?

    @vadmium
    Copy link
    Member

    vadmium commented Nov 28, 2015

    See bpo-25751 for some demo exploits on Linux, if anyone wants inspiration for test cases. Maybe this should be applied as a bug fix. I haven’t looked at the patch, other than confirming it removes all five os.popen() calls.

    @vadmium
    Copy link
    Member

    vadmium commented Nov 28, 2015

    I think it is better to return None without an exception, to keep the current behaviour, and because that’s what the documentation implies.

    @vadmium
    Copy link
    Member

    vadmium commented Dec 1, 2015

    Marking for bug fix in 2.7, requested in bpo-25751.

    @vadmium vadmium added type-bug An unexpected behavior, bug, or error and removed type-feature A feature request or enhancement labels Dec 1, 2015
    @vadmium
    Copy link
    Member

    vadmium commented May 8, 2016

    There are a few review comments that probably need addressing.

    @vadmium
    Copy link
    Member

    vadmium commented May 10, 2016

    I merged Victor’s patch with the current code and addressed most of the comments:

    • restore re.escape()
    • single "-l" + name argument
    • copy with dict(os.environ)
    • redirect GCC stderr=STDOUT
    • changed tempfile cleanup to try / finally

    I also added a test case.

    I kept Victor’s behaviour of not raising OSError when the command is missing. I think this should be considered separately, and only changed for 3.6+, if at all. The buggy code was added in bpo-4861.

    I only have Linux and GCC, but I briefly tested each platform-specific branch by hacking the “if” statements and creating mock crle, ldconfig, etc commands, so I am somewhat confident that everything is still working.

    @vadmium
    Copy link
    Member

    vadmium commented May 10, 2016

    Here is a possible patch for Python 2. One snag is that ctypes is currently supposed to be compatible with Python 2.3, but subprocess was added in 2.4. The patch assumes it is okay to lift that compatibility restriction.

    The main differences are:

    • shutil.which() does not exist in Python 2. In _findLib_gcc() and the Gnu version of _get_soname(), I restored the mini shell script that runs “type” to check if commands are available. However I pass the library and file names as proper arguments, rather than inserting them into the shell syntax.

    • subprocess.DEVNULL does not exist. Manually opened os.devnull where necessary.

    • There was an extra Popen conversion for the Gnu “ldconfig -p” call. In Python 3, this was already converted thanks to revision 19d9f0a177de (bpo-11258).

    • No context manager support for Popen objects. Instead, use communicate() where appropriate, or manually close and wait.

    Again, I tested the Python 2 patch on Linux, but with mock platform-specific commands to exercise each new Popen() call.

    @vadmium
    Copy link
    Member

    vadmium commented May 10, 2016

    Uploading the fake commands I used for testing.

    @vadmium
    Copy link
    Member

    vadmium commented May 22, 2016

    FTR the Python 2.3 compatibility restriction was lifted; see <https://mail.python.org/pipermail/python-dev/2016-May/144502.html\>.

    @vadmium
    Copy link
    Member

    vadmium commented Jun 10, 2016

    Updated Python 2 patch merged with recent changes.

    I will commit at least the Python 3 version soon, because the existing code sets a bad example for potential additions (bpo-26439).

    @serhiy-storchaka
    Copy link
    Member

    It looks to me that the command used in _findLib_gcc always fails.

    $ LANG=C LC_ALL=C gcc -Wl,-t -o ttt -lc
    /usr/bin/ld: mode elf_i386
    /usr/lib/gcc/i686-linux-gnu/5/../../../i386-linux-gnu/crt1.o
    /usr/lib/gcc/i686-linux-gnu/5/../../../i386-linux-gnu/crti.o
    /usr/lib/gcc/i686-linux-gnu/5/crtbegin.o
    /lib/i386-linux-gnu/libc.so.6
    (/usr/lib/i386-linux-gnu/libc_nonshared.a)elf-init.oS
    /lib/i386-linux-gnu/ld-linux.so.2
    /lib/i386-linux-gnu/ld-linux.so.2
    -lgcc_s (/usr/lib/gcc/i686-linux-gnu/5/libgcc_s.so)
    /lib/i386-linux-gnu/libc.so.6
    /lib/i386-linux-gnu/ld-linux.so.2
    -lgcc_s (/usr/lib/gcc/i686-linux-gnu/5/libgcc_s.so)
    /usr/lib/gcc/i686-linux-gnu/5/crtend.o
    /usr/lib/gcc/i686-linux-gnu/5/../../../i386-linux-gnu/crtn.o
    /usr/lib/gcc/i686-linux-gnu/5/../../../i386-linux-gnu/crt1.o: In function `_start':
    (.text+0x18): undefined reference to `main'
    /usr/bin/ld: link errors found, deleting executable `ttt'
    collect2: error: ld returned 1 exit status

    Is it OK?

    @vadmium
    Copy link
    Member

    vadmium commented Jun 10, 2016

    Yes it is okay. The code is compiling a dummy file without main(), just to see what libraries GCC tries to link with it. It is only interested in extracting the line matching *libc.so.*, which in your case should be

    /lib/i386-linux-gnu/libc.so.6

    So you should find that ctypes.util._findLib_gcc("c") still returns this path, even though the compile command technically fails.

    @vstinner
    Copy link
    Member Author

    Maybe the failure should be explained in a comment? (Sorry I din't read the
    patch.)

    @vadmium
    Copy link
    Member

    vadmium commented Jun 10, 2016

    Yes a comment sounds like a good idea. Here is a new Py 3 patch.

    @serhiy-storchaka
    Copy link
    Member

    ctypes_util_popen-5.py3.patch LGTM.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jun 14, 2016

    New changeset 0715d403cae2 by Martin Panter in branch '3.5':
    Issue bpo-22636: avoid using a shell in the ctypes.util module
    https://hg.python.org/cpython/rev/0715d403cae2

    New changeset 60613ecad578 by Martin Panter in branch 'default':
    Issue bpo-22636: Merge ctypes.util shell injection fixes from 3.5
    https://hg.python.org/cpython/rev/60613ecad578

    @vadmium
    Copy link
    Member

    vadmium commented Jun 14, 2016

    Updated Py 2 patch to v5 with the added GCC comment

    @vadmium
    Copy link
    Member

    vadmium commented Jun 14, 2016

    An Open Indiana buildbot failed. The old code let the shell print any errors about missing programs to /dev/null, so I will change the subprocess calls to handle OSError.

    ======================================================================
    ERROR: setUpModule (ctypes.test.test_loading)
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "/export/home/buildbot/32bits/3.5.cea-indiana-x86/build/Lib/ctypes/test/test_loading.py", line 19, in setUpModule
        libc_name = find_library("c")
      File "/export/home/buildbot/32bits/3.5.cea-indiana-x86/build/Lib/ctypes/util.py", line 238, in find_library
        return _get_soname(_findLib_crle(name, is64) or _findLib_gcc(name))
      File "/export/home/buildbot/32bits/3.5.cea-indiana-x86/build/Lib/ctypes/util.py", line 145, in _get_soname
        stderr=subprocess.DEVNULL)
      File "/export/home/buildbot/32bits/3.5.cea-indiana-x86/build/Lib/subprocess.py", line 947, in __init__
        restore_signals, start_new_session)
      File "/export/home/buildbot/32bits/3.5.cea-indiana-x86/build/Lib/subprocess.py", line 1551, in _execute_child
        raise child_exception_type(errno_num, err_msg)
    FileNotFoundError: [Errno 2] No such file or directory: '/usr/ccs/bin/dump'

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jun 14, 2016

    New changeset 96d297e9a8a8 by Martin Panter in branch '3.5':
    Issue bpo-22636: Handle OSError from subprocess, e.g. if command not found
    https://hg.python.org/cpython/rev/96d297e9a8a8

    New changeset a6a36bb6ee50 by Martin Panter in branch 'default':
    Issue bpo-22636: Merge ctypes.util from 3.5
    https://hg.python.org/cpython/rev/a6a36bb6ee50

    @vadmium
    Copy link
    Member

    vadmium commented Jun 14, 2016

    Updated Py 2 patch to handle OSError when shell=True is not used

    @serhiy-storchaka
    Copy link
    Member

    ctypes_util_popen-6.py2.patch LGTM.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jun 18, 2016

    New changeset a09ae70f3489 by Victor Stinner in branch '2.7':
    Issue bpo-22636: Avoid using a shell in the ctypes.util module
    https://hg.python.org/cpython/rev/a09ae70f3489

    @vadmium
    Copy link
    Member

    vadmium commented Jun 18, 2016

    Sorry about impersonating your name as committer Victor. I have been fixing this problem in recent patches, but because I imported your patch a while ago I forgot about it.

    @vadmium vadmium closed this as completed Jun 18, 2016
    @vstinner
    Copy link
    Member Author

    No problemo.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    topic-ctypes type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants