From 901de5fbcea3ad1cbde9587011cc3d9da627bddd Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Sat, 13 Sep 2014 19:50:44 +0900 Subject: [PATCH] Add nghttp2_option_set_recv_client_preface() By default, nghttp2 library only handles HTTP/2 frames and does not recognize first 24 bytes of client connection preface. This design choice is done due to the fact that server may want to detect the application protocol based on first few bytes on clear text communication. But for simple servers which only speak HTTP/2, it is easier for developers if nghttp2 library takes care of client connection preface. If this option is used with nonzero val, nghttp2 library checks first 24 bytes client connection preface. If it is not a valid one, nghttp2_session_recv() and nghttp2_session_mem_recv() will return error NGHTTP2_ERR_BAD_PREFACE, which is fatal error. --- lib/includes/nghttp2/nghttp2.h | 33 ++++++++++++++++++++++- lib/nghttp2_option.c | 7 +++++ lib/nghttp2_option.h | 7 ++++- lib/nghttp2_session.c | 32 ++++++++++++++++++++++ lib/nghttp2_session.h | 2 ++ src/HttpServer.cc | 41 +++++----------------------- src/HttpServer.h | 4 +-- tests/main.c | 2 ++ tests/nghttp2_session_test.c | 49 ++++++++++++++++++++++++++++++++++ tests/nghttp2_session_test.h | 1 + 10 files changed, 139 insertions(+), 39 deletions(-) diff --git a/lib/includes/nghttp2/nghttp2.h b/lib/includes/nghttp2/nghttp2.h index d3f81a30..9c580a3f 100644 --- a/lib/includes/nghttp2/nghttp2.h +++ b/lib/includes/nghttp2/nghttp2.h @@ -341,7 +341,12 @@ typedef enum { /** * The user callback function failed. This is a fatal error. */ - NGHTTP2_ERR_CALLBACK_FAILURE = -902 + NGHTTP2_ERR_CALLBACK_FAILURE = -902, + /** + * Invalid connection preface was received and further processing is + * not possible. + */ + NGHTTP2_ERR_BAD_PREFACE = -903 } nghttp2_error; /** @@ -1736,6 +1741,24 @@ void nghttp2_option_set_no_auto_window_update(nghttp2_option *option, int val); void nghttp2_option_set_peer_max_concurrent_streams(nghttp2_option *option, uint32_t val); +/** + * @function + * + * By default, nghttp2 library only handles HTTP/2 frames and does not + * recognize first 24 bytes of client connection preface. This design + * choice is done due to the fact that server may want to detect the + * application protocol based on first few bytes on clear text + * communication. But for simple servers which only speak HTTP/2, it + * is easier for developers if nghttp2 library takes care of client + * connection preface. + * + * If this option is used with nonzero |val|, nghttp2 library checks + * first 24 bytes client connection preface. If it is not a valid + * one, `nghttp2_session_recv()` and `nghttp2_session_mem_recv()` will + * return error :enum:`NGHTTP2_ERR_BAD_PREFACE`, which is fatal error. + */ +void nghttp2_option_set_recv_client_preface(nghttp2_option *option, int val); + /** * @function * @@ -1986,6 +2009,10 @@ ssize_t nghttp2_session_mem_send(nghttp2_session *session, * Out of memory. * :enum:`NGHTTP2_ERR_CALLBACK_FAILURE` * The callback function failed. + * :enum:`NGHTTP2_ERR_BAD_PREFACE` + * Invalid client preface was detected. This error only returns + * when |session| was configured as server and + * `nghttp2_option_set_recv_client_preface()` is used. */ int nghttp2_session_recv(nghttp2_session *session); @@ -2017,6 +2044,10 @@ int nghttp2_session_recv(nghttp2_session *session); * Out of memory. * :enum:`NGHTTP2_ERR_CALLBACK_FAILURE` * The callback function failed. + * :enum:`NGHTTP2_ERR_BAD_PREFACE` + * Invalid client preface was detected. This error only returns + * when |session| was configured as server and + * `nghttp2_option_set_recv_client_preface()` is used. */ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, const uint8_t *in, size_t inlen); diff --git a/lib/nghttp2_option.c b/lib/nghttp2_option.c index adb59bc0..914714a1 100644 --- a/lib/nghttp2_option.c +++ b/lib/nghttp2_option.c @@ -52,3 +52,10 @@ void nghttp2_option_set_peer_max_concurrent_streams(nghttp2_option *option, option->opt_set_mask |= NGHTTP2_OPT_PEER_MAX_CONCURRENT_STREAMS; option->peer_max_concurrent_streams = val; } + +void nghttp2_option_set_recv_client_preface +(nghttp2_option *option, int val) +{ + option->opt_set_mask |= NGHTTP2_OPT_RECV_CLIENT_PREFACE; + option->recv_client_preface = val; +} diff --git a/lib/nghttp2_option.h b/lib/nghttp2_option.h index d5bad011..aefd87be 100644 --- a/lib/nghttp2_option.h +++ b/lib/nghttp2_option.h @@ -56,7 +56,8 @@ typedef enum { * will be overwritten if the local endpoint receives * SETTINGS_MAX_CONCURRENT_STREAMS from the remote endpoint. */ - NGHTTP2_OPT_PEER_MAX_CONCURRENT_STREAMS = 1 << 1 + NGHTTP2_OPT_PEER_MAX_CONCURRENT_STREAMS = 1 << 1, + NGHTTP2_OPT_RECV_CLIENT_PREFACE = 1 << 2, } nghttp2_option_flag; /** @@ -76,6 +77,10 @@ struct nghttp2_option { * NGHTTP2_OPT_NO_AUTO_WINDOW_UPDATE */ uint8_t no_auto_window_update; + /** + * NGHTTP2_OPT_RECV_CLIENT_PREFACE + */ + uint8_t recv_client_preface; }; #endif /* NGHTTP2_OPTION_H */ diff --git a/lib/nghttp2_session.c b/lib/nghttp2_session.c index e0711281..d156800a 100644 --- a/lib/nghttp2_session.c +++ b/lib/nghttp2_session.c @@ -398,6 +398,12 @@ static int session_new(nghttp2_session **session_ptr, option->peer_max_concurrent_streams; } + + if(option->opt_set_mask & NGHTTP2_OPT_RECV_CLIENT_PREFACE) { + + (*session_ptr)->opt_flags |= NGHTTP2_OPTMASK_RECV_CLIENT_PREFACE; + + } } (*session_ptr)->callbacks = *callbacks; @@ -405,6 +411,15 @@ static int session_new(nghttp2_session **session_ptr, session_inbound_frame_reset(*session_ptr); + if(server && + ((*session_ptr)->opt_flags & NGHTTP2_OPTMASK_RECV_CLIENT_PREFACE)) { + + nghttp2_inbound_frame *iframe = &(*session_ptr)->iframe; + + iframe->state = NGHTTP2_IB_READ_CLIENT_PREFACE; + iframe->payloadleft = NGHTTP2_CLIENT_CONNECTION_PREFACE_LEN; + } + return 0; fail_aob_framebuf: @@ -4338,6 +4353,23 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, for(;;) { switch(iframe->state) { + case NGHTTP2_IB_READ_CLIENT_PREFACE: + readlen = nghttp2_min(inlen, iframe->payloadleft); + + if(memcmp(NGHTTP2_CLIENT_CONNECTION_PREFACE + + NGHTTP2_CLIENT_CONNECTION_PREFACE_LEN - iframe->payloadleft, + in, readlen) != 0) { + return NGHTTP2_ERR_BAD_PREFACE; + } + + iframe->payloadleft -= readlen; + in += readlen; + + if(iframe->payloadleft == 0) { + session_inbound_frame_reset(session); + } + + break; case NGHTTP2_IB_READ_HEAD: { int on_begin_frame_called = 0; diff --git a/lib/nghttp2_session.h b/lib/nghttp2_session.h index ac60dd96..4c6943ee 100644 --- a/lib/nghttp2_session.h +++ b/lib/nghttp2_session.h @@ -45,6 +45,7 @@ */ typedef enum { NGHTTP2_OPTMASK_NO_AUTO_WINDOW_UPDATE = 1 << 0, + NGHTTP2_OPTMASK_RECV_CLIENT_PREFACE = 1 << 1, } nghttp2_optmask; typedef enum { @@ -65,6 +66,7 @@ typedef struct { /* Internal state when receiving incoming frame */ typedef enum { /* Receiving frame header */ + NGHTTP2_IB_READ_CLIENT_PREFACE, NGHTTP2_IB_READ_HEAD, NGHTTP2_IB_READ_NBYTE, NGHTTP2_IB_READ_HEADER_BLOCK, diff --git a/src/HttpServer.cc b/src/HttpServer.cc index e992b44a..b93e6744 100644 --- a/src/HttpServer.cc +++ b/src/HttpServer.cc @@ -104,6 +104,7 @@ void append_nv(Stream *stream, const std::vector& nva) Config::Config() : stream_read_timeout{60, 0}, stream_write_timeout{60, 0}, + session_option(nullptr), data_ptr(nullptr), padding(0), num_worker(1), @@ -115,7 +116,10 @@ Config::Config() no_tls(false), error_gzip(false), early_response(false) -{} +{ + nghttp2_option_new(&session_option); + nghttp2_option_set_recv_client_preface(session_option, 1); +} Stream::Stream(Http2Handler *handler, int32_t stream_id) : handler(handler), @@ -337,7 +341,6 @@ Http2Handler::Http2Handler(Sessions *sessions, settings_timerev_(nullptr), pending_data_(nullptr), pending_datalen_(0), - left_connhd_len_(NGHTTP2_CLIENT_CONNECTION_PREFACE_LEN), fd_(fd) { nghttp2_buf_wrap_init(&sendbuf_, sendbufarray_, sizeof(sendbufarray_)); @@ -651,27 +654,6 @@ int Http2Handler::on_read() nread = rv; bufp = buf; - if(left_connhd_len_ > 0) { - auto len = std::min(left_connhd_len_, nread); - const char *conhead = NGHTTP2_CLIENT_CONNECTION_PREFACE; - - if(memcmp(conhead + NGHTTP2_CLIENT_CONNECTION_PREFACE_LEN - - left_connhd_len_, bufp, len) != 0) { - return -1; - } - - left_connhd_len_ -= len; - nread -= len; - - if(nread == 0) { - wait_events(); - - return 0; - } - - bufp += len; - } - rv = nghttp2_session_mem_recv(session_, bufp, nread); if(rv < 0) { std::cerr << "nghttp2_session_mem_recv() returned error: " @@ -736,7 +718,8 @@ int Http2Handler::on_connect() { int r; - r = nghttp2_session_server_new(&session_, sessions_->get_callbacks(), this); + r = nghttp2_session_server_new2(&session_, sessions_->get_callbacks(), this, + sessions_->get_config()->session_option); if(r != 0) { return r; } @@ -947,16 +930,6 @@ const Config* Http2Handler::get_config() const return sessions_->get_config(); } -size_t Http2Handler::get_left_connhd_len() const -{ - return left_connhd_len_; -} - -void Http2Handler::set_left_connhd_len(size_t left) -{ - left_connhd_len_ = left; -} - void Http2Handler::remove_settings_timer() { if(settings_timerev_) { diff --git a/src/HttpServer.h b/src/HttpServer.h index d7e3fa70..09b63975 100644 --- a/src/HttpServer.h +++ b/src/HttpServer.h @@ -66,6 +66,7 @@ struct Config { std::string dh_param_file; timeval stream_read_timeout; timeval stream_write_timeout; + nghttp2_option *session_option; void *data_ptr; size_t padding; size_t num_worker; @@ -138,8 +139,6 @@ public: int64_t session_id() const; Sessions* get_sessions() const; const Config* get_config() const; - size_t get_left_connhd_len() const; - void set_left_connhd_len(size_t left); void remove_settings_timer(); void terminate_session(uint32_t error_code); int tls_handshake(); @@ -159,7 +158,6 @@ private: event *settings_timerev_; const uint8_t *pending_data_; size_t pending_datalen_; - size_t left_connhd_len_; int fd_; uint8_t sendbufarray_[65536]; }; diff --git a/tests/main.c b/tests/main.c index 4338ec04..1c8f3e6f 100644 --- a/tests/main.c +++ b/tests/main.c @@ -243,6 +243,8 @@ int main(int argc, char* argv[]) test_nghttp2_session_graceful_shutdown) || !CU_add_test(pSuite, "session_on_header_temporal_failure", test_nghttp2_session_on_header_temporal_failure) || + !CU_add_test(pSuite, "session_recv_client_preface", + test_nghttp2_session_recv_client_preface) || !CU_add_test(pSuite, "frame_pack_headers", test_nghttp2_frame_pack_headers) || !CU_add_test(pSuite, "frame_pack_headers_frame_too_large", diff --git a/tests/nghttp2_session_test.c b/tests/nghttp2_session_test.c index b11930fc..9bca793c 100644 --- a/tests/nghttp2_session_test.c +++ b/tests/nghttp2_session_test.c @@ -6400,3 +6400,52 @@ void test_nghttp2_session_on_header_temporal_failure(void) nghttp2_hd_deflate_free(&deflater); nghttp2_session_del(session); } + +void test_nghttp2_session_recv_client_preface(void) +{ + nghttp2_session *session; + nghttp2_session_callbacks callbacks; + nghttp2_option *option; + ssize_t rv; + + memset(&callbacks, 0, sizeof(callbacks)); + + nghttp2_option_new(&option); + nghttp2_option_set_recv_client_preface(option, 1); + + /* Check success case */ + nghttp2_session_server_new2(&session, &callbacks, NULL, option); + + CU_ASSERT(session->opt_flags & NGHTTP2_OPTMASK_RECV_CLIENT_PREFACE); + + rv = nghttp2_session_mem_recv + (session, + (const uint8_t*)NGHTTP2_CLIENT_CONNECTION_PREFACE, + NGHTTP2_CLIENT_CONNECTION_PREFACE_LEN); + + CU_ASSERT(rv == NGHTTP2_CLIENT_CONNECTION_PREFACE_LEN); + CU_ASSERT(NGHTTP2_IB_READ_HEAD == session->iframe.state); + + nghttp2_session_del(session); + + /* Check bad case */ + nghttp2_session_server_new2(&session, &callbacks, NULL, option); + + /* Feed preface with one byte less */ + rv = nghttp2_session_mem_recv + (session, + (const uint8_t*)NGHTTP2_CLIENT_CONNECTION_PREFACE, + NGHTTP2_CLIENT_CONNECTION_PREFACE_LEN - 1); + + CU_ASSERT(rv == NGHTTP2_CLIENT_CONNECTION_PREFACE_LEN - 1); + CU_ASSERT(NGHTTP2_IB_READ_CLIENT_PREFACE == session->iframe.state); + CU_ASSERT(1 == session->iframe.payloadleft); + + rv = nghttp2_session_mem_recv(session, (const uint8_t*)"\0", 1); + + CU_ASSERT(NGHTTP2_ERR_BAD_PREFACE == rv); + + nghttp2_session_del(session); + + nghttp2_option_del(option); +} diff --git a/tests/nghttp2_session_test.h b/tests/nghttp2_session_test.h index 4b1be44b..af54d3ac 100644 --- a/tests/nghttp2_session_test.h +++ b/tests/nghttp2_session_test.h @@ -113,5 +113,6 @@ void test_nghttp2_session_stream_attach_data_subtree(void); void test_nghttp2_session_keep_closed_stream(void); void test_nghttp2_session_graceful_shutdown(void); void test_nghttp2_session_on_header_temporal_failure(void); +void test_nghttp2_session_recv_client_preface(void); #endif /* NGHTTP2_SESSION_TEST_H */