Subject: Re: Misc review notes for draft-18 p1

From: Henrik Nordström <[email-masked]>
Date: Mon, 06 Feb 2012 05:10:40 +0100

mån 2012-01-30 klockan 13:00 +1300 skrev Amos Jeffries:

> I have a client in a situation which requires relaying CONNECT requests 
> between proxies. We can safely negotiate chunking between two 1.1 
> proxies to enable keep-alive optimizations on these, if it were to be 
> defined as having no body at all that would break.
> 
> The same problem may also apply to Upgrade for unknown protocols. When 
> the upgraded protocol is completed, having the handlers close the 
> connection and cycle through TCP TIME_WAIT instead of cleanly releasing 
> it for re-use by the outer layer protocol again would be a major annoyance.

CONNECT and Upgrade is both in the same boat of making tunnels, not
proxying as such.

Try avoid abusing CONNECT (or Upgrade) for uses it's not intented for.

Regards
Henrik

---

From: Mark Nottingham <[email-masked]>
Date: Tue, 7 Feb 2012 14:18:31 +1100

On 27/01/2012, at 2:56 AM, Willy Tarreau wrote:

> Hi,
> 
> I haven't finished reading p1 but I already have some comments, so
> I'm sending them here and will proceed with what remains.

Hi Willy,

Thanks for that. I'll add a few comments below.

> 2.1. Client/Server Messaging, page 11
> 
>>  Note that 1xx responses (Section 7.1 of [Part2]) are not final;
>>  therefore, a server can send zero or more 1xx responses, followed by
>>  exactly one final response (with any other status code).
> 
> This parts falls here quite out of context in my opinion. Neither
> responses nor status core nor messaging has been defined yet and all
> of a sudden we get this. I suggest we move this to P2 7.1 and replace
> it with a small note such as :
> 
>  Note that sometimes a server may send multiple responses, see Section
>  7.1 of [Part2] for more details about interim responses.

I'll leave it to the editors to take this as input.

> 2.4. Intermediaries, page 13
> 
> Context :
>>>>>> 
>>      UA =========== A =========== B =========== C =========== O
>>                 <             <             <             <
> ...
> 
>>  For example, B might be receiving
>>  requests from many clients other than A, and/or forwarding requests
>>  to servers other than C, at the same time that it is handling A's
>>  request.
> 
> I'd underline that there is no single path between a UA and an intermediary,
> and that sometimes direct and indirect communications are possible. It helps
> remind people that rewriting URLs along the path is not always a good idea.
> I'd suggest this then :
> 
>    For example, B might be receiving requests from many clients other than A
>    including UA/C/O, and/or forwarding requests to servers other than C, at
>    the same time that it is handling A's request.

To the editors (generally, I agree).

> Later :
> 
>>  An HTTP-to-HTTP proxy is called a "transforming proxy" if it is
>>  designed or configured to modify request or response messages in a
>>  semantically meaningful way (i.e., modifications, beyond those
>>  required by normal HTTP processing, that change the message in a way
>>  that would be significant to the original sender or potentially
>>  significant to downstream recipients).
> 
> It is not totally clear to me if a compressing proxy is a transforming
> proxy, nor if one that rewrites Location headers to normalize them is
> a transforming proxy.

There's always going to be a fuzzy line here, I think.

> 2.7.1. http URI scheme
> 
>>   If the host identifier is provided as an IP literal or IPv4 address,
> 
> I did not find a clear definition of the term "IP literal". Also, does it
> cover the bracketed format of IPv6 ?
> 
> 
> 3.3. Message Body
> 
>>  The length of the message-body is determined by one of the following
>>  (in order of precedence):
>> 
>>  1.  Any response to a HEAD request and any response with a status
>>      code of 100-199, 204, or 304 is always terminated by the first
>>      empty line after the header fields, regardless of the header
>>      fields present in the message, and thus cannot contain a message-
>>      body.
> 
> Now that we've included the CONNECT method in the spec, I think it makes
> sense to define whether it has a body or not in case of success. I've
> found myself sometimes adding "Content-length: 0" as well as huge values
> in the past on some CONNECT requests to help interoperability with broken
> proxies, as well as "Connection: close" on these similar requests. Obviously
> the implementations were faulty but a faulty implementation often results
> from ambiguous specs.
> 
> Could we suggest that as a first rule, a 200 response to a CONNECT request
> implies an infinite content length (I don't like that very much since it's
> false), or that it has no message body and that the connection is immediately
> switched to a tunnel ?
> 
>   0. Any response with a status code of 200 to a CONNECT request does not
>      contain any message-body and immediately switches to a tunnel (Section
>      6.9 of [Part2]).
> 
> Also, since I've seen some implementations send "Content-length: 0" on
> CONNECT requests (which I happened to mimmick once), I'm realizing that
> it's not always obvious what to send on responses where no content is
> expected. Would it make sense to insist on the fact that it is not
> necessary to send "Content-length: 0" on messages which do not have a
> body by the rules above ?

This should be covered by <http://trac.tools.ietf.org/wg/httpbis/trac/ticket/250>.

> 3.5. Message Parsing Robustness
> 
>>  Likewise, although the line terminator for the start-line and header
>>  fields is the sequence CRLF, we recommend that recipients recognize a
>>  single LF as a line terminator and ignore any CR.
> 
> Does this mean that CR CR CR CR CR CR LF should be interpreted as a single
> LF ? It kinds of scares me on the risk of smuggling attacks. I'd rather
> suggest :
> 
>    ... we recommend that recipients recognize a single LF as a line
>    terminator and ignore the optional preceeding CR. Messages containing
>    a CR not followed by an LF MUST be rejected.

I've created <http://trac.tools.ietf.org/wg/httpbis/trac/ticket/340>.

>>  When a server listening only for HTTP request messages, or processing
>>  what appears from the start-line to be an HTTP request message,
>>  receives a sequence of octets that does not match the HTTP-message
> 
> Wouldn't "does not *exactly* match" be better ? I'm used to find
> crappy requests in my logs which are blocked but which some not-so-lazy
> implementations would let pass (eg: multiple SP).

I don't think that adding a word here is going to change those implementations behaviour, unfortunately. 

>>  grammar aside from the robustness exceptions listed above, the server
>>  MUST respond with an HTTP/1.1 400 (Bad Request) response.
> 
> I would also suggest that clients and proxies protect themselves against
> malformed response messages, which are problematic in shared hosting
> environments. This could be summarized like this :
> 
>    In general, any agent which receives a malformed message MUST NOT try
>    to fix it if there is any possibility that any other implementation
>    along the chain understands it differently. In such conditions, the
>    message MUST be rejected.

This will make too many implementations non-conformant.

> 4.1. Types of Request Target
> 
>> Note: The "no rewrite" rule prevents the proxy from changing the
> 
> I did not find reference to this "no rewrite" rule.
> 
> 
> 4.2. The Resource Identified by a Request
> 
>>  1.  If request-target is an absolute-URI, the host is part of the
>>      request-target.  Any Host header field value in the request MUST
>>      be ignored.
>> 
>>  2.  If the request-target is not an absolute-URI, and the request
>>      includes a Host header field, the host is determined by the Host
>>      header field value.
>> 
>>  3.  If the host as determined by rule 1 or 2 is not a valid host on
>>      the server, the response MUST be a 400 (Bad Request) error
>>      message.
> 
> Rule 3 might be difficult to apply in massively hosted environments, as
> I easily imagine that there could be a large "vhosts" directory with
> all the hosts roots presented by their names there. The server would
> then simply try to "cd $host" to check for the host's validity, which
> might seem appropriate at first. But using a host of ".." or a host
> containing a slash would have dramatic effects.
> 
> I don't know what recommendation we could add here because we can't
> add boring long sentences, but avoiding such simple traps would be
> nice. Maybe we should just add :
> 
>    For instance, a host should never be ".." nor contain a slash.

That's an implementation-specific security concern, not a general HTTP one.

> 7.4. Use of HTTP by other protocols
> 
> It would make sense to list WebSocket here too since it's the first large
> scale user of the Upgrade mechanism.
> 
> 
> 8.4. TE
> 
>>  The presence of the keyword "trailers" indicates that the client is
>>  willing to accept trailer fields in a chunked transfer-coding, as
> 
> Is it only limited to the client ? Nowhere it's said that a server cannot
> advertise "TE: trailers" in responses so that a client knows it can emit
> chunked-encoded messages with trailers in further requests (eg: backups
> with SHA1 at the end). Replace "client" with "sender" maybe ?

The current wording doesn't prohibit other uses (or users) in the future... 

That said, Julian is right, we need to clarify this.

> 8.5. Trailer
> 
>>  If no Trailer header field is present, the trailer SHOULD NOT include
>>  any header fields.  See Section 5.1.1 for restrictions on the use of
>>  trailer fields in a "chunked" transfer-coding.
> 
> in 5.1.1 p37, it's said :
> 
>>  A server using chunked transfer-coding in a response MUST NOT use the
>>  trailer for any header fields unless at least one of the following is
>>  true:
> 
> Is the SHOULD NOT vs MUST NOT difference on purpose ? It seems to indicate
> that there should be a tolerance when parsing unadvertised trailers (I'm
> fine with this, just checking whether the wording is expected).

I think so.

> A.1.2 Keep-Alive Connections
> 
>>  Clients are also encouraged to consider the use of Connection: keep-
>>  alive in requests carefully; while they can enable persistent
>>  connections with HTTP/1.0 servers, clients using them need will need
>>  to monitor the connection for "hung" requests (which indicate that
>>  the client ought stop sending the header),
> 
> I know a number of people who use the term "the header" to designate all
> the headers section. I must say that when I read this sentence, it was
> unclear to me upon first reading that the intent was in fact to stop
> sending "Connection: keep-alive" in subsequent requests, as it can also
> be understood as "stop sending the headers as long as the connection
> hangs" (which does not make sense).
> 
> I'd suggest the following change :
> 
> -   the client ought stop sending the header),
> +   the client ought stop using this header in further communications with
> +   the server),

.. or just "in future requests." 

To the editors.

> At a number of places it is suggested to "close the connection". I
> think we could add an annex such as the following one, with references
> everywhere we suggest closing the connection, as well as one pointer
> in "6.1.2.2 Pipelining" :
> 
>    A.x.x Closing a Connection
> 
>    When a server needs to close a connection, it must ensure that doing so
>    will not risk prematurely terminate any previous response. When TCP
>    segments are still in flight during a socket close, operating systems
>    generally turn the socket to orphaned state, during which lingering data
>    will still be emitted but any received data would cause an immediate
>    connection abort. The connection may also be aborted when the system
>    is getting low on orphaned sockets. This means that a close before all
>    lingering data are acknowledged by the client might result in a loss of
>    unacknowledged data. This is a very common issue when performing a
>    redirect upon a POST request before all the client's body has been read.
>    While this is not always an issue when a server wants to abort a current
>    request, it becomes a real issue when the client tries to pipeline requests,
>    because aborting the current request may also result in destroying previous
>    unacknowledged response too, possibly causing a client to retry already
>    processed requests that it believes were ignored.
> 
>    The proper way for a server to close a connection without risking issues
>    described above is the following :
> 
>       1) shutdown the transmit channel, usually using the shutdown() system
>          call.
>       2) drain any incoming data and (if possible) check for any lingering
>          data in the transmit queue.
>       3) when the receive channel reports a shutdown, or when all transmitted
>          data have been acknowledged, or when enough time has elapsed, perform
>          the close() on the socket.
> 
>    Operating systems do not always easily report the amount of lingering data
>    and will not always wake up when the queue is empty. A tradeoff has to be
>    found between keeping connections alive for too long a time and risking
>    closing too early and having some clients get truncated or empty responses.

My .02 - this seems more like implementation-specific advice; there are cases where this will not be the case. What do others think?

--
Mark Nottingham   http://www.mnot.net/

---

From: Mark Nottingham <[email-masked]>
Date: Tue, 7 Feb 2012 14:21:37 +1100

Ticket: <http://trac.tools.ietf.org/wg/httpbis/trac/ticket/341>

Cheers,

On 30/01/2012, at 11:17 AM, Amos Jeffries wrote:

> On 27/01/2012 8:36 a.m., Willy Tarreau wrote:
>> 
>> (...)
>>> "match" means "match"; I don't think there's any ambiguity here...
>> There's no ambiguity, it's just to emphasize on the need to perform
>> strict matching. A large number of HTTP parsers are much too lazy,
>> causing nightmares when trying to filter undesired communications,
>> or even to define new protocol extensions. For instance on my old
>> Apache 1.3 here :
>> 
>>     $ telnet www 60080
>>     Connected to www.
>>     Escape character is '^]'.
>>     HEAD     /           HTTP/1.1     ergeargoaejgoiejgaoeg
>>     Host:   ,,,,
>>     Invalid/header name: blah
>> 
>>     HTTP/1.1 200 OK
>>     Date: Thu, 26 Jan 2012 19:07:02 GMT
>>     Server: Apache
>>     Last-Modified: Mon, 01 Jun 2009 16:47:12 GMT
>>     ETag: "47038-3ad7-46b4c2d81a400"
>>     Accept-Ranges: bytes
>>     Content-Length: 15063
>>     Connection: close
>>     Content-Type: text/html
>> 
>>     Connection closed by foreign host.
>> 
>> "SP" is *one* SP, still multiple SPs are accepted in the request
>> line. Same for forbidden chars in the header name. And I'm not
>> specifically targeting Apache here, I just took the first example
>> I had handy, it's far from being alone. It looks like strchr(),
>> strtok(), sscanf() or split() depending on the language and
>> implementation are common ways to parse requests. This is part
>> of what caused all the mess in the hybi WG, delaying it by one
>> year trying to find solutions against various implementations.
> 
> 
> FWIW: we argued this out in Squid a while back.
> The conclusion was to accept any series of non-wrapping BWS before/after the method and URL. Ignoring the BWS. All other formats and garbage to be treated as HTTP/0.9 mess and 400 the result if the suspected URL(+garbage) fails to parse as a usable URI in its entirety.
> 
> A few vendors have hit it with their SP padding practices so far. But by and large it works.
> 
> AYJ
>

---

From: Mark Nottingham <[email-masked]>
Date: Tue, 7 Feb 2012 15:20:57 +1100

My .02 - I'm +1 on everything except the last sentence; read literally, it prohibits a CR not followed by a LF *anywhere* in the message, and even if that's fixed, it's too prohibitive (the ABNF already requires CRLF). 

That makes it:

>    single LF as a line terminator and ignore the preceding CR, if present.

BTW, I think we're getting to wordsmithing here, does anyone disagree with the general sentiment?

Regards,

---

From: Amos Jeffries <[email-masked]>
Date: Tue, 07 Feb 2012 17:58:58 +1300

> There's always going to be a fuzzy line here, I think.

The emphasis on *semantic* changes is clear. The fuzziness only happens 
when HTTP intracacies make you loose sight of what is semantic vs 
transport or syntax details.

I'm inclined to consider it in the form of whether the change is limited 
in its effects.
  * if the client were to base its next request on this response and go 
straight to the origin would it matter?
  * if the client were also passing this request via another path would 
it matter?

So normalizing and fixing headers to conform to spec is non-transforming 
(usually). The upstream is required to to have produced such in the 
first place.

Compressing the body changes the representation with effects cascading 
down into ETag. Conditional and range requests to the origin will not 
succeed as intended. So is transforming.

Transfer-encoding compressions should not be altering the end-to-end 
representation so should be considered non-transforming.

> This should be covered by<http://trac.tools.ietf.org/wg/httpbis/trac/ticket/250>.

FWIW: +1 on Dans wording. My special use-case fits in very cleanly with 
that Content-Length/Transfer-Encoding exception.

> I've created<http://trac.tools.ietf.org/wg/httpbis/trac/ticket/340>.
>

> My .02 - this seems more like implementation-specific advice; there are cases where this will not be the case. What do others think?

Methinks this would fall under editorial changes for the proposal to 
shift the transport details and in particular the TCP-specific details 
into a clearly defined area somewhere. Pipelining, keep-alive and 
closing connections are all transport concepts specific to TCP or 
similar with stream/sequence properties.

AYJ

---

From: Amos Jeffries <[email-masked]>
Date: Tue, 07 Feb 2012 18:14:11 +1300

> BTW, I think we're getting to wordsmithing here, does anyone disagree with the general sentiment?

Yes.

IME, the series of *CR LF has already been a smuggling risk for years 
and implementations are already handling it properly (or need 
vulnerability bugs reported immediately). Prohibiting it from being sent 
and instructing that it ought to be erased if found in the 
request/statsu/headers lines should be enough.

It would be nice to go to straight LF as line delimiter. But that would 
probably break a lot of things.

FWIW we accept multiple trailing CR before the LF in Squid but 400 any 
attempt to use it elsewhere in the first line. Its acceptance in the 
header area is more complex, being accepted as BWS and in some specific 
field values where encoding or content may need it.

AYJ

---

From: Mark Nottingham <[email-masked]>
Date: Mon, 13 Feb 2012 15:10:06 +1100

Looking at this a bit more.

We can't use OWS or BWS here, because they both include obs-fold.

So, proposal:

Add a new construct:

SSP = SP /  1*BSP         ; preferred single space
BSP = ( HTAB / SP )       ; "bad" space

And change Request-Line and Status-Line to:

     Request-Line   = Method SSP request-target SSP HTTP-Version BSP CRLF
     Status-Line = HTTP-Version SSP Status-Code SSP Reason-Phrase BSP CRLF

With appropriate text cautioning against generation of BSP, but advising consumption of it.

Thoughts?

---

From: Willy Tarreau <[email-masked]>
Date: Mon, 13 Feb 2012 08:16:31 +0100

Hi Mark,

> With appropriate text cautioning against generation of BSP, but advising consumption of it.

FWIW, haproxy has never allowed HTABs nor multiple spaces here and still
we dont get error reports. In fact the only times I catch this, it comes
from a poorly-written attack script.

I don't think we should relax the parsing rules, and it seems like Amos and
I were on the same line of "exactly matching" one SP.

Regards,
Willy

---

From: Amos Jeffries <[email-masked]>
Date: Tue, 14 Feb 2012 01:36:48 +1300

> from a poorly-written attack script.

Ah. Thank you for that ammo. :)

Yes.

I got argued into allowing multiple-SP while updating the parser on 
grounds of tolerance before a strict version went out. To my knowledge 
we have not had any bug reports about these internal SP either way, and 
no problems outside of testing software which made some effort to check 
the tolerance bounds.

The two problems I do know of are un-encoded SP within the URL from some 
Windows apps (not sure where its actually coming from, or if they are 
dead yet) and trailer SP bytes after the version string. The dev for 
those apps doing trailers have been fairly responsive towards fixing 
that at the client end already.

With the current RFC it is rather difficult to bring up evidence against 
OWS between "HTTP-version CRLF", despite being the common sense 
interpretation. The simple words "exactly match" would go a long way 
towards clarity there without any interoperability problem.

AYJ

---

From: Willy Tarreau <[email-masked]>
Date: Mon, 13 Feb 2012 13:58:27 +0100

Hi Amos,

> Ah. Thank you for that ammo. :)

You're welcome :-)

(...)
> that at the client end already.

OK. I've never encountered these ones except in attacks. For instance,
here with a capture of such a failure on my reverse proxy :

  [02/Feb/2012:00:47:19.447] frontend http-in (#1): invalid request
  src 88.191.95.247, session #189428, backend <NONE> (#-1), server <NONE> (#-1)
  HTTP internal state 26, buffer flags 0x00809002, event #12
  request length 222 bytes, error at position 55:
 
  00000  GET //wp/wp-content/themes/automotive-blog-theme/Quick Cash Auto/timth
  00070+ umb.php HTTP/1.1\r\n
  00088  Accept: */*\r\n
  00101  Accept-Language: en-us\r\n
  00125  Accept-Encoding: gzip, deflate\r\n
  00157  User-Agent: Mozilla/5.0\r\n
  00182  Host: demo.1wt.eu\r\n
  00201  Connection: Close\r\n
  00220  \r\n
 
It was rejected because of the embedded space :-)

> towards clarity there without any interoperability problem.

That was my point too. Mark, would you accept to update the wording when
two implementers feel like there's a risk of not being careful enough with
current wording ?

Regards,
Willy

---

From: Zhong Yu <[email-masked]>
Date: Mon, 13 Feb 2012 08:30:58 -0600

>     Request-Line   = Method SSP request-target SSP HTTP-Version BSP CRLF

Doesn't that mean a white space is *required* between HTTP-Version and CRLF?

It's good info from Willy/Amos that almost all requests generated use
single SP, thanks.

Zhong Yu

---

From: Julian Reschke <[email-masked]>
Date: Mon, 13 Feb 2012 16:58:00 +0100

On 2012-02-07 04:21, Mark Nottingham wrote:
> Ticket:<http://trac.tools.ietf.org/wg/httpbis/trac/ticket/341>
> ...

I'm not sure we need to bake error handling into the ABNF. If we do it 
here, why not for HTTP-date?

Best regards, Julian

---

From: Mark Nottingham <[email-masked]>
Date: Tue, 21 Feb 2012 15:31:48 +1100

OK, but currently we don't talk about multiple spaces (or other whitespace) in the top-line anywhere; what's the appropriate vehicle?

IMO we should at least consider the BNF, because we do so for BWS elsewhere, and the more explicit, the better. Dates are complex, and it's reasonable not to make their BNF even more so.

On 14/02/2012, at 2:58 AM, Julian Reschke wrote:

> I'm not sure we need to bake error handling into the ABNF. If we do it here, why not for HTTP-date?
> 
> Best regards, Julian

---

From: "Roy T. Fielding" <[email-masked]>
Date: Mon, 20 Feb 2012 21:46:18 -0800

> OK, but currently we don't talk about multiple spaces (or other whitespace) in the top-line anywhere; what's the appropriate vehicle?

There is no vehicle needed.  As I said in the ticket, the only interoperable behavior
is to send a single SP. If some implementations choose to receive more than one space,
that's their choice -- it is still not interoperable to send more than one space.
The ABNF defines what is valid to send, not what is robust to parse.  I'd rather change
Apache httpd's parser to forbid extra spaces than make the request-line less predictable.

....Roy

---

From: Willy Tarreau <[email-masked]>
Date: Tue, 21 Feb 2012 07:14:31 +0100

> Apache httpd's parser to forbid extra spaces than make the request-line less predictable.

Agreed!

Willy

---

From: Mark Nottingham <[email-masked]>
Date: Tue, 21 Feb 2012 17:15:25 +1100

On 21/02/2012, at 4:46 PM, Roy T. Fielding wrote:

> The ABNF defines what is valid to send, not what is robust to parse.

I know we flirt with this in a few places, but it would be nice to come out and say so explicitly somewhere.

---

From: Willy Tarreau <[email-masked]>
Date: Tue, 21 Feb 2012 07:21:28 +0100

> I know we flirt with this in a few places, but it would be nice to come out and say so explicitly somewhere.

In fact I think it's already stated in p1-1.1 :

   This document also uses ABNF to define valid protocol elements
   (Section 1.2).  In addition to the prose requirements placed upon
   them, Senders MUST NOT generate protocol elements that are invalid.

   Unless noted otherwise, Recipients MAY take steps to recover a usable
   protocol element from an invalid construct.  However, HTTP does not
   define specific error handling mechanisms, except in cases where it
   has direct impact on security.  This is because different uses of the
   protocol require different error handling strategies; for example, a
   Web browser may wish to transparently recover from a response where
   the Location header field doesn't parse according to the ABNF,
   whereby in a systems control protocol using HTTP, this type of error
   recovery could lead to dangerous consequences.

If that's not enough, maybe Roy's sentence above could be inserted before
"Senders MUST NOT..." in the first paragraph ?

Willy

---

From: Mark Nottingham <[email-masked]>
Date: Mon, 27 Feb 2012 17:47:54 +1100

OK, I'm marking this one as WONTFIX. Editors, feel free to incorporate Roy's pithy "The ABNF defines what is valid to send, not what is robust to parse" in p1 1.1 (and elsewhere as appropriate).

---

From: Mark Nottingham <[email-masked]>
Date: Mon, 27 Feb 2012 17:52:54 +1100

Assigning for -19.

On 07/02/2012, at 4:14 PM, Amos Jeffries wrote:

> IME, the series of *CR LF has already been a smuggling risk for years and implementations are already handling it properly (or need vulnerability bugs reported immediately). Prohibiting it from being sent and instructing that it ought to be erased if found in the request/statsu/headers lines should be enough.
> 
> It would be nice to go to straight LF as line delimiter. But that would probably break a lot of things.
> 
> 
> FWIW we accept multiple trailing CR before the LF in Squid but 400 any attempt to use it elsewhere in the first line. Its acceptance in the header area is more complex, being accepted as BWS and in some specific field values where encoding or content may need it.
> 
> AYJ
> 
>