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

Segfault in hashlib in OpenSSL FIPS mode using non-FIPS-compliant hashes, if "ssl" imported before "hashlib" #53392

Closed
davidmalcolm opened this issue Jul 2, 2010 · 18 comments
Assignees
Labels
stdlib Python modules in the Lib dir type-crash A hard crash of the interpreter, possibly with a core dump

Comments

@davidmalcolm
Copy link
Member

BPO 9146
Nosy @gpshead, @pitrou, @tiran, @davidmalcolm, @dstufft, @stratakis, @iritkatriel
PRs
  • bpo-9146: Raise a ValueError if OpenSSL fails to init a hash function #1777
  • [3.6] bpo-9146: Raise a ValueError if OpenSSL fails to init a hash func #3274
  • bpo-9146: add the missing NEWS entry. #3275
  • Files
  • add_ssl_init_to_hashlib.patch
  • remove-unusable-hashes-from-hashopenssl.patch
  • hashopenssl-fips-mode-errors-v3.patch
  • hashopenssl-fips-exceptions.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 = 'https://github.com/tiran'
    closed_at = <Date 2020-09-19.23:56:13.780>
    created_at = <Date 2010-07-02.19:10:08.718>
    labels = ['library', 'type-crash']
    title = 'Segfault in hashlib in OpenSSL FIPS mode using non-FIPS-compliant hashes, if "ssl" imported before "hashlib"'
    updated_at = <Date 2020-09-19.23:56:13.780>
    user = 'https://github.com/davidmalcolm'

    bugs.python.org fields:

    activity = <Date 2020-09-19.23:56:13.780>
    actor = 'gregory.p.smith'
    assignee = 'christian.heimes'
    closed = True
    closed_date = <Date 2020-09-19.23:56:13.780>
    closer = 'gregory.p.smith'
    components = ['Library (Lib)']
    creation = <Date 2010-07-02.19:10:08.718>
    creator = 'dmalcolm'
    dependencies = []
    files = ['17841', '17846', '17884', '46765']
    hgrepos = []
    issue_num = 9146
    keywords = ['patch']
    message_count = 18.0
    messages = ['109121', '109122', '109123', '109124', '109153', '109154', '109411', '109451', '109485', '109809', '275028', '290801', '290805', '294329', '294368', '301199', '301200', '377164']
    nosy_count = 9.0
    nosy_names = ['gregory.p.smith', 'pitrou', 'christian.heimes', 'dmalcolm', 'jpokorny', 'dstufft', 'cstratak', 'kscguru', 'iritkatriel']
    pr_nums = ['1777', '3274', '3275']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'crash'
    url = 'https://bugs.python.org/issue9146'
    versions = ['Python 2.7', 'Python 3.5', 'Python 3.6']

    @davidmalcolm
    Copy link
    Member Author

    Having run:
    prelink --undo --all
    the following works OK:
    OPENSSL_FORCE_FIPS_MODE=1 python -c "import hashlib; m = m = hashlib.md5(); m.update('abc')"

    but the following segfaults:
    OPENSSL_FORCE_FIPS_MODE=1 python -c "import ssl; import hashlib; m = m = hashlib.md5(); m.update('abc')"

    and the following gives the same segfault, in a simpler way (strictly speaking one shouldn't directly import _ssl, but this most directly reproduces the crash):
    OPENSSL_FORCE_FIPS_MODE=1 python -c "import _ssl; import hashlib; m = m = hashlib.md5(); m.update('abc')"

    (gdb) bt
    #0 0x0000000000000000 in ?? ()
    #1 0x00007fffee978736 in EVP_hash (self=0xaed3c0, vp=0x95b6a4, len=3) at /home/david/coding/python-svn/trunk-fips/Modules/_hashopenssl.c:112
    #2 0x00007fffee978cb5 in EVP_update (self=0xaed3c0, args=('abc',)) at /home/david/coding/python-svn/trunk-fips/Modules/_hashopenssl.c:247
    #3 0x0000000000567faa in PyCFunction_Call (func=<built-in method update of _hashlib.HASH object at remote 0xaed3c0>, arg=('abc',), kw=0x0)
    at Objects/methodobject.c:81
    and the segfault is due to EVP_DigestUpdate calling through the ctx->update function pointer, which in this case is NULL.

    (gdb) p self->ctx
    $2 = {digest = 0x7ffff0c955a0, engine = 0x0, flags = 0, md_data = 0x0, pctx = 0x0, update = 0}

    self->ctx->update == NULL and self->ctx->digest == &bad_md, as set up by:

    The setup is here: (different run of gdb, so different addresses):

    (gdb) bt
    #0 EVP_DigestInit_ex (ctx=0x7fffecdc7b80, type=0x7fffefc6fc60, impl=0x0) at
    digest.c:249
    #1 0x00007fffecbc53ce in init_hashlib () at
    /usr/src/debug/Python-2.6.2/Modules/_hashopenssl.c:513

    #ifdef OPENSSL_FIPS
                      if (FIPS_mode())
                              {
                              if (!(type->flags & EVP_MD_FLAG_FIPS)
                               && !(ctx->flags & EVP_MD_CTX_FLAG_NON_FIPS_ALLOW))
                                      {
                                      EVPerr(EVP_F_EVP_DIGESTINIT_EX,
    EVP_R_DISABLED_FOR_FIPS);
      =>                              ctx->digest = &bad_md;
                                      return 0;
                                      }
                              }
    #endif

    (seen on x86_64 Linux; Fedora 13, with openssl-1.0.0-1.fc13.x86_64 on SVN trunk, and on other builds)

    Clearly, the Python process should not segfault or abort.

    I don't think it's clear what the behavior should be here, though - how is the Python standard library to be used in conjunction with OpenSSL's FIPS mode?

    From page 18 of the OpenSSL's FIPS guide: http://ftp.openssl.org/docs/fips/UserGuide.pdf
    "By design, the OpenSSL API attempts to disable non­FIPS algorithms, when in FIPS mode, at the 
    EVP level and via most low level function calls.  Failure to check the return code from low level 
    functions could result in unexpected behavior.  Note also that sufficiently creative or unusual use of 
    the API may still allow the use of non­FIPS algorithms.  The non­FIPS algorithm disabling is 
    intended as a aid to the developer in preventing the accidental use of non­FIPS algorithms in FIPS 
    mode, and not as an absolute guarantee. It is the responsibility of the application developer to 
    ensure that no non­FIPS algorithms are used when in FIPS mode."

    It seems odd that the behavior of "md5" within "hashlib" can vary depending on whether "_ssl" was imported first. Reducing surprises for the programmer suggests to me that the behavior for these two cases should be harmonized.

    It also seems odd that SSL can refuse the usage of MD5 whilst other implementations exist and are available; my understanding of FIPS mode is that it's intended for locked-down environments that wish to ensure that only known-good crypto is used (and e.g. MD5 is known to be be broken for some uses, see: http://www.kb.cert.org/vuls/id/836068)

    Possible approaches:
    (A) change hashlib so that it continues to support md5 if ssl/_ssl is imported first, even in FIPS mode
    (B) change hashlib so that in FIPS mode, it fails to support md5 even if ssl/_ssl is not imported first
    (B.1) by not recognizing "md5" as a digest, leading to NameError exceptions and similar (I dislike this approach, as it leads to obscure failures)
    (B.2) by raising a new exception e.g. "ProhibitedInFIPSMode" or somesuch (so that the failure is obvious and searchable)
    (C.*) as per (B.*), but support an explicit override: hashlib.md5(non_fips_allow=True)

    Thoughts?

    @davidmalcolm davidmalcolm added stdlib Python modules in the Lib dir type-crash A hard crash of the interpreter, possibly with a core dump labels Jul 2, 2010
    @pitrou
    Copy link
    Member

    pitrou commented Jul 2, 2010

    First, is it only with 2.7 or 2.6?
    Second, I don't really get the point of the FIPS mode. The PDF you linked to seems full of bureaucratic jargon.
    Third, I can't reproduce under Mandriva, but perhaps it's because it's using OpenSSL 1.0.0 (which the PDF says isn't supported).
    Fourth, if MD5 is insecure and FIPS disables insecure algorithm, then why should hashlib allow MD5 hashing when FIPS mode is enabled?
    Fifth, please take a look at the OpenSSL initialization routine in _sslmodule.c and try to transplant it to the hashlib initialization routine:

    /* Init OpenSSL */
    SSL_load_error_strings();
    SSL_library_init();
    OpenSSL_add_all_algorithms();
    

    @davidmalcolm
    Copy link
    Member Author

    Thanks

    First, is it only with 2.7 or 2.6?
    I've seen this with both 2.6 tarball builds and SVN trunk; in both cases against openssl-1.0.0-1.[

    Second, I don't really get the point of the FIPS mode. The PDF you linked to seems full of bureaucratic jargon.
    Agreed.

    Third, I can't reproduce under Mandriva, but perhaps it's because it's using OpenSSL 1.0.0 (which the PDF says isn't supported).
    Thanks for the info. This is my fault, sorry. I'm using openssl-1.0.0-1 on Fedora; on doublechecking I see that we appear to be carrying various patches in our openssl packages, which I assume are a backport from a more recent version of the FIPS support:
    (see http://cvs.fedoraproject.org/viewvc/devel/openssl/ ); I'll see if I can get more information from other Fedorans to verify this, and on what version of openssl upstream this is equivalent to.

    Fourth, if MD5 is insecure and FIPS disables insecure algorithm, then why should hashlib allow MD5 hashing when FIPS mode is enabled?
    My understanding is that some sites wish to prohibit the use of MD5 as an authentication mechanism, but don't need to prohibit it where the use is not "cryptography", e.g. to compute an index into a hash table where intentionally induced hash collisions can not break the application's security.

    Fifth, please take a look at the OpenSSL initialization routine in _sslmodule.c and try to transplant it to the hashlib initialization routine:
    [snip]

    Done; with the attached patch to SVN trunk, I don't need the initial "import ssl" to reproduce the segfault; the following will segfault (in the same place):
    OPENSSL_FORCE_FIPS_MODE=1 gdb --args ./python -c "import hashlib; m = m = hashlib.md5(); m.update('abc')"

    @pitrou
    Copy link
    Member

    pitrou commented Jul 2, 2010

    with the attached patch to SVN trunk, I don't need the initial "import
    ssl" to reproduce the segfault

    Nice, so at least that oddity is eliminated :)

    So I guess it's down to the A, B, and C approaches you suggested.
    Of course, if we choose to allow MD5, we must find a way for OpenSSL to accept running an MD5 hash...

    @davidmalcolm
    Copy link
    Member Author

    Attached patch checks for errors in the initialization of _hashlib, and only registers the names that are actually available.

    It also contains the ssl init from the first patch.

    I added a _hashlib._errors dict, containing errors, so that you can examine them at runtime:

    $ OPENSSL_FORCE_FIPS_MODE=1 ./python
    Python 2.7rc2+ (trunk:82445, Jul  2 2010, 14:00:30) 
    [GCC 4.4.3 20100422 (Red Hat 4.4.3-18)] on linux2
    Type "help", "copyright", "credits" or "license" for more information.
    >>> import _hashlib
    [35786 refs]
    >>> _hashlib._errors
    {'md5': '_hashopenssl.c:541: error:060800A0:digital envelope routines:EVP_DigestInit_ex:unknown cipher'}
    [35825 refs]
    >>> dir (_hashlib)
    ['__doc__', '__file__', '__name__', '__package__', '_errors', 'new', 'openssl_sha1', 'openssl_sha224', 'openssl_sha256', 'openssl_sha384', 'openssl_sha512']
    [35838 refs]
    (note the absence of openssl_md5)
    Note that hashlib (as opposed to _hashlib) seems to gracefully fall back to Python's _md5 module when in this state:
    >>> import hashlib
    [36107 refs]
    >>> m = m = hashlib.md5(); m.update('abc\n'); print m.hexdigest()
    0bee89b07a248e27c83fc3d5951213c1
    [36109 refs]

    This seems to be option (A) from my initial message.

    @davidmalcolm
    Copy link
    Member Author

    Not quite ready yet:

    Named methods work:
    $ OPENSSL_FORCE_FIPS_MODE=1 ./python -c "import hashlib; m = hashlib.md5(); m.update('abc\n'); print m.hexdigest()"0bee89b07a248e27c83fc3d5951213c1
    [15741 refs]

    but lookup by name still fails:
    OPENSSL_FORCE_FIPS_MODE=1 ./python -c "import hashlib; m = hashlib.new('md5'); m.update('abc\n'); print m.hexdigest()"
    Segmentation fault (core dumped)

    @davidmalcolm
    Copy link
    Member Author

    I'm attaching an updated patch which:

    • adds error checking to the various places where EVP_DigestInit is called
    • adds a test to test_hashlib to ensure that hashlib still works gracefully when OPENSSL_FORCE_FIPS_MODE=1 is set in the environment
    Note that in this mode:
    >>> _hashlib.new('md5')
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    ValueError: error:060800A0:digital envelope routines:EVP_DigestInit_ex:unknown cipher
    [57670 refs]

    but hashlib falls back to using the "md5" module instead.

    I started writing a test for _hashlib (as opposed to hashlib), but it's too hard to express a runtime conditional on whether OPENSSL_FORCE_FIPS_MODE will actually affect the behavior of EVP_DigestInit across the versions of openssl that might be installed on the system.

    I'm still waiting to hear back from the Fedora OpenSSL packager for info on how to reproduce this on a vanilla OpenSSL.

    @gpshead
    Copy link
    Member

    gpshead commented Jul 7, 2010

    I'm pretty sure Python setup.py does not build the non-openssl md5, sha1, sha256 and sha512 extension modules at all when openssl is present. So falling back on them is not likely to work unless anyone who wants this crazy force fips mode thing to not prevent them from existing in Python goes to the extra effort to compile them.

    @davidmalcolm
    Copy link
    Member Author

    Thanks.

    The relevant code in setup.py is all wrapped with --pydebug:
    if COMPILED_WITH_PYDEBUG or not have_usable_openssl:

    All of my testing had been --with-pydebug.

    Rebuilding without --with-pydebug leads to some interesting failures; as you say, if OpenSSL has md5 support disabled, this isn't going to work unless the _md5 module is manually enabled.

    Notes on some of the failures seen:

    $ OPENSSL_FORCE_FIPS_MODE=1 ./python 
    Python 2.7.0+ (trunk:82622M, Jul  7 2010, 12:08:16) 
    [GCC 4.4.3 20100422 (Red Hat 4.4.3-18)] on linux2
    Type "help", "copyright", "credits" or "license" for more information.
    >>> import md5
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
      File "/home/david/coding/python-svn/2.7-fips/Lib/md5.py", line 10, in <module>
        from hashlib import md5
      File "/home/david/coding/python-svn/2.7-fips/Lib/hashlib.py", line 136, in <module>
        globals()[__func_name] = __get_hash(__func_name)
      File "/home/david/coding/python-svn/2.7-fips/Lib/hashlib.py", line 100, in __get_openssl_constructor
        return __get_builtin_constructor(name)
      File "/home/david/coding/python-svn/2.7-fips/Lib/hashlib.py", line 71, in __get_builtin_constructor
        import _md5
    ImportError: No module named _md5
    
    >>> import hashlib
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
      File "/home/david/coding/python-svn/2.7-fips/Lib/hashlib.py", line 136, in <module>
        globals()[__func_name] = __get_hash(__func_name)
      File "/home/david/coding/python-svn/2.7-fips/Lib/hashlib.py", line 100, in __get_openssl_constructor
        return __get_builtin_constructor(name)
      File "/home/david/coding/python-svn/2.7-fips/Lib/hashlib.py", line 71, in __get_builtin_constructor
        import _md5
    ImportError: No module named _md5

    Changing:
    Index: Lib/hashlib.py
    ===================================================================

    --- Lib/hashlib.py      (revision 82622)
    +++ Lib/hashlib.py      (working copy)
    @@ -134,7 +134,7 @@
         # version not supporting that algorithm.
         try:
             globals()[__func_name] = __get_hash(__func_name)
    -    except ValueError:
    +    except (ValueError, ImportError):
             import logging
             logging.exception('code for hash %s was not found.', __func_name)
     
    indicates that it's just md5 that's not found, and enables "import hashlib" to work, albeit with a traceback upon import:
    ERROR:root:code for hash md5 was not found.
    Traceback (most recent call last):
      File "/home/david/coding/python-svn/2.7-fips/Lib/hashlib.py", line 136, in <module>
        globals()[__func_name] = __get_hash(__func_name)
      File "/home/david/coding/python-svn/2.7-fips/Lib/hashlib.py", line 100, in __get_openssl_constructor
        return __get_builtin_constructor(name)
      File "/home/david/coding/python-svn/2.7-fips/Lib/hashlib.py", line 71, in __get_builtin_constructor
        import _md5
    ImportError: No module named _md5
    "import md5" also then fails in an obscure way:
    >>> import md5
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
      File "/home/david/coding/python-svn/2.7-fips/Lib/md5.py", line 10, in <module>
        from hashlib import md5
    ImportError: cannot import name md5

    @davidmalcolm
    Copy link
    Member Author

    I've filed bpo-9216 to discuss this at a higher level, with an API proposal

    @tiran
    Copy link
    Member

    tiran commented Sep 8, 2016

    I can no longer reproduce the crash with Python 2.7 and 3.5 (Fedora 24 with OpenSSL 1.0.2h). Is this still a problem for you?

    @kscguru
    Copy link
    Mannequin

    kscguru mannequin commented Mar 30, 2017

    I tripped over this exact issue a few months ago, while working on a FIPSified OpenSSL library (custom platform).

    Attached a different (more minimal) patch; this one focuses more narrowly on the exact failure mode. It's based on 'master' (~3.7), applies cleanly / tests_hashlib passes. My employer has been using this patch successfully (in a FIPS environment) for ~3 months now, and I'm looking to upstream.

    Earlier, dmalcolm identified that OpenSSL's EVP_DigestUpdate dereferences a NULL in FIPS mode on non-FIPS algorithms. OpenSSL is not quite that bad ... turns out, EVP_DigestInit returns an error if FIPS disallows an algorithm, and ignoring the error (which _hashopenssl.c currently does) results in later dereferencing NULL. It's straightforward to add the right error checks and convert them to Python exceptions, which seems the most Pythonic way to handle the error - and it also minimizes the FIPS-only codepaths.

    There's one catch, _hashopenssl.c likes to pre-construct several algorithms at module import and raising an exception during import leads hashlib.py to silently drop the exception (hashlib.py:158-161). So I made the pre-constructors lazy: the first use of any constructor will attempt to initialize it, and raise the exception on first use if FIPS mode makes it unusable. The reason for choosing this approach is Lib/hashlib.py and __get_openssl_constructor(), which already has logic to handle exactly this ValueError by falling back to __get_builtin_constructor() (and the less-optimized _md5/_sha1/_sha256 modules).

    In the context of bpo-9216 (a usedforsecurity flag which can be passed to OpenSSL to allow non-FIPS algorithms), this change has the net effect of silently falling back from OpenSSL's MD5 implementation to python's _md5 C code when in FIPS mode. That's not exactly the intent of FIPS mode (python's _md5 module is certainly not FIPS-compliant), but converting a crash to a catchable exception is a major improvement. I like the discussion in bpo-9216, and see this change as complementary: it avoids crashing and moves policy handling of how to handle FIPS mode _hashopenssl.c to hashlib.py, and my gut feeling is that policy logic is better placed in a .py library than in a .c library.

    The advantage of this patch is that the new exceptions are obviously correct regardless of FIPS considerations, and the lazy constructors maintain the spirit of amortizing EVP_DigestInit calls while also reporting any error at a more useful point.

    @gpshead
    Copy link
    Member

    gpshead commented Mar 30, 2017

    I like your patch, raising an exception is indeed the right thing to do. i'll get this patch in.

    whether or not the built-in non-openssl based _md5 and _sha1 module exist in "fips" mode is a separate build time issue - lets keep this one just dealing with the always undesirable segfault.

    @gpshead gpshead added the 3.7 (EOL) end of life label Mar 30, 2017
    @gpshead gpshead self-assigned this Mar 30, 2017
    @gpshead
    Copy link
    Member

    gpshead commented May 24, 2017

    New changeset 07244a8 by Gregory P. Smith in branch 'master':
    bpo-9146: Raise a ValueError if OpenSSL fails to init a hash func. (bpo-1777)
    07244a8

    @gpshead
    Copy link
    Member

    gpshead commented May 24, 2017

    Resolved for 3.7, assigning to christian to deal with the backports as I believe he has employer motivation to see those in (should be trivial).

    @gpshead gpshead removed the 3.7 (EOL) end of life label May 24, 2017
    @gpshead gpshead assigned tiran and unassigned gpshead May 24, 2017
    @gpshead
    Copy link
    Member

    gpshead commented Sep 3, 2017

    New changeset 4f01388 by Gregory P. Smith in branch 'master':
    bpo-9146: add the missing NEWS entry. (bpo-3275)
    4f01388

    @gpshead
    Copy link
    Member

    gpshead commented Sep 3, 2017

    New changeset 31b8efe by Gregory P. Smith in branch '3.6':
    [3.6] bpo-9146: Raise a ValueError if OpenSSL fails to init a hash func (bpo-3274)
    31b8efe

    @iritkatriel
    Copy link
    Member

    Looks like this is complete and can be closed.

    @gpshead gpshead closed this as completed Sep 19, 2020
    @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
    stdlib Python modules in the Lib dir type-crash A hard crash of the interpreter, possibly with a core dump
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants