On Wed, Aug 31, 2016 at 9:57 PM, Yann Ylavic <[email protected]> wrote:
> On Wed, Aug 31, 2016 at 9:47 PM, William A Rowe Jr <[email protected]>
> wrote:
>> On Aug 31, 2016 2:31 PM, "Yann Ylavic" <[email protected]> wrote:
>>>
>>> On Wed, Aug 31, 2016 at 9:05 PM, William A Rowe Jr <[email protected]>
>>> wrote:
>>> >
>>> > One more reviewer would be appreciated to get these in sync, in
>>> > preparation
>>> > for the stronger parser on both of the release branches.
>>>
>>> My +1 on backport-2.2.x-r892678.patch, but I couldn't apply
>>> backport-2.2.x-r951900-r1178566-r1185385-r1188745-r1352911-r1433613.patch
>>> on the current 2.2.x...
>>
>> Just to clarify, these patches are necessarily cumulative
>
> I just backported the two accepted ones, still none of the two
> remaining ones applies cleanly (first)...
Attached the failing hunks for server/protocol.c (with
backport-2.2.x-r951900-r1178566-r1185385-r1188745-r1352911-r1433613.patch).
--- server/protocol.c.applied-r892678 2016-08-23 10:23:22.057250193 -0500
+++ server/protocol.c 2016-08-23 10:34:25.926298467 -0500
@@ -732,19 +757,29 @@
* finding the end-of-line. This is only going to happen if it
* exceeds the configured limit for a field size.
*/
- if (rv == APR_ENOSPC && field) {
- /* ensure ap_escape_html will terminate correctly */
- field[len - 1] = '\0';
+ if (rv == APR_ENOSPC) {
+ const char *field_escaped;
+ if (field && len) {
+ /* ensure ap_escape_html will terminate correctly */
+ field[len - 1] = '\0';
+ field_escaped = ap_escape_html(r->pool, field);
+ }
+ else {
+ field_escaped = field = "";
+ }
+
apr_table_setn(r->notes, "error-notes",
apr_psprintf(r->pool,
"Size of a request header field "
"exceeds server limit.<br />\n"
- "<pre>\n%.*s\n</pre>/n",
- field_name_len(field),
- ap_escape_html(r->pool, field)));
+ "<pre>\n%.*s\n</pre>\n",
+ field_name_len(field_escaped),
+ field_escaped));
ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r,
- "Request header exceeds LimitRequestFieldSize: "
- "%.*s", field_name_len(field), field);
+ "Request header exceeds LimitRequestFieldSize%s"
+ "%.*s",
+ *field ? ": " : "",
+ field_name_len(field), field);
}
return;
}
@@ -760,18 +795,21 @@
apr_size_t fold_len = last_len + len + 1; /* trailing null */
if (fold_len >= (apr_size_t)(r->server->limit_req_fieldsize)) {
+ const char *field_escaped;
+
r->status = HTTP_BAD_REQUEST;
/* report what we have accumulated so far before the
* overflow (last_field) as the field with the problem
*/
+ field_escaped = ap_escape_html(r->pool, last_field);
apr_table_setn(r->notes, "error-notes",
apr_psprintf(r->pool,
"Size of a request header field "
"after folding "
"exceeds server limit.<br />\n"
"<pre>\n%.*s\n</pre>\n",
- field_name_len(last_field),
- ap_escape_html(r->pool, last_field)));
+ field_name_len(field_escaped),
+ field_escaped));
ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r,
"Request header exceeds LimitRequestFieldSize "
"after folding: %.*s",
@@ -801,6 +839,9 @@
apr_table_setn(r->notes, "error-notes",
"The number of request header fields "
"exceeds this server's limit.");
+ ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r,
+ "Number of request headers exceeds "
+ "LimitRequestFields");
return;
}
@@ -814,7 +855,7 @@
(int)LOG_NAME_MAX_LEN,
ap_escape_html(r->pool,
last_field)));
- ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
+ ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r,
"Request header field is missing ':' "
"separator: %.*s", (int)LOG_NAME_MAX_LEN,
last_field);
@@ -874,6 +915,9 @@
* field-name, following RFC 2616, 4.2.
*/
apr_table_compress(r->headers_in, APR_OVERLAP_TABLES_MERGE);
+
+ /* enforce LimitRequestFieldSize for merged headers */
+ apr_table_do(table_do_fn_check_lengths, r, r->headers_in, NULL);
}
AP_DECLARE(void) ap_get_mime_headers(request_rec *r)
@@ -943,13 +987,14 @@
if (!read_request_line(r, tmp_bb)) {
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");
+ if (r->status == HTTP_REQUEST_URI_TOO_LARGE) {
+ ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r,
+ "request failed: client's request-line exceeds LimitRequestLine (longer than %d)",
+ r->server->limit_req_line);
}
- else {
- ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
- "request failed: URI too long (longer than %d)", r->server->limit_req_line);
+ else if (r->method == NULL) {
+ ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r,
+ "request failed: invalid characters in URI");
}
ap_send_error_response(r, 0);
ap_update_child_status(conn->sbh, SERVER_BUSY_LOG, r);
@@ -986,7 +1031,7 @@
ap_get_mime_headers_core(r, tmp_bb);
if (r->status != HTTP_OK) {
- ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
+ ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r,
"request failed: error reading the headers");
ap_send_error_response(r, 0);
ap_update_child_status(conn->sbh, SERVER_BUSY_LOG, r);
@@ -1033,7 +1078,7 @@
* headers! Have to dink things just to make sure the error message
* comes through...
*/
- ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
+ ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r,
"client sent invalid HTTP/0.9 request: HEAD %s",
r->uri);
r->header_only = 0;
@@ -1075,7 +1120,7 @@
* a Host: header, and the server MUST respond with 400 if it doesn't.
*/
r->status = HTTP_BAD_REQUEST;
- ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
+ ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r,
"client sent HTTP/1.1 request without hostname "
"(see RFC2616 section 14.23): %s", r->uri);
}
@@ -1297,7 +1342,7 @@
if (strcasecmp(ap_getword(r->pool, &auth_line, ' '), "Basic")) {
/* Client tried to authenticate using wrong auth scheme */
- ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
+ ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r,
"client used wrong authentication scheme: %s", r->uri);
ap_note_basic_auth_failure(r);
return HTTP_UNAUTHORIZED;
