Re: svn commit: r1769669 [2/2] - in /httpd/httpd/branches/2.4.x-merge-http-strict: ./ docs/manual/ docs/manual/mod/ include/ server/

[Eric Covener]

I think the log severity changes below could use some eyes, especially
in context of 2.2.  Are these lowered because they're redundant?  I
haven't yet looked.

I am tempted to leave the old severities for 2.2 and wait and see if
it's confusing in 2.4 (should not have to enable DEBUG to see the
cause of a 400 error)



> @@ -937,7 +1010,7 @@ AP_DECLARE(void) ap_get_mime_headers_cor
>
>  if (last_field == NULL) {
>  r->status = HTTP_BAD_REQUEST;
> -ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(03442)
> +ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(03442)
>"Line folding encountered before first"
>" header line");
>  return;
> @@ -945,7 +1018,7 @@ AP_DECLARE(void) ap_get_mime_headers_cor
>
>  if (field[1] == '\0') {
>  r->status = HTTP_BAD_REQUEST;
> -ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(03443)
> +ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(03443)
>"Empty folded line encountered");
>  return;
>  }
> @@ -991,9 +1064,7 @@ AP_DECLARE(void) ap_get_mime_headers_cor
>  }
>  memcpy(last_field + last_len, field, len +1); /* +1 for nul */
>  /* Replace obs-fold w/ SP per RFC 7230 3.2.4 */
> -if (strict || strictspaces) {
> -last_field[last_len] = ' ';
> -}
> +last_field[last_len] = ' ';
>  last_len += len;
>
>  /* We've appended this obs-fold line to last_len, proceed to
> @@ -1024,22 +1095,9 @@ AP_DECLARE(void) ap_get_mime_headers_cor
>  {
>  /* Not Strict ('Unsafe' mode), using the legacy parser */
>
> -if (strictspaces && strpbrk(last_field, "\n\v\f\r")) {
> -r->status = HTTP_BAD_REQUEST;
> -ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, 
> APLOGNO(03451)
> -  "Request header presented bad whitespace "
> -  "(disallowed by StrictWhitespace)");
> -return;
> -}
> -else {
> -char *ll = last_field;
> -while ((ll = strpbrk(ll, "\n\v\f\r")))
> -*(ll++) = ' ';
> -}
> -
>  if (!(value = strchr(last_field, ':'))) { /* Find ':' or */
>  r->status = HTTP_BAD_REQUEST;   /* abort bad request */
> -ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, 
> APLOGNO(00564)
> +ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, 
> APLOGNO(00564)
>"Request header field is missing ':' "
>"separator: %.*s", (int)LOG_NAME_MAX_LEN,
>last_field);
> @@ -1051,11 +1109,11 @@ AP_DECLARE(void) ap_get_mime_headers_cor
>
>  *value++ = '\0'; /* NUL-terminate at colon */
>
> -if (strictspaces && strpbrk(last_field, " \t")) {
> +if (strpbrk(last_field, "\t\n\v\f\r ")) {
>  r->status = HTTP_BAD_REQUEST;
> -ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, 
> APLOGNO(03452)
> -  "Request header field name with whitespace 
> "
> -  "(disallowed by StrictWhitespace)");
> +ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, 
> APLOGNO(03452)
> +  "Request header field name presented"
> +  " invalid whitespace");
>  return;
>  }
>
> @@ -1063,15 +1121,17 @@ AP_DECLARE(void) ap_get_mime_headers_cor
>   ++value;/* Skip to start of value   */
>  }
>
> -/* Strip LWS after field-name: */
> -while (tmp_field > last_field
> -   && (*tmp_field == ' ' || *tmp_field == '\t')) {
> -*(tmp_field--) = '\0';
> +if (strpbrk(value, "\n\v\f\r")) {
> +r->status = HTTP_BAD_REQUEST;
> +ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, 
> APLOGNO(03451)
> +  "Request header field value presented"
> +  " bad whitespace");
> +return;
>  }
>
>  if (tmp_field == last_field) {
>  r->status = HTTP_BAD_REQUEST;
> -ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, 
> APLOGNO(03453)
> +ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, 
> APLOGNO(03453)
>"Request header field name was empty");
>  return;
>   

Re: svn commit: r1769669 [2/2] - in /httpd/httpd/branches/2.4.x-merge-http-strict: ./ docs/manual/ docs/manual/mod/ include/ server/

[William A Rowe Jr]

On Thu, Dec 22, 2016 at 9:29 AM, Eric Covener  wrote:

> I think the log severity changes below could use some eyes, especially
> in context of 2.2.  Are these lowered because they're redundant?  I
> haven't yet looked.
>
> I am tempted to leave the old severities for 2.2 and wait and see if
> it's confusing in 2.4 (should not have to enable DEBUG to see the
> cause of a 400 error)


Log severity is lowered because successfully throwing off a (generally
deliberately) bad request is of no interest to an operator at loglevel
error/warn, any more than any successful request is of interest.

This is just log pollution. There is no action for the operator to take.

Re: svn commit: r1769669 [2/2] - in /httpd/httpd/branches/2.4.x-merge-http-strict: ./ docs/manual/ docs/manual/mod/ include/ server/

[Rainer Jung]

Am 22.12.2016 um 18:25 schrieb William A Rowe Jr:

On Thu, Dec 22, 2016 at 9:29 AM, Eric Covener mailto:cove...@gmail.com>> wrote:

I think the log severity changes below could use some eyes, especially
in context of 2.2.  Are these lowered because they're redundant?  I
haven't yet looked.

I am tempted to leave the old severities for 2.2 and wait and see if
it's confusing in 2.4 (should not have to enable DEBUG to see the
cause of a 400 error)


Log severity is lowered because successfully throwing off a (generally
deliberately) bad request is of no interest to an operator at loglevel
error/warn, any more than any successful request is of interest.

This is just log pollution. There is no action for the operator to take.


Hmmm, but if it isn't about a deliberately bad request but a bad client, 
the admin could consider setting "HttpProtocolOptions Unsafe" (and now 
get what he asked for. I must admit, I didn't check whether the log 
lines in question are in code areas which are controlled by 
HttpProtocolOptions or not.


Regards,

Rainer

Re: svn commit: r1769669 [2/2] - in /httpd/httpd/branches/2.4.x-merge-http-strict: ./ docs/manual/ docs/manual/mod/ include/ server/

[William A Rowe Jr]

On Thu, Dec 22, 2016 at 11:37 AM, Rainer Jung 
wrote:

> Am 22.12.2016 um 18:25 schrieb William A Rowe Jr:
>
>> On Thu, Dec 22, 2016 at 9:29 AM, Eric Covener > > wrote:
>>
>> I think the log severity changes below could use some eyes, especially
>> in context of 2.2.  Are these lowered because they're redundant?  I
>> haven't yet looked.
>>
>> I am tempted to leave the old severities for 2.2 and wait and see if
>> it's confusing in 2.4 (should not have to enable DEBUG to see the
>> cause of a 400 error)
>>
>>
>> Log severity is lowered because successfully throwing off a (generally
>> deliberately) bad request is of no interest to an operator at loglevel
>> error/warn, any more than any successful request is of interest.
>>
>> This is just log pollution. There is no action for the operator to take.
>>
>
> Hmmm, but if it isn't about a deliberately bad request but a bad client,
> the admin could consider setting "HttpProtocolOptions Unsafe" (and now get
> what he asked for. I must admit, I didn't check whether the log lines in
> question are in code areas which are controlled by HttpProtocolOptions or
> not.
>

That's true. However, every admin/operator knows that if the app dev team
or users are complaining that the request doesn't work, the first thing any
admin does is enable loglevel debug to see what's going on.

The CTL's aren't override-able (see the security reports 2.4 detailed
description) by our group concensus, and my UnsafeWhitespace proposal was
rejected. But there is still information at debug level to know what sort
of problem occurred without breaking out wireshark first.

Once that is sorted, it's still noise and needs to be disabled again. The
broken clients will be the very odd exception, not a rule.