Skip to content

Commit

Permalink
Core: SSL layer changes:
Browse files Browse the repository at this point in the history 
* Renamed SSL method write_ciphertext_ready() to
  read_cleartext_ready() for clarity.

* It's important that read_cleartext_ready() returns an accurate
  status.  To this end, add ssl_get_bytes_avail to the return
  expression for PolarSSL:

    return !ct_in.empty() || ssl_get_bytes_avail(ssl);

  This will also consider buffering inside of PolarSSL,
  and avoid potential deadlocks.

  Other SSL modules (AppleCrypto and OpenSSL) have been
  commented to warn of this issue.

* Factored out constants such as SHOULD_RETRY to namespace
  SSLConst.

* Added flags var to SSL configs.

* Added new SSL flag LOG_VERIFY_STATUS.  If disabled,
  makes for a quiet SSL negotiation if no errors.

* Detect SSL partial writes and designate a new error status
  code (SSL_PARTIAL_WRITE).

* In ProtoStackBase, detect unclassified errors from SSL layer
  (throw unknown_status_from_ssl_layer).

* PolarSSL module now recognizes Close Notify status and returns
  SSLConst::PEER_CLOSE_NOTIFY.

* In ProtoStackBase, factored out some error handling into
  common method.
  • Loading branch information
@jamesyonan
jamesyonan committed Aug 11, 2014
1 parent 4c56d6b commit d9b5cdf
Show file tree
Hide file tree
Showing 7 changed files with 129 additions and 52 deletions.
18 changes: 9 additions & 9 deletions openvpn/applecrypto/ssl/sslctx.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
#include <openvpn/pki/epkibase.hpp>
#include <openvpn/applecrypto/cf/cfsec.hpp>
#include <openvpn/applecrypto/cf/error.hpp>
#include <openvpn/ssl/sslconsts.hpp>

// An SSL Context is essentially a configuration that can be used
// to generate an arbitrary number of actual SSL connections objects.
Expand All @@ -74,10 +75,12 @@ namespace openvpn {
// The data needed to construct an AppleSSLContext.
struct Config
{
Config() : ssl_debug_level(0) {}
Config() : ssl_debug_level(0),
flags(0) {}

Mode mode;
int ssl_debug_level;
unsigned int flags; // defined in sslconsts.hpp
CF::Array identity; // as returned by load_identity
Frame::Ptr frame;

Expand Down Expand Up @@ -114,10 +117,6 @@ namespace openvpn {
public:
typedef boost::intrusive_ptr<SSL> Ptr;

enum {
SHOULD_RETRY = -1
};

void start_handshake()
{
SSLHandshake(ssl);
Expand All @@ -127,10 +126,10 @@ namespace openvpn {
{
size_t actual = 0;
const OSStatus status = SSLWrite(ssl, data, size, &actual);
if (status < 0 || actual != size)
if (status < 0)
{
if (status == errSSLWouldBlock)
return SHOULD_RETRY;
return SSLConst::SHOULD_RETRY;
else
throw CFException("AppleSSLContext::SSL::write_cleartext failed", status);
}
Expand All @@ -147,7 +146,7 @@ namespace openvpn {
if (status < 0)
{
if (status == errSSLWouldBlock)
return SHOULD_RETRY;
return SSLConst::SHOULD_RETRY;
else
throw CFException("AppleSSLContext::SSL::read_cleartext failed", status);
}
Expand All @@ -158,7 +157,8 @@ namespace openvpn {
throw ssl_ciphertext_in_overflow();
}

bool write_ciphertext_ready() const {
bool read_cleartext_ready() const {
// fixme: need to detect data buffered at SSL layer
return !ct_in.empty();
}

Expand Down
1 change: 1 addition & 0 deletions openvpn/client/cliopt.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,7 @@ namespace openvpn {
ClientSSLAPI::Config cc;
cc.set_external_pki_callback(config.external_pki);
cc.frame = frame;
cc.flags = SSLConst::LOG_VERIFY_STATUS;
#ifdef OPENVPN_SSL_DEBUG
cc.ssl_debug_level = OPENVPN_SSL_DEBUG;
#endif
Expand Down
2 changes: 2 additions & 0 deletions openvpn/error/error.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ namespace openvpn {
TCP_CONNECT_ERROR, // client error on TCP connect
UDP_CONNECT_ERROR, // client error on UDP connect
SSL_ERROR, // errors resulting from read/write on SSL object
SSL_PARTIAL_WRITE, // SSL object did not process all written cleartext
ENCAPSULATION_ERROR, // exceptions thrown during packet encapsulation
EPKI_CERT_ERROR, // error obtaining certificate from External PKI provider
EPKI_SIGN_ERROR, // error obtaining RSA signature from External PKI provider
Expand Down Expand Up @@ -124,6 +125,7 @@ namespace openvpn {
"TCP_CONNECT_ERROR",
"UDP_CONNECT_ERROR",
"SSL_ERROR",
"SSL_PARTIAL_WRITE",
"ENCAPSULATION_ERROR",
"EPKI_CERT_ERROR",
"EPKI_SIGN_ERROR",
Expand Down
16 changes: 8 additions & 8 deletions openvpn/openssl/ssl/sslctx.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
#include <openvpn/ssl/nscert.hpp>
#include <openvpn/ssl/tlsver.hpp>
#include <openvpn/ssl/tls_remote.hpp>
#include <openvpn/ssl/sslconsts.hpp>
#include <openvpn/openssl/util/error.hpp>
#include <openvpn/openssl/pki/x509.hpp>
#include <openvpn/openssl/pki/crl.hpp>
Expand Down Expand Up @@ -85,6 +86,7 @@ namespace openvpn {
{
Config() : external_pki(NULL),
ssl_debug_level(0),
flags(0),
ns_cert_type(NSCert::NONE),
tls_version_min(TLSVersion::UNDEF),
local_cert_enabled(true),
Expand All @@ -99,6 +101,7 @@ namespace openvpn {
ExternalPKIBase* external_pki;
Frame::Ptr frame;
int ssl_debug_level;
unsigned int flags; // defined in sslconsts.hpp
NSCert::Type ns_cert_type;
std::vector<unsigned int> ku; // if defined, peer cert X509 key usage must match one of these values
std::string eku; // if defined, peer cert X509 extended key usage must match this OID/string
Expand Down Expand Up @@ -225,10 +228,6 @@ namespace openvpn {
public:
typedef boost::intrusive_ptr<SSL> Ptr;

enum {
SHOULD_RETRY = -1
};

void start_handshake()
{
SSL_do_handshake(ssl);
Expand All @@ -237,10 +236,10 @@ namespace openvpn {
ssize_t write_cleartext_unbuffered(const void *data, const size_t size)
{
const int status = BIO_write(ssl_bio, data, size);
if (status != int(size))
if (status < 0)
{
if (status == -1 && BIO_should_retry(ssl_bio))
return SHOULD_RETRY;
return SSLConst::SHOULD_RETRY;
else
OPENVPN_THROW(OpenSSLException, "OpenSSLContext::SSL::write_cleartext: BIO_write failed, size=" << size << " status=" << status);
}
Expand All @@ -256,7 +255,7 @@ namespace openvpn {
if (status < 0)
{
if (status == -1 && BIO_should_retry(ssl_bio))
return SHOULD_RETRY;
return SSLConst::SHOULD_RETRY;
else
OPENVPN_THROW(OpenSSLException, "OpenSSLContext::SSL::read_cleartext: BIO_read failed, cap=" << capacity << " status=" << status);
}
Expand All @@ -267,7 +266,8 @@ namespace openvpn {
throw ssl_ciphertext_in_overflow();
}

bool write_ciphertext_ready() const {
bool read_cleartext_ready() const {
// fixme: need to detect data buffered at SSL layer
return !bmq_stream::memq_from_bio(ct_in)->empty();
}

Expand Down
23 changes: 11 additions & 12 deletions openvpn/polarssl/ssl/sslctx.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
#include <openvpn/ssl/nscert.hpp>
#include <openvpn/ssl/tlsver.hpp>
#include <openvpn/ssl/tls_remote.hpp>
#include <openvpn/ssl/sslconsts.hpp>

#include <openvpn/polarssl/pki/x509cert.hpp>
#include <openvpn/polarssl/pki/x509crl.hpp>
Expand Down Expand Up @@ -94,6 +95,7 @@ namespace openvpn {
{
Config() : external_pki(NULL),
ssl_debug_level(0),
flags(0),
ns_cert_type(NSCert::NONE),
tls_version_min(TLSVersion::UNDEF),
local_cert_enabled(true),
Expand All @@ -110,6 +112,7 @@ namespace openvpn {
ExternalPKIBase* external_pki;
Frame::Ptr frame;
int ssl_debug_level;
unsigned int flags; // defined in sslconsts.hpp
NSCert::Type ns_cert_type;
std::vector<unsigned int> ku; // if defined, peer cert X509 key usage must match one of these values
std::string eku; // if defined, peer cert X509 extended key usage must match this OID/string
Expand Down Expand Up @@ -263,13 +266,6 @@ namespace openvpn {
public:
typedef boost::intrusive_ptr<SSL> Ptr;

// special return value from read functions -- indicates
// that no cleartext data is available now (until more
// ciphertext is pushed into the SSL engine)
enum {
SHOULD_RETRY = -1
};

void start_handshake()
{
ssl_handshake(ssl);
Expand All @@ -281,7 +277,7 @@ namespace openvpn {
if (status < 0)
{
if (status == CT_WOULD_BLOCK)
return SHOULD_RETRY;
return SSLConst::SHOULD_RETRY;
else if (status == CT_INTERNAL_ERROR)
throw PolarSSLException("SSL write: internal error");
else
Expand All @@ -299,7 +295,9 @@ namespace openvpn {
if (status < 0)
{
if (status == CT_WOULD_BLOCK)
return SHOULD_RETRY;
return SSLConst::SHOULD_RETRY;
else if (status == POLARSSL_ERR_SSL_PEER_CLOSE_NOTIFY)
return SSLConst::PEER_CLOSE_NOTIFY;
else if (status == CT_INTERNAL_ERROR)
throw PolarSSLException("SSL read: internal error");
else
Expand All @@ -312,8 +310,8 @@ namespace openvpn {
throw ssl_ciphertext_in_overflow();
}

bool write_ciphertext_ready() const {
return !ct_in.empty();
bool read_cleartext_ready() const {
return !ct_in.empty() || ssl_get_bytes_avail(ssl);
}

void write_ciphertext(const BufferPtr& buf)
Expand Down Expand Up @@ -754,7 +752,8 @@ namespace openvpn {
bool fail = false;

// log status
OPENVPN_LOG_SSL(status_string(cert, depth, flags));
if (self->config.flags & SSLConst::LOG_VERIFY_STATUS)
OPENVPN_LOG_SSL(status_string(cert, depth, flags));

// leaf-cert verification
if (depth == 0)
Expand Down
73 changes: 50 additions & 23 deletions openvpn/ssl/protostack.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@
#include <openvpn/reliable/relsend.hpp>
#include <openvpn/reliable/relack.hpp>
#include <openvpn/frame/frame.hpp>
#include <openvpn/error/excode.hpp>
#include <openvpn/ssl/sslconsts.hpp>

// ProtoStackBase is designed to allow general-purpose protocols (including
// but not limited to OpenVPN) to run over SSL, where the underlying transport
Expand Down Expand Up @@ -77,6 +79,7 @@ namespace openvpn {
typedef ReliableRecvTemplate<PACKET> ReliableRecv;

OPENVPN_SIMPLE_EXCEPTION(proto_stack_invalidated);
OPENVPN_SIMPLE_EXCEPTION(unknown_status_from_ssl_layer);

enum NetSendType {
NET_SEND_SSL,
Expand Down Expand Up @@ -270,7 +273,6 @@ namespace openvpn {

// END of VIRTUAL METHODS


// app data -> SSL -> protocol encapsulation -> reliability layer -> network
void down_stack_app()
{
Expand All @@ -280,19 +282,30 @@ namespace openvpn {
while (!app_write_queue.empty())
{
BufferPtr& buf = app_write_queue.front();
ssize_t size;
try {
const ssize_t size = ssl_->write_cleartext_unbuffered(buf->data(), buf->size());
if (size == SSLContext::SSL::SHOULD_RETRY)
break;
size = ssl_->write_cleartext_unbuffered(buf->data(), buf->size());
}
catch (...)
{
if (stats)
stats->error(Error::SSL_ERROR);
invalidate(Error::SSL_ERROR);
error(Error::SSL_ERROR);
throw;
}
app_write_queue.pop_front();
if (size == buf->size())
app_write_queue.pop_front();
else if (size == SSLConst::SHOULD_RETRY)
break;
else if (size >= 0)
{
// error if written size is different from what we asked for
error(Error::SSL_PARTIAL_WRITE);
break;
}
else
{
error(Error::SSL_ERROR);
throw unknown_status_from_ssl_layer();
}
}

// encapsulate SSL ciphertext packets
Expand All @@ -307,9 +320,7 @@ namespace openvpn {
}
catch (...)
{
if (stats)
stats->error(Error::ENCAPSULATION_ERROR);
invalidate(Error::ENCAPSULATION_ERROR);
error(Error::ENCAPSULATION_ERROR);
throw;
}

Expand All @@ -334,9 +345,7 @@ namespace openvpn {
}
catch (...)
{
if (stats)
stats->error(Error::ENCAPSULATION_ERROR);
invalidate(Error::ENCAPSULATION_ERROR);
error(Error::ENCAPSULATION_ERROR);
throw;
}

Expand Down Expand Up @@ -375,7 +384,7 @@ namespace openvpn {

// read cleartext data from SSL object
if (ssl_started_)
while (ssl_->write_ciphertext_ready())
while (ssl_->read_cleartext_ready())
{
ssize_t size;
if (!to_app_buf)
Expand All @@ -387,17 +396,28 @@ namespace openvpn {
catch (...)
{
// SSL fatal errors will invalidate the session
if (stats)
stats->error(Error::SSL_ERROR);
invalidate(Error::SSL_ERROR);
error(Error::SSL_ERROR);
throw;
}
if (size == SSLContext::SSL::SHOULD_RETRY)
break;
to_app_buf->set_size(size);
if (size >= 0)
{
to_app_buf->set_size(size);

// pass cleartext data to app
app_recv(to_app_buf);
// pass cleartext data to app
app_recv(to_app_buf);
}
else if (size == SSLConst::SHOULD_RETRY)
break;
else if (size == SSLConst::PEER_CLOSE_NOTIFY)
{
error(Error::SSL_ERROR);
throw ErrorCode(Error::CLIENT_HALT, true, "SSL Close Notify received");
}
else
{
error(Error::SSL_ERROR);
throw unknown_status_from_ssl_layer();
}
}
}

Expand All @@ -406,6 +426,13 @@ namespace openvpn {
next_retransmit_ = *now + rel_send.until_retransmit(*now);
}

void error(const Error::Type reason)
{
if (stats)
stats->error(reason);
invalidate(reason);
}

private:
typename SSLContext::SSL::Ptr ssl_;
Frame::Ptr frame_;
Expand Down
Loading

0 comments on commit d9b5cdf

Please sign in to comment.