Internal OpenSSL initialization affecting the application

[Arpit Baldeva]

Hi,

I recently pinned down a sporadic race condition in my application due to grpc intializing OpenSSL internally. The problem is that OpenSSL has some global callbacks that grpc is trying to initialize on it's own without the authorization of the application. The problem is in the 

init_openssl call in ssl_transport_security.cc which trounces similar calls from the application. The situation is made worse(race condition) if some application threads already have locks acquired via previous calls and then grpc changes the stuff underneath before the locks are released (hence the race condition). 

My application has usage of OpenSSL in a wider context than just grpc. I get the point that grpc is wrapping this up to make it easier for standing up an application with grpc but I think such type of things should be accompanied by a compile/run time option supplied at grpc_init that let's application decide the right owner. I am fine with the option defaulting to grpc initializing the OpenSSL internally. 

Thoughts?

Thanks.

[jian...@google.com]

Hi Arpit,

grpc_init initializes OpenSSL for a short period (~2 days) and the code was later removed. Do you still the problem, if you fetch the latest master?

[Arpit Baldeva]

I am using grpc-1.10.0 and it has that code. Looking at the latest master, it still has that code - https://github.com/grpc/grpc/blob/master/src/core/tsi/ssl_transport_security.cc - see the init_openssl function.

The problem is not just that grpc_init initialized the OpenSSL. The problem is really that grpc is initializing OpenSSL internally and it can trounce the application settings. Grpc should let application choose if they want it to handle the OpenSSL initialization or not. 

Thanks. 

[jian...@google.com]

Could you describe the race condition in details?

In gRPC ssl_transport_security, init_openssl() is only called when ssl transport security is needed. In your application, you are calling SSL_library_init() in a separate thread in parallel?

[Arpit Baldeva]

Yes, there are two parallel threads that do this at the same time. What I was noticing is that at application shutdown, my application complained about the lock I handed over to OpenSSL not being unlocked. Looking into it, I realized that during initialization, OpenSSL grabbed a lock from my callback, during this time, grpc replaced the locks and OpenSSL unlock call got re-routed. But again, this is just a side effect. The real problem is simply the fact that grpc is setting some global callbacks on OpenSSL that should best be left to the application. I understand that this is probably done to make it easy for the grpc integrators but there are applications such as mine where there are other usage of OpenSSL and I'd rather manage the global settings myself than rely on grpc to do it.

[Jiangtao Li]

I don't see a convenient way to do in gRPC. SSL init can be called multiple times, as long as it is not called in the same time. It seems that the best way to address is adding mutex in your application to make sure SSL init is not called simultaneously. 

--
You received this message because you are subscribed to a topic in the Google Groups "grpc.io" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/grpc-io/Z48vpXRnJaw/unsubscribe.
To unsubscribe from this group and all its topics, send an email to grpc-io+u...@googlegroups.com.
To post to this group, send email to grp...@googlegroups.com.
Visit this group at https://groups.google.com/group/grpc-io.
To view this discussion on the web visit https://groups.google.com/d/msgid/grpc-io/3df13436-d771-4450-85c3-d2683b08e976%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Arpit Baldeva]

Again, I am not sure why you are focusing on the race condition.  Race condition is not the problem. Fact that grpc decides to put some global callbacks on the OpenSSL is the problem. It should not do that. Why can't this be made optional via a compile time flag or some run time initialization parameter?

[Arpit Baldeva]

To explain further, a library such as grpc intializing global settings of another library is not a good practice. Looking at libcurl (https://curl.haxx.se/libcurl/c/curl_global_init.html) , PostgreSQL(https://www.postgresql.org/docs/9.1/static/libpq-ssl.html ) (and may be there are more libraries), these take an initialization flag that to decide whether OpenSSL should be initialized internally or not.

Actually, looking at https://stackoverflow.com/questions/21874152/openssl-thread-safety-callback-function-registration-with-both-direct-call-and-i , the simplest solution here is for grpc to check if locking callback already exists by calling CRYPTO_get_id_callback and if so, skip putting it's own locks. Rest of the init functions in OpenSSL are idempotent and hence can be called multiple times. And looks like grpc is not cleaning up OpenSSL so we are fine there too (looks like clean up calls are getting no-op in future - https://stackoverflow.com/questions/35802643/will-ignoring-to-call-openssl-evp-cleanup-result-in-serious-flaws-or-memory-leak ) .

[Jiangtao Li]

Which version of OpenSSL are you using? Or you are using BoringSSL? OpenSSL 1.1 or BoringSSL does not have such problems on OpenSSL init.

For OpenSSL 1.0x, it is a valid concern. Let me check what is the best way to resolve this issue (pass a compiler flag, environment variable, or some API changes).

Thanks,

Jiangtao

To view this discussion on the web visit https://groups.google.com/d/msgid/grpc-io/8821745f-9b23-4e3e-b4cf-b88bce513b4b%40googlegroups.com.

[Arpit Baldeva]

I am on 1.0.2k so yeah it is a problem on that version. 

I think the simplest fix is what I mentioned in last email -  grpc init_openssl implementation can check if locking callback already exists by calling CRYPTO_get_id_callback and if so, skip putting it's own locks. The application can ensure that it has the right initialization in place before doing anything with grpc.

Thanks.

[jian...@google.com]

Arpit,

Could you please patch https://github.com/grpc/grpc/pull/15143 and see whether it solves your problem.

[Arpit Baldeva]

Thanks for the patch. It did fix the issue!

What release version are you targeting?

Thanks.

[Jiangtao Li]

Good to know. Once the patch approved and merged. It will be in next grpc release.

Thanks,

Jiangtao

To view this discussion on the web visit https://groups.google.com/d/msgid/grpc-io/d178eff0-8225-4a0b-b63f-ac15afe425e3%40googlegroups.com.

[CM]

This change didn't really fix the problem -- there is still a race here with other thread(s) un-initializing OpenSSL. Old problem, the only correct solution is to leave OpenSSL initialization to user.

[Jiangtao Li]

Initializing OpenSSL outside gRPC is not practical in many situations. Our internal expert says the race condition only exist for OpenSSL 1.0.x and 0.9.x. We are on best effort to support older versions of OpenSSL, while our main support is BoringSSL and OpenSSL 1.1.

Thanks,

Jiangtao

To view this discussion on the web visit https://groups.google.com/d/msgid/grpc-io/221ffdb6-aa2c-457d-8772-32ace46173dc%40googlegroups.com.

[Arpit Baldeva]

Grpc does not un-initialize OpenSSL. If you have other thread that is un-initing it, you can easily coordinate that after grpc shutdown.  The patch is only ensuring that it is not overriding application settings. While the cleanest solution is to provide an explicit flag, the solution in the patch works as long as the user knows what’s happening behind the scenes. 

Sent from my iPhone

To view this discussion on the web visit https://groups.google.com/d/msgid/grpc-io/CACcm8_i7f6HdGG3G72p_FN2UqwyUzaDyDVKGE3oNP1Xbi9GSwA%40mail.gmail.com.

[Michael Kilburn]

Well, there is no official way to fully uninitialize OpenSSL. Also, that is not what I meant by my remark -- SSL-library-init functions are not threadsafe, afaik. I.e. calling them from different threads at the same time is unsafe. And it is not easy to guarantee that this won't happen if grpc is doing it under the covers.

To unsubscribe from this group and all its topics, send an email to grpc-io+unsubscribe@googlegroups.com.

To post to this group, send email to grp...@googlegroups.com.
Visit this group at https://groups.google.com/group/grpc-io.
To view this discussion on the web visit https://groups.google.com/d/msgid/grpc-io/221ffdb6-aa2c-457d-8772-32ace46173dc%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to a topic in the Google Groups "grpc.io" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/grpc-io/Z48vpXRnJaw/unsubscribe.

To unsubscribe from this group and all its topics, send an email to grpc-io+unsubscribe@googlegroups.com.

To post to this group, send email to grp...@googlegroups.com.
Visit this group at https://groups.google.com/group/grpc-io.
To view this discussion on the web visit https://groups.google.com/d/msgid/grpc-io/CACcm8_i7f6HdGG3G72p_FN2UqwyUzaDyDVKGE3oNP1Xbi9GSwA%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

--

Sincerely yours,
Michael.

[Arpit Baldeva]

Your original remark said - there is still a race here with other thread(s) un-initializing OpenSSL  (talking about un-initializing)

Now you are saying - that is not what I meant by my remark -- SSL-library-init functions are not threadsafe, afaik. (talking about initializing)

My response was for the former. The original problem I had was that grpc would internally trounce my settings and I can not tell everyone else in my setting to wait for some unknown time when grpc would trounce those settings in some internal thread that I don't know about and only use OpenSSL after it. Now, it's an easier problem that I just need to make sure the OpenSSL is ready to use before I call grpc_init. As grpc does not do any shutdown on OpenSSL, I can ensure that my application is only cleaning it up after I am done with grpc.

As for your latest remark that SSL-library-init functions are not threadsafe, they could also be wrapped inside the callback check. I have not done any research on them being not thread-safe so if you have reference literature on it, that'd be good to see. Still as the calls are idempotent, this thread synchronization is an easy problem.  

[Michael Kilburn]

Yeah, "un-initialization functions" meant to mean "any of initialization or uninitialization functions". I didn't communicate my point correctly.

>  As grpc does not do any shutdown on OpenSSL, I can ensure that my application is only cleaning it up after I am done with grpc.

... which is going to work swell if someone loads/unloads grpc dynamically ;-)

>  As for your latest remark that SSL-library-init functions are not threadsafe, they could also be wrapped inside the callback check. 

Oh! Good thing you reminded me -- "callback check" in this pull request isn't threadsafe. :-) I.e. this:

 if (!CRYPTO_get_locking_callback()) {

   int num_locks = CRYPTO_num_locks();

   GPR_ASSERT(num_locks > 0);

   g_openssl_mutexes = static_cast<gpr_mu*>(

       gpr_malloc(static_cast<size_t>(num_locks) * sizeof(gpr_mu)));

   for (int i = 0; i < num_locks; i++) {

     gpr_mu_init(&g_openssl_mutexes[i]);

   }

   CRYPTO_set_locking_callback(openssl_locking_cb);

   CRYPTO_set_id_callback(openssl_thread_id_cb);

 } else {

   gpr_log(GPR_INFO, "OpenSSL callback has already been set.");

 }

is broken. Does it need an explanation or it is obvious?

> I have not done any research on them being not thread-safe so if you have reference literature on it, that'd be good to see.

Library init functions typically aren't threadsafe.

>  Still as the calls are idempotent, this thread synchronization is an easy problem.   

If you know every detail of every call -- yes, you can add synchronization in all spots. Problem is that when you use 3rdparty lib that uses grpc under the cover (or it uses other lib that uses grpc) -- I bet $50 it is not going to be done correctly.

--

Sincerely yours,
Michael.

[Arpit Baldeva]

While I could reply in more details, I'll just add that if you don't feel happy with this patch, may be feel free to contribute a better patch (and for the record, I agree that passing a flag to let grpc know whether to initialize OpenSSL or not is the right thing to do and was the first solution I proposed in original mail).

It's clear that the patch is not handling your ways of using grpc. It handles mine without any threading issue/race conditions and I am happy with it.

[jian...@google.com]

Michael,

I think we are trying stop supporting OpenSSL without ALPN (i.e., OpenSSL 1.0.1 and below) soon in gRPC.

If you feel this still needs to be addressed. Welcome to communicate with Nicolas, and if he agrees, feel free to contribute to gRPC code with your proposed solution.

To view this discussion on the web visit https://groups.google.com/d/msgid/grpc-io/221ffdb6-aa2c-457d-8772-32ace46173dc%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to a topic in the Google Groups "grpc.io" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/grpc-io/Z48vpXRnJaw/unsubscribe.

To unsubscribe from this group and all its topics, send an email to grpc-io+u...@googlegroups.com.
To post to this group, send email to grp...@googlegroups.com.
Visit this group at https://groups.google.com/group/grpc-io.

To view this discussion on the web visit https://groups.google.com/d/msgid/grpc-io/CACcm8_i7f6HdGG3G72p_FN2UqwyUzaDyDVKGE3oNP1Xbi9GSwA%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

--

Sincerely yours,
Michael.

--

Sincerely yours,
Michael.

[Michael Kilburn]

Afaik, all existing OpenSSL versions can't be safely initialized in the way you do it. I already explained why -- there is no point going through it again.

The only proper way dealing with this (that I know) -- declare that your library uses OpenSSL and leave end user to initialize it. If your user is snother library writer -- he'll have to do the same. Only "top-lvl user" will know for sure when and how to properly in it those libs. Same for libcurl/etc. I think the proper term for this is "global state poisoning" -- basically because you use OpenSSL and it has global state that needs to initialized -- it poisons you and now you have global state that needs to be initialized. And there is no way to auto-initialize it in such way that it works in every circumstance.

I know I can contribute, but my point that to solve it properly you'll have to break exiting clients. I doubt you'll accept PR like that.

To unsubscribe from this group and all its topics, send an email to grpc-io+unsubscribe@googlegroups.com.

To post to this group, send email to grp...@googlegroups.com.
Visit this group at https://groups.google.com/group/grpc-io.

To view this discussion on the web visit https://groups.google.com/d/msgid/grpc-io/73d94255-1f90-418f-92ec-524e9cf6f2d6%40googlegroups.com.