*) mod_http2: :scheme pseudo-header values, not matching the

     connection scheme, are forwarded via absolute uris to the
     http protocol processing to preserve semantics of the request.
     Checks on combinations of pseudo-headers values/absence
     have been added as described in RFC 7540.
     Fixes <https://github.com/icing/mod_h2/issues/230>.

git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1897940 13f79535-47bb-0310-9956-ffa450edef68

diff --git a/changes-entries/http2_request_scheme.txt b/changes-entries/http2_request_scheme.txt
index 93a6d6348b3362c2def03b31441f9d04c5000c21..36744decdaaf56b91e3d949fd6535410237414e2 100644 (file)
--- a/changes-entries/http2_request_scheme.txt
+++ b/changes-entries/http2_request_scheme.txt
@@ -1,3 +1,7 @@
-  *) mod_http2: when a h2 request carries a ':scheme' pseudoheader,
-    it gives a 400 response if the scheme does not match the
-    connection. Fixes <https://github.com/icing/mod_h2/issues/230>.
+  *) mod_http2: :scheme pseudo-header values, not matching the
+     connection scheme, are forwarded via absolute uris to the
+     http protocol processing to preserve semantics of the request.
+     Checks on combinations of pseudo-headers values/absence
+     have been added as described in RFC 7540.
+     Fixes <https://github.com/icing/mod_h2/issues/230>.
+     [Stefan Eissing]

diff --git a/modules/http2/h2_c2.c b/modules/http2/h2_c2.c
index 4e689c7c11c56275e3042392dfa83c84ef6773de..00a783d28e7a895ef95aaf5f7c6ed9a481197733 100644 (file)
--- a/modules/http2/h2_c2.c
+++ b/modules/http2/h2_c2.c
@@ -574,8 +574,8 @@ static apr_status_t c2_process(h2_conn_ctx_t *conn_ctx, conn_rec *c)
     }
 
     ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, c,
-                  "h2_c2(%s-%d): created request_rec",
-                  conn_ctx->id, conn_ctx->stream_id);
+                  "h2_c2(%s-%d): created request_rec for %s",
+                  conn_ctx->id, conn_ctx->stream_id, r->the_request);
     conn_ctx->server = r->server;
 
     /* the request_rec->server carries the timeout value that applies */

diff --git a/modules/http2/h2_request.c b/modules/http2/h2_request.c
index 54721c45af163099f5b987250cb660ccc3f6e3ca..e759c2e4558a11b52543b4029c116c9a8e295e9f 100644 (file)
--- a/modules/http2/h2_request.c
+++ b/modules/http2/h2_request.c
@@ -16,7 +16,11 @@
  
 #include <assert.h>
 
-#include <apr_strings.h>
+#include "apr.h"
+#include "apr_strings.h"
+#include "apr_lib.h"
+#include "apr_strmatch.h"
+
 #include <ap_mmn.h>
 
 #include <httpd.h>
@@ -25,6 +29,7 @@
 #include <http_protocol.h>
 #include <http_request.h>
 #include <http_log.h>
+#include <http_ssl.h>
 #include <http_vhost.h>
 #include <util_filter.h>
 #include <ap_mpm.h>
@@ -170,7 +175,7 @@ apr_status_t h2_request_add_header(h2_request *req, apr_pool_t *pool,
 apr_status_t h2_request_end_headers(h2_request *req, apr_pool_t *pool, int eos, size_t raw_bytes)
 {
     const char *s;
-    
+
     /* rfc7540, ch. 8.1.2.3:
      * - if we have :authority, it overrides any Host header 
      * - :authority MUST be omitted when converting h1->h2, so we
@@ -295,9 +300,30 @@ request_rec *h2_create_request_rec(const h2_request *req, conn_rec *c)
 
     /* Time to populate r with the data we have. */
     r->request_time = req->request_time;
-    r->the_request = apr_psprintf(r->pool, "%s %s HTTP/2.0",
-                                  req->method, req->path ? req->path : "");
-    r->headers_in = apr_table_clone(r->pool, req->headers);
+    AP_DEBUG_ASSERT(req->authority);
+    if (req->scheme && (ap_cstr_casecmp(req->scheme,
+                        ap_ssl_conn_is_ssl(c->master? c->master : c)? "https" : "http")
+                        || !ap_cstr_casecmp("CONNECT", req->method))) {
+        /* Client sent a non-matching ':scheme' pseudo header. Forward this
+         * via an absolute URI in the request line.
+         */
+        r->the_request = apr_psprintf(r->pool, "%s %s://%s%s HTTP/2.0",
+                                      req->method, req->scheme, req->authority,
+                                      req->path ? req->path : "");
+    }
+    else if (req->path) {
+        r->the_request = apr_psprintf(r->pool, "%s %s HTTP/2.0",
+                                      req->method, req->path);
+    }
+    else {
+        /* We should only come here on a request that is errored already.
+         * create a request line that passes parsing, we'll die anyway.
+         */
+        AP_DEBUG_ASSERT(req->http_status != H2_HTTP_STATUS_UNSET);
+        r->the_request = apr_psprintf(r->pool, "%s / HTTP/2.0", req->method);
+    }
+
+    r->headers_in = apr_table_copy(r->pool, req->headers);
 
     /* Start with r->hostname = NULL, ap_check_request_header() will get it
      * form Host: header, otherwise we get complains about port numbers.
@@ -399,6 +425,8 @@ request_rec *h2_create_request_rec(const h2_request *req, conn_rec *c)
     return r;
 
 die:
+    ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, c,
+                  "ap_die(%d) for %s", access_status, r->the_request);
     ap_die(access_status, r);
 
     /* ap_die() sent the response through the output filters, we must now

diff --git a/modules/http2/h2_stream.c b/modules/http2/h2_stream.c
index 725f6a7bb47bbdb348c5528f7610c50975010bc5..406465112bfe288aca13853e0f6d883731e368c0 100644 (file)
--- a/modules/http2/h2_stream.c
+++ b/modules/http2/h2_stream.c
@@ -17,7 +17,10 @@
 #include <assert.h>
 #include <stddef.h>
 
-#include <apr_strings.h>
+#include "apr.h"
+#include "apr_strings.h"
+#include "apr_lib.h"
+#include "apr_strmatch.h"
 
 #include <httpd.h>
 #include <http_core.h>
@@ -770,33 +773,97 @@ apr_status_t h2_stream_end_headers(h2_stream *stream, int eos, size_t raw_bytes)
 {
     apr_status_t status;
     val_len_check_ctx ctx;
-    
-    status = h2_request_end_headers(stream->rtmp, stream->pool, eos, raw_bytes);
-    if (APR_SUCCESS == status) {
-        set_policy_for(stream, stream->rtmp);
+    int is_http_or_https;
+    h2_request *req = stream->rtmp;
 
-        ctx.maxlen = stream->session->s->limit_req_fieldsize;
-        ctx.failed_key = NULL;
-        apr_table_do(table_check_val_len, &ctx, stream->rtmp->headers, NULL);
-        if (ctx.failed_key) {
-            if (!h2_stream_is_ready(stream)) {
-                ap_log_cerror(APLOG_MARK, APLOG_INFO, 0, stream->session->c1,
-                              H2_STRM_LOG(APLOGNO(10230), stream,"Request header exceeds "
-                                          "LimitRequestFieldSize: %.*s"),
-                              (int)H2MIN(strlen(ctx.failed_key), 80), ctx.failed_key);
-            }
-            set_error_response(stream, HTTP_REQUEST_HEADER_FIELDS_TOO_LARGE);
-            /* keep on returning APR_SUCCESS, so that we send a HTTP response and
-             * do not RST the stream. */
+    status = h2_request_end_headers(req, stream->pool, eos, raw_bytes);
+    if (APR_SUCCESS != status || req->http_status != H2_HTTP_STATUS_UNSET) goto cleanup;
+
+    /* keep on returning APR_SUCCESS for error responses, so that we
+     * send it and do not RST the stream.
+     */
+    set_policy_for(stream, req);
+
+    ctx.maxlen = stream->session->s->limit_req_fieldsize;
+    ctx.failed_key = NULL;
+    apr_table_do(table_check_val_len, &ctx, req->headers, NULL);
+    if (ctx.failed_key) {
+        if (!h2_stream_is_ready(stream)) {
+            ap_log_cerror(APLOG_MARK, APLOG_INFO, 0, stream->session->c1,
+                          H2_STRM_LOG(APLOGNO(10230), stream,"Request header exceeds "
+                                      "LimitRequestFieldSize: %.*s"),
+                          (int)H2MIN(strlen(ctx.failed_key), 80), ctx.failed_key);
         }
-        if (stream->rtmp->scheme && strcasecmp(stream->rtmp->scheme,
-            ap_ssl_conn_is_ssl(stream->session->c1)? "https" : "http")) {
-                ap_log_cerror(APLOG_MARK, APLOG_INFO, 0, stream->session->c1,
-                              H2_STRM_LOG(APLOGNO(10379), stream,"Request :scheme '%s' and "
-                              "connection do not match."), stream->rtmp->scheme);
+        set_error_response(stream, HTTP_REQUEST_HEADER_FIELDS_TOO_LARGE);
+        goto cleanup;
+    }
+
+    /* http(s) scheme. rfc7540, ch. 8.1.2.3:
+     * This [:path] pseudo-header field MUST NOT be empty for "http" or "https"
+     * URIs; "http" or "https" URIs that do not contain a path component
+     * MUST include a value of '/'.  The exception to this rule is an
+     * OPTIONS request for an "http" or "https" URI that does not include
+     * a path component; these MUST include a ":path" pseudo-header field
+     * with a value of '*'
+     *
+     * All HTTP/2 requests MUST include exactly one valid value for the
+     * ":method", ":scheme", and ":path" pseudo-header fields, unless it is
+     * a CONNECT request.
+     */
+    is_http_or_https = (!req->scheme
+            || !(ap_cstr_casecmpn(req->scheme, "http", 4) != 0
+                 || (req->scheme[4] != '\0'
+                     && (apr_tolower(req->scheme[4]) != 's'
+                         || req->scheme[5] != '\0'))));
+
+    /* CONNECT. rfc7540, ch. 8.3:
+     * In HTTP/2, the CONNECT method is used to establish a tunnel over a
+     * single HTTP/2 stream to a remote host for similar purposes.  The HTTP
+     * header field mapping works as defined in Section 8.1.2.3 ("Request
+     * Pseudo-Header Fields"), with a few differences.  Specifically:
+     *   o  The ":method" pseudo-header field is set to "CONNECT".
+     *   o  The ":scheme" and ":path" pseudo-header fields MUST be omitted.
+     *   o  The ":authority" pseudo-header field contains the host and port to
+     *      connect to (equivalent to the authority-form of the request-target
+     *      of CONNECT requests (see [RFC7230], Section 5.3)).
+     */
+    if (!ap_cstr_casecmp(req->method, "CONNECT")) {
+        if (req->scheme || req->path) {
+            ap_log_cerror(APLOG_MARK, APLOG_INFO, 0, stream->session->c1,
+                          H2_STRM_LOG(APLOGNO(), stream, "Request to CONNECT "
+                          "with :scheme or :path specified, sending 400 answer"));
             set_error_response(stream, HTTP_BAD_REQUEST);
+            goto cleanup;
         }
-        stream->request = stream->rtmp;
+    }
+    else if (is_http_or_https) {
+        if (!req->path) {
+            ap_log_cerror(APLOG_MARK, APLOG_INFO, 0, stream->session->c1,
+                          H2_STRM_LOG(APLOGNO(), stream, "Request for http(s) "
+                          "resource without :path, sending 400 answer"));
+            set_error_response(stream, HTTP_BAD_REQUEST);
+            goto cleanup;
+        }
+        if (!req->scheme) {
+            req->scheme = ap_ssl_conn_is_ssl(stream->session->c1)? "https" : "http";
+        }
+    }
+
+    if (req->scheme && (req->path && req->path[0] != '/')) {
+        /* We still have a scheme, which means we need to pass an absolute URI into
+         * our HTTP protocol handling and the missing '/' at the start will prevent
+         * us from doing so (as it then confuses path and authority). */
+        ap_log_cerror(APLOG_MARK, APLOG_INFO, 0, stream->session->c1,
+                      H2_STRM_LOG(APLOGNO(10379), stream, "Request :scheme '%s' and "
+                      "path '%s' do not allow creating an absolute URL. Failing "
+                      "request with 400."), req->scheme, req->path);
+        set_error_response(stream, HTTP_BAD_REQUEST);
+        goto cleanup;
+    }
+
+cleanup:
+    if (APR_SUCCESS == status) {
+        stream->request = req;
         stream->rtmp = NULL;
     }
     return status;
@@ -1108,6 +1175,7 @@ const h2_priority *h2_stream_get_priority(h2_stream *stream,
 
 int h2_stream_is_ready(h2_stream *stream)
 {
+    /* Have we sent a response or do we have the response in our buffer? */
     if (stream->response) {
         return 1;
     }

diff --git a/test/modules/http2/test_003_get.py b/test/modules/http2/test_003_get.py
index 09ea31167de8e9898fb9def0aba490720e5d75d8..ccef11fbdeb2bbe405ca7120a54a90cc95658362 100644 (file)
--- a/test/modules/http2/test_003_get.py
+++ b/test/modules/http2/test_003_get.py
@@ -215,7 +215,15 @@ content-type: text/html
     # use an invalid scheme
     def test_h2_003_51(self, env):
         url = env.mkurl("https", "cgi", "/")
-        opt = ["-H:scheme: http"]
+        opt = ["-H:scheme: invalid"]
         r = env.nghttp().get(url, options=opt)
         assert r.exit_code == 0, r
         assert r.response['status'] == 400
+
+    # use an differing scheme, but one that is acceptable
+    def test_h2_003_52(self, env):
+        url = env.mkurl("https", "cgi", "/")
+        opt = ["-H:scheme: http"]
+        r = env.nghttp().get(url, options=opt)
+        assert r.exit_code == 0, r
+        assert r.response['status'] == 200