From a17755c6760ac6fd2c1a09d7636bf8975dfc7adf Mon Sep 17 00:00:00 2001 From: Guillaume Roguez <guillaume.roguez@savoirfairelinux.com> Date: Fri, 17 Mar 2017 16:23:57 -0400 Subject: [PATCH] Fix DhParams copy-assignment and copy-constructor DhParams copy-assignement was implemented as non-op operator. This is weird for a copy to do nothing! No intends are indicated to help. Seems only done to help MSVC compiler, but this is WRONG and cause a SECURITY FAILURE as the DhParam is not set as supposed to be. Consequence observed on UWP daemon: the anonymous connection cannot be done and the certificate is always send in cleartext. The fix consists in using the dedicated gnutls API to copy correctly the internal dh_params C structure. This is used to implement copy-assignement and copy-constructor methods Reviewed-by: Andreas Traczyk <andreas.traczyk@savoirfairelinux.com> Change-Id: I49d3a3fd2c0a2ed5fde0bd251efbad589c830c70 --- src/security/tls_session.cpp | 19 +++++++++++++++++++ src/security/tls_session.h | 19 +++++++++++++------ 2 files changed, 32 insertions(+), 6 deletions(-) diff --git a/src/security/tls_session.cpp b/src/security/tls_session.cpp index 326137e9fa..0ebabea36d 100644 --- a/src/security/tls_session.cpp +++ b/src/security/tls_session.cpp @@ -82,6 +82,25 @@ DhParams::DhParams(const std::vector<uint8_t>& data) throw std::runtime_error(std::string("Error importing DH params: ") + gnutls_strerror(ret_pem) + " " + gnutls_strerror(ret_der)); } +DhParams& +DhParams::operator=(const DhParams& other) +{ + if (not params_) { + // We need a valid DH params pointer for the copy + gnutls_dh_params_t new_params_; + auto err = gnutls_dh_params_init(&new_params_); + if (err != GNUTLS_E_SUCCESS) + throw std::runtime_error(std::string("Error initializing DH params: ") + gnutls_strerror(err)); + params_.reset(new_params_); + } + + auto err = gnutls_dh_params_cpy(params_.get(), other.get()); + if (err != GNUTLS_E_SUCCESS) + throw std::runtime_error(std::string("Error copying DH params: ") + gnutls_strerror(err)); + + return *this; +} + std::vector<uint8_t> DhParams::serialize() const { diff --git a/src/security/tls_session.h b/src/security/tls_session.h index b1854b6c9f..2c37ef1ca4 100644 --- a/src/security/tls_session.h +++ b/src/security/tls_session.h @@ -22,6 +22,7 @@ #pragma once #include "threadloop.h" +#include "noncopyable.h" #include <gnutls/gnutls.h> #include <gnutls/dtls.h> @@ -41,6 +42,7 @@ #include <atomic> #include <iterator> #include <array> +#include <stdexcept> namespace ring { class IceTransport; @@ -71,13 +73,18 @@ class DhParams { public: DhParams() = default; DhParams(DhParams&&) = default; - - DhParams& operator =(const DhParams& other){ - return *this; + DhParams(const DhParams& other) { + *this = other; } - /** Take ownership of gnutls_dh_params */ - explicit DhParams(gnutls_dh_params_t p) : params_(p, gnutls_dh_params_deinit) {}; + DhParams& operator=(DhParams&& other) = default; + DhParams& operator=(const DhParams& other); + + /// \brief Construct by taking ownership of given gnutls DH params + /// + /// User should not call gnutls_dh_params_deinit on given \a raw_params. + /// The object is stolen and its live is manager by our object. + explicit DhParams(gnutls_dh_params_t p) : params_ {p, gnutls_dh_params_deinit} {} /** Deserialize DER or PEM encoded DH-params */ DhParams(const std::vector<uint8_t>& data); @@ -99,7 +106,7 @@ public: static DhParams generate(); private: - std::unique_ptr<gnutls_dh_params_int, decltype(gnutls_dh_params_deinit)&> params_ {nullptr, gnutls_dh_params_deinit}; + std::unique_ptr<gnutls_dh_params_int, decltype(gnutls_dh_params_deinit)*> params_ {nullptr, gnutls_dh_params_deinit}; }; struct TlsParams { -- GitLab