Skip to content

Commit 6de6ebb

Browse files
bneradtcmcfarlen
authored andcommitted
Call SSL_get_negotiated_group not during read early data (#12224)
Recent versions of OpenSSL define SSL_CB_HANDSHAKE_DONE as follows: > Callback has been called because a handshake is finished. It also > occurs if the handshake is paused to allow the exchange of early data. Calling SSL_get_negotiated_group duing early data situations was causing a crash. This moves the calling of SSL_get_negotiated_group next to _record_tls_handshake_end_time where it will reliably not be called in the context of processing early data. Fixes: #12140 (cherry picked from commit afbb1f5)
1 parent ce27946 commit 6de6ebb

File tree

5 files changed

+36
-23
lines changed

5 files changed

+36
-23
lines changed

include/iocore/net/TLSBasicSupport.h

+1
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,7 @@ class TLSBasicSupport
7979

8080
void _record_tls_handshake_begin_time();
8181
void _record_tls_handshake_end_time();
82+
void _update_end_of_handshake_stats();
8283

8384
/**
8485
* Implementation should schedule either TS_EVENT_SSL_VERIFY_SERVER or TS_EVENT_SSL_VERIFY_CLIENT accordingly.

src/iocore/net/QUICNetVConnection.cc

+1
Original file line numberDiff line numberDiff line change
@@ -250,6 +250,7 @@ QUICNetVConnection::_switch_to_established_state()
250250
{
251251
QUICConDebug("Enter state_connection_established");
252252
this->_record_tls_handshake_end_time();
253+
this->_update_end_of_handshake_stats();
253254
SET_HANDLER((NetVConnHandler)&QUICNetVConnection::state_established);
254255
this->_start_application();
255256
this->_handshake_completed = true;

src/iocore/net/SSLNetVConnection.cc

+1-1
Original file line numberDiff line numberDiff line change
@@ -1321,7 +1321,7 @@ SSLNetVConnection::sslServerHandShakeEvent(int &err)
13211321

13221322
if (this->get_tls_handshake_begin_time()) {
13231323
this->_record_tls_handshake_end_time();
1324-
Metrics::Counter::increment(ssl_rsb.total_success_handshake_count_in);
1324+
this->_update_end_of_handshake_stats();
13251325
}
13261326

13271327
if (this->get_tunnel_type() != SNIRoutingType::NONE) {

src/iocore/net/SSLUtils.cc

-22
Original file line numberDiff line numberDiff line change
@@ -1077,28 +1077,6 @@ ssl_callback_info(const SSL *ssl, int where, int ret)
10771077
}
10781078
Metrics::Counter::increment(it->second);
10791079
}
1080-
1081-
#if defined(OPENSSL_IS_BORINGSSL)
1082-
uint16_t group_id = SSL_get_group_id(ssl);
1083-
if (group_id != 0) {
1084-
const char *group_name = SSL_get_group_name(group_id);
1085-
if (auto it = tls_group_map.find(group_name); it != tls_group_map.end()) {
1086-
Metrics::Counter::increment(it->second);
1087-
} else {
1088-
Warning("Unknown TLS Group");
1089-
}
1090-
}
1091-
#elif defined(SSL_get_negotiated_group)
1092-
int nid = SSL_get_negotiated_group(const_cast<SSL *>(ssl));
1093-
if (nid != NID_undef) {
1094-
if (auto it = tls_group_map.find(nid); it != tls_group_map.end()) {
1095-
Metrics::Counter::increment(it->second);
1096-
} else {
1097-
auto other = tls_group_map.find(SSL_GROUP_STAT_OTHER_KEY);
1098-
Metrics::Counter::increment(other->second);
1099-
}
1100-
}
1101-
#endif // OPENSSL_IS_BORINGSSL or SSL_get_negotiated_group
11021080
}
11031081
}
11041082

src/iocore/net/TLSBasicSupport.cc

+33
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,9 @@
2424

2525
#include "SSLStats.h"
2626
#include "iocore/net/TLSBasicSupport.h"
27+
#if defined(OPENSSL_IS_BORINGSSL)
28+
#include "tscore/Diags.h" // For Warning
29+
#endif // OPENSSL_IS_BORINGSSL
2730
#include "tsutil/DbgCtl.h"
2831

2932
#include <cinttypes>
@@ -201,3 +204,33 @@ TLSBasicSupport::_record_tls_handshake_end_time()
201204
Dbg(dbg_ctl_ssl, "ssl handshake time:%" PRId64, ssl_handshake_time);
202205
Metrics::Counter::increment(ssl_rsb.total_handshake_time, ssl_handshake_time);
203206
}
207+
208+
void
209+
TLSBasicSupport::_update_end_of_handshake_stats()
210+
{
211+
Metrics::Counter::increment(ssl_rsb.total_success_handshake_count_in);
212+
213+
#if defined(OPENSSL_IS_BORINGSSL)
214+
SSL *ssl = this->_get_ssl_object();
215+
uint16_t group_id = SSL_get_group_id(ssl);
216+
if (group_id != 0) {
217+
const char *group_name = SSL_get_group_name(group_id);
218+
if (auto it = tls_group_map.find(group_name); it != tls_group_map.end()) {
219+
Metrics::Counter::increment(it->second);
220+
} else {
221+
Warning("Unknown TLS Group");
222+
}
223+
}
224+
#elif defined(SSL_get_negotiated_group)
225+
SSL *ssl = this->_get_ssl_object();
226+
int nid = SSL_get_negotiated_group(const_cast<SSL *>(ssl));
227+
if (nid != NID_undef) {
228+
if (auto it = tls_group_map.find(nid); it != tls_group_map.end()) {
229+
Metrics::Counter::increment(it->second);
230+
} else {
231+
auto other = tls_group_map.find(SSL_GROUP_STAT_OTHER_KEY);
232+
Metrics::Counter::increment(other->second);
233+
}
234+
}
235+
#endif // OPENSSL_IS_BORINGSSL or SSL_get_negotiated_group
236+
}

0 commit comments

Comments
 (0)