[jira] [Commented] (MESOS-5989) Libevent SSL Socket downgrade code accesses uninitialized memory / assumes single peek is sufficient.

["Vinod Kone (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/MESOS-5989?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16705403#comment-16705403 ] 

Vinod Kone commented on MESOS-5989:
-----------------------------------

[~bmahler] Is this still an issue?

> 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
(v7.6.3#76005)