[glib-networking/mcatanzaro/openssl-ocsp: 1/2] openssl: check for Must-Staple extension on server cert

[Michael Catanzaro <mcatanzaro src gnome org>]

commit 9085253ac20a23370453703ea23ee648ce782659
Author: Michael Catanzaro <mcatanzaro redhat com>
Date:   Wed Dec 15 15:47:20 2021 -0600

    openssl: check for Must-Staple extension on server cert
    
    Previously we decided not to do this because it cannot be done properly,
    which would require us to check each certificate in the chain for the
    Must-Staple extension. The problem is we don't know which certificates
    actually get used in the final verification path constructed by OpenSSL,
    and don't want to consider certificates that are not used because that
    leads to mistakes like #179.
    
    But we *can* check Must-Staple on just the server cert. So let's do
    that. Then we can get some actual real security benefit, which is
    otherwise not possible because we have to ignore missing OCSP responses
    if Must-Staple is not used.
    
    Now, this isn't good enough -- we really ought to check for Must-Staple
    on intermediate certificates too -- but it seems to be the best that is
    possible with OpenSSL nowadays.

 tls/openssl/gtlsclientconnection-openssl.c | 21 ++++------
 tls/openssl/gtlsdatabase-openssl.c         | 61 +++++++++++++++++++++++++-----
 2 files changed, 58 insertions(+), 24 deletions(-)
---
diff --git a/tls/openssl/gtlsclientconnection-openssl.c b/tls/openssl/gtlsclientconnection-openssl.c
index 64b17f4d..40f85785 100644
--- a/tls/openssl/gtlsclientconnection-openssl.c
+++ b/tls/openssl/gtlsclientconnection-openssl.c
@@ -222,23 +222,13 @@ verify_ocsp_response (GTlsClientConnectionOpenssl *openssl,
 
   ssl = g_tls_connection_openssl_get_ssl (G_TLS_CONNECTION_OPENSSL (openssl));
   len = SSL_get_tlsext_status_ocsp_resp (ssl, &p);
-  if (!p)
+  if (p)
     {
-      /* OpenSSL doesn't provide an API to determine if the chain requires
-       * an OCSP response (known as MustStaple) using the status_request 
-       * X509v3 extension. We also have no way of correctly knowing the full
-       * chain OpenSSL will internally use to do this ourselves.
-       * So for now we silently continue ignoring the missing response.
-       * This does mean that this does not provide real security as an attacker
-       * can easily bypass this.
-       */
-      return 0;
+      resp = d2i_OCSP_RESPONSE (NULL, (const unsigned char **)&p, len);
+      if (!resp)
+        return G_TLS_CERTIFICATE_GENERIC_ERROR;
     }
 
-  resp = d2i_OCSP_RESPONSE (NULL, (const unsigned char **)&p, len);
-  if (!resp)
-    return G_TLS_CERTIFICATE_GENERIC_ERROR;
-
   database = g_tls_connection_get_database (G_TLS_CONNECTION (openssl));
 
   /* If there's no database, then G_TLS_CERTIFICATE_UNKNOWN_CA must be flagged,
@@ -246,6 +236,9 @@ verify_ocsp_response (GTlsClientConnectionOpenssl *openssl,
    */
   g_assert (database);
 
+  /* Note we have to call this even if resp is NULL, because it will check
+   * whether Must-Staple is set.
+   */
   return g_tls_database_openssl_verify_ocsp_response (G_TLS_DATABASE_OPENSSL (database),
                                                       peer_certificate,
                                                       resp);
diff --git a/tls/openssl/gtlsdatabase-openssl.c b/tls/openssl/gtlsdatabase-openssl.c
index 52a6c335..6ae34958 100644
--- a/tls/openssl/gtlsdatabase-openssl.c
+++ b/tls/openssl/gtlsdatabase-openssl.c
@@ -348,38 +348,79 @@ g_tls_database_openssl_new (GError **error)
   return g_initable_new (G_TYPE_TLS_DATABASE_OPENSSL, NULL, error, NULL);
 }
 
+#if (OPENSSL_VERSION_NUMBER >= 0x0090808fL) && !defined(OPENSSL_NO_TLSEXT) && !defined(OPENSSL_NO_OCSP)
+static gboolean
+check_for_ocsp_must_staple (X509 *cert)
+{
+  int idx = -1; /* We ignore the return of this as we only expect one extension. */
+  STACK_OF(ASN1_INTEGER) *features = X509_get_ext_d2i (cert, NID_tlsfeature, NULL, &idx);
+
+  if (!features)
+    return FALSE;
+
+  for (guint i = 0; i < sk_ASN1_INTEGER_num (features); i++)
+    {
+      const long feature_id = ASN1_INTEGER_get (sk_ASN1_INTEGER_value (features, i));
+      if (feature_id == 5 || feature_id == 17) /* status_request, status_request_v2 */
+        {
+          sk_ASN1_INTEGER_pop_free (features, ASN1_INTEGER_free);
+          return TRUE;
+        }
+    }
+
+  sk_ASN1_INTEGER_pop_free (features, ASN1_INTEGER_free);
+  return FALSE;
+}
+#endif
+
 GTlsCertificateFlags
 g_tls_database_openssl_verify_ocsp_response (GTlsDatabaseOpenssl *self,
                                              GTlsCertificate     *chain,
                                              OCSP_RESPONSE       *resp)
 {
   GTlsCertificateFlags errors = 0;
-#if (OPENSSL_VERSION_NUMBER >= 0x0090808fL) && !defined(OPENSSL_NO_TLSEXT) && \
-  !defined(OPENSSL_NO_OCSP)
+#if (OPENSSL_VERSION_NUMBER >= 0x0090808fL) && !defined(OPENSSL_NO_TLSEXT) && !defined(OPENSSL_NO_OCSP)
   GTlsDatabaseOpensslPrivate *priv;
   STACK_OF(X509) *chain_openssl = NULL;
   OCSP_BASICRESP *basic_resp = NULL;
   int ocsp_status = 0;
   int i;
 
-  ocsp_status = OCSP_response_status (resp);
-  if (ocsp_status != OCSP_RESPONSE_STATUS_SUCCESSFUL)
+  chain_openssl = convert_certificate_chain_to_openssl (G_TLS_CERTIFICATE_OPENSSL (chain));
+  priv = g_tls_database_openssl_get_instance_private (self);
+  if ((chain_openssl == NULL) ||
+      (priv->store == NULL))
     {
       errors = G_TLS_CERTIFICATE_GENERIC_ERROR;
       goto end;
     }
 
-  basic_resp = OCSP_response_get1_basic (resp);
-  if (basic_resp == NULL)
+  /* OpenSSL doesn't provide an API to determine if the chain requires
+   * an OCSP response (known as Must-Staple) using the status_request
+   * X509v3 extension. We also seem to have no way of correctly knowing the
+   * final certificate path that OpenSSL will internally use, so can't do it
+   * ourselves. So for now we will check only the server certificate to see if
+   * it sets Must-Staple. This is inconsistent with GnuTLS's behavior, but it
+   * seems to be the best we can do. Checking *every* certificate for Must-
+   * Staple would be wrong because we don't want to check certificates that
+   * OpenSSL does not actually use as part of its final certification path.
+   */
+  if (resp == NULL)
+    {
+      if (check_for_ocsp_must_staple (sk_X509_value (chain_openssl, 0)))
+        errors = G_TLS_CERTIFICATE_GENERIC_ERROR;
+      goto end;
+    }
+
+  ocsp_status = OCSP_response_status (resp);
+  if (ocsp_status != OCSP_RESPONSE_STATUS_SUCCESSFUL)
     {
       errors = G_TLS_CERTIFICATE_GENERIC_ERROR;
       goto end;
     }
 
-  chain_openssl = convert_certificate_chain_to_openssl (G_TLS_CERTIFICATE_OPENSSL (chain));
-  priv = g_tls_database_openssl_get_instance_private (self);
-  if ((chain_openssl == NULL) ||
-      (priv->store == NULL))
+  basic_resp = OCSP_response_get1_basic (resp);
+  if (basic_resp == NULL)
     {
       errors = G_TLS_CERTIFICATE_GENERIC_ERROR;
       goto end;