python310Packages.cffi: patch closures to work on M1 machines #187636

tjni · 2022-08-20T20:21:37Z

Summary

Currently, a feature in cffi called closures (i.e. callbacks) is broken on M1 machines. A notable consequence is that pyOpenSSL is broken on M1 machines as well, and that is depended upon by many packages. I believe that, even though closures are broken still on M1 machines in certain cases (seems to be cases where apps are signed), it can be unbroken for other cases.

The patch is simple and mergeable but will leave to maintainers to determine whether they feel the benefit is worth the risk now, or if it is better to wait for more developments.

Details

Trusts the libffi library inside of nixpkgs on Apple devices.

When Apple's fork of libffi is not detected, cffi assumes that libffi uses a strategy for creating closures (i.e. callbacks) that is in certain cases susceptible to a security exploit.

Based on some analysis I did:

https://groups.google.com/g/python-cffi/c/xU0Usa8dvhk

I believe that libffi already contains the code from Apple's fork that is deemed safe to trust in cffi.

It uses a more sophisticated strategy for creating trampolines to support closures that works on Apple Silicon, while the simple approach that cffi falls back on does not, so this patch enables code that uses closures on M1 Macs again.

Notably, pyOpenSSL is impacted and will be fixed by this, reported in pyca/pyopenssl#873

Note that libffi closures still will not work on signed apps without the com.apple.security.cs.allow-unsigned-executable-memory entitlement while libffi/libffi#621 is still open (which I haven't tested but is my best guess from reading).

I am hopeful that all of these changes will be upstreamed back into cffi and libffi

Note for pyOpenSSL

Many tests that used to fail now pass, but there are still three or four (I can't remember exactly) failures that use a callback. I looked into one and do not have a root cause, but I don't believe it's because callbacks do not work. In that test, printing something during SSL handshake led to the callback working, so it feels more like something related to buffers.

Things done

tpwrules · 2022-08-20T21:05:57Z

Can you offer guidance on how to properly test this? I own an M1 machine.

Note that libffi closures still will not work on signed apps without the com.apple.security.cs.allow-unsigned-executable-memory entitlement

I'm not sure what the implication of this is, will pyOpenSSL work as it did before? Do I need to make any system configuration changes to test this?

tjni · 2022-08-20T21:19:36Z

When it comes to this patch, I tested on my M1 machine by re-enabling the tests in cffi and by enabling and running the tests in pyOpenSSL.

Note that libffi closures still will not work on signed apps without the com.apple.security.cs.allow-unsigned-executable-memory entitlement

I'm not sure what the implication of this is, will pyOpenSSL work as it did before? Do I need to make any system configuration changes to test this?

You asked a great question, and I also do not know how to test this. I think it should be possible to trigger this by code signing an app that ends up using libffi closures, but I do not know how to do this. I asked for help on this in libffi/libffi#621. In theory, I expect this issue should cause a crash due to accessing memory it is not allowed to, leading to a segmentation fault. This is not the same as cffi, which seems to throw a Python exception, but in both cases the code should not function.

pkgs/development/python-modules/cffi/default.nix

tpwrules · 2022-08-21T23:26:07Z

On macOS 12.5 with Nix 2.8.1 and commit 4cfaf206adb833ac497945cb42b60fa6bd5970dd, but you said this is to be expected?

pyopenssl failure log

============================= test session starts ==============================
platform darwin -- Python 3.10.6, pytest-7.1.2, pluggy-1.0.0
OpenSSL: b'OpenSSL 1.1.1q  5 Jul 2022'
cryptography: 37.0.4
rootdir: /private/tmp/nix-build-python3.10-pyopenssl-22.0.0.drv-0/pyOpenSSL-22.0.0, configfile: setup.cfg, testpaths: tests
plugins: flaky-3.7.0
collected 526 items / 9 deselected / 517 selected                              

tests/test_crypto.py ................................................... [  9%]
........................................................................ [ 23%]
........................................................................ [ 37%]
........................................................................ [ 51%]
...............                                                          [ 54%]
tests/test_debug.py .                                                    [ 54%]
tests/test_rand.py ....                                                  [ 55%]
tests/test_ssl.py ...............................................FF..... [ 65%]
.......F.F.......................................................s...... [ 79%]
........................................................................ [ 93%]
...............................                                          [ 99%]
tests/test_util.py .                                                     [100%]

=================================== FAILURES ===================================
_____________________ TestContext.test_set_keylog_callback _____________________

self = <tests.test_ssl.TestContext object at 0x1029c7610>

    @pytest.mark.skipif(
        not getattr(_lib, "Cryptography_HAS_KEYLOG", None),
        reason="SSL_CTX_set_keylog_callback unavailable",
    )
    def test_set_keylog_callback(self):
        """
        `Context.set_keylog_callback` accepts a callable which will be
        invoked when key material is generated or received.
        """
        called = []
    
        def keylog(conn, line):
            called.append((conn, line))
    
        server_context = Context(TLSv1_2_METHOD)
        server_context.set_keylog_callback(keylog)
        server_context.use_certificate(
            load_certificate(FILETYPE_PEM, root_cert_pem)
        )
        server_context.use_privatekey(
            load_privatekey(FILETYPE_PEM, root_key_pem)
        )
    
        client_context = Context(SSLv23_METHOD)
    
        self._handshake_test(server_context, client_context)
    
>       assert called
E       assert []

tests/test_ssl.py:1054: AssertionError
______________________ TestContext.test_set_proto_version ______________________

self = <tests.test_ssl.TestContext object at 0x1029edae0>

    def test_set_proto_version(self):
        if OP_NO_TLSv1_3 is None:
            high_version = TLS1_2_VERSION
            low_version = TLS1_1_VERSION
        else:
            high_version = TLS1_3_VERSION
            low_version = TLS1_2_VERSION
    
        server_context = Context(TLS_METHOD)
        server_context.use_certificate(
            load_certificate(FILETYPE_PEM, root_cert_pem)
        )
        server_context.use_privatekey(
            load_privatekey(FILETYPE_PEM, root_key_pem)
        )
        server_context.set_min_proto_version(high_version)
    
        client_context = Context(TLS_METHOD)
        client_context.set_max_proto_version(low_version)
    
>       with pytest.raises(Error, match="unsupported protocol"):
E       Failed: DID NOT RAISE <class 'OpenSSL.SSL.Error'>

tests/test_ssl.py:1078: Failed
________________ TestContext.test_set_verify_callback_exception ________________

self = <tests.test_ssl.TestContext object at 0x1035502e0>

    def test_set_verify_callback_exception(self):
        """
        If the verify callback passed to `Context.set_verify` raises an
        exception, verification fails and the exception is propagated to the
        caller of `Connection.do_handshake`.
        """
        serverContext = Context(TLSv1_2_METHOD)
        serverContext.use_privatekey(
            load_privatekey(FILETYPE_PEM, root_key_pem)
        )
        serverContext.use_certificate(
            load_certificate(FILETYPE_PEM, root_cert_pem)
        )
    
        clientContext = Context(TLSv1_2_METHOD)
    
        def verify_callback(*args):
            raise Exception("silly verify failure")
    
        clientContext.set_verify(VERIFY_PEER, verify_callback)
    
>       with pytest.raises(Exception) as exc:
E       Failed: DID NOT RAISE <class 'Exception'>

tests/test_ssl.py:1430: Failed
_______________ TestContext.test_set_verify_default_callback[1] ________________

self = <tests.test_ssl.TestContext object at 0x1035506a0>, mode = 1

    @pytest.mark.parametrize("mode", [SSL.VERIFY_PEER, SSL.VERIFY_NONE])
    def test_set_verify_default_callback(self, mode):
        """
        If the verify callback is omitted, the preverify value is used.
        """
        serverContext = Context(TLSv1_2_METHOD)
        serverContext.use_privatekey(
            load_privatekey(FILETYPE_PEM, root_key_pem)
        )
        serverContext.use_certificate(
            load_certificate(FILETYPE_PEM, root_cert_pem)
        )
    
        clientContext = Context(TLSv1_2_METHOD)
        clientContext.set_verify(mode, None)
    
        if mode == SSL.VERIFY_PEER:
>           with pytest.raises(Exception) as exc:
E           Failed: DID NOT RAISE <class 'Exception'>

tests/test_ssl.py:1497: Failed
=============================== warnings summary ===============================
../../../../nix/store/4x9h2x4llx2lzgv9dh5wvrk3awz2k375-python3.10-pytest-7.1.2/lib/python3.10/site-packages/_pytest/config/__init__.py:1252
  /nix/store/4x9h2x4llx2lzgv9dh5wvrk3awz2k375-python3.10-pytest-7.1.2/lib/python3.10/site-packages/_pytest/config/__init__.py:1252: PytestConfigWarning: Unknown config option: strict
  
    self._warn_or_fail_if_strict(f"Unknown config option: {key}\n")

tests/test_crypto.py:39
  /private/tmp/nix-build-python3.10-pyopenssl-22.0.0.drv-0/pyOpenSSL-22.0.0/tests/test_crypto.py:39: DeprecationWarning: PKCS#7 support in pyOpenSSL is deprecated. You should use the APIs in cryptography.
    from OpenSSL.crypto import PKCS7, load_pkcs7_data

tests/test_crypto.py:40
  /private/tmp/nix-build-python3.10-pyopenssl-22.0.0.drv-0/pyOpenSSL-22.0.0/tests/test_crypto.py:40: DeprecationWarning: PKCS#12 support in pyOpenSSL is deprecated. You should use the APIs in cryptography.
    from OpenSSL.crypto import PKCS12, load_pkcs12

tests/test_crypto.py::TestCRL::test_export_md5_digest
  /nix/store/4x9h2x4llx2lzgv9dh5wvrk3awz2k375-python3.10-pytest-7.1.2/lib/python3.10/site-packages/_pytest/python.py:192: PytestRemovedIn8Warning: Passing None has been deprecated.
  See https://docs.pytest.org/en/latest/how-to/capture-warnings.html#additional-use-cases-of-warnings-in-tests for alternatives in common use cases.
    result = testfunction(**testargs)

tests/test_ssl.py::TestContext::test_set_cipher_list[hello world:AES128-SHA1]
  /private/tmp/nix-build-python3.10-pyopenssl-22.0.0.drv-0/pyOpenSSL-22.0.0/tests/test_ssl.py:509: DeprecationWarning: str for cipher_list is no longer accepted, use bytes
    context.set_cipher_list(cipher_string)

tests/test_ssl.py::TestConnection::test_client_set_session
  /private/tmp/nix-build-python3.10-pyopenssl-22.0.0.drv-0/pyOpenSSL-22.0.0/tests/test_ssl.py:2686: DeprecationWarning: str for buf is no longer accepted, use bytes
    ctx.set_session_id("unity-test")

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
===Flaky Test Report===

test_gmtime_adj_notBefore passed 1 out of the required 1 times. Success!
test_gmtime_adj_notAfter passed 1 out of the required 1 times. Success!
test_set_cipher_list_no_cipher_match passed 1 out of the required 1 times. Success!

===End Flaky Test Report===
=========================== short test summary info ============================
FAILED tests/test_ssl.py::TestContext::test_set_keylog_callback - assert []
FAILED tests/test_ssl.py::TestContext::test_set_proto_version - Failed: DID N...
FAILED tests/test_ssl.py::TestContext::test_set_verify_callback_exception - F...
FAILED tests/test_ssl.py::TestContext::test_set_verify_default_callback[1] - ...
====== 4 failed, 512 passed, 1 skipped, 9 deselected, 6 warnings in 2.37s ======
error: builder for '/nix/store/gizmi40jq6x7bza747v16vx5baf719jx-python3.10-pyopenssl-22.0.0.drv' failed with exit code 1;

tjni · 2022-08-21T23:30:04Z

It is also what I saw. I had looked into test_set_keylog_callback before and don't think it's because of cffi, but I still need to find time to look into the other failures and understand them.

SuperSandro2000 · 2022-10-03T19:10:28Z

@ofborg eval

toonn

The security implications make me a bit uncomfortable but I'm not sure we have much of a choice? LGTM

emilytrau · 2022-10-04T14:04:12Z

This was something I stumbled on as well and can at least share my crack at it. I substituted libffi for Apple's system version, it also works but I could only find the trampoline that loads the library at runtime. emilytrau@56c8491

winterqt · 2022-10-04T14:10:24Z

works but I could only find the trampoline that loads the library at runtime

What do you mean by this?

If I'm understanding this correctly: the only difference between your patches is that @tjni's patch uses the system copy, while yours pulls it from our packaged SDK, right?

emilytrau · 2022-10-04T14:18:14Z

From my understanding(?) Apple doesn't ship the actual libffi in the SDK and what I pulled (a .tbd) is a trampoline that loads the OS-tied version(?)

winterqt · 2022-10-04T14:29:48Z

~~That's correct, I believe -- I'm not sure if there's an actual difference between these two, then (since they both use the copy from libsystem on the host)? @toonn can probably confirm.~~ See below for a correction.

tjni · 2022-10-05T01:07:20Z

In practice, if this can be made to work with Apple's fork of libffi, I don't mind that solution either. It would best mimic the status quo for most users of libffi/cffi on Mac who don't use nixpkgs, and it includes the code in libffi/libffi#621.

I'm currently under the impression that libffi/libffi#621 is the only gap remaining for closures to work on signed apps with hardened runtime enabled, so I treated that gap as the only one that would make using Apple's preferred to using the open source version, but I haven't done a thorough analysis of the differences.

With respect to security, I don't yet fully understand how the implementation in libffi works around the write+executable memory requirement for closures using MAP_SHARED -- so I haven't been able to progress with a solution upstream. But I'm pretty confident that libffi built on aarch64-darwin uses static trampolines, which sidesteps this issue entirely.

If the decision is made to continue using libffi on darwin, to address the security concerns, perhaps it would be better to scope the patch only to aarch64-darwin or just darwin (for which I am less confident that the MAP_SHARED attack surface isn't present, but in which case stock cffi trusts right now anyway) (cffi already does this using the __APPLE__ symbol).

winterqt · 2022-10-05T01:10:18Z

Oh, I see, this is using libffi from Nixpkgs -- sorry, I misunderstood.

toonn

I tried building this on x86_64-darwin and both Python 2.7 and 3.10 cffi tests failed with one error. Maybe a rebase can help since it's been a while? Though currently a Rustc fix is going through staging-next, so until that's in staging I think we'd need to wait.

elohmeier · 2022-11-13T18:06:36Z

I can confirm this fixes the issue with pyopenssl. Tested it using scrapy (nix run .#python3Packages.scrapy shell 'https://nixos.org') which fails without the fix. LGTM

tjni · 2022-11-13T19:06:04Z

I will take some time today to get this into a mergeable state again.

tjni · 2022-11-14T08:29:27Z

I tried building this on x86_64-darwin and both Python 2.7 and 3.10 cffi tests failed with one error.

@toonn After the most recent rebase, I can build cffi successfully on aarch64-darwin and x86_64-darwin using Python 3.10. There are many errors for me when run against Python 2.7.

SuperSandro2000 · 2022-11-15T20:48:39Z

There are many errors for me when run against Python 2.7.

Python 2.7 is EOL. I am not sure if we even need to worry.

tjni · 2022-11-16T03:11:57Z

In the spirit of do no harm, I disabled tests that were failing on Python 2.7 + Darwin so that the package still builds. If we find this does break someone's use case, we can follow up by reverting this change for that Python version.

dotlambda · 2022-11-16T03:21:10Z

There are many errors for me when run against Python 2.7.

Python 2.7 is EOL. I am not sure if we even need to worry.

I agree. Let's not add more code to deal with Python 2.7.

domenkozar · 2022-11-18T15:29:47Z

Can we merge this?

pkgs/development/python-modules/cffi/default.nix

dotlambda · 2022-11-19T21:58:42Z

If you want we can just disable tests on Python 2.7, but I think we shouldn't care at all.

tjni · 2022-11-19T22:18:32Z

I have a small preference to keep the disabled tests for now so that this builds on Python 2.7 if only because disabling the tests in this case was easy, and I don't want knowingly to break someone's potential flow with no warning. If you or others feel strongly enough about removing the Python 2.7 workaround here, I don't mind removing it either though; let me know.

I'm supportive overall of spending less time and less code on Python 2.7, and I don't personally currently plan on spending any non-trivial time on supporting this package for Python 2.7.

Is there already a plan for phasing out Python 2.7 from nixpkgs? If not, I feel having that discussion somewhere (forums, issue, RFC?) is worth it.

dotlambda · 2022-11-19T22:26:23Z

I have a small preference to keep the disabled tests for now so that this builds on Python 2.7 if only because disabling the tests in this case was easy, and I don't want knowingly to break someone's potential flow with no warning.

Then please do so in pkgs/development/python2-modules.

Is there already a plan for phasing out Python 2.7 from nixpkgs?

Not really. See e.g. #148779.

Trusts the libffi library inside of nixpkgs on Apple devices. When Apple's fork of libffi is not detected, cffi assumes that libffi uses a strategy for creating closures (i.e. callbacks) that is in certain cases susceptible to a security exploit. Based on some analysis I did: https://groups.google.com/g/python-cffi/c/xU0Usa8dvhk I believe that libffi already contains the code from Apple's fork that is deemed safe to trust in cffi. It uses a more sophisticated strategy for creating trampolines to support closures that works on Apple Silicon, while the simple approach that cffi falls back on does not, so this patch enables code that uses closures on M1 Macs again. Notably, pyOpenSSL is impacted and will be fixed by this, reported in pyca/pyopenssl#873 Note that libffi closures still will not work on signed apps without the com.apple.security.cs.allow-unsigned-executable-memory entitlement while libffi/libffi#621 is still open (which I haven't tested but is my best guess from reading). I am hopeful that all of these changes will be upstreamed back into cffi and libffi, and that this comment provides enough breadcrumbs for future maintainers to track and clean this up.

tjni · 2022-11-21T00:17:48Z

Moved the disabled tests logic to pkgs/development/python2-modules.

github-actions · 2022-11-28T13:25:00Z

Successfully created backport PR #203450 for release-22.11.

github-actions · 2022-11-28T13:25:02Z

Git push to origin failed for release-22.11 with exitcode 1

nixos-discourse · 2022-12-28T19:48:06Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/nix-macos-monthly/12330/35

tjni requested review from FRidh and jonringer as code owners August 20, 2022 20:21

github-actions bot added the 6.topic: python label Aug 20, 2022

tjni changed the title ~~cffi: patch closures to work on M1 machines~~ python310Packages.cffi: patch closures to work on M1 machines Aug 20, 2022

ofborg bot requested review from LnL7 and domenkozar August 21, 2022 09:07

ofborg bot added 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+ labels Aug 21, 2022

SuperSandro2000 reviewed Aug 21, 2022

View reviewed changes

pkgs/development/python-modules/cffi/default.nix Show resolved Hide resolved

SuperSandro2000 added the 2.status: merge conflict This PR has merge conflicts with the target branch label Sep 4, 2022

tjni removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Sep 16, 2022

SuperSandro2000 requested a review from a team October 3, 2022 19:10

toonn approved these changes Oct 4, 2022

View reviewed changes

bobby285271 added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Oct 4, 2022

wegank mentioned this pull request Oct 11, 2022

python3Packages.cairocffi: segmentation fault on aarch64-darwin #153783

Closed

toonn reviewed Oct 21, 2022

View reviewed changes

dotlambda requested changes Nov 19, 2022

View reviewed changes

pkgs/development/python-modules/cffi/default.nix Outdated Show resolved Hide resolved

dotlambda requested changes Nov 19, 2022

View reviewed changes

pkgs/development/python-modules/cffi/default.nix Outdated Show resolved Hide resolved

domenkozar merged commit f1bdd95 into NixOS:staging Nov 28, 2022

domenkozar added the backport release-22.11 label Nov 28, 2022

github-actions bot mentioned this pull request Nov 28, 2022

[Backport release-22.11] python310Packages.cffi: patch closures to work on M1 machines #203450

Merged

1 task

tjni deleted the cffi branch November 28, 2022 17:05

tjni mentioned this pull request Dec 1, 2022

python310Packages.pyopenssl: unbreak on aarch64-darwin #204002

Merged

13 tasks

mrene mentioned this pull request Apr 6, 2024

google-cloud-sdk: fix gcloud storage sign-url error due to missing pyopenssl dependency #301986

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

python310Packages.cffi: patch closures to work on M1 machines #187636

python310Packages.cffi: patch closures to work on M1 machines #187636

tjni commented Aug 20, 2022 •

edited by SuperSandro2000

Loading

tpwrules commented Aug 20, 2022

tjni commented Aug 20, 2022

tpwrules commented Aug 21, 2022

tjni commented Aug 21, 2022

SuperSandro2000 commented Oct 3, 2022

toonn left a comment

emilytrau commented Oct 4, 2022

winterqt commented Oct 4, 2022

emilytrau commented Oct 4, 2022

winterqt commented Oct 4, 2022 •

edited

Loading

tjni commented Oct 5, 2022 •

edited

Loading

winterqt commented Oct 5, 2022

toonn left a comment

elohmeier commented Nov 13, 2022

tjni commented Nov 13, 2022

tjni commented Nov 14, 2022

SuperSandro2000 commented Nov 15, 2022

tjni commented Nov 16, 2022

dotlambda commented Nov 16, 2022

domenkozar commented Nov 18, 2022

dotlambda commented Nov 19, 2022

tjni commented Nov 19, 2022

dotlambda commented Nov 19, 2022

tjni commented Nov 21, 2022

github-actions bot commented Nov 28, 2022

github-actions bot commented Nov 28, 2022

nixos-discourse commented Dec 28, 2022

python310Packages.cffi: patch closures to work on M1 machines #187636

python310Packages.cffi: patch closures to work on M1 machines #187636

Conversation

tjni commented Aug 20, 2022 • edited by SuperSandro2000 Loading

Things done

tpwrules commented Aug 20, 2022

tjni commented Aug 20, 2022

tpwrules commented Aug 21, 2022

tjni commented Aug 21, 2022

SuperSandro2000 commented Oct 3, 2022

toonn left a comment

Choose a reason for hiding this comment

emilytrau commented Oct 4, 2022

winterqt commented Oct 4, 2022

emilytrau commented Oct 4, 2022

winterqt commented Oct 4, 2022 • edited Loading

tjni commented Oct 5, 2022 • edited Loading

winterqt commented Oct 5, 2022

toonn left a comment

Choose a reason for hiding this comment

elohmeier commented Nov 13, 2022

tjni commented Nov 13, 2022

tjni commented Nov 14, 2022

SuperSandro2000 commented Nov 15, 2022

tjni commented Nov 16, 2022

dotlambda commented Nov 16, 2022

domenkozar commented Nov 18, 2022

dotlambda commented Nov 19, 2022

tjni commented Nov 19, 2022

dotlambda commented Nov 19, 2022

tjni commented Nov 21, 2022

github-actions bot commented Nov 28, 2022

github-actions bot commented Nov 28, 2022

nixos-discourse commented Dec 28, 2022

tjni commented Aug 20, 2022 •

edited by SuperSandro2000

Loading

winterqt commented Oct 4, 2022 •

edited

Loading

tjni commented Oct 5, 2022 •

edited

Loading