On 08/29/2016 06:25 PM, William A Rowe Jr wrote:
> Thanks all for the feedback. Status and follow-up questions inline
>
> On Thu, Aug 25, 2016 at 10:02 PM, William A Rowe Jr <[email protected]
> <mailto:[email protected]>> wrote:
>
> A couple key questions now that the full refactoring of legacy vs. strict
> is mostly complete (there remain potential
> issues with some of the 3-4 yr old changes on trunk which I'll raise in
> other posts.) But speaking only to the
> request line and request header parsing...
>
> 1. Does it make sense to emit these parsing failures at the info level?
> Or debug level (or in trunk/2.4, only at the
> trace level?) Granted some legitimate internal diagnostics may be
> required, so it needs to have some potential
> visibility, but the vast majority of such traffic is abusive and doesn't
> need a place in most error logs.
>
> This is now changed, all are DEBUG except where the admin toggles for Limit's
> are broken (since this is a relatively common case where an admin sets some
> arbitrarily short limit without understanding fields such as User-Agent or
> Cookies.)
>
> Discussion item:
>
> Limit* based failures could also become debug. Thoughts?
We shouldn't change this for 2.2/2.4. For trunk I am +0.
>
> 2. Should we ban \r\n\v\f unequivocally from request and request header
> fields altogether, or is there a legitimate
> need to support these? Or should these follow the UnsafeWhitespace toggle
> and be permitted?
>
> I am changing this right now. Specifically,
>
> * Leading whitespace on the URI line prohibited (and always was)
> * \r\n\v\f as well as \t always trigger 400 in the request line (new
> behavior)
> * multiple or trailing spaces only permitted in Unsafe mode (always
> permitted before)
> * trailing text following trailing spaces after protocol is always 400 (new
> behavior)
>
> * Spaces in or trailing the request field name always prohibited (new per
> RFC7230 3.2.4)
> * \r\n\v\f always triggers 400 in the request line (new behavior)
>
> Discussion items:
>
> I'm working on the patch to require CRLF line termination within
> ap_rgetline_core().
> This is necessary because any CR is eaten before this function completes.
> It's
> not clear how this can be done trivially without negative impacts on other
> consumers
> of the function. But since current 'fold' flag, which we don't use, was a
> bool (0 or 1),
> adding a second control bit-flag of value 2 for CR-required seems the most
> obvious
> way to go, with a minor MMN bump. Thoughts?
Seems sensible.
>
> Two choices, CRLF is always required, or verifying this becomes a byproduct
> of toggling the Strict (vs Unsafe) mode. Thoughts?
>
> 3. Do we need multiple
>
> layers of 'Strict'ness, or should there be a single toggle, or no toggle,
> no tolerant input at all in the next
> 2.2/2.4 releases?
>
> This is now changed for whitespace, no such toggle anymore, it falls under
> Strict|Unsafe.
>
> Discussion item:
>
> I am not sold that StrictURI can be collapsed into this flag. Right now, not
> even
> httpd itself promises to correctly encode resulting URI's, AIUI. Until we
> have our
> own house in order, it seems we need to remain flexible about this. The
> \t\v\r\f\0
> characters are always now prohibited, so it's considerably more safe. Strict
> further
> bans all unencoded ctrl's in the URL. So StrictURI takes this one step
> further, and
> bans all unencoded obs-text along with SP / '"' / '<' / '>' / '\' / '^' / '`'
> / '{' / '|' / '}'
>
> Since it's expected that a number of sites will have to relax UnsafeURI due to
> these encoding issues, even with the resulting URI's generated by httpd
> servers,
> and will have to do so for *public facing* interfaces, I strongly believe
> that this
> flag needs to remain distinct, or we will have lots of servers with entirely
> unsafe
> parsing, not with only limited exposure by accepting bad URIs. Thoughts?
Given the situation you describe it sounds sensible to keep this distinct.
>
> 4. Should the next 2.4/2.2 releases default to Strict at all? Or remain
> permissive (Unsafe) and allow the user to
> toggle these to Strict(... Whitespace... URI)?
>
> Real world direct observation especially appreciated from actual
> deployments.
>
> Strict (and StrictURI) remain the default. The Allow0.9 and LenientMethods
StrictURI as a default only makes sense if we have our own house in order (see
above), otherwise it should be opt in.
> remain the defaults.
>
> Discussion item:
>
> RegisteredMethods was always wrong for proxy, CGI and other cases, was
> nonsense per the spec, and cannot become a default. The Strict mode now
> requires all methods to conform to 'token' text, so we have significant added
> protection here.
>
> Do folks believe we want to ship with Require1.0 out of the box as the
> default,
> even with the 2.2.x/2.4.x backports? It seems the RFC is correct on this, that
> nearly all of these will be badly formed HTTP/1.x requests. Thoughts?
>
>
Are there really 0.9 clients still in the wild?
Regards
Rüdiger
