[ 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)