From db751ba82f8980bc3095fc01cbd4458480b804fc Mon Sep 17 00:00:00 2001
From: Stan Hu <stanhu@gmail.com>
Date: Sat, 2 Apr 2022 14:24:36 -0700
Subject: [PATCH] Fail hard if SSL certs or keys are invalid (#2848)

* Fail hard if OpenSSL cannot load cert or key

Previously if an invalid SSL cert or key (e.g. blank file, mismatching
private and public key, etc.) were specified, Puma would quietly
accept them, bind to the port, and mysteriously fail with an error
only after a client initiates a connection:

```
SSL error, peer: ::1, peer cert: : #<Puma::MiniSSL::SSLError: OpenSSL error: error:1417A0C1:SSL routines:tls_post_process_client_hello:no shared cipher - 193>
```

We now check the OpenSSL return values and raise an exception if the
cert or key cannot be loaded.

* Add SSL invalid tests

Co-authored-by: MSP-Greg <Greg.mpls@gmail.com>
---
 ext/puma_http11/mini_ssl.c   | 28 ++++++++++++++++++++-----
 test/test_puma_server_ssl.rb | 40 ++++++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+), 5 deletions(-)

diff --git a/ext/puma_http11/mini_ssl.c b/ext/puma_http11/mini_ssl.c
index 9615f24a..5ff219db 100644
--- a/ext/puma_http11/mini_ssl.c
+++ b/ext/puma_http11/mini_ssl.c
@@ -30,6 +30,12 @@ typedef struct {
 
 VALUE eError;
 
+NORETURN(void raise_file_error(const char* caller, const char *filename));
+
+void raise_file_error(const char* caller, const char *filename) {
+  rb_raise(eError, "%s: error in file '%s': %s", caller, filename, ERR_error_string(ERR_get_error(), NULL));
+}
+
 void engine_free(void *ptr) {
   ms_conn *conn = ptr;
   ms_cert_buf* cert_buf = (ms_cert_buf*)SSL_get_app_data(conn->ssl);
@@ -244,12 +250,18 @@ sslctx_initialize(VALUE self, VALUE mini_ssl_ctx) {
 
   if (!NIL_P(cert)) {
     StringValue(cert);
-    SSL_CTX_use_certificate_chain_file(ctx, RSTRING_PTR(cert));
+
+    if (SSL_CTX_use_certificate_chain_file(ctx, RSTRING_PTR(cert)) != 1) {
+      raise_file_error("SSL_CTX_use_certificate_chain_file", RSTRING_PTR(cert));
+    }
   }
 
   if (!NIL_P(key)) {
     StringValue(key);
-    SSL_CTX_use_PrivateKey_file(ctx, RSTRING_PTR(key), SSL_FILETYPE_PEM);
+
+    if (SSL_CTX_use_PrivateKey_file(ctx, RSTRING_PTR(key), SSL_FILETYPE_PEM) != 1) {
+      raise_file_error("SSL_CTX_use_PrivateKey_file", RSTRING_PTR(key));
+    }
   }
 
   if (!NIL_P(cert_pem)) {
@@ -257,7 +269,9 @@ sslctx_initialize(VALUE self, VALUE mini_ssl_ctx) {
     BIO_puts(bio, RSTRING_PTR(cert_pem));
     x509 = PEM_read_bio_X509(bio, NULL, NULL, NULL);
 
-    SSL_CTX_use_certificate(ctx, x509);
+    if (SSL_CTX_use_certificate(ctx, x509) != 1) {
+      raise_file_error("SSL_CTX_use_certificate", RSTRING_PTR(cert_pem));
+    }
   }
 
   if (!NIL_P(key_pem)) {
@@ -265,7 +279,9 @@ sslctx_initialize(VALUE self, VALUE mini_ssl_ctx) {
     BIO_puts(bio, RSTRING_PTR(key_pem));
     pkey = PEM_read_bio_PrivateKey(bio, NULL, NULL, NULL);
 
-    SSL_CTX_use_PrivateKey(ctx, pkey);
+    if (SSL_CTX_use_PrivateKey(ctx, pkey) != 1) {
+      raise_file_error("SSL_CTX_use_PrivateKey", RSTRING_PTR(key_pem));
+    }
   }
 
   verification_flags = rb_funcall(mini_ssl_ctx, rb_intern_const("verification_flags"), 0);
@@ -278,7 +294,9 @@ sslctx_initialize(VALUE self, VALUE mini_ssl_ctx) {
 
   if (!NIL_P(ca)) {
     StringValue(ca);
-    SSL_CTX_load_verify_locations(ctx, RSTRING_PTR(ca), NULL);
+    if (SSL_CTX_load_verify_locations(ctx, RSTRING_PTR(ca), NULL) != 1) {
+      raise_file_error("SSL_CTX_load_verify_locations", RSTRING_PTR(ca));
+    }
   }
 
   ssl_options = SSL_OP_CIPHER_SERVER_PREFERENCE | SSL_OP_SINGLE_ECDH_USE | SSL_OP_NO_COMPRESSION;
diff --git a/test/test_puma_server_ssl.rb b/test/test_puma_server_ssl.rb
index 4d08d0ec..9ec72191 100644
--- a/test/test_puma_server_ssl.rb
+++ b/test/test_puma_server_ssl.rb
@@ -231,6 +231,46 @@ class TestPumaServerSSL < Minitest::Test
 
     assert busy_threads.zero?, "Our connection is wasn't dropped"
   end
+
+  unless Puma.jruby?
+    def test_invalid_cert
+      assert_raises(Puma::MiniSSL::SSLError) do
+        start_server { |ctx| ctx.cert = __FILE__ }
+      end
+    end
+
+    def test_invalid_key
+      assert_raises(Puma::MiniSSL::SSLError) do
+        start_server { |ctx| ctx.key = __FILE__ }
+      end
+    end
+
+    def test_invalid_cert_pem
+      assert_raises(Puma::MiniSSL::SSLError) do
+        start_server { |ctx|
+          ctx.instance_variable_set(:@cert, nil)
+          ctx.cert_pem = 'Not a valid pem'
+        }
+      end
+    end
+
+    def test_invalid_key_pem
+      assert_raises(Puma::MiniSSL::SSLError) do
+        start_server { |ctx|
+          ctx.instance_variable_set(:@key, nil)
+          ctx.key_pem = 'Not a valid pem'
+        }
+      end
+    end
+
+    def test_invalid_ca
+      assert_raises(Puma::MiniSSL::SSLError) do
+        start_server { |ctx|
+          ctx.ca = __FILE__
+        }
+      end
+    end
+  end
 end if ::Puma::HAS_SSL
 
 # client-side TLS authentication tests