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

Posix getenv makes no guarantee of lifetime of returned string #66879

Closed
aidanhs mannequin opened this issue Oct 21, 2014 · 5 comments
Closed

Posix getenv makes no guarantee of lifetime of returned string #66879

aidanhs mannequin opened this issue Oct 21, 2014 · 5 comments
Labels
3.8 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump

Comments

@aidanhs
Copy link
Mannequin

aidanhs mannequin commented Oct 21, 2014

BPO 22689
Nosy @vstinner, @serhiy-storchaka, @miss-islington
PRs
  • bpo-22689: Copy the result of getenv() in sys_breakpointhook(). #8194
  • [3.7] bpo-22689: Copy the result of getenv() in sys_breakpointhook(). (GH-8194) #8206
  • 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 2018-07-10.22:16:35.137>
    created_at = <Date 2014-10-21.21:03:44.910>
    labels = ['interpreter-core', '3.8', 'type-crash']
    title = 'Posix getenv makes no guarantee of lifetime of returned string'
    updated_at = <Date 2018-07-10.22:16:35.134>
    user = 'https://bugs.python.org/aidanhs'

    bugs.python.org fields:

    activity = <Date 2018-07-10.22:16:35.134>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2018-07-10.22:16:35.137>
    closer = 'vstinner'
    components = ['Interpreter Core']
    creation = <Date 2014-10-21.21:03:44.910>
    creator = 'aidanhs'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 22689
    keywords = ['patch']
    message_count = 5.0
    messages = ['229788', '229792', '321341', '321342', '321397']
    nosy_count = 4.0
    nosy_names = ['vstinner', 'serhiy.storchaka', 'aidanhs', 'miss-islington']
    pr_nums = ['8194', '8206']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'crash'
    url = 'https://bugs.python.org/issue22689'
    versions = ['Python 3.8']

    @aidanhs
    Copy link
    Mannequin Author

    aidanhs mannequin commented Oct 21, 2014

    Posix says the following on the subject of getenv:

    The returned string pointer might be invalidated or the string content might be overwritten by a subsequent call to getenv()

    (http://pubs.opengroup.org/onlinepubs/9699919799/functions/getenv.html)

    Unfortunately, in Modules/getpath.c:

    static void
    calculate_path(void)
    {
    [...]
        char *_rtpypath = Py_GETENV("PYTHONPATH"); /* XXX use wide version on Windows */
        wchar_t *rtpypath = NULL;
        wchar_t *home = Py_GetPythonHome();
        char *_path = getenv("PATH");

    So 3 potential getenv calls in quick succession, meaning _rtpypath and home can become junk before they get used and Python crashes before it can start up (it becomes unable to find the site module).

    Unfortunately it looks like the assumption that getenv pointers will remain safe forever is used in a few places in python.

    Explicit notes on the correct use of getenv: https://www.securecoding.cert.org/confluence/display/seccode/ENV34-C.+Do+not+store+pointers+returned+by+certain+functions

    Someone's apparently seen this before (but didn't report it?) - http://sourceforge.net/p/edk2/mailman/edk2-devel/thread/66BD57653246D24E9698B0A6509545A86DDB863C@ORSMSX109.amr.corp.intel.com/

    @aidanhs aidanhs mannequin added interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump labels Oct 21, 2014
    @aidanhs
    Copy link
    Mannequin Author

    aidanhs mannequin commented Oct 21, 2014

    In case it matters, I'm compiling using Emscripten which implements getenv like so: https://github.com/kripken/emscripten/blob/1.25.2/src/library.js#L3323

    (I personally think it's a bizarre way to do it, but technically I think it's ok?)

    @serhiy-storchaka
    Copy link
    Member

    New changeset f60bf0e by Serhiy Storchaka in branch 'master':
    bpo-22689: Copy the result of getenv() in sys_breakpointhook(). (GH-8194)
    f60bf0e

    @miss-islington
    Copy link
    Contributor

    New changeset 6f4fbf8 by Miss Islington (bot) in branch '3.7':
    bpo-22689: Copy the result of getenv() in sys_breakpointhook(). (GH-8194)
    6f4fbf8

    @vstinner
    Copy link
    Member

    char \*_rtpypath = Py_GETENV("PYTHONPATH"); /* XXX use wide version on Windows \*/
    

    Python now copies the env var. In master, Modules/main.c:

    int res = config_get_env_var_dup(&path, L"PYTHONPATH", "PYTHONPATH");

    Moreover, bytes are decoded to Unicode (wchar_t) on UNIX.

    This issue is now 4 years old and Serhiy just fixed one issue, so I close the issue.

    Even if there is a risk of an issue, nobody came up with a concrete way to trigger a bug, so I don't think that it's a big issue. For example, the reported bug was on Py_GETENV("PYTHONPATH"), whereas this code is critical for Python: if it fails, everybody will complain. Except that since the bug has been reported, nobody ever saw an issue with this code. The code is part of the early code to initialize Python, when there is not possible to execute arbitrary code nor have a second thread, so we should be fine.

    @vstinner vstinner added the 3.8 only security fixes label Jul 10, 2018
    @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
    3.8 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants