Subject: Connection-level rejection of oversized DATA frames?

From: Jonathan Thackray <[email-masked]>
Date: Mon, 26 Jan 2015 22:23:25 +0000

Hi all,

At the moment, section 4.2 of draft-ietf-httpbis-http2-16 says:

   An endpoint MUST send a FRAME_SIZE_ERROR error if a frame exceeds the
   size defined in SETTINGS_MAX_FRAME_SIZE, any limit defined for the
   frame type, or it is too small to contain mandatory frame data.  A
   frame size error in a frame that could alter the state of the entire
   connection MUST be treated as a connection error (Section 5.4.1);
   this includes any frame carrying a header block (Section 4.3) (that
   is, HEADERS, PUSH_PROMISE, and CONTINUATION), SETTINGS, and any frame
   with a stream identifier of 0.

I read this to mean that if the server sets the maximum frame size it
will accept to 256KB, and a client sends a DATA frame with a length of
16MB (defined in the header) followed by 16MB of client data, the server
has no choice but to read the data, as the error is at the stream-level
(i.e. RST_STREAM), as the second sentence in that paragraph doesn’t apply.

The draft text doesn’t allow rejecting this frame early without reading
_ALL_ the data, which is a waste of bandwidth. I think the server should be
able to send a GOAWAY connection error of type FRAME_SIZE_ERROR, followed
by closing the TCP/TLS socket. With the current draft spec, the only way
to handle this now is to send a RST_STREAM of type FRAME_SIZE_ERROR and
then send a GOAWAY( NO_ERROR ) to avoid reading the large amount of client
data following, but this is more ambiguous.

Is there any reason why we couldn't add the following text to the bottom
of section 6.1 which says:    (could be either MAY or MUST)

   Receipt of a DATA frame with a length greater than the receiver's
   SETTINGS_MAX_FRAME_SIZE setting MAY be treated as a connection error
   (Section 5.4.1) of type FRAME_SIZE_ERROR.

Or alternatively, we could define all frame size errors to be connection
errors, which would simplify the spec and implementation logic. Given that
this is a new protocol, this would help avoid poor implementations. Frame
size errors should only happen due to poor programming or fuzzing/cracking
attempts.

We could then remove this entire paragraph from section 4.2:

   A frame size error in a frame that could alter the state of the entire
   connection MUST be treated as a connection error (Section 5.4.1);
   this includes any frame carrying a header block (Section 4.3) (that
   is, HEADERS, PUSH_PROMISE, and CONTINUATION), SETTINGS, and any frame
   with a stream identifier of 0.

and replace it with:

   A frame size error in a frame MUST be treated as a connection error
   (Section 5.4.1).

The only stream-level FRAME_SIZE_ERROR defined in the specification appears
to be:

   A PRIORITY frame with a length other than 5 octets MUST be treated as
   a stream error (Section 5.4.2) of type FRAME_SIZE_ERROR.

What do others think? I appreciate this is "last-minute", but I think we
should sort out this nit -- the smaller change of just adding the extra text
for the DATA frame would be sufficient for me, but I’d prefer it if we could
tidy up and make the text and specification clearer. Happy to prepare a PR
of revised text for this.

Thanks,
Jonathan.

---

From: Mike Bishop <[email-masked]>
Date: Mon, 26 Jan 2015 22:34:09 +0000

You're right, the server should be able to send that GOAWAY frame -- and already can, with the current draft text.  Note that this text specifies in which cases the FRAME_SIZE_ERROR MUST be a connection error, but does *not* say that other cases MUST NOT be connection errors or MUST be stream errors.  In the other circumstances, implementations are free to choose whether to kill just the stream or the entire connection in response to a violation.

See 5.4.1:
> An endpoint can end a connection at any time. In particular, an endpoint MAY choose to treat a stream error as a connection error. Endpoints SHOULD send a GOAWAY frame when ending a connection, providing that circumstances permit it.

-----Original Message-----
From: Jonathan Thackray [mailto:[email-masked]] On Behalf Of Jonathan Thackray
Sent: Monday, January 26, 2015 2:23 PM
To: [email-masked]
Subject: Connection-level rejection of oversized DATA frames?

Hi all,

I read this to mean that if the server sets the maximum frame size it will accept to 256KB, and a client sends a DATA frame with a length of 16MB (defined in the header) followed by 16MB of client data, the server has no choice but to read the data, as the error is at the stream-level (i.e. RST_STREAM), as the second sentence in that paragraph doesn’t apply.

The draft text doesn’t allow rejecting this frame early without reading _ALL_ the data, which is a waste of bandwidth. I think the server should be able to send a GOAWAY connection error of type FRAME_SIZE_ERROR, followed by closing the TCP/TLS socket. With the current draft spec, the only way to handle this now is to send a RST_STREAM of type FRAME_SIZE_ERROR and then send a GOAWAY( NO_ERROR ) to avoid reading the large amount of client data following, but this is more ambiguous.

Or alternatively, we could define all frame size errors to be connection errors, which would simplify the spec and implementation logic. Given that this is a new protocol, this would help avoid poor implementations. Frame size errors should only happen due to poor programming or fuzzing/cracking attempts.

and replace it with:

The only stream-level FRAME_SIZE_ERROR defined in the specification appears to be:

What do others think? I appreciate this is "last-minute", but I think we should sort out this nit -- the smaller change of just adding the extra text for the DATA frame would be sufficient for me, but I’d prefer it if we could tidy up and make the text and specification clearer. Happy to prepare a PR of revised text for this.

Thanks,
Jonathan.

---

From: Martin Thomson <[email-masked]>
Date: Mon, 26 Jan 2015 15:06:59 -0800

On 26 January 2015 at 14:34, Mike Bishop <[email-masked]> wrote:
> An endpoint can end a connection at any time. In particular, an endpoint MAY choose to treat a stream error as a connection error.

My favourite part of the spec.  I see a lot of that going on, and I
think that it's healthy.

---

From: Mike Bishop <[email-masked]>
Date: Mon, 26 Jan 2015 23:29:11 +0000

But as this is at least the second thread on which we've seen this misread, is there an editorial change that would make it clearer?  (Personally, I think it's pretty clearly stated, but perhaps we need suggestions from the people who didn't read it that way.)

-----Original Message-----
From: Martin Thomson [mailto:[email-masked]] 
Sent: Monday, January 26, 2015 3:07 PM
To: Mike Bishop
Cc: Jonathan Thackray; [email-masked]
Subject: Re: Connection-level rejection of oversized DATA frames?

> An endpoint can end a connection at any time. In particular, an endpoint MAY choose to treat a stream error as a connection error.

My favourite part of the spec.  I see a lot of that going on, and I think that it's healthy.

---

From: Martin Thomson <[email-masked]>
Date: Mon, 26 Jan 2015 15:57:09 -0800

> But as this is at least the second thread on which we've seen this misread, is there an editorial change that would make it clearer?  (Personally, I think it's pretty clearly stated, but perhaps we…

(Me too.)

I'm happy to entertain a narrowly-scoped, editorial change.  The
window of opportunity is closing though.

---

From: Hervé Ruellan <[email-masked]>
Date: Tue, 27 Jan 2015 17:19:35 +0100

A possibility would be to change the definition of the stream error to:

An error in an individual stream is a stream error. An endpoint MAY 
choose to treat a stream error as a connection error.

Or just put the second sentence after the definitions of connection and 
stream errors.

Hervé.