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

SSL: register OpenSSL threading callbacks when we can't access Qt's OpenSSL. #3174

Merged
merged 4 commits into from Jul 17, 2017

Conversation

@mkrautz
Copy link
Member

commented Jul 15, 2017

We neglected to register our own callbacks for locking and thread IDs
when we removed the restriction that we only allow one copy of OpenSSL
in the address space. (f544524)

This commit remedies that by providing our own set of callbacks for
locking and getting thread IDs to OpenSSL.

Previously, we just expected that Qt would properly initialize OpenSSL.
However, when Qt and us use separate copies of OpenSSL -- we have to do
it ourselves.

@mkrautz mkrautz force-pushed the mkrautz:ssl-locking branch 6 times, most recently from 61ef7fa to ce18763 Jul 15, 2017

@mkrautz mkrautz changed the title WIP: SSL: register OpenSSL threading callbacks when we can't access Qt's OpenSSL. SSL: register OpenSSL threading callbacks when we can't access Qt's OpenSSL. Jul 15, 2017

@mkrautz mkrautz requested review from Kissaki, hacst and davidebeatrici Jul 15, 2017

@Kissaki
Copy link
Member

left a comment

Looks plausible. Didn’t check the OpenSSL docs on this.

So apparently we forgot to initialize MumbleSSL in tests. We have no compile time guarantee that we do. Should we make it a compile time requirement (RAII) or leave it? Maybe the classes that depend on OpenSSL should require a MumbleSSL instance, which then either initializes an additional OpenSSL or not.

Apart from the two comments, LGTM.

#include <openssl/rand.h>

/// SSLStresser is a thread that runs operations on OpenSSL's
/// RAND infrastructure, in an attemp to crash/segfault

This comment has been minimized.

Copy link
@Kissaki

Kissaki Jul 16, 2017

Member

typo attemp[t]

///
/// The idea is that without proper locking, the data races we're causing
/// should quite quickly cause the process to crash if proper locking isn't
/// available.

This comment has been minimized.

Copy link
@Kissaki

Kissaki Jul 16, 2017

Member

without proper locking … if proper locking isn't available

Can drop the second, duplicate part

src/SSL.cpp Outdated

#include "Version.h"

void MumbleSSL::initialize() {
// Let Qt initialize its copy of OpenSSL, if it's different than
// Mumble's.

This comment has been minimized.

Copy link
@Kissaki

Kissaki Jul 16, 2017

Member

Maybe mention that that is a side effect of calling supportsSsl? While this comment implies it, it could be made explicit.

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2017

So apparently we forgot to initialize MumbleSSL in tests. We have no compile time guarantee that we do. Should we make it a compile time requirement (RAII) or leave it? Maybe the classes that depend on OpenSSL should require a MumbleSSL instance, which then either initializes an additional OpenSSL or not.

I think it's hard to do well. A lot of the callers aren't a "class", but are static methods. If we had to call ensureInitialized() for each of them, I think that'd be excessive. (In terms of source-code noise, at least).

I'd like to keep what we have here now.

A better solution would perhaps be to have our own MAIN macro like http://doc.qt.io/qt-5/qtest.html#QTEST_MAIN
Then we could ensure things that should be initialized, are initialized.

@mkrautz mkrautz force-pushed the mkrautz:ssl-locking branch from ce18763 to 7f1206c Jul 16, 2017

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2017

@Kissaki PTAL

@Kissaki
Copy link
Member

left a comment

LGTM

Tell me if you want me to verify the OpenSSL API/stuff as well.

@mkrautz mkrautz force-pushed the mkrautz:ssl-locking branch 2 times, most recently from 747f494 to a1f58cb Jul 16, 2017

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2017

@Kissaki PTAL, updated with cleanups.

@hacst
Copy link
Member

left a comment

See comments.

Also do you happen to have a link to documentation describing the callbacks? I pretty much found nothing except examples of other projects using it. I guess I should no longer be surprised by the horrible quality of OpenSSL documentation but maybe I just missed it...

On another note I found openssl/openssl#1260 which indicates CRYPTO_set_locking_callback is gone (or at least a NOP and soon to be gone) in OpenSSL 1.1.0. Does this patch have to account for that?

Apart from that looks sensible to me. As mentioned I couldn't find OpenSSL docs on the callback but our use matched other projects so it's hopefully fine (having to say that in crypto code context makes me cringe so hard...).

// So, in all cases, it's safe to use unsigned long.
//
// The trouble is, since the return type of QThread::currentThreadId()
// difers between platoforms, we have to be clever about how we convert

This comment has been minimized.

Copy link
@hacst

hacst Jul 16, 2017

Member

typos: differs, platforms

/// seed the RAND system. This is controlled by the |seed|
/// parameter to the constructor.
///
/// We never clean these threads up -- instead, we rely on the

This comment has been minimized.

Copy link
@hacst

hacst Jul 16, 2017

Member

Outdated?


#include "SSLLocks.h"

static QMutex **locks = NULL;

This comment has been minimized.

Copy link
@hacst

hacst Jul 16, 2017

Member

Looks like we leak these. Why not a std::vector instead? (boost::ptr_vector is an alternative that would retain memory layout/semantics. Don't think we need to care about that though. Do we?)

As mentioned on IRC I'm a big fan of leak-free execution so you know that if you see a leak it actually is a problem. Then again I have no clue what the Qt policy is on that to begin with. In non Qt C++ & Boost applications I had very good results with leak sanitizer based on such a zero leaks policy.

This comment has been minimized.

Copy link
@mkrautz

mkrautz Jul 17, 2017

Author Member

I'd like to avoid static globals for now. We can't use Q_STATIC_GLOBAL either in 1.3.x, since we need to support Qt 4.
So I've added a destroy() function for now as a compromise. We can revisit and clean stuff up once we can throw away a lot of the legacy baggage.

This comment has been minimized.

Copy link
@hacst

hacst Jul 17, 2017

Member

Oh. I didn't realize we had limitations there. Fine with me.

@Kissaki

This comment has been minimized.

Copy link
Member

commented Jul 16, 2017

Commit a1f58cb mentions that OpenSSL 1.1 provides its own locking, and that in that case the test is not really that useful (does not test our logic as it does for versions prior to 1.1).

SSL: add destroy() function to the SSL module.
This is in preparation for adding proper OpenSSL multithreaded locking
for when we use our own copy of OpenSSL instead of Qt's.

@mkrautz mkrautz force-pushed the mkrautz:ssl-locking branch from a1f58cb to fbc230d Jul 17, 2017

mkrautz added 3 commits Jul 16, 2017
SSL: register OpenSSL threading callbacks when we can't access Qt's O…
…penSSL.

We neglected to register our own callbacks for locking and thread IDs
when we removed the restriction that we only allow one copy of OpenSSL
in the address space. (f544524)

This commit remedies that by providing our own set of callbacks for
locking and getting thread IDs to OpenSSL.

Previously, we just expected that Qt would properly initialize OpenSSL.
However, when Qt and us use separate copies of OpenSSL -- we have to do
it ourselves.
src/tests: update tests to initialize and destroy the MumbleSSL modul…
…e to ensure OpenSSL is properly initialized.
src/tests: add TestSSLLocks test for testing our OpenSSL locking impl…
…ementation.

This test tests that our SSLLocks class works correctly.

Note: Since OpenSSL 1.1, OpenSSL provides their own locking for Windows
and POSIX systems. So if you're playing around with this test, you should
only be able to make it crash OpenSSL versions prior to 1.1.

@mkrautz mkrautz force-pushed the mkrautz:ssl-locking branch from fbc230d to f6fb4d8 Jul 17, 2017

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented Jul 17, 2017

Also do you happen to have a link to documentation describing the callbacks? I pretty much found nothing except examples of other projects using it. I guess I should no longer be surprised by the horrible quality of OpenSSL documentation but maybe I just missed it...

https://www.openssl.org/blog/blog/2017/02/21/threads/

https://www.openssl.org/docs/man1.0.2/crypto/threads.html

Those references were what I used.

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented Jul 17, 2017

@hacst, @Kissaki PTAL, I've addressed your comments.

@hacst
hacst approved these changes Jul 17, 2017
@hacst

This comment has been minimized.

Copy link
Member

commented Jul 17, 2017

LGTM

@mkrautz mkrautz merged commit 5b82a7a into mumble-voip:master Jul 17, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.