Merge r892678, r1100511, r1102124 from trunk:

Reject requests containing (invalid) NULL characters in request line
or request headers.
PR 43039

use APR_STATUS_IS_TIMEUP() instead of direct comparison with APR_TIMEUP.

Use APR_STATUS_IS_... in some more cases.

While this is not strictly necessary everywhere, it makes it much easier
to find the problematic cases.

Submitted by: niq, covener, sf
Reviewed  by: wrowe, covener, ylavic

git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/branches/2.2.x@1758671 13f79535-47bb-0310-9956-ffa450edef68

diff --git a/CHANGES b/CHANGES
index 5154da648346c11c16c63b12a77f1870683c0f7e..07c02d03d601eaea427790757927166896ed06c0 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -4,6 +4,9 @@ Changes with Apache 2.2.32
   *) core: CVE-2016-5387: Mitigate [f]cgi "httpoxy" issues.
      [Dominic Scheirlinck <dominic vendhq.com>, Yann Ylavic]
 
+  *) Core: reject NULLs in request line or request headers.
+     PR 43039 [Nick Kew]
+
   *) mod_ssl: Fix a possible memory leak on restart for custom [EC]DH params.
      [Jan Kaluza, Yann Ylavic]
 

diff --git a/STATUS b/STATUS
index f60d7facaf3dc41fa64ac4e7ef8831098558d6d3..3fc37d54a100728ddbaf761df32e550199bee994 100644 (file)
--- a/STATUS
+++ b/STATUS
@@ -103,19 +103,6 @@ RELEASE SHOWSTOPPERS:
 PATCHES ACCEPTED TO BACKPORT FROM TRUNK:
   [ start all new proposals below, under PATCHES PROPOSED. ]
 
-  *) core: Reject requests containing (invalid) NULL characters in request line
-     or request headers. (Including embedded %00 in URL).
-     (Use APR_STATUS_IS_... in some more cases.)
-     Trunk version of patch
-         http://svn.apache.org/r892678
-         http://svn.apache.org/r1100511
-         http://svn.apache.org/r1102124
-     Backport: (trunk works as well)
-         https://raw.githubusercontent.com/wrowe/patches/master/backport-2.2.x-r892678.patch
-     Submitted by niq, status legibility fixes by covener, sf
-     PR: 43039
-     +1: wrowe, covener, ylavic
-
   *) core: Limit to ten the number of tolerated empty lines between request.
      Before this commit, the maximum number of empty lines was the same as
      configured LimitRequestFields, defaulting to 100, which was way too much.

diff --git a/server/protocol.c b/server/protocol.c
index e9611a142a0d321e96ed1eeb03c3bea31987cbbb..c3a034e504d73c2728f4dde7fc131206cf3161b1 100644 (file)
--- a/server/protocol.c
+++ b/server/protocol.c
@@ -433,8 +433,13 @@ AP_DECLARE(apr_status_t) ap_rgetline_core(char **s, apr_size_t n,
             }
         }
     }
-
     *read = bytes_handled;
+
+    /* PR#43039: We shouldn't accept NULL bytes within the line */
+    if (strlen(*s) < bytes_handled - 1) {
+        return APR_EINVAL;
+    }
+
     return APR_SUCCESS;
 }
 
@@ -597,12 +602,15 @@ static int read_request_line(request_rec *r, apr_bucket_brigade *bb)
              * buffer before finding the end-of-line.  This is only going to
              * happen if it exceeds the configured limit for a request-line.
              */
-            if (rv == APR_ENOSPC) {
+            if (APR_STATUS_IS_ENOSPC(rv)) {
                 r->status    = HTTP_REQUEST_URI_TOO_LARGE;
             }
             else if (APR_STATUS_IS_TIMEUP(rv)) {
                 r->status = HTTP_REQUEST_TIME_OUT;
             }
+            else if (APR_STATUS_IS_EINVAL(rv)) {
+                r->status = HTTP_BAD_REQUEST;
+            }
             r->proto_num = HTTP_VERSION(1,0);
             r->protocol  = apr_pstrdup(r->pool, "HTTP/1.0");
             return 0;
@@ -908,9 +916,16 @@ request_rec *ap_read_request(conn_rec *conn)
 
     /* Get the request... */
     if (!read_request_line(r, tmp_bb)) {
-        if (r->status == HTTP_REQUEST_URI_TOO_LARGE) {
-            ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
-                          "request failed: URI too long (longer than %d)", r->server->limit_req_line);
+        if (r->status == HTTP_REQUEST_URI_TOO_LARGE
+            || r->status == HTTP_BAD_REQUEST) {
+            if (r->status == HTTP_BAD_REQUEST) {
+                ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
+                              "request failed: invalid characters in URI");
+            }
+            else {
+                ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
+                              "request failed: URI too long (longer than %d)", r->server->limit_req_line);
+            }
             ap_send_error_response(r, 0);
             ap_update_child_status(conn->sbh, SERVER_BUSY_LOG, r);
             ap_run_log_transaction(r);