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

Can libcrypto dependency be optional? #113

Closed
methane opened this issue Sep 6, 2018 · 17 comments
Closed

Can libcrypto dependency be optional? #113

methane opened this issue Sep 6, 2018 · 17 comments

Comments

@methane
Copy link

methane commented Sep 6, 2018

libcrypto_path = find_library(b'crypto' if sys.version_info < (3,) else 'crypto')
if not libcrypto_path:
raise LibraryNotFoundError('The library libcrypto could not be found')
libcrypto = CDLL(libcrypto_path)

This code may cause SEGV, when binary incompatible OpenSSL version is used in same process.
In our case, we use custom (newer) OpenSSL for Python's ssl, uwsgi, cryptography, and pysqlcipher.
But asn1crypto loads system's libcrypto and it caused SEGV.

We solved the SEGV by manually removing _perf module. But this manual step is not idiomatic.
How about making libcrypto dependency optional and make it asn1crypto (and pyca/cryptography)
safe by default?

@wbond
Copy link
Owner

wbond commented Sep 6, 2018

Can you provide an example environment where you experience the segfault, so that I may reproduce? OS, OS version, Python version and python packages installed?

@methane
Copy link
Author

methane commented Sep 6, 2018

Our environment is very old (Debian 5.0.3!) and we build almost everything under custom prefix.
So reproducing it is difficult. I don't know I can reproduce it in more easy-to-reproduce environment.

But I think mixing binary incompatible OpenSSL in one process is bad thing anyway, even if SEGV is not reproducible in modern environment.

@wbond
Copy link
Owner

wbond commented Sep 6, 2018

Can you at least provide an example of how you are using asn1crypto? See, asn1crypto is a dependency of cryptography, so if it were that simple of an issue, we've have many reports about it.

The performance code is already optional, but I need to know how to detect the "failure" environment and skip loading the code that improved performance.

@methane
Copy link
Author

methane commented Sep 6, 2018

I'm sorry, it was old issue and we forgot exact step. This is only backtrace remains in chat log.

server:dev0230  2018-05-29 15:16:26 +0900 - *** backtrace of 2201 ***
server:dev0230  uWSGI worker 1(uwsgi_backtrace+0x2e) [0x462bfe]
server:dev0230  uWSGI worker 1(uwsgi_segfault+0x21) [0x462d81]
server:dev0230  /lib/libc.so.6 [0x7f279c9fdf60]
server:dev0230  /usr/local/app/openssl-1.0.2/lib/libcrypto.so.1.0.0(OPENSSL_ia32_cpuid+0x5) [0x7f279d0e8785]
server:dev0230  *** end of backtrace ***

OPENSSL_ia32_cpuid ABI was changed between 0.9.8 and 1.0.1.
Googling OPENSSL_ia32_cpuid SEGV show some scenarios, but not directly relating to asn1crypto.

@njsmith
Copy link

njsmith commented Sep 7, 2018

The reason you don't see this more often is that the segfault is caused by this uwsgi bug: unbit/uwsgi#1590

uwsgi by default is built in such a way that it injects a bunch of random shared library symbols into any extension modules it loads. This breaks all kinds of modules, and there isn't any general solution except for fixing uwsgi to not do that.

If asn1crypto wants to add a hack to detect when it's running in a broken environment like this, it could do that with some code like:

# This opens a magic "dll" that represents "all symbols in the process-global namespace"
# (equivalent to dlopen(NULL, flags))
ambient_namespace = ctypes.CDLL(None)
# Check whether openssl symbols are cluttering up the process-global namespace
if hasattr(ambient_namespace, "some_openssl_symbol")):
    # this process is messed up, trying to dlopen libcrypto and use it will probably segfault
    # (though maybe you can use the ambient version instead, idk)
    ...

@wbond
Copy link
Owner

wbond commented Sep 7, 2018

Thanks for such useful info @njsmith!

This perf enhancement may end end up going away in light of there being doubt via #100 that computing a public key from a private key may allow for timing attacks. In that case we'd defer to a full crypto package for when a private key structure does not include the public key.

@njsmith
Copy link

njsmith commented Sep 7, 2018

Oh, I should mention that everything I said is Linux-specific (or really ELF specific, so it probably applies to some less-popular Unixes too). Windows and MacOS are immune to this issue and I suspect will raise an error if you try to do CDLL(None).

@methane
Copy link
Author

methane commented Sep 7, 2018

Thanks for your info, @njsmith.

In our case, our infra-team prefers "single OpenSSL dependency". They use custom build of
uwsgi, cryptography, Python's ssl module, pysqlcipher, and maybe more.
That's why we don't use binary wheel of cryptography. And this time, conflict may happened
between uwsgi and asn1crypto's OpenSSL. Using static binary of cryptography may not fix
the issue, but I'm not sure.

@njsmith
Copy link

njsmith commented Sep 7, 2018

@methane sounds like what you really want then is a way to control which libcrypto asn1crypto uses?

@methane
Copy link
Author

methane commented Sep 7, 2018

@njsmith We want to specify which libcrypto library is used in whole application.
So stop using libcrypto from this library is better than adding one way to control
libcrypto path used by this library.

@njsmith
Copy link

njsmith commented Sep 7, 2018

That's true, if asn1crypto stops using libcrypto entirely then the whole issue goes away :-)

@wbond
Copy link
Owner

wbond commented Jul 13, 2019

I've removed usage of libcrypto with 564e744, which will be part of the next release

@wbond wbond closed this as completed Jul 13, 2019
@anthony-arnold
Copy link

I've removed usage of libcrypto with 564e744, which will be part of the next release

Any hint as to when this will be?

@wbond
Copy link
Owner

wbond commented Aug 8, 2019

Any hint as to when this will be?

When I find the time to finish up the remaining items before release, or someone else does. Currently #124 needs to be handled, and ideally a solution for #117.

@anthony-arnold
Copy link

Any hint as to when this will be?

When I find the time to finish up the remaining items before release, or someone else does. Currently #124 needs to be handled, and ideally a solution for #117.

Of course, wasn't trying to be pushy, just wondering if there was a plan or not. I'm a complete noob when it comes to ASN.1 but if there's anything I can do to help please let me know.

This bug popped for us after installing impyla and thrift_sasl. We were already using SCP/Paramiko which is how we came to depend on this library via the cryptography package. I'm assuming the SASL library is linking against a different libcrypto.

@wbond
Copy link
Owner

wbond commented Aug 10, 2019

Of course, wasn't trying to be pushy, just wondering if there was a plan or not.

There is a plan, but the big missing thing is time. Unfortunately in one sense this is a hobby project for me, so there is only so much free time I have.

Another complicating factor is that this package has been packaged for almost every Linux and BSD distro known. Because of that, I don't want to be pushing versions left and right, and certainly not breaking backwards compatibility unless necessary.

Because of those, it may be a little while until I tag a new release. If everything goes to plan, the release after that will be 1.0.0, at which point hopefully things will be pretty stable.

@wbond
Copy link
Owner

wbond commented Aug 10, 2019

One thing you may be able to do is a git URL to asn1crypto instead of grabbing from pypi. Just be sure to pin to a specific hash instead of master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants