add basics for OpenSSL err handling

Add the basic structure required for properly handling the error queue. Otherwise SSL_get_error() can return junk.
dormando · May 22, 2024 · c3a0941 · c3a0941
1 parent 84a2db0
commit c3a0941
Show file tree

Hide file tree

Showing 3 changed files with 60 additions and 9 deletions.
diff --git a/proxy_network.c b/proxy_network.c
@@ -1181,6 +1181,7 @@ static void proxy_beconn_tls_handler(const int fd, const short which, void *arg)
             if (_proxy_beconn_checkconnect(be) == -1) {
                 return;
             }
+            // TODO: check return code.
             mcp_tls_connect(be);
             // fall through to handshake attempt.
         }
@@ -1193,9 +1194,11 @@ static void proxy_beconn_tls_handler(const int fd, const short which, void *arg)
         _set_main_event(be, be->event_thread->base, EV_READ, &tmp_time, proxy_beconn_tls_handler);
         return;
     } else if (ret == 1) {
-        // complete.
-        // TODO: handle errors.
-        mcp_tls_send_validate(be);
+        // handshake complete.
+        if (mcp_tls_send_validate(be) != 1) {
+            _reset_bad_backend(be, P_BE_FAIL_BADVALIDATE);
+            return;
+        }
 
         // switch to another handler for the final stage.
         _set_main_event(be, be->event_thread->base, EV_READ, &tmp_time, proxy_bevalidate_tls_handler);

diff --git a/proxy_tls.c b/proxy_tls.c
@@ -3,6 +3,20 @@
 #include "proxy.h"
 #include "proxy_tls.h"
 #include <openssl/ssl.h>
+#include <openssl/err.h>
+
+/* Notes on ERR_clear_error() and friends:
+ * - Errors from SSL calls leave errors on a thread-local "error stack"
+ * - If an error is received from an SSL call, the stack needs to be inspected
+ *   and cleared.
+ * - The error stack _must_ be clear before any SSL_get_error() calls, as it
+ *   may return garbage.
+ * - ERR_clear_error() is not "free", so we would prefer to avoid calling it
+ *   before hotpath calls. Thus, we should ensure it's called _after_ any
+ *   hotpath call that receives any kind of error.
+ * - We should also call it _before_ any non-hotpath SSL calls (such as
+ *   SSL_connect()) for defense against bugs in our code or OpenSSL.
+ */
 
 int mcp_tls_init(proxy_ctx_t *ctx) {
     if (ctx->tls_ctx) {
@@ -47,8 +61,23 @@ int mcp_tls_connect(struct mcp_backendconn_s *be) {
     // TODO: check return code. can fail if BIO fails to alloc.
     SSL_set_fd(be->ssl, mcmc_fd(be->client));
 
-    // TODO: care about the error code.
-    int ret = SSL_connect(be->ssl);
+    ERR_clear_error();
+    int n = SSL_connect(be->ssl);
+    int ret = 1;
+    // TODO: complete error handling.
+    if (n == 1) {
+        // Successfully established and handshake complete.
+    } else if (n == 0) {
+        // Not successsful, but shut down normally.
+        ERR_clear_error();
+        ret = -1;
+    } else if (n < 0) {
+        // Not successful. Check for temporary error.
+
+        // clear all errors in case of other junk.
+        ERR_clear_error();
+        ret = -1;
+    }
 
     return ret;
 }
@@ -58,6 +87,8 @@ int mcp_tls_handshake(struct mcp_backendconn_s *be) {
         return 1;
     }
 
+    // Non hot path, so clear errors before running.
+    ERR_clear_error();
     int n = SSL_do_handshake(be->ssl);
     if (n == 1) {
         return 1;
@@ -69,20 +100,32 @@ int mcp_tls_handshake(struct mcp_backendconn_s *be) {
     // leaving this note just in case.
     if (err == SSL_ERROR_WANT_READ ||
         err == SSL_ERROR_WANT_WRITE) {
+        // So far as I can tell there would be an error on the queue here.
+        ERR_clear_error();
         return 0;
     } else {
         // TODO: can get the full error message and give to the caller to log
         // to proxyevents?
+        ERR_clear_error();
         return -1;
     }
 }
 
-// TODO: error processing.
-void mcp_tls_send_validate(struct mcp_backendconn_s *be) {
+int mcp_tls_send_validate(struct mcp_backendconn_s *be) {
     const char *str = "version\r\n";
     const size_t len = strlen(str);
 
-    SSL_write(be->ssl, str, len);
+    // Non hot path, clear errors.
+    ERR_clear_error();
+    int n = SSL_write(be->ssl, str, len);
+
+    // TODO: more detailed error checking.
+    if (n < 0 || n != len) {
+        ERR_clear_error();
+        return -1;
+    }
+
+    return 1;
 }
 
 int mcp_tls_read(struct mcp_backendconn_s *be) {
@@ -92,8 +135,11 @@ int mcp_tls_read(struct mcp_backendconn_s *be) {
         int err = SSL_get_error(be->ssl, n);
         if (err == SSL_ERROR_WANT_WRITE ||
             err == SSL_ERROR_WANT_READ) {
+            ERR_clear_error();
             return -1;
         } else {
+            // TODO: log detailed error.
+            ERR_clear_error();
             return -2;
         }
     } else {
@@ -142,8 +188,10 @@ int mcp_tls_writev(struct mcp_backendconn_s *be, int iovcnt) {
         int err = SSL_get_error(be->ssl, n);
         if (err == SSL_ERROR_WANT_WRITE ||
             err == SSL_ERROR_WANT_READ) {
+            ERR_clear_error();
             return -1;
         }
+        ERR_clear_error();
         return -2;
     }
 

diff --git a/proxy_tls.h b/proxy_tls.h
@@ -5,7 +5,7 @@ int mcp_tls_init(proxy_ctx_t *ctx);
 int mcp_tls_backend_init(proxy_ctx_t *ctx, struct mcp_backendconn_s *be);
 int mcp_tls_connect(struct mcp_backendconn_s *be);
 int mcp_tls_handshake(struct mcp_backendconn_s *be);
-void mcp_tls_send_validate(struct mcp_backendconn_s *be);
+int mcp_tls_send_validate(struct mcp_backendconn_s *be);
 int mcp_tls_read(struct mcp_backendconn_s *be);
 int mcp_tls_writev(struct mcp_backendconn_s *be, int iovcnt);