| b.liu | e958203 | 2025-04-17 19:18:16 +0800 | [diff] [blame] | 1 | From 68c69f516f95df1faa42e5647e9ce7cfdc41ac38 Mon Sep 17 00:00:00 2001 |
| 2 | From: Nanang Izzuddin <nanang@teluu.com> |
| 3 | Date: Wed, 16 Jun 2021 12:15:29 +0700 |
| 4 | Subject: [PATCH 2/2] - Fix silly mistake: accepted active socket created |
| 5 | without group lock in SSL socket. - Replace assertion with normal validation |
| 6 | check of SSL socket instance in OpenSSL verification callback (verify_cb()) |
| 7 | to avoid crash, e.g: if somehow race condition with SSL socket destroy |
| 8 | happens or OpenSSL application data index somehow gets corrupted. |
| 9 | |
| 10 | --- |
| 11 | pjlib/src/pj/ssl_sock_imp_common.c | 3 +- |
| 12 | pjlib/src/pj/ssl_sock_ossl.c | 45 +++++++++++++++++++++++++----- |
| 13 | 2 files changed, 40 insertions(+), 8 deletions(-) |
| 14 | |
| 15 | --- a/pjlib/src/pj/ssl_sock_imp_common.c |
| 16 | +++ b/pjlib/src/pj/ssl_sock_imp_common.c |
| 17 | @@ -949,6 +949,7 @@ static pj_bool_t asock_on_accept_complet |
| 18 | |
| 19 | /* Create active socket */ |
| 20 | pj_activesock_cfg_default(&asock_cfg); |
| 21 | + asock_cfg.grp_lock = ssock->param.grp_lock; |
| 22 | asock_cfg.async_cnt = ssock->param.async_cnt; |
| 23 | asock_cfg.concurrency = ssock->param.concurrency; |
| 24 | asock_cfg.whole_data = PJ_TRUE; |
| 25 | @@ -964,7 +965,7 @@ static pj_bool_t asock_on_accept_complet |
| 26 | goto on_return; |
| 27 | |
| 28 | pj_grp_lock_add_ref(glock); |
| 29 | - asock_cfg.grp_lock = ssock->param.grp_lock = glock; |
| 30 | + ssock->param.grp_lock = glock; |
| 31 | pj_grp_lock_add_handler(ssock->param.grp_lock, ssock->pool, ssock, |
| 32 | ssl_on_destroy); |
| 33 | } |
| 34 | --- a/pjlib/src/pj/ssl_sock_ossl.c |
| 35 | +++ b/pjlib/src/pj/ssl_sock_ossl.c |
| 36 | @@ -327,7 +327,8 @@ static pj_status_t STATUS_FROM_SSL_ERR(c |
| 37 | ERROR_LOG("STATUS_FROM_SSL_ERR", err, ssock); |
| 38 | } |
| 39 | |
| 40 | - ssock->last_err = err; |
| 41 | + if (ssock) |
| 42 | + ssock->last_err = err; |
| 43 | return GET_STATUS_FROM_SSL_ERR(err); |
| 44 | } |
| 45 | |
| 46 | @@ -344,7 +345,8 @@ static pj_status_t STATUS_FROM_SSL_ERR2( |
| 47 | /* Dig for more from OpenSSL error queue */ |
| 48 | SSLLogErrors(action, ret, err, len, ssock); |
| 49 | |
| 50 | - ssock->last_err = ssl_err; |
| 51 | + if (ssock) |
| 52 | + ssock->last_err = ssl_err; |
| 53 | return GET_STATUS_FROM_SSL_ERR(ssl_err); |
| 54 | } |
| 55 | |
| 56 | @@ -587,6 +589,13 @@ static pj_status_t init_openssl(void) |
| 57 | |
| 58 | /* Create OpenSSL application data index for SSL socket */ |
| 59 | sslsock_idx = SSL_get_ex_new_index(0, "SSL socket", NULL, NULL, NULL); |
| 60 | + if (sslsock_idx == -1) { |
| 61 | + status = STATUS_FROM_SSL_ERR2("Init", NULL, -1, ERR_get_error(), 0); |
| 62 | + PJ_LOG(1,(THIS_FILE, |
| 63 | + "Fatal error: failed to get application data index for " |
| 64 | + "SSL socket")); |
| 65 | + return status; |
| 66 | + } |
| 67 | |
| 68 | return status; |
| 69 | } |
| 70 | @@ -614,21 +623,36 @@ static int password_cb(char *buf, int nu |
| 71 | } |
| 72 | |
| 73 | |
| 74 | -/* SSL password callback. */ |
| 75 | +/* SSL certificate verification result callback. |
| 76 | + * Note that this callback seems to be always called from library worker |
| 77 | + * thread, e.g: active socket on_read_complete callback, which should have |
| 78 | + * already been equipped with race condition avoidance mechanism (should not |
| 79 | + * be destroyed while callback is being invoked). |
| 80 | + */ |
| 81 | static int verify_cb(int preverify_ok, X509_STORE_CTX *x509_ctx) |
| 82 | { |
| 83 | - pj_ssl_sock_t *ssock; |
| 84 | - SSL *ossl_ssl; |
| 85 | + pj_ssl_sock_t *ssock = NULL; |
| 86 | + SSL *ossl_ssl = NULL; |
| 87 | int err; |
| 88 | |
| 89 | /* Get SSL instance */ |
| 90 | ossl_ssl = X509_STORE_CTX_get_ex_data(x509_ctx, |
| 91 | SSL_get_ex_data_X509_STORE_CTX_idx()); |
| 92 | - pj_assert(ossl_ssl); |
| 93 | + if (!ossl_ssl) { |
| 94 | + PJ_LOG(1,(THIS_FILE, |
| 95 | + "SSL verification callback failed to get SSL instance")); |
| 96 | + goto on_return; |
| 97 | + } |
| 98 | |
| 99 | /* Get SSL socket instance */ |
| 100 | ssock = SSL_get_ex_data(ossl_ssl, sslsock_idx); |
| 101 | - pj_assert(ssock); |
| 102 | + if (!ssock) { |
| 103 | + /* SSL socket may have been destroyed */ |
| 104 | + PJ_LOG(1,(THIS_FILE, |
| 105 | + "SSL verification callback failed to get SSL socket " |
| 106 | + "instance (sslsock_idx=%d).", sslsock_idx)); |
| 107 | + goto on_return; |
| 108 | + } |
| 109 | |
| 110 | /* Store verification status */ |
| 111 | err = X509_STORE_CTX_get_error(x509_ctx); |
| 112 | @@ -706,6 +730,7 @@ static int verify_cb(int preverify_ok, X |
| 113 | if (PJ_FALSE == ssock->param.verify_peer) |
| 114 | preverify_ok = 1; |
| 115 | |
| 116 | +on_return: |
| 117 | return preverify_ok; |
| 118 | } |
| 119 | |
| 120 | @@ -1213,6 +1238,12 @@ static void ssl_destroy(pj_ssl_sock_t *s |
| 121 | static void ssl_reset_sock_state(pj_ssl_sock_t *ssock) |
| 122 | { |
| 123 | ossl_sock_t *ossock = (ossl_sock_t *)ssock; |
| 124 | + |
| 125 | + /* Detach from SSL instance */ |
| 126 | + if (ossock->ossl_ssl) { |
| 127 | + SSL_set_ex_data(ossock->ossl_ssl, sslsock_idx, NULL); |
| 128 | + } |
| 129 | + |
| 130 | /** |
| 131 | * Avoid calling SSL_shutdown() if handshake wasn't completed. |
| 132 | * OpenSSL 1.0.2f complains if SSL_shutdown() is called during an |