Handle multiple SSL_ERROR_WANT_ASYNC which is returned by ASYNC engine #1387

nidhidamodaran · 2020-06-14T14:46:48Z

@afrind , Please review the updated changes as per your comments.

The changes are required to handle cases where multiple SSL_ERROR_WANT_ASYNC is returned by the async engine.

Also I have removed the code which closes the async fd as this is my crypto driver handle and should not be closed by upper layers as I require it further in my SSL connection

afrind

Can you add a unit test that reproduces the issue you are experiencing that demonstrates your fix solves the problem?

afrind · 2020-06-16T23:23:14Z

folly/io/async/AsyncPipe.cpp

@@ -48,8 +48,6 @@ void AsyncPipeReader::close() {

    if (closeCb_) {
      closeCb_(fd_);
-    } else {
-      netops::close(fd_);


Removing this seems wrong - close() no longer closes the underlying fd when closeCb_ is not defined?

This fd was added in async list by underlying openssl. Isn't it better to let the ssl library close the fd once operation is complete.

Also, If I close here, my driver handle will be closed and cannot be used for any further in the SSL connection.

@afrind I will elaborate my use case too here:

I have a crypto device to which asymmetric and symmetric operations from openssl are offloaded. Instead of pipe fd, I am using the driver handle of this device as an async fd, which is signalled whenever the crypto operation is completed. I am using this same async fd throughout the handshake.

This handle has to be open throughout the length of an SSL connection (for record processing too). Also in this design, for each of crypto operation done in ASYNC mode, the SSL_ERROR_WANT_ASYNC is returned. As such after such a request is processed at folly layer and SSL_accept restarted, another crypto operation may return same error and thereby returning multiple WANT_ASYNC errors in sequence, which the existing folly is not handling correctly.

The issue is that if sslSocket_->restartSSLAccept again returns WANT_ASYNC error, asyncOperationFinishCallback_ is not be set null and further events not processed

AsyncPipeWriter is generic and can have uses other than this use case. I think what you want to do instead is set the closeCB_ to a no-op in your case?

@afrind , Thanks for the review. I have removed the change so as to not break other use cases. Please review rest of the code changes.

nidhidamodaran · 2020-06-22T08:46:53Z

test_patch.txt

@afrind , please use the above patch to reproduce issue I currently face. I created a test case to return multiple SSL_ERROR_WANT_ASYNC, but when run it always returns a SSL_ERROR_WANT_READ between two continuous SSL_ERROR_WANT_ASYNC errors. Though the attached patch is not really creating an async job, but you can use it to quickly reproduce the error.

The original folly code will hang with this change, ie when 2 continuous SSL_ERROR_WANT_ASYNC is received, and this PR fixes the issue. Please verify.
Let me know if you need any further details

nidhidamodaran · 2020-06-22T10:01:47Z

Also please note :Existing folly code works correctly if between two SSL_ERRROR_WANT_ASYNC error codes, a non-fatal error code is returned as this will clean all stale data structures.

nidhidamodaran · 2020-06-23T10:19:35Z

@afrind , I have added a testcase to return SSL_ERROR_WANT_ASYNC multiple times. I have also committed cert/key/CAcert as the one given in repo doesn't seem to be working.

Please review. Please note the error is only reproducible if between 2 SSL_ERROR_WANT_ASYNC errors, no other error codes are returned, say SSL_ERROR_WANT_READ.

facebook-github-bot

@afrind has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

yfeldblum · 2020-06-30T20:08:04Z

Legit.

Can we avoid checking in new keys/certs? It should be possible to generate small keys quickly and in-memory at unit-test time.

nidhidamodaran · 2020-07-01T15:00:49Z

@afrind @yfeldblum , I have removed the certs committed earlier and used the existing certs. Please review.

facebook-github-bot

@afrind has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

afrind

These comments came from another developer internally

afrind · 2020-07-10T17:01:41Z

folly/io/async/test/AsyncSSLSocketTest.cpp

@@ -1525,6 +1525,79 @@ static int kRSAEvbExIndex = -1;
 static int kRSASocketExIndex = -1;
 static constexpr StringPiece kEngineId = "AsyncSSLSocketTest";

+int customRsaPubDec(int flen, const unsigned char *from, unsigned char *to, RSA *rsa,


Should be 'static'

afrind · 2020-07-10T17:02:08Z

folly/io/async/test/AsyncSSLSocketTest.cpp

+  return ret;
+}
+
+int verifyCb(X509_STORE_CTX *ctx, void *arg) {


Should be 'static'

afrind · 2020-07-10T17:02:45Z

folly/io/async/test/AsyncSSLSocketTest.cpp

+
+  ssl::EvpPkeyUniquePtr publicEvpPkey(X509_get_pubkey(cert));
+  EVP_PKEY *pkey = publicEvpPkey.get();
+  RSA* rsa = EVP_PKEY_get1_RSA(publicEvpPkey.get());


I think this is a leak (openssl _get1 increments (adds by 1) the reference count of the undelrying object), and the reason why tests aren't catching this is because we disable this test when running under ASAN mode.

See line 1762 for where we disable

afrind · 2020-07-10T17:04:03Z

folly/io/async/AsyncSSLSocket.h

      pipeReader_->setReadCB(nullptr);
      sslSocket_->setAsyncOperationFinishCallback(nullptr);
+      sslSocket_->restartSSLAccept();


Please add a comment here explaining why order is significant

@afrind I have addressed all your comments, please review

facebook-github-bot

@afrind has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

afrind · 2020-07-24T22:21:31Z

The test failed with a SEGV internally in opt mode?

*** Aborted at 1595628991 (Unix time, try 'date -d @1595628991') ***
*** Signal 11 (SIGSEGV) (0x0) received by PID 2984783 (pthread TID 0x7fbba672a3c0) (linux TID 2984783) (code: 128), stack trace: ***
    @ 000000000061b6f5 folly::symbolizer::(anonymous namespace)::signalHandler(int, siginfo_t*, void*)
                       ./folly/experimental/symbolizer/SignalHandler.cpp:442
    @ 0000000000000000 (unknown)
    @ 00000000001d6e1e RSA_free
    @ 0000000000193f22 EVP_PKEY_free
    @ 000000000022cbfb pubkey_cb
    @ 00000000000a365e asn1_item_embed_free
    @ 00000000000a397f asn1_template_free
    @ 00000000000a3642 asn1_item_embed_free
    @ 00000000000a397f asn1_template_free
    @ 00000000000a3642 asn1_item_embed_free
    @ 00000000000a38b6 ASN1_item_free
    @ 000000000020b53a OPENSSL_sk_pop_free
    @ 000000000003dffa SSL_free
    @ 000000000056f100 folly::AsyncSSLSocket::~AsyncSSLSocket()
                       ./folly/Memory.h:302
                       -> ./folly/io/async/AsyncSSLSocket.cpp
    @ 000000000056f6bd folly::AsyncSSLSocket::~AsyncSSLSocket()
                       ./folly/io/async/AsyncSSLSocket.cpp:315
    @ 000000000058b634 folly::AsyncSocket::destroy()
                       ./folly/io/async/DelayedDestruction.h:55
                       -> ./folly/io/async/AsyncSocket.cpp
    @ 0000000000448019 folly::AsyncSSLSocketTest_OpenSSL110MultipleAsyncTest_Test::TestBody()
                       ./folly/io/async/DelayedDestruction.h:70
                       -> ./folly/io/async/test/AsyncSSLSocketTest.cpp
    @ 000000000069433e void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*)
                       /home/engshare/third-party2/googletest/master/src/googletest/googletest/src/gtest.cc:2473
                       -> /home/engshare/third-party2/googletest/master/src/googletest/googletest/src/gtest-all.cc
    @ 0000000000685281 testing::Test::Run() [clone .part.595]
                       /home/engshare/third-party2/googletest/master/src/googletest/googletest/src/gtest.cc:2490
                       -> /home/engshare/third-party2/googletest/master/src/googletest/googletest/src/gtest-all.cc
    @ 0000000000685701 testing::TestInfo::Run() [clone .part.596]
                       /home/engshare/third-party2/googletest/master/src/googletest/googletest/src/gtest.cc:2481
                       -> /home/engshare/third-party2/googletest/master/src/googletest/googletest/src/gtest-all.cc
    @ 0000000000685894 testing::TestCase::Run() [clone .part.597]
                       /home/engshare/third-party2/googletest/master/src/googletest/googletest/src/gtest.cc:2801
                       -> /home/engshare/third-party2/googletest/master/src/googletest/googletest/src/gtest-all.cc
    @ 0000000000686e34 testing::internal::UnitTestImpl::RunAllTests()
                       /home/engshare/third-party2/googletest/master/src/googletest/googletest/src/gtest.cc:4735
                       -> /home/engshare/third-party2/googletest/master/src/googletest/googletest/src/gtest-all.cc
    @ 0000000000687124 testing::UnitTest::Run()
                       /home/engshare/third-party2/googletest/master/src/googletest/googletest/src/gtest.cc:2473
                       -> /home/engshare/third-party2/googletest/master/src/googletest/googletest/src/gtest-all.cc
    @ 000000000048abe5 main
                       ./third-party-buck/platform007/build/googletest/include/gtest/gtest.h:2255
                       -> ./folly/io/async/test/AsyncSSLSocketTest2.cpp
    @ 00000000000211a5 __libc_start_main
    @ 0000000000420679 _start
                       /home/engshare/third-party2/glibc/2.26/src/glibc-2.26/csu/../sysdeps/x86_64/start.S:120

nidhidamodaran · 2020-07-28T06:30:23Z

@afrind please help me reproduce this error. How do I build folly in opt mode ?

afrind · 2020-07-28T15:29:13Z

I'm actually not sure since we use a different build system internally. I think it's a combination of -NDEBUG and -O3 mostly? If you are linking against jemalloc, try MALLOC_CONF=junk:true,abort:true ?

Handle multiple ASYNC_pause which is returned by ASYNC engine

3b76422

facebook-github-bot added the CLA Signed label Jun 14, 2020

afrind reviewed Jun 16, 2020

View reviewed changes

nidhidamodaran requested a review from afrind June 22, 2020 09:27

Added test case which returns multiple SSL_ERROR_WANT_ASYNC errors

f7ece7e

Revert pipe cleanup change done in last commit

68899c7

facebook-github-bot reviewed Jun 30, 2020

View reviewed changes

Remove certs/key added in earlier commit

1a406ee

facebook-github-bot reviewed Jul 1, 2020

View reviewed changes

afrind reviewed Jul 10, 2020

View reviewed changes

Addressed FB internal review comments

291c7e4

ammarfaizi2 approved these changes Jul 21, 2020

View reviewed changes

facebook-github-bot reviewed Jul 24, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle multiple SSL_ERROR_WANT_ASYNC which is returned by ASYNC engine #1387

Handle multiple SSL_ERROR_WANT_ASYNC which is returned by ASYNC engine #1387

nidhidamodaran commented Jun 14, 2020

afrind left a comment

afrind Jun 16, 2020

nidhidamodaran Jun 17, 2020

nidhidamodaran Jun 22, 2020 •

edited

afrind Jun 29, 2020

nidhidamodaran Jun 30, 2020

nidhidamodaran commented Jun 22, 2020 •

edited

nidhidamodaran commented Jun 22, 2020

nidhidamodaran commented Jun 23, 2020 •

edited

facebook-github-bot left a comment

yfeldblum commented Jun 30, 2020

nidhidamodaran commented Jul 1, 2020

facebook-github-bot left a comment

afrind left a comment

afrind Jul 10, 2020

afrind Jul 10, 2020

afrind Jul 10, 2020

afrind Jul 10, 2020

afrind Jul 10, 2020

nidhidamodaran Jul 13, 2020

facebook-github-bot left a comment

afrind commented Jul 24, 2020

nidhidamodaran commented Jul 28, 2020

afrind commented Jul 28, 2020

Handle multiple SSL_ERROR_WANT_ASYNC which is returned by ASYNC engine #1387

Are you sure you want to change the base?

Handle multiple SSL_ERROR_WANT_ASYNC which is returned by ASYNC engine #1387

Conversation

nidhidamodaran commented Jun 14, 2020

afrind left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nidhidamodaran Jun 22, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nidhidamodaran commented Jun 22, 2020 • edited

nidhidamodaran commented Jun 22, 2020

nidhidamodaran commented Jun 23, 2020 • edited

facebook-github-bot left a comment

Choose a reason for hiding this comment

yfeldblum commented Jun 30, 2020

nidhidamodaran commented Jul 1, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

afrind left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

afrind commented Jul 24, 2020

nidhidamodaran commented Jul 28, 2020

afrind commented Jul 28, 2020

nidhidamodaran Jun 22, 2020 •

edited

nidhidamodaran commented Jun 22, 2020 •

edited

nidhidamodaran commented Jun 23, 2020 •

edited