CDRIVER-3668 support OCSP back to OpenSSL 1.0.1 #623

kevinAlbs · 2020-05-29T00:12:33Z

Adds polyfills and makes a few tweaks to support OpenSSL back to 1.0.1.

Summary of changes:

change SSL_CTX_set_tlsext_status_type to SSL_set_tlsext_status_type.
polyfill SSL_get0_verified_chain, NID_tlsfeature, and hostname check.
check for status_request from the tlsfeature extension when inspecting peer certificate.
skip time check for older OpenSSL when updating cache entries.
perform the OCSP check after the handshake, since sometimes the peer certificate is not available in the callback in OpenSSL <= 1.0.2.
check tlsDisableOCSPEndpointCheck before reaching out to a responder.
make tlsDisableOCSPEndpointCheck and tlsDisableCertificateRevocationCheck URI options implicitly enable TLS.
enable OCSP tests on OpenSSL and macOS that were skipped.
add OCSP tests for OpenSSL 1.0.1.
update OCSP OpenSSL documentation.
change OCSP verification logs from MONGOC_DEBUG to TRACE in successful cases.

The first commit has the changes, the second has the config.yml regeneration.

Full evergreen patch build: https://evergreen.mongodb.com/version/5ed03422a4cf471c38658ada

- change SSL_CTX_set_tlsext_status_type to SSL_set_tlsext_status_type. - polyfill SSL_get0_verified_chain, NID_tlsfeature, and hostname check. - check for status_request from the tlsfeature extension when inspecting peer certificate. - skip time check for older OpenSSL when updating cache entries. - perform the OCSP check after the handshake, since sometimes the peer certificate is not available in the callback in OpenSSL <= 1.0.2. - check tlsDisableOCSPEndpointCheck before reaching out to a responder. - make tlsDisableOCSPEndpointCheck and tlsDisableCertificateRevocationCheck URI options implicitly enable TLS. - enable OCSP tests on OpenSSL and macOS that were skipped. - add OCSP tests for OpenSSL 1.0.1. - update OCSP OpenSSL documentation. - change OCSP verification logs from MONGOC_DEBUG to TRACE in successful cases.

kevinAlbs · 2020-05-29T00:14:51Z

build/evergreen_config_lib/tasks.py

        super(CompileTask, self).__init__(task_name=task_name,
                                          depends_on=depends_on,
                                          tags=tags,
                                          **kwargs)

-        self.extra_commands = extra_commands or []
+        self.suffix_commands = suffix_commands or []
+        self.prefix_commands = prefix_commands or []


I added "prefix_commands" here, so I could create a debug-compile task that used OpenSSL 1.0.1 specifically. That needs to call the "install ssl" evergreen function first.

kevinAlbs · 2020-05-29T00:21:11Z

src/libmongoc/src/mongoc/mongoc-ocsp-cache.c

+    * If a driver would accept a stapled OCSP response and that response has a
+    * later nextUpdate than the response already in the cache, drivers SHOULD
+    * replace the older entry in the cache with the fresher response. */
+   return -1;


I'll admit, this is not ideal. After searching a bit, and checking with Shreyas to see how the server polyfilled the time comparison, it seems like we'd need to parse the string representation of the time to something we can easily compare:
https://github.com/mongodb/mongo/blob/f627ff829ba96487fccb6945d0c2807b1bb51d52/src/mongo/util/net/ssl_manager_openssl.cpp#L375-L412

It doesn't look like it's required by the spec to check nextUpdate when updating the cache. By always comparing "a" to be before "b", we won't overwrite cache entries. They'll still get removed when they're detected as being expired by OCSP_check_validity.

If this seems ok with you, I'll create a ticket to do this as post-4.4 follow-up.

Created https://jira.mongodb.org/browse/CDRIVER-3707

kevinAlbs · 2020-05-29T00:22:36Z

src/libmongoc/src/mongoc/mongoc-openssl.c

+#else
+   /* TLS versions before 1.1.0 did not define the TLS Feature extension. */
+   tlsfeature_nid =
+      OBJ_create ("1.3.6.1.5.5.7.1.24", "tlsfeature", "TLS Feature");


The server does something similar to define the OID when it is not predefined in OpenSSL:
https://github.com/mongodb/mongo/blob/f627ff829ba96487fccb6945d0c2807b1bb51d52/src/mongo/util/net/ssl_manager_openssl.cpp#L357-L361

kevinAlbs · 2020-05-29T00:23:34Z

src/libmongoc/src/mongoc/mongoc-openssl.c

+
+STACK_OF (X509) * _get_verified_chain (SSL *ssl)
+{
+   X509_STORE *store = NULL;


I based this off of what the server does: https://github.com/mongodb/mongo/blob/f627ff829ba96487fccb6945d0c2807b1bb51d52/src/mongo/util/net/ssl_manager_openssl.cpp#L312-L327

kevinAlbs · 2020-05-29T00:27:27Z

src/libmongoc/src/mongoc/mongoc-openssl.c

+
+#define TLSFEATURE_STATUS_REQUEST 5
+
+/* Parse just enough of a DER encoded data to check if a SEQUENCE of INTEGER


The server has a full DER parser. We only need to parse this one thing, a SEQUENCE of INTEGER.
I used https://docs.microsoft.com/en-us/windows/win32/seccertenroll/about-sequence as a reference for the expected sequence, and this website: http://lapo.it/asn1js/ to interactively view a parsed cert (here is an example of the mustStaple cert on that website).

kevinAlbs · 2020-05-29T00:30:52Z

src/libmongoc/src/mongoc/mongoc-openssl.c

@@ -492,7 +664,7 @@ _contact_ocsp_responder (OCSP_CERTID *id, X509 *peer)
   url_stack = X509_get1_ocsp (peer);
   for (i = 0; i < sk_OPENSSL_STRING_num (url_stack) && !resp; i++) {
      url = sk_OPENSSL_STRING_value (url_stack, i);
-      MONGOC_DEBUG ("Contacting OCSP responder '%s'", url);
+      TRACE ("Contacting OCSP responder '%s'", url);


I noticed that running example-client would print messages to stdout even on success. Debug logs go to stdout by default if no log handler is set. Though that seems like an odd default, I also don't see other cases of debug logs used for successful log messages, just non-critical errors. So I changed these to TRACE logs, which require the driver to be configured with -DENABLE_TRACE.

kevinAlbs · 2020-05-29T00:32:44Z

src/libmongoc/src/mongoc/mongoc-openssl.c

      if (must_staple) {
         MONGOC_ERROR ("Server must contain a stapled response");
         ret = OCSP_CB_REVOKED;
         GOTO (done);
      }

-      if (!(resp = _contact_ocsp_responder (id, peer))) {
+      if (opts->disable_endpoint_check ||


Reach out to a responder unless the user set tlsDisableOCSPEndpointCheck in the URI.

kevinAlbs · 2020-05-29T00:36:33Z

src/libmongoc/src/mongoc/mongoc-stream-tls-openssl.c

+#ifdef MONGOC_ENABLE_OCSP_OPENSSL
+      /* Validate OCSP */
+      if (openssl->ocsp_opts &&
+          1 != _mongoc_ocsp_tlsext_status (ssl, openssl->ocsp_opts)) {


This is no longer done in the callback. I observed on OpenSSL 1.0.2/1.0.1 that against some servers, the callback would be unable to obtain the peer cert (i.e. SSL_get_peer_certificate returned NULL).

So instead, like the hostname check, this now does the OCSP check after the handshake.

Aside: One advantage of the callback is that when the callback returns a failure, that alerts the server the reason for the failure. Doing the OCSP check outside of the handshake means a failure would just close the connection client side. That does not seem like a big loss.

kevinAlbs · 2020-05-29T00:38:14Z

src/libmongoc/tests/test-libmongoc.c

@@ -244,7 +248,7 @@ extern void
 test_aws_install (TestSuite *suite);
 extern void
 test_streamable_ismaster_install (TestSuite *suite);
-#ifdef MONGOC_ENABLE_OCSP_OPENSSL
+#if defined(MONGOC_ENABLE_OCSP_OPENSSL) && OPENSSL_VERSION_NUMBER >= 0x10101000L
 extern void
 test_ocsp_cache_install (TestSuite *suite);


Because of the behavior change with the time, I left the cache tests limited to 1.1.1. I'd like to add these tests back as future work (along with the time polyfill).

kevinAlbs · 2020-05-29T00:49:05Z

src/libmongoc/src/mongoc/mongoc-openssl.c

         MONGOC_ERROR ("Failed to parse OCSP response");
         ret = OCSP_CB_ERROR;
         GOTO (done);
      }
   } else {
-      MONGOC_DEBUG ("Server does not contain a stapled response");
-      must_staple = X509_get_ext_d2i (peer, NID_tlsfeature, 0, 0) != NULL;


This previously checked for the existence of the tlsfeature extension. But after looking at the server code I realized this should be looking for the status_request extension within the tlsfeature extension:
https://github.com/mongodb/mongo/blob/4dcdc501520558d602897aed9aa61781265328d7/src/mongo/util/net/ssl_manager_openssl.cpp#L1605-L1616

pymongo does the same:
https://github.com/mongodb/mongo-python-driver/blob/29960237dcdd9d90402d6c76c4296aec3d01d3fc/pymongo/ocsp_support.py#L292-L298

markbenvenuto · 2020-05-29T21:25:27Z

src/libmongoc/src/mongoc/mongoc-openssl.c

+
+   for (i = 2; i < length; i += 3) {
+      /* Expect an integer, representable in one byte. */
+      if (length < i + 3 || data[i] != 0x02 || data[i + 1] >= 127) {


You need to validate the length of the INTEGER is 1 byte otherwise your i+3 logic will start reading the wrong data.

Imagine if the first INTEGER was 2 bytes instead of 1. Your iterator logic would be completely broken.

The JS viewer is pretty cool. Here is my ASN.1 reference - https://www.itu.int/rec/T-REC-X.690-201508-I

Ah, good catch! I was checking for a multi-byte length, but not a multi-byte integer. And thanks for the reference.

This was also ripe for unit testing, so I added tests.

src/libmongoc/src/mongoc/mongoc-openssl.c

- change SSL_CTX_set_tlsext_status_type to SSL_set_tlsext_status_type. - polyfill SSL_get0_verified_chain, NID_tlsfeature, and hostname check. - check for status_request from the tlsfeature extension when inspecting peer certificate. - skip time check for older OpenSSL when updating cache entries. - perform the OCSP check after the handshake, since sometimes the peer certificate is not available in the callback in OpenSSL <= 1.0.2. - check tlsDisableOCSPEndpointCheck before reaching out to a responder. - make tlsDisableOCSPEndpointCheck and tlsDisableCertificateRevocationCheck URI options implicitly enable TLS. - enable OCSP tests on OpenSSL and macOS that were skipped. - add OCSP tests for OpenSSL 1.0.1. - update OCSP OpenSSL documentation. - change OCSP verification logs from MONGOC_DEBUG to TRACE in successful cases.

kevinAlbs added 2 commits May 28, 2020 20:10

regenerate tests

ed27348

kevinAlbs commented May 29, 2020

View reviewed changes

kevinAlbs requested a review from bazile-clyde May 29, 2020 00:38

kevinAlbs commented May 29, 2020

View reviewed changes

markbenvenuto reviewed May 29, 2020

View reviewed changes

fix and test DER parsing

54df966

markbenvenuto approved these changes Jun 2, 2020

View reviewed changes

bazile-clyde reviewed Jun 5, 2020

View reviewed changes

src/libmongoc/src/mongoc/mongoc-openssl.c Show resolved Hide resolved

bazile-clyde reviewed Jun 5, 2020

View reviewed changes

src/libmongoc/src/mongoc/mongoc-openssl.c Show resolved Hide resolved

bazile-clyde reviewed Jun 5, 2020

View reviewed changes

src/libmongoc/src/mongoc/mongoc-openssl.c Show resolved Hide resolved

bazile-clyde approved these changes Jun 5, 2020

View reviewed changes

kevinAlbs merged commit 733322e into mongodb:master Jun 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CDRIVER-3668 support OCSP back to OpenSSL 1.0.1 #623

CDRIVER-3668 support OCSP back to OpenSSL 1.0.1 #623

kevinAlbs commented May 29, 2020 •

edited

kevinAlbs May 29, 2020

kevinAlbs May 29, 2020

kevinAlbs Jun 5, 2020

kevinAlbs May 29, 2020

kevinAlbs May 29, 2020 •

edited

kevinAlbs May 29, 2020 •

edited

kevinAlbs May 29, 2020 •

edited

kevinAlbs May 29, 2020

kevinAlbs May 29, 2020 •

edited

kevinAlbs May 29, 2020

kevinAlbs May 29, 2020

markbenvenuto May 29, 2020

kevinAlbs May 29, 2020


		#define TLSFEATURE_STATUS_REQUEST 5

		/* Parse just enough of a DER encoded data to check if a SEQUENCE of INTEGER

CDRIVER-3668 support OCSP back to OpenSSL 1.0.1 #623

CDRIVER-3668 support OCSP back to OpenSSL 1.0.1 #623

Conversation

kevinAlbs commented May 29, 2020 • edited

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

kevinAlbs May 29, 2020 • edited

Choose a reason for hiding this comment

kevinAlbs May 29, 2020 • edited

Choose a reason for hiding this comment

kevinAlbs May 29, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinAlbs May 29, 2020 • edited

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

kevinAlbs commented May 29, 2020 •

edited

kevinAlbs May 29, 2020 •

edited

kevinAlbs May 29, 2020 •

edited

kevinAlbs May 29, 2020 •

edited

kevinAlbs May 29, 2020 •

edited