maskit opened a new pull request, #9840:
URL: https://github.com/apache/trafficserver/pull/9840

   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 https://github.com/apache/trafficserver/pull/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.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to