Fix crash on config reload with BoringSSL

[maskit]

ATS crashed due to double free if it reloads a cert file that contains chained certs.

According to BoringSSL documentation,

SSL_CTX_add_extra_chain_cert calls SSL_CTX_add0_chain_cert.

and

SSL_CTX_add0_chain_cert appends x509 to ctx's certificate chain. On success, it returns one and takes ownership of x509. Otherwise, it returns zero.
OPENSSL_EXPORT int SSL_CTX_add0_chain_cert(SSL_CTX *ctx, X509 *x509);

So we cannot free cert here.

It kinda made sense before #9177 because we used to call SSL_CTX_use_certificate here instead (which is incorrect as the PR explains) and SSL_CTX_use_certificate does not seem like to take ownership according to BoringSSL documentation.

SSL_CTX_use_certificate sets ctx's leaf certificate to x509. It returns one on success and zero on failure.

I checked OpenSSL's implementation and it looks like OpenSSL takes ownership too. The behavior is the same, but BoringSSL aborts during the second free because of internal sanity check. I don't think we need ifdef for BoringSSL.

[jpeach]

If SSL_CTX_add_extra_chain_cert, but a subsequent operation fails, we would free the certificate at the failed label. Wouldn't that also be a double free when the SSL_CTX is freed?

[maskit]

I'm not sure if I understand your comment correctly.

If SSL_CTX_add_extra_chain_cert succeeds, PEM_read_bio_X509 will be called. And if that fails, cert is nullptr and we leave this while loop. Since cert is nullptr, it won't be freed even if we go to fail label later.

If SSL_CTX_add_extra_chain_cert fails, SSL_CTX doesn't have ownership, so the cert has to be freed. We jump to fail label, and the cert will be freed.

[jpeach]

Oh, I missed the extra read at the end of the loop. IMHO, writing it like this would be a lot clearer:

      // Continue to fetch certs to associate intermediate certificates.
      while ((cert = PEM_read_bio_X509(biop, nullptr, nullptr, nullptr)) {
          if (!SSL_CTX_add_extra_chain_cert(client_ctx.get(), cert)) {
            SSLError("failed to attach client chain certificate from %s", client_cert.c_str());
            goto fail;
          }
      }

[jpeach]

While I'm looking at this code, it seems a bit fragile that cleanup_bio is non-static and is the one to set BIO_NOCLOSE, rather than setting it at BIO_new_mem_buf create time (since cleanup_bio should easily be applied to a BIO that does actually own its memory).

[zwoop]

Cherry-picked to v9.2.x