You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@mesos.apache.org by "Benjamin Mahler (JIRA)" <ji...@apache.org> on 2016/08/04 18:28:20 UTC
[jira] [Created] (MESOS-5989) Libevent SSL Socket downgrade code
accesses uninitialized memory / assumes single peek is sufficient.
Benjamin Mahler created MESOS-5989:
--------------------------------------
Summary: Libevent SSL Socket downgrade code accesses uninitialized memory / assumes single peek is sufficient.
Key: MESOS-5989
URL: https://issues.apache.org/jira/browse/MESOS-5989
Project: Mesos
Issue Type: Bug
Components: libprocess
Reporter: Benjamin Mahler
Priority: Critical
See the XXX comment below.
https://github.com/apache/mesos/blob/1.0.0/3rdparty/libprocess/src/libevent_ssl_socket.cpp#L912-L920
{code}
void LibeventSSLSocketImpl::peek_callback(
evutil_socket_t fd,
short what,
void* arg)
{
CHECK(__in_event_loop__);
CHECK(what & EV_READ);
char data[6];
// Try to peek the first 6 bytes of the message.
ssize_t size = ::recv(fd, data, 6, MSG_PEEK);
// Based on the function 'ssl23_get_client_hello' in openssl, we
// test whether to dispatch to the SSL or non-SSL based accept based
// on the following rules:
// 1. If there are fewer than 3 bytes: non-SSL.
// 2. If the 1st bit of the 1st byte is set AND the 3rd byte is
// equal to SSL2_MT_CLIENT_HELLO: SSL.
// 3. If the 1st byte is equal to SSL3_RT_HANDSHAKE AND the 2nd
// byte is equal to SSL3_VERSION_MAJOR and the 6th byte is
// equal to SSL3_MT_CLIENT_HELLO: SSL.
// 4. Otherwise: non-SSL.
// For an ascii based protocol to falsely get dispatched to SSL it
// needs to:
// 1. Start with an invalid ascii character (0x80).
// 2. OR have the first 2 characters be a SYN followed by ETX, and
// then the 6th character be SOH.
// These conditions clearly do not constitute valid HTTP requests,
// and are unlikely to collide with other existing protocols.
bool ssl = false; // Default to rule 4.
// XXX: data[0] data[1] are guaranteed to be set, but not data[>=2]
if (size < 2) { // Rule 1.
ssl = false;
} else if ((data[0] & 0x80) && data[2] == SSL2_MT_CLIENT_HELLO) { // Rule 2.
ssl = true;
} else if (data[0] == SSL3_RT_HANDSHAKE &&
data[1] == SSL3_VERSION_MAJOR &&
data[5] == SSL3_MT_CLIENT_HELLO) { // Rule 3.
ssl = true;
}
AcceptRequest* request = reinterpret_cast<AcceptRequest*>(arg);
// We call 'event_free()' here because it ensures the event is made
// non-pending and inactive before it gets deallocated.
event_free(request->peek_event);
request->peek_event = nullptr;
if (ssl) {
accept_SSL_callback(request);
} else {
// Downgrade to a non-SSL socket.
Try<Socket> create = Socket::create(Socket::POLL, fd);
if (create.isError()) {
request->promise.fail(create.error());
} else {
request->promise.set(create.get());
}
delete request;
}
}
{code}
This code accesses potentially uninitialized memory. Secondly, the code assumes that a single peek is sufficient for determining whether the incoming data is an SSL connection. There seems to be an assumption that in the SSL path, we are guaranteed to peek a sufficient number of bytes when the socket is ready to read. It's not clear what is providing this guarantee, or if this is incorrect.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)