From cbef6fd0c67056419d0d97faf863512c1bd118a3 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Wed, 11 Sep 2013 23:24:32 +0900 Subject: [PATCH] nghttpx: Replace \r\n with space when constructing HTTP/1 headers --- src/http2.cc | 18 ++++++++++++++++++ src/http2.h | 6 ++++++ src/http2_test.cc | 20 ++++++++++++++++++++ src/http2_test.h | 1 + src/shrpx-unittest.cc | 2 ++ src/shrpx_http2_upstream.cc | 6 +++++- src/shrpx_http_downstream_connection.cc | 14 +++++++++----- src/shrpx_https_upstream.cc | 2 ++ src/shrpx_spdy_upstream.cc | 6 +++++- 9 files changed, 68 insertions(+), 7 deletions(-) diff --git a/src/http2.cc b/src/http2.cc index e94cfc31..3ca642fc 100644 --- a/src/http2.cc +++ b/src/http2.cc @@ -90,6 +90,22 @@ void capitalize(std::string& s, size_t offset) } } +bool check_header_value(const char *value) +{ + return strpbrk(value, "\r\n") == nullptr; +} + +bool check_header_value(const nghttp2_nv* nv) +{ + size_t i; + for(i = 0; i < nv->valuelen; ++i) { + if(nv->value[i] == '\r' || nv->value[i] == '\n') { + return false; + } + } + return true; +} + void sanitize_header_value(std::string& s, size_t offset) { for(size_t i = offset, eoi = s.size(); i < eoi; ++i) { @@ -290,6 +306,7 @@ void build_http1_headers_from_norm_headers capitalize(hdrs, hdrs.size()-headers[i].first.size()); hdrs += ": "; hdrs += headers[i].second; + sanitize_header_value(hdrs, hdrs.size() - headers[i].second.size()); hdrs += "\r\n"; ++i; } else if(rv > 0) { @@ -303,6 +320,7 @@ void build_http1_headers_from_norm_headers capitalize(hdrs, hdrs.size()-headers[i].first.size()); hdrs += ": "; hdrs += headers[i].second; + sanitize_header_value(hdrs, hdrs.size() - headers[i].second.size()); hdrs += "\r\n"; } } diff --git a/src/http2.h b/src/http2.h index 77d8de68..5c4acb72 100644 --- a/src/http2.h +++ b/src/http2.h @@ -42,6 +42,12 @@ const char* get_status_string(int status_code); void capitalize(std::string& s, size_t offset); +// Returns false if |value| contains \r or \n. +bool check_header_value(const char *value); + +// Returns false if |nv->value| contains \r or \n. +bool check_header_value(const nghttp2_nv *nv); + void sanitize_header_value(std::string& s, size_t offset); // Copies the |field| component value from |u| and |url| to the diff --git a/src/http2_test.cc b/src/http2_test.cc index e3cf7682..3f24d950 100644 --- a/src/http2_test.cc +++ b/src/http2_test.cc @@ -162,6 +162,26 @@ void test_http2_build_http1_headers_from_norm_headers(void) "Te: 8\r\n" "Te: 9\r\n" "Zulu: 12\r\n"); + + hdrs.clear(); + auto hd2 = std::vector> + {{"alpha", "bravo\r\ncharlie\r\n"}}; + http2::build_http1_headers_from_norm_headers(hdrs, hd2); + CU_ASSERT(hdrs == "Alpha: bravo charlie \r\n"); +} + +void test_http2_check_header_value(void) +{ + CU_ASSERT(http2::check_header_value("alpha")); + CU_ASSERT(!http2::check_header_value("alpha\r")); + CU_ASSERT(!http2::check_header_value("alpha\n")); + + nghttp2_nv nv1 = MAKE_NV("alpha", "bravo"); + CU_ASSERT(http2::check_header_value(&nv1)); + nghttp2_nv nv2 = MAKE_NV("alpha", "bravo\r"); + CU_ASSERT(!http2::check_header_value(&nv2)); + nghttp2_nv nv3 = MAKE_NV("alpha", "bravo\n"); + CU_ASSERT(!http2::check_header_value(&nv3)); } } // namespace shrpx diff --git a/src/http2_test.h b/src/http2_test.h index 488b923a..34cc5560 100644 --- a/src/http2_test.h +++ b/src/http2_test.h @@ -33,6 +33,7 @@ void test_http2_get_header(void); void test_http2_value_lws(void); void test_http2_copy_norm_headers_to_nv(void); void test_http2_build_http1_headers_from_norm_headers(void); +void test_http2_check_header_value(void); } // namespace shrpx diff --git a/src/shrpx-unittest.cc b/src/shrpx-unittest.cc index 74b90387..327875bc 100644 --- a/src/shrpx-unittest.cc +++ b/src/shrpx-unittest.cc @@ -81,6 +81,8 @@ int main(int argc, char* argv[]) shrpx::test_http2_copy_norm_headers_to_nv) || !CU_add_test(pSuite, "http2_build_http1_headers_from_norm_headers", shrpx::test_http2_build_http1_headers_from_norm_headers) || + !CU_add_test(pSuite, "http2_check_header_value", + shrpx::test_http2_check_header_value) || !CU_add_test(pSuite, "downstream_normalize_request_headers", shrpx::test_downstream_normalize_request_headers) || !CU_add_test(pSuite, "downstream_normalize_response_headers", diff --git a/src/shrpx_http2_upstream.cc b/src/shrpx_http2_upstream.cc index 5eafdf10..3026f135 100644 --- a/src/shrpx_http2_upstream.cc +++ b/src/shrpx_http2_upstream.cc @@ -231,7 +231,11 @@ int on_frame_recv_callback if(!host || !path || !method || http2::value_lws(host) || http2::value_lws(path) || http2::value_lws(method) || - (!is_connect && (!scheme || http2::value_lws(scheme)))) { + (!is_connect && (!scheme || http2::value_lws(scheme))) || + !http2::check_header_value(host) || + !http2::check_header_value(path) || + !http2::check_header_value(method) || + (scheme && !http2::check_header_value(scheme))) { upstream->rst_stream(downstream, NGHTTP2_PROTOCOL_ERROR); return 0; } diff --git a/src/shrpx_http_downstream_connection.cc b/src/shrpx_http_downstream_connection.cc index 3d997d2d..d624026b 100644 --- a/src/shrpx_http_downstream_connection.cc +++ b/src/shrpx_http_downstream_connection.cc @@ -114,11 +114,11 @@ int HttpDownstreamConnection::attach_downstream(Downstream *downstream) int HttpDownstreamConnection::push_request_headers() { + // Assume that method and request path do not contain \r\n. std::string hdrs = downstream_->get_request_method(); hdrs += " "; hdrs += downstream_->get_request_path(); - hdrs += " "; - hdrs += "HTTP/1.1\r\n"; + hdrs += " HTTP/1.1\r\n"; downstream_->normalize_request_headers(); auto end_headers = std::end(downstream_->get_request_headers()); http2::build_http1_headers_from_norm_headers @@ -132,6 +132,7 @@ int HttpDownstreamConnection::push_request_headers() hdrs += "X-Forwarded-For: "; if(xff != end_headers) { hdrs += (*xff).second; + http2::sanitize_header_value(hdrs, hdrs.size() - (*xff).second.size()); hdrs += ", "; } hdrs += downstream_->get_upstream()->get_client_handler()->get_ipaddr(); @@ -139,22 +140,23 @@ int HttpDownstreamConnection::push_request_headers() } else if(xff != end_headers) { hdrs += "X-Forwarded-For: "; hdrs += (*xff).second; + http2::sanitize_header_value(hdrs, hdrs.size() - (*xff).second.size()); hdrs += "\r\n"; } if(downstream_->get_request_method() != "CONNECT") { hdrs += "X-Forwarded-Proto: "; if(util::istartsWith(downstream_->get_request_path(), "http:")) { - hdrs += "http"; + hdrs += "http\r\n"; } else { - hdrs += "https"; + hdrs += "https\r\n"; } - hdrs += "\r\n"; } auto expect = downstream_->get_norm_request_header("expect"); if(expect != end_headers && !util::strifind((*expect).second.c_str(), "100-continue")) { hdrs += "Expect: "; hdrs += (*expect).second; + http2::sanitize_header_value(hdrs, hdrs.size() - (*expect).second.size()); hdrs += "\r\n"; } auto via = downstream_->get_norm_request_header("via"); @@ -162,12 +164,14 @@ int HttpDownstreamConnection::push_request_headers() if(via != end_headers) { hdrs += "Via: "; hdrs += (*via).second; + http2::sanitize_header_value(hdrs, hdrs.size() - (*via).second.size()); hdrs += "\r\n"; } } else { hdrs += "Via: "; if(via != end_headers) { hdrs += (*via).second; + http2::sanitize_header_value(hdrs, hdrs.size() - (*via).second.size()); hdrs += ", "; } hdrs += http::create_via_header_value(downstream_->get_request_major(), diff --git a/src/shrpx_https_upstream.cc b/src/shrpx_https_upstream.cc index 3fd1dc05..c7b6b8cc 100644 --- a/src/shrpx_https_upstream.cc +++ b/src/shrpx_https_upstream.cc @@ -684,12 +684,14 @@ int HttpsUpstream::on_downstream_header_complete(Downstream *downstream) if(via != end_headers) { hdrs += "Via: "; hdrs += (*via).second; + http2::sanitize_header_value(hdrs, hdrs.size() - (*via).second.size()); hdrs += "\r\n"; } } else { hdrs += "Via: "; if(via != end_headers) { hdrs += (*via).second; + http2::sanitize_header_value(hdrs, hdrs.size() - (*via).second.size()); hdrs += ", "; } hdrs += http::create_via_header_value diff --git a/src/shrpx_spdy_upstream.cc b/src/shrpx_spdy_upstream.cc index db0b7066..03189948 100644 --- a/src/shrpx_spdy_upstream.cc +++ b/src/shrpx_spdy_upstream.cc @@ -180,7 +180,11 @@ void on_ctrl_recv_callback downstream->add_request_header(nv[i], nv[i+1]); } } - if(!path || !host || !method) { + if(!path || !host || !method || + !http2::check_header_value(host) || + !http2::check_header_value(path) || + !http2::check_header_value(method) || + (scheme && !http2::check_header_value(scheme))) { upstream->rst_stream(downstream, SPDYLAY_INTERNAL_ERROR); return; }