Skip to content

Commit 436bc41

Browse files
etretr
committed
Refactor client cert methods with RAII and fix security issues
- Add scoped_x509_cert RAII wrapper to eliminate code duplication across 6 certificate extraction methods - Add null check in get_tls_session() to prevent null pointer dereference when MHD_get_connection_info returns nullptr - Fix TOCTOU race condition in SNI credential caching by re-checking cache after acquiring exclusive lock
1 parent 48d03af commit 436bc41

File tree

2 files changed

+88
-101
lines changed

2 files changed

+88
-101
lines changed

‎src/http_request.cpp‎

Lines changed: 79 additions & 100 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,54 @@
3131

3232
#ifdef HAVE_GNUTLS
3333
#include <gnutls/x509.h>
34+
35+
// RAII wrapper for gnutls_x509_crt_t to ensure proper cleanup
36+
class scoped_x509_cert {
37+
public:
38+
scoped_x509_cert() : cert_(nullptr), valid_(false) {}
39+
40+
~scoped_x509_cert() {
41+
if (cert_ != nullptr) {
42+
gnutls_x509_crt_deinit(cert_);
43+
}
44+
}
45+
46+
// Initialize from a TLS session's peer certificate
47+
// Returns true if certificate was successfully loaded
48+
bool init_from_session(gnutls_session_t session) {
49+
unsigned int list_size = 0;
50+
const gnutls_datum_t* cert_list = gnutls_certificate_get_peers(session, &list_size);
51+
52+
if (cert_list == nullptr || list_size == 0) {
53+
return false;
54+
}
55+
56+
if (gnutls_x509_crt_init(&cert_) != GNUTLS_E_SUCCESS) {
57+
cert_ = nullptr;
58+
return false;
59+
}
60+
61+
if (gnutls_x509_crt_import(cert_, &cert_list[0], GNUTLS_X509_FMT_DER) != GNUTLS_E_SUCCESS) {
62+
gnutls_x509_crt_deinit(cert_);
63+
cert_ = nullptr;
64+
return false;
65+
}
66+
67+
valid_ = true;
68+
return true;
69+
}
70+
71+
bool is_valid() const { return valid_; }
72+
gnutls_x509_crt_t get() const { return cert_; }
73+
74+
// Non-copyable
75+
scoped_x509_cert(const scoped_x509_cert&) = delete;
76+
scoped_x509_cert& operator=(const scoped_x509_cert&) = delete;
77+
78+
private:
79+
gnutls_x509_crt_t cert_;
80+
bool valid_;
81+
};
3482
#endif // HAVE_GNUTLS
3583

3684
namespace httpserver {
@@ -319,6 +367,10 @@ bool http_request::has_tls_session() const {
319367
gnutls_session_t http_request::get_tls_session() const {
320368
const MHD_ConnectionInfo * conninfo = MHD_get_connection_info(underlying_connection, MHD_CONNECTION_INFO_GNUTLS_SESSION);
321369

370+
if (conninfo == nullptr) {
371+
return nullptr;
372+
}
373+
322374
return static_cast<gnutls_session_t>(conninfo->tls_session);
323375
}
324376

@@ -335,35 +387,23 @@ bool http_request::has_client_certificate() const {
335387
}
336388

337389
std::string http_request::get_client_cert_dn() const {
338-
if (!has_client_certificate()) {
339-
return "";
340-
}
341-
342-
gnutls_session_t session = get_tls_session();
343-
unsigned int list_size = 0;
344-
const gnutls_datum_t* cert_list = gnutls_certificate_get_peers(session, &list_size);
345-
346-
gnutls_x509_crt_t cert;
347-
if (gnutls_x509_crt_init(&cert) != GNUTLS_E_SUCCESS) {
390+
if (!has_tls_session()) {
348391
return "";
349392
}
350393

351-
if (gnutls_x509_crt_import(cert, &cert_list[0], GNUTLS_X509_FMT_DER) != GNUTLS_E_SUCCESS) {
352-
gnutls_x509_crt_deinit(cert);
394+
scoped_x509_cert cert;
395+
if (!cert.init_from_session(get_tls_session())) {
353396
return "";
354397
}
355398

356399
size_t dn_size = 0;
357-
gnutls_x509_crt_get_dn(cert, nullptr, &dn_size);
400+
gnutls_x509_crt_get_dn(cert.get(), nullptr, &dn_size);
358401

359402
std::string dn(dn_size, '\0');
360-
if (gnutls_x509_crt_get_dn(cert, &dn[0], &dn_size) != GNUTLS_E_SUCCESS) {
361-
gnutls_x509_crt_deinit(cert);
403+
if (gnutls_x509_crt_get_dn(cert.get(), &dn[0], &dn_size) != GNUTLS_E_SUCCESS) {
362404
return "";
363405
}
364406

365-
gnutls_x509_crt_deinit(cert);
366-
367407
// Remove trailing null if present
368408
if (!dn.empty() && dn.back() == '\0') {
369409
dn.pop_back();
@@ -373,35 +413,23 @@ std::string http_request::get_client_cert_dn() const {
373413
}
374414

375415
std::string http_request::get_client_cert_issuer_dn() const {
376-
if (!has_client_certificate()) {
377-
return "";
378-
}
379-
380-
gnutls_session_t session = get_tls_session();
381-
unsigned int list_size = 0;
382-
const gnutls_datum_t* cert_list = gnutls_certificate_get_peers(session, &list_size);
383-
384-
gnutls_x509_crt_t cert;
385-
if (gnutls_x509_crt_init(&cert) != GNUTLS_E_SUCCESS) {
416+
if (!has_tls_session()) {
386417
return "";
387418
}
388419

389-
if (gnutls_x509_crt_import(cert, &cert_list[0], GNUTLS_X509_FMT_DER) != GNUTLS_E_SUCCESS) {
390-
gnutls_x509_crt_deinit(cert);
420+
scoped_x509_cert cert;
421+
if (!cert.init_from_session(get_tls_session())) {
391422
return "";
392423
}
393424

394425
size_t dn_size = 0;
395-
gnutls_x509_crt_get_issuer_dn(cert, nullptr, &dn_size);
426+
gnutls_x509_crt_get_issuer_dn(cert.get(), nullptr, &dn_size);
396427

397428
std::string dn(dn_size, '\0');
398-
if (gnutls_x509_crt_get_issuer_dn(cert, &dn[0], &dn_size) != GNUTLS_E_SUCCESS) {
399-
gnutls_x509_crt_deinit(cert);
429+
if (gnutls_x509_crt_get_issuer_dn(cert.get(), &dn[0], &dn_size) != GNUTLS_E_SUCCESS) {
400430
return "";
401431
}
402432

403-
gnutls_x509_crt_deinit(cert);
404-
405433
// Remove trailing null if present
406434
if (!dn.empty() && dn.back() == '\0') {
407435
dn.pop_back();
@@ -411,40 +439,27 @@ std::string http_request::get_client_cert_issuer_dn() const {
411439
}
412440

413441
std::string http_request::get_client_cert_cn() const {
414-
if (!has_client_certificate()) {
415-
return "";
416-
}
417-
418-
gnutls_session_t session = get_tls_session();
419-
unsigned int list_size = 0;
420-
const gnutls_datum_t* cert_list = gnutls_certificate_get_peers(session, &list_size);
421-
422-
gnutls_x509_crt_t cert;
423-
if (gnutls_x509_crt_init(&cert) != GNUTLS_E_SUCCESS) {
442+
if (!has_tls_session()) {
424443
return "";
425444
}
426445

427-
if (gnutls_x509_crt_import(cert, &cert_list[0], GNUTLS_X509_FMT_DER) != GNUTLS_E_SUCCESS) {
428-
gnutls_x509_crt_deinit(cert);
446+
scoped_x509_cert cert;
447+
if (!cert.init_from_session(get_tls_session())) {
429448
return "";
430449
}
431450

432451
size_t cn_size = 0;
433-
gnutls_x509_crt_get_dn_by_oid(cert, GNUTLS_OID_X520_COMMON_NAME, 0, 0, nullptr, &cn_size);
452+
gnutls_x509_crt_get_dn_by_oid(cert.get(), GNUTLS_OID_X520_COMMON_NAME, 0, 0, nullptr, &cn_size);
434453

435454
if (cn_size == 0) {
436-
gnutls_x509_crt_deinit(cert);
437455
return "";
438456
}
439457

440458
std::string cn(cn_size, '\0');
441-
if (gnutls_x509_crt_get_dn_by_oid(cert, GNUTLS_OID_X520_COMMON_NAME, 0, 0, &cn[0], &cn_size) != GNUTLS_E_SUCCESS) {
442-
gnutls_x509_crt_deinit(cert);
459+
if (gnutls_x509_crt_get_dn_by_oid(cert.get(), GNUTLS_OID_X520_COMMON_NAME, 0, 0, &cn[0], &cn_size) != GNUTLS_E_SUCCESS) {
443460
return "";
444461
}
445462

446-
gnutls_x509_crt_deinit(cert);
447-
448463
// Remove trailing null if present
449464
if (!cn.empty() && cn.back() == '\0') {
450465
cn.pop_back();
@@ -469,34 +484,22 @@ bool http_request::is_client_cert_verified() const {
469484
}
470485

471486
std::string http_request::get_client_cert_fingerprint_sha256() const {
472-
if (!has_client_certificate()) {
473-
return "";
474-
}
475-
476-
gnutls_session_t session = get_tls_session();
477-
unsigned int list_size = 0;
478-
const gnutls_datum_t* cert_list = gnutls_certificate_get_peers(session, &list_size);
479-
480-
gnutls_x509_crt_t cert;
481-
if (gnutls_x509_crt_init(&cert) != GNUTLS_E_SUCCESS) {
487+
if (!has_tls_session()) {
482488
return "";
483489
}
484490

485-
if (gnutls_x509_crt_import(cert, &cert_list[0], GNUTLS_X509_FMT_DER) != GNUTLS_E_SUCCESS) {
486-
gnutls_x509_crt_deinit(cert);
491+
scoped_x509_cert cert;
492+
if (!cert.init_from_session(get_tls_session())) {
487493
return "";
488494
}
489495

490496
unsigned char fingerprint[32]; // SHA-256 is 32 bytes
491497
size_t fingerprint_size = sizeof(fingerprint);
492498

493-
if (gnutls_x509_crt_get_fingerprint(cert, GNUTLS_DIG_SHA256, fingerprint, &fingerprint_size) != GNUTLS_E_SUCCESS) {
494-
gnutls_x509_crt_deinit(cert);
499+
if (gnutls_x509_crt_get_fingerprint(cert.get(), GNUTLS_DIG_SHA256, fingerprint, &fingerprint_size) != GNUTLS_E_SUCCESS) {
495500
return "";
496501
}
497502

498-
gnutls_x509_crt_deinit(cert);
499-
500503
// Convert to hex string
501504
std::string hex_fingerprint;
502505
hex_fingerprint.reserve(fingerprint_size * 2);
@@ -510,53 +513,29 @@ std::string http_request::get_client_cert_fingerprint_sha256() const {
510513
}
511514

512515
time_t http_request::get_client_cert_not_before() const {
513-
if (!has_client_certificate()) {
514-
return -1;
515-
}
516-
517-
gnutls_session_t session = get_tls_session();
518-
unsigned int list_size = 0;
519-
const gnutls_datum_t* cert_list = gnutls_certificate_get_peers(session, &list_size);
520-
521-
gnutls_x509_crt_t cert;
522-
if (gnutls_x509_crt_init(&cert) != GNUTLS_E_SUCCESS) {
516+
if (!has_tls_session()) {
523517
return -1;
524518
}
525519

526-
if (gnutls_x509_crt_import(cert, &cert_list[0], GNUTLS_X509_FMT_DER) != GNUTLS_E_SUCCESS) {
527-
gnutls_x509_crt_deinit(cert);
520+
scoped_x509_cert cert;
521+
if (!cert.init_from_session(get_tls_session())) {
528522
return -1;
529523
}
530524

531-
time_t not_before = gnutls_x509_crt_get_activation_time(cert);
532-
gnutls_x509_crt_deinit(cert);
533-
534-
return not_before;
525+
return gnutls_x509_crt_get_activation_time(cert.get());
535526
}
536527

537528
time_t http_request::get_client_cert_not_after() const {
538-
if (!has_client_certificate()) {
539-
return -1;
540-
}
541-
542-
gnutls_session_t session = get_tls_session();
543-
unsigned int list_size = 0;
544-
const gnutls_datum_t* cert_list = gnutls_certificate_get_peers(session, &list_size);
545-
546-
gnutls_x509_crt_t cert;
547-
if (gnutls_x509_crt_init(&cert) != GNUTLS_E_SUCCESS) {
529+
if (!has_tls_session()) {
548530
return -1;
549531
}
550532

551-
if (gnutls_x509_crt_import(cert, &cert_list[0], GNUTLS_X509_FMT_DER) != GNUTLS_E_SUCCESS) {
552-
gnutls_x509_crt_deinit(cert);
533+
scoped_x509_cert cert;
534+
if (!cert.init_from_session(get_tls_session())) {
553535
return -1;
554536
}
555537

556-
time_t not_after = gnutls_x509_crt_get_expiration_time(cert);
557-
gnutls_x509_crt_deinit(cert);
558-
559-
return not_after;
538+
return gnutls_x509_crt_get_expiration_time(cert.get());
560539
}
561540
#endif // HAVE_GNUTLS
562541

‎src/webserver.cpp‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -551,9 +551,17 @@ int webserver::sni_cert_callback_func(void* cls,
551551
return -1;
552552
}
553553

554-
// Cache the credentials
554+
// Cache the credentials with double-check to avoid race condition
555555
{
556556
std::unique_lock lock(ws->sni_credentials_mutex);
557+
// Re-check after acquiring exclusive lock - another thread may have inserted
558+
auto it = ws->sni_credentials_cache.find(name);
559+
if (it != ws->sni_credentials_cache.end()) {
560+
// Another thread already cached credentials, use theirs and free ours
561+
gnutls_certificate_free_credentials(new_creds);
562+
*creds = it->second;
563+
return 0;
564+
}
557565
ws->sni_credentials_cache[name] = new_creds;
558566
}
559567

0 commit comments

Comments
 (0)