From 20900b133ec8b0b8455d8ed7310741d578f936c8 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Thu, 30 Oct 2014 21:47:38 +0900 Subject: [PATCH] nghttpx: Fix heap-use-after-free in ClientHandler object This bug was found by scan-build --- src/shrpx_client_handler.cc | 178 ++++++++++++++++++++---------------- src/shrpx_client_handler.h | 4 +- 2 files changed, 101 insertions(+), 81 deletions(-) diff --git a/src/shrpx_client_handler.cc b/src/shrpx_client_handler.cc index 7271a767..f135a268 100644 --- a/src/shrpx_client_handler.cc +++ b/src/shrpx_client_handler.cc @@ -143,35 +143,9 @@ namespace { void upstream_http2_connhd_readcb(bufferevent *bev, void *arg) { // This callback assumes upstream is Http2Upstream. - uint8_t data[NGHTTP2_CLIENT_CONNECTION_PREFACE_LEN]; auto handler = static_cast(arg); - auto leftlen = handler->get_left_connhd_len(); - auto input = bufferevent_get_input(bev); - auto readlen = evbuffer_remove(input, data, leftlen); - if(readlen == -1) { + if(handler->on_http2_connhd_read() != 0) { delete handler; - return; - } - if(memcmp(NGHTTP2_CLIENT_CONNECTION_PREFACE + - NGHTTP2_CLIENT_CONNECTION_PREFACE_LEN - leftlen, - data, readlen) != 0) { - // There is no downgrade path here. Just drop the connection. - if(LOG_ENABLED(INFO)) { - CLOG(INFO, handler) << "invalid client connection header"; - } - delete handler; - return; - } - leftlen -= readlen; - handler->set_left_connhd_len(leftlen); - if(leftlen == 0) { - handler->set_bev_cb(upstream_readcb, upstream_writecb, upstream_eventcb); - // Run on_read to process data left in buffer since they are not - // notified further - if(handler->on_read() != 0) { - delete handler; - return; - } } } } // namespace @@ -180,49 +154,9 @@ namespace { void upstream_http1_connhd_readcb(bufferevent *bev, void *arg) { // This callback assumes upstream is HttpsUpstream. - uint8_t data[NGHTTP2_CLIENT_CONNECTION_PREFACE_LEN]; auto handler = static_cast(arg); - auto leftlen = handler->get_left_connhd_len(); - auto input = bufferevent_get_input(bev); - auto readlen = evbuffer_copyout(input, data, leftlen); - if(readlen == -1) { + if(handler->on_http1_connhd_read() != 0) { delete handler; - return; - } - if(memcmp(NGHTTP2_CLIENT_CONNECTION_PREFACE + - NGHTTP2_CLIENT_CONNECTION_PREFACE_LEN - leftlen, - data, readlen) != 0) { - if(LOG_ENABLED(INFO)) { - CLOG(INFO, handler) << "This is HTTP/1.1 connection, " - << "but may be upgraded to HTTP/2 later."; - } - // Reset header length for later HTTP/2 upgrade - handler->set_left_connhd_len(NGHTTP2_CLIENT_CONNECTION_PREFACE_LEN); - handler->set_bev_cb(upstream_readcb, upstream_writecb, upstream_eventcb); - if(handler->on_read() != 0) { - delete handler; - return; - } - return; - } - if(evbuffer_drain(input, readlen) == -1) { - delete handler; - return; - } - leftlen -= readlen; - handler->set_left_connhd_len(leftlen); - if(leftlen == 0) { - if(LOG_ENABLED(INFO)) { - CLOG(INFO, handler) << "direct HTTP/2 connection"; - } - handler->direct_http2_upgrade(); - handler->set_bev_cb(upstream_readcb, upstream_writecb, upstream_eventcb); - // Run on_read to process data left in buffer since they are not - // notified further - if(handler->on_read() != 0) { - delete handler; - return; - } } } } // namespace @@ -385,7 +319,9 @@ int ClientHandler::validate_next_proto() // At this point, input buffer is already filled with some // bytes. The read callback is not called until new data // come. So consume input buffer here. - upstream_http2_connhd_readcb(bev_, this); + if(on_http2_connhd_read() != 0) { + return -1; + } return 0; } else { @@ -456,6 +392,100 @@ int ClientHandler::on_event() return upstream_->on_event(); } +int ClientHandler::on_http2_connhd_read() +{ + // This callback assumes upstream is Http2Upstream. + uint8_t data[NGHTTP2_CLIENT_CONNECTION_PREFACE_LEN]; + auto input = bufferevent_get_input(bev_); + auto readlen = evbuffer_remove(input, data, left_connhd_len_); + + if(readlen == -1) { + return -1; + } + + if(memcmp(NGHTTP2_CLIENT_CONNECTION_PREFACE + + NGHTTP2_CLIENT_CONNECTION_PREFACE_LEN - left_connhd_len_, + data, readlen) != 0) { + // There is no downgrade path here. Just drop the connection. + if(LOG_ENABLED(INFO)) { + CLOG(INFO, this) << "invalid client connection header"; + } + + return -1; + } + + left_connhd_len_ -= readlen; + + if(left_connhd_len_ > 0) { + return 0; + } + + set_bev_cb(upstream_readcb, upstream_writecb, upstream_eventcb); + + // Run on_read to process data left in buffer since they are not + // notified further + if(on_read() != 0) { + return -1; + } + + return 0; +} + +int ClientHandler::on_http1_connhd_read() +{ + uint8_t data[NGHTTP2_CLIENT_CONNECTION_PREFACE_LEN]; + auto input = bufferevent_get_input(bev_); + auto readlen = evbuffer_copyout(input, data, left_connhd_len_); + + if(readlen == -1) { + return -1; + } + + if(memcmp(NGHTTP2_CLIENT_CONNECTION_PREFACE + + NGHTTP2_CLIENT_CONNECTION_PREFACE_LEN - left_connhd_len_, + data, readlen) != 0) { + if(LOG_ENABLED(INFO)) { + CLOG(INFO, this) << "This is HTTP/1.1 connection, " + << "but may be upgraded to HTTP/2 later."; + } + + // Reset header length for later HTTP/2 upgrade + left_connhd_len_ = NGHTTP2_CLIENT_CONNECTION_PREFACE_LEN; + set_bev_cb(upstream_readcb, upstream_writecb, upstream_eventcb); + + if(on_read() != 0) { + return -1; + } + + return 0; + } + + if(evbuffer_drain(input, readlen) == -1) { + return -1; + } + + left_connhd_len_ -= readlen; + + if(left_connhd_len_ > 0) { + return 0; + } + + if(LOG_ENABLED(INFO)) { + CLOG(INFO, this) << "direct HTTP/2 connection"; + } + + direct_http2_upgrade(); + set_bev_cb(upstream_readcb, upstream_writecb, upstream_eventcb); + + // Run on_read to process data left in buffer since they are not + // notified further + if(on_read() != 0) { + return -1; + } + + return 0; +} + const std::string& ClientHandler::get_ipaddr() const { return ipaddr_; @@ -552,16 +582,6 @@ ConnectBlocker* ClientHandler::get_http1_connect_blocker() const return http1_connect_blocker_; } -size_t ClientHandler::get_left_connhd_len() const -{ - return left_connhd_len_; -} - -void ClientHandler::set_left_connhd_len(size_t left) -{ - left_connhd_len_ = left; -} - void ClientHandler::direct_http2_upgrade() { upstream_= util::make_unique(this); diff --git a/src/shrpx_client_handler.h b/src/shrpx_client_handler.h index 0430f6ba..54e8a026 100644 --- a/src/shrpx_client_handler.h +++ b/src/shrpx_client_handler.h @@ -75,8 +75,6 @@ public: Http2Session* get_http2_session() const; void set_http1_connect_blocker(ConnectBlocker *http1_connect_blocker); ConnectBlocker* get_http1_connect_blocker() const; - size_t get_left_connhd_len() const; - void set_left_connhd_len(size_t left); // Call this function when HTTP/2 connection header is received at // the start of the connection. void direct_http2_upgrade(); @@ -91,6 +89,8 @@ public: bool get_tls_handshake() const; void set_tls_renegotiation(bool f); bool get_tls_renegotiation() const; + int on_http2_connhd_read(); + int on_http1_connhd_read(); private: std::unique_ptr upstream_; std::string ipaddr_;