Formatting and style guide improvements for opensslstreamadapter.cc
This change is part of a long set of changes to improve the overall code quality
of the the cryptography code in WebRTC. This is a set of low risk refactorings.
More complex refactorings will be saved for a different CL.
This change updates the conditions to move away from:
if (a)
b = c;
to
if (a) {
b = c;
}
The code style guide allows for either but in security critical code this has
been known to cause issues as it is very easy to forget the braces when
adding additional code to conditionals.
Bug: webrtc:9860
Change-Id: I2ec07a4129fe4756b90f6b295d62a4cadbc1f71f
Reviewed-on: https://webrtc-review.googlesource.com/c/106140
Reviewed-by: Steve Anton <steveanton@webrtc.org>
Reviewed-by: Qingsi Wang <qingsi@webrtc.org>
Commit-Queue: Benjamin Wright <benwright@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#25186}
This commit is contained in:
Benjamin Wright
committed by
Commit Bot
Commit Bot
parent
f714ee1f8f
commit
d4d5f8a0ec
@ -22,6 +22,7 @@
|
||||
#endif
|
||||
|
||||
#include <memory>
|
||||
#include <utility>
|
||||
#include <vector>
|
||||
|
||||
#include "rtc_base/checks.h"
|
||||
@ -180,8 +181,9 @@ static BIO_METHOD* BIO_stream_method() {
|
||||
|
||||
static BIO* BIO_new_stream(StreamInterface* stream) {
|
||||
BIO* ret = BIO_new(BIO_stream_method());
|
||||
if (ret == nullptr)
|
||||
if (ret == nullptr) {
|
||||
return nullptr;
|
||||
}
|
||||
BIO_set_data(ret, stream);
|
||||
return ret;
|
||||
}
|
||||
@ -196,14 +198,16 @@ static int stream_new(BIO* b) {
|
||||
}
|
||||
|
||||
static int stream_free(BIO* b) {
|
||||
if (b == nullptr)
|
||||
if (b == nullptr) {
|
||||
return 0;
|
||||
}
|
||||
return 1;
|
||||
}
|
||||
|
||||
static int stream_read(BIO* b, char* out, int outl) {
|
||||
if (!out)
|
||||
if (!out) {
|
||||
return -1;
|
||||
}
|
||||
StreamInterface* stream = static_cast<StreamInterface*>(BIO_get_data(b));
|
||||
BIO_clear_retry_flags(b);
|
||||
size_t read;
|
||||
@ -218,8 +222,9 @@ static int stream_read(BIO* b, char* out, int outl) {
|
||||
}
|
||||
|
||||
static int stream_write(BIO* b, const char* in, int inl) {
|
||||
if (!in)
|
||||
if (!in) {
|
||||
return -1;
|
||||
}
|
||||
StreamInterface* stream = static_cast<StreamInterface*>(BIO_get_data(b));
|
||||
BIO_clear_retry_flags(b);
|
||||
size_t written;
|
||||
@ -362,8 +367,9 @@ std::string OpenSSLStreamAdapter::SslCipherSuiteToName(int cipher_suite) {
|
||||
}
|
||||
|
||||
bool OpenSSLStreamAdapter::GetSslCipherSuite(int* cipher_suite) {
|
||||
if (state_ != SSL_CONNECTED)
|
||||
if (state_ != SSL_CONNECTED) {
|
||||
return false;
|
||||
}
|
||||
|
||||
const SSL_CIPHER* current_cipher = SSL_get_current_cipher(ssl_);
|
||||
if (current_cipher == nullptr) {
|
||||
@ -380,18 +386,20 @@ int OpenSSLStreamAdapter::GetSslVersion() const {
|
||||
|
||||
int ssl_version = SSL_version(ssl_);
|
||||
if (ssl_mode_ == SSL_MODE_DTLS) {
|
||||
if (ssl_version == DTLS1_VERSION)
|
||||
if (ssl_version == DTLS1_VERSION) {
|
||||
return SSL_PROTOCOL_DTLS_10;
|
||||
else if (ssl_version == DTLS1_2_VERSION)
|
||||
} else if (ssl_version == DTLS1_2_VERSION) {
|
||||
return SSL_PROTOCOL_DTLS_12;
|
||||
}
|
||||
} else {
|
||||
if (ssl_version == TLS1_VERSION)
|
||||
if (ssl_version == TLS1_VERSION) {
|
||||
return SSL_PROTOCOL_TLS_10;
|
||||
else if (ssl_version == TLS1_1_VERSION)
|
||||
} else if (ssl_version == TLS1_1_VERSION) {
|
||||
return SSL_PROTOCOL_TLS_11;
|
||||
else if (ssl_version == TLS1_2_VERSION)
|
||||
} else if (ssl_version == TLS1_2_VERSION) {
|
||||
return SSL_PROTOCOL_TLS_12;
|
||||
}
|
||||
}
|
||||
|
||||
return -1;
|
||||
}
|
||||
@ -403,15 +411,11 @@ bool OpenSSLStreamAdapter::ExportKeyingMaterial(const std::string& label,
|
||||
bool use_context,
|
||||
uint8_t* result,
|
||||
size_t result_len) {
|
||||
int i;
|
||||
|
||||
i = SSL_export_keying_material(ssl_, result, result_len, label.c_str(),
|
||||
if (SSL_export_keying_material(ssl_, result, result_len, label.c_str(),
|
||||
label.length(), const_cast<uint8_t*>(context),
|
||||
context_len, use_context);
|
||||
|
||||
if (i != 1)
|
||||
context_len, use_context) != 1) {
|
||||
return false;
|
||||
|
||||
}
|
||||
return true;
|
||||
}
|
||||
|
||||
@ -419,8 +423,9 @@ bool OpenSSLStreamAdapter::SetDtlsSrtpCryptoSuites(
|
||||
const std::vector<int>& ciphers) {
|
||||
std::string internal_ciphers;
|
||||
|
||||
if (state_ != SSL_NONE)
|
||||
if (state_ != SSL_NONE) {
|
||||
return false;
|
||||
}
|
||||
|
||||
for (std::vector<int>::const_iterator cipher = ciphers.begin();
|
||||
cipher != ciphers.end(); ++cipher) {
|
||||
@ -429,8 +434,9 @@ bool OpenSSLStreamAdapter::SetDtlsSrtpCryptoSuites(
|
||||
++entry) {
|
||||
if (*cipher == entry->id) {
|
||||
found = true;
|
||||
if (!internal_ciphers.empty())
|
||||
if (!internal_ciphers.empty()) {
|
||||
internal_ciphers += ":";
|
||||
}
|
||||
internal_ciphers += entry->internal_name;
|
||||
break;
|
||||
}
|
||||
@ -442,8 +448,9 @@ bool OpenSSLStreamAdapter::SetDtlsSrtpCryptoSuites(
|
||||
}
|
||||
}
|
||||
|
||||
if (internal_ciphers.empty())
|
||||
if (internal_ciphers.empty()) {
|
||||
return false;
|
||||
}
|
||||
|
||||
srtp_ciphers_ = internal_ciphers;
|
||||
return true;
|
||||
@ -451,14 +458,16 @@ bool OpenSSLStreamAdapter::SetDtlsSrtpCryptoSuites(
|
||||
|
||||
bool OpenSSLStreamAdapter::GetDtlsSrtpCryptoSuite(int* crypto_suite) {
|
||||
RTC_DCHECK(state_ == SSL_CONNECTED);
|
||||
if (state_ != SSL_CONNECTED)
|
||||
if (state_ != SSL_CONNECTED) {
|
||||
return false;
|
||||
}
|
||||
|
||||
const SRTP_PROTECTION_PROFILE* srtp_profile =
|
||||
SSL_get_selected_srtp_profile(ssl_);
|
||||
|
||||
if (!srtp_profile)
|
||||
if (!srtp_profile) {
|
||||
return false;
|
||||
}
|
||||
|
||||
*crypto_suite = srtp_profile->id;
|
||||
RTC_DCHECK(!SrtpCryptoSuiteToName(*crypto_suite).empty());
|
||||
@ -470,8 +479,8 @@ bool OpenSSLStreamAdapter::IsTlsConnected() {
|
||||
}
|
||||
|
||||
int OpenSSLStreamAdapter::StartSSL() {
|
||||
if (state_ != SSL_NONE) {
|
||||
// Don't allow StartSSL to be called twice.
|
||||
if (state_ != SSL_NONE) {
|
||||
return -1;
|
||||
}
|
||||
|
||||
@ -532,15 +541,17 @@ StreamResult OpenSSLStreamAdapter::Write(const void* data,
|
||||
case SSL_ERROR:
|
||||
case SSL_CLOSED:
|
||||
default:
|
||||
if (error)
|
||||
if (error) {
|
||||
*error = ssl_error_code_;
|
||||
}
|
||||
return SR_ERROR;
|
||||
}
|
||||
|
||||
// OpenSSL will return an error if we try to write zero bytes
|
||||
if (data_len == 0) {
|
||||
if (written)
|
||||
if (written) {
|
||||
*written = 0;
|
||||
}
|
||||
return SR_SUCCESS;
|
||||
}
|
||||
|
||||
@ -567,8 +578,9 @@ StreamResult OpenSSLStreamAdapter::Write(const void* data,
|
||||
case SSL_ERROR_ZERO_RETURN:
|
||||
default:
|
||||
Error("SSL_write", (ssl_error ? ssl_error : -1), 0, false);
|
||||
if (error)
|
||||
if (error) {
|
||||
*error = ssl_error_code_;
|
||||
}
|
||||
return SR_ERROR;
|
||||
}
|
||||
// not reached
|
||||
@ -583,31 +595,29 @@ StreamResult OpenSSLStreamAdapter::Read(void* data,
|
||||
case SSL_NONE:
|
||||
// pass-through in clear text
|
||||
return StreamAdapterInterface::Read(data, data_len, read, error);
|
||||
|
||||
case SSL_WAIT:
|
||||
case SSL_CONNECTING:
|
||||
return SR_BLOCK;
|
||||
|
||||
case SSL_CONNECTED:
|
||||
if (waiting_to_verify_peer_certificate()) {
|
||||
return SR_BLOCK;
|
||||
}
|
||||
break;
|
||||
|
||||
case SSL_CLOSED:
|
||||
return SR_EOS;
|
||||
|
||||
case SSL_ERROR:
|
||||
default:
|
||||
if (error)
|
||||
if (error) {
|
||||
*error = ssl_error_code_;
|
||||
}
|
||||
return SR_ERROR;
|
||||
}
|
||||
|
||||
// Don't trust OpenSSL with zero byte reads
|
||||
if (data_len == 0) {
|
||||
if (read)
|
||||
if (read) {
|
||||
*read = 0;
|
||||
}
|
||||
return SR_SUCCESS;
|
||||
}
|
||||
|
||||
@ -620,8 +630,9 @@ StreamResult OpenSSLStreamAdapter::Read(void* data,
|
||||
RTC_LOG(LS_VERBOSE) << " -- success";
|
||||
RTC_DCHECK_GT(code, 0);
|
||||
RTC_DCHECK_LE(code, data_len);
|
||||
if (read)
|
||||
if (read) {
|
||||
*read = code;
|
||||
}
|
||||
|
||||
if (ssl_mode_ == SSL_MODE_DTLS) {
|
||||
// Enforce atomic reads -- this is a short read
|
||||
@ -630,8 +641,9 @@ StreamResult OpenSSLStreamAdapter::Read(void* data,
|
||||
if (pending) {
|
||||
RTC_LOG(LS_INFO) << " -- short DTLS read. flushing";
|
||||
FlushInput(pending);
|
||||
if (error)
|
||||
if (error) {
|
||||
*error = SSE_MSG_TRUNC;
|
||||
}
|
||||
return SR_ERROR;
|
||||
}
|
||||
}
|
||||
@ -650,8 +662,9 @@ StreamResult OpenSSLStreamAdapter::Read(void* data,
|
||||
break;
|
||||
default:
|
||||
Error("SSL_read", (ssl_error ? ssl_error : -1), 0, false);
|
||||
if (error)
|
||||
if (error) {
|
||||
*error = ssl_error_code_;
|
||||
}
|
||||
return SR_ERROR;
|
||||
}
|
||||
// not reached
|
||||
@ -700,7 +713,7 @@ StreamState OpenSSLStreamAdapter::GetState() const {
|
||||
return SS_OPEN;
|
||||
default:
|
||||
return SS_CLOSED;
|
||||
};
|
||||
}
|
||||
// not reached
|
||||
}
|
||||
|
||||
@ -710,6 +723,7 @@ void OpenSSLStreamAdapter::OnEvent(StreamInterface* stream,
|
||||
int events_to_signal = 0;
|
||||
int signal_error = 0;
|
||||
RTC_DCHECK(stream == this->stream());
|
||||
|
||||
if ((events & SE_OPEN)) {
|
||||
RTC_LOG(LS_VERBOSE) << "OpenSSLStreamAdapter::OnEvent SE_OPEN";
|
||||
if (state_ != SSL_WAIT) {
|
||||
@ -723,6 +737,7 @@ void OpenSSLStreamAdapter::OnEvent(StreamInterface* stream,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
if ((events & (SE_READ | SE_WRITE))) {
|
||||
RTC_LOG(LS_VERBOSE) << "OpenSSLStreamAdapter::OnEvent"
|
||||
<< ((events & SE_READ) ? " SE_READ" : "")
|
||||
@ -747,6 +762,7 @@ void OpenSSLStreamAdapter::OnEvent(StreamInterface* stream,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
if ((events & SE_CLOSE)) {
|
||||
RTC_LOG(LS_VERBOSE) << "OpenSSLStreamAdapter::OnEvent(SE_CLOSE, " << err
|
||||
<< ")";
|
||||
@ -756,9 +772,11 @@ void OpenSSLStreamAdapter::OnEvent(StreamInterface* stream,
|
||||
RTC_DCHECK(signal_error == 0);
|
||||
signal_error = err;
|
||||
}
|
||||
if (events_to_signal)
|
||||
|
||||
if (events_to_signal) {
|
||||
StreamAdapterInterface::OnEvent(stream, events_to_signal, signal_error);
|
||||
}
|
||||
}
|
||||
|
||||
int OpenSSLStreamAdapter::BeginSSL() {
|
||||
RTC_DCHECK(state_ == SSL_CONNECTING);
|
||||
@ -770,12 +788,14 @@ int OpenSSLStreamAdapter::BeginSSL() {
|
||||
// First set up the context.
|
||||
RTC_DCHECK(ssl_ctx_ == nullptr);
|
||||
ssl_ctx_ = SetupSSLContext();
|
||||
if (!ssl_ctx_)
|
||||
if (!ssl_ctx_) {
|
||||
return -1;
|
||||
}
|
||||
|
||||
bio = BIO_new_stream(static_cast<StreamInterface*>(stream()));
|
||||
if (!bio)
|
||||
if (!bio) {
|
||||
return -1;
|
||||
}
|
||||
|
||||
ssl_ = SSL_new(ssl_ctx_);
|
||||
if (!ssl_) {
|
||||
@ -805,8 +825,9 @@ int OpenSSLStreamAdapter::BeginSSL() {
|
||||
// commonly supported. BoringSSL doesn't need explicit configuration and has
|
||||
// a reasonable default set.
|
||||
EC_KEY* ecdh = EC_KEY_new_by_curve_name(NID_X9_62_prime256v1);
|
||||
if (ecdh == nullptr)
|
||||
if (ecdh == nullptr) {
|
||||
return -1;
|
||||
}
|
||||
SSL_set_options(ssl_, SSL_OP_SINGLE_ECDH_USE);
|
||||
SSL_set_tmp_ecdh(ssl_, ecdh);
|
||||
EC_KEY_free(ecdh);
|
||||
@ -886,9 +907,10 @@ void OpenSSLStreamAdapter::Error(const char* context,
|
||||
state_ = SSL_ERROR;
|
||||
ssl_error_code_ = err;
|
||||
Cleanup(alert);
|
||||
if (signal)
|
||||
if (signal) {
|
||||
StreamAdapterInterface::OnEvent(stream(), SE_CLOSE, err);
|
||||
}
|
||||
}
|
||||
|
||||
void OpenSSLStreamAdapter::Cleanup(uint8_t alert) {
|
||||
RTC_LOG(LS_INFO) << "Cleanup";
|
||||
@ -990,8 +1012,9 @@ SSL_CTX* OpenSSLStreamAdapter::SetupSSLContext() {
|
||||
ctx = SSL_CTX_new(method);
|
||||
#endif // OPENSSL_IS_BORINGSSL
|
||||
|
||||
if (ctx == nullptr)
|
||||
if (ctx == nullptr) {
|
||||
return nullptr;
|
||||
}
|
||||
|
||||
#ifdef OPENSSL_IS_BORINGSSL
|
||||
SSL_CTX_set_min_proto_version(
|
||||
@ -1176,17 +1199,19 @@ static const cipher_list OK_ECDSA_ciphers[] = {
|
||||
bool OpenSSLStreamAdapter::IsAcceptableCipher(int cipher, KeyType key_type) {
|
||||
if (key_type == KT_RSA) {
|
||||
for (const cipher_list& c : OK_RSA_ciphers) {
|
||||
if (cipher == c.cipher)
|
||||
if (cipher == c.cipher) {
|
||||
return true;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
if (key_type == KT_ECDSA) {
|
||||
for (const cipher_list& c : OK_ECDSA_ciphers) {
|
||||
if (cipher == c.cipher)
|
||||
if (cipher == c.cipher) {
|
||||
return true;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
return false;
|
||||
}
|
||||
@ -1195,17 +1220,19 @@ bool OpenSSLStreamAdapter::IsAcceptableCipher(const std::string& cipher,
|
||||
KeyType key_type) {
|
||||
if (key_type == KT_RSA) {
|
||||
for (const cipher_list& c : OK_RSA_ciphers) {
|
||||
if (cipher == c.cipher_str)
|
||||
if (cipher == c.cipher_str) {
|
||||
return true;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
if (key_type == KT_ECDSA) {
|
||||
for (const cipher_list& c : OK_ECDSA_ciphers) {
|
||||
if (cipher == c.cipher_str)
|
||||
if (cipher == c.cipher_str) {
|
||||
return true;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
return false;
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user