Construct a full chain when parsing the SSL peer.

This includes the peer certificate along with the rest of the chain when we are parsing the SSL object's peer. Note the tricky semantics around ownership -- the peer_certificate method changes the refcount while peer_chain does not. Ouch!
xrootd · Jun 27, 2020 · 21dd14e · 21dd14e
1 parent a925639
commit 21dd14e
Show file tree

Hide file tree

Showing 2 changed files with 35 additions and 14 deletions.
diff --git a/src/XrdCrypto/XrdCryptosslAux.cc b/src/XrdCrypto/XrdCryptosslAux.cc
@@ -382,11 +382,6 @@ int XrdCryptosslX509ParseStack(void* ssl_conn, XrdCryptoX509Chain *chain)
 {
    EPNAME("X509ParseStack");
    SSL* ssl = (SSL*) ssl_conn;
-   STACK_OF(X509) *st_x509 = SSL_get_peer_cert_chain(ssl);
-
-   if (!st_x509) {
-     return 0;
-   }
 
    int nci = 0;
    // Make sure we got a chain where to add the certificates
@@ -395,12 +390,40 @@ int XrdCryptosslX509ParseStack(void* ssl_conn, XrdCryptoX509Chain *chain)
       return nci;
    }
 
+   STACK_OF(X509) *st_x509 = SSL_get_peer_cert_chain(ssl);
+      // NOTE: SSL_get_peer_certificate increments the refcount;
+      // we must free it or pass along ownership.
+   X509 *peer_cert = SSL_get_peer_certificate(ssl);
+
+   if (peer_cert) {
+      XrdCryptoX509 *c = new XrdCryptosslX509(peer_cert);
+      if (c) {
+         chain->PushBack(c);
+         nci ++;
+      } else {
+         X509_free(peer_cert);
+      }
+   }
+   if (!st_x509) {
+     return nci;
+   }
+
    for (int i=0; i < sk_X509_num(st_x509); i++) {
       X509 *cert = sk_X509_value(st_x509, i);
       XrdCryptoX509 *c = new XrdCryptosslX509(cert);
       if (c) {
+         // The SSL_get_peer_chain method does not increment the
+         // refcount; the XrdCryptoX509 object assumes it owns
+         // the X509* but also does not increment the refcount.
+         // Hence, we increment manually.
+#if OPENSSL_VERSION_NUMBER < 0x010100000L
+         CRYPTO_add(&(cert->references), 1, CRYPTO_LOCK_X509);
+#else
+         X509_up_ref(cert);
+#endif
          chain->PushBack(c);
       } else {
+         X509_free(cert);
          DEBUG("could not create certificate: memory exhausted?");
          chain->Reorder();
          return nci;

diff --git a/src/XrdHttp/XrdHttpProtocol.cc b/src/XrdHttp/XrdHttpProtocol.cc
@@ -297,15 +297,8 @@ int XrdHttpProtocol::GetVOMSData(XrdLink *lp) {
   const char * dn = chain.EECname();
 
   if (!dn) {
-    X509* peer_cert = SSL_get_peer_certificate(ssl);
-
-    if (peer_cert) {
-      TRACE(DEBUG, "SSL_get_peer_certificate returned: " << peer_cert);
-      dn = X509_NAME_oneline(X509_get_subject_name(peer_cert), NULL, 0);
-    } else {
-      // There is no certificate info
-      return 0;
-    }
+    chain.Cleanup();
+    return 0;
   }
 
   if (SecEntity.moninfo) {
@@ -315,6 +308,11 @@ int XrdHttpProtocol::GetVOMSData(XrdLink *lp) {
   SecEntity.moninfo = strdup(dn);
   TRACEI(DEBUG, " Subject name is : '" << SecEntity.moninfo << "'");
 
+    // X509Chain doesn't assume it owns the underlying certs unless
+    // you explicitly invoke the Cleanup method; the destructor will
+    // otherwise leak these.
+  chain.Cleanup();
+
   if (servGMap) {
     mape = servGMap->dn2user(SecEntity.moninfo, bufname, sizeof(bufname), 0);
     if ( !mape && SecEntity.moninfo[0] ) {