Re: Can servers generate responses to malformed requests in h2?

Hi,

On Mon, Sep 25, 2023 at 10:24 AM Stefan Eissing <stefan@eissing.org> wrote:

> Agree with Cory and Lucas here.
>
> - Sending a HTTP response is preferable, since this gives much better
> error reporting on the client side.
> - Violations of HTTP/2 framing layer should error on the h2 layer.
> Basically, the server no longer trusts the client.
> - Sending a RESPONSE with subsequent RST is a good pattern
>

This seems perfectly fine in H2. If the intent is to make it clear the
client needs to stop sending more things (or to tell it clearly anything
already sent is going to be ignored), then HTTP/3 is less clear IMO.
Section 4.1.2 [1] states

> Intermediaries that process HTTP requests or responses (i.e., any
intermediary not acting as a tunnel) MUST NOT forward a malformed request
or response. Malformed requests or responses that are detected MUST be
treated as a stream error
<https://datatracker.ietf.org/doc/html/rfc9114#errors> of type
H3_MESSAGE_ERROR
<https://datatracker.ietf.org/doc/html/rfc9114#H3_MESSAGE_ERROR>.
>
> For malformed requests, a server MAY send an HTTP response indicating the
error prior to closing or resetting the stream. Clients MUST NOT accept a
malformed response. Note that these requirements are intended to protect
against several types of common attacks against HTTP; they are deliberately
strict because being permissive can expose implementations to these
vulnerabilities.

The H3 term "stream error" is defined in Section 8

> When a stream cannot be completed successfully, QUIC allows the
application to abruptly terminate (reset) that stream and communicate a
reason; see Section 2.4 <https://www.rfc-editor.org/rfc/rfc9000#section-2.4>
of [QUIC-TRANSPORT <https://datatracker.ietf.org/doc/html/rfc9000>]. This
is referred to as a "stream error".

Note that this is distinct from "abort reading", which would be the server
emitting a STOP_SENDING to tell the client to stop sending more frames on
the indicated stream.

So, wild idea, perhaps what we would benefit from something in the HTTP
semantics that explains more generically how servers should deal with the
bytes that follow a malformed request, which can then be translated into
wire-format-specific rules.

Cheers
Lucas

[1] - https://datatracker.ietf.org/doc/html/rfc9114#section-4.1.2
[2] - https://datatracker.ietf.org/doc/html/rfc9114#section-8


> Apache httpd prefers to send a response on error. And if it receives an
> client frames on that stream after that, it sends a RST.
>
> Cheers,
> Stefan
>
> > Am 25.09.2023 um 09:33 schrieb Cory Benfield <cory@lukasa.co.uk>:
> >
> > For what it's worth, there is a reading of the specification that
> > holds all of these statements as true, and not in conflict.
> >
> > Again, quoting from Martin's summary at the top:
> >
> >> Intermediaries that process HTTP requests or responses (i.e., any
> intermediary not acting as a tunnel) MUST NOT forward a malformed request
> or response. Malformed requests or responses that are detected MUST be
> treated as a stream error (Section 5.4.2) of type PROTOCOL_ERROR.
> >>
> >> For malformed requests, a server MAY send an HTTP response prior to
> closing or resetting the stream. Clients MUST NOT accept a malformed
> response.
> >
> > For servers, this allows the following series of actions on a malformed
> request:
> >
> > 1. Send HEADERS containing 400 Bad Request with END_STREAM set.
> > 2. Follow the frame immediately with RST_STREAM with PROTOCOL_ERROR.
> >
> > The protocol action of this is clear: you are terminating your end of
> > the stream with END_STREAM, and then sending RST_STREAM to prevent any
> > further frame being received.
> >
> > I should also note that § 5.4.2 (Stream Error Handling) doesn't
> > actually contain normative language around the RST_STREAM frame. It
> > reads:
> >
> >> An endpoint that detects a stream error sends a RST_STREAM frame
> (Section 6.4) that contains the stream identifier of the stream where the
> error occurred.
> >
> > Note that is not "MUST send", it's just "sends". If the RST_STREAM
> > frame is not necessary, one can argue that the spec allows you to omit
> > it. But in either case, you can choose to both send a response _and_
> > send RST_STREAM. Sending RST_STREAM is not forbidden on closed
> > streams.
> >
> > Otherwise I agree with Martin's analysis here.
> >
> > On Mon, 25 Sept 2023 at 06:59, Martin Thomson <mt@lowentropy.net> wrote:
> >>
> >> On Mon, Sep 25, 2023, at 13:33, Glenn Strauss wrote:
> >>> My understanding is that I could treat all HTTP request header errors
> >>> received as HTTP/2 requests as h2 stream error PROTOCOL_ERROR.  If that
> >>> is what is desired by the RFC authors, please issue an update or errata
> >>> and I'll change that code in lighttpd.
> >>
> >> That was never really the intent.  The goal of HTTP/2 was to limit
> stream errors to those cases where errors are found in HTTP/2-specific
> stuff (frame format and ordering mainly).
> >>
> >> This encroaches into "headers" in only two ways: pseudo-fields and
> Content-Length.  Control data and length delimiting are functions in
> HTTP/1.1 that are not so cleanly separate from the rest of the protocol.
> Those are considered part of HTTP/2 proper and not HTTP in general.
> >>
> >> For Content-Length in particular, we originally decided that this was
> necessary because of the way in which the length of requests and responses
> was a function that HTTP/2 provided and we wanted to ensure that there was
> no way for a Content-Length field to create an alternative narrative.  The
> believe was that that could be used for request-smuggling in some cases.
> >>
> >> So our response was to make these stream errors.  Perhaps we should
> have allowed either type of error in the case of Content-Length, but that
> was the decision at the time.
> >>
> >>> However, I currently am unable to understand why h2 PROTOCOL_ERROR
> >>> (or H3_GENERAL_PROTOCOL_ERROR) is somehow better than 400 Bad Request.
> >>
> >> I think that your "another reason" below might point to why this is
> preferable.  A client that makes a request via a proxy that upgrades it to
> HTTP/2 or HTTP/3 is not responsible for errors that arise in translation to
> the target request format, so it is in no position to correct that error.
> The error is something that the proxy caused and so the proxy needs to
> handle it.
> >>
> >> Maybe that's no real help to a client who is dealing with a busted
> proxy and who is presented with a request that times out or whatever the
> proxy does in that case.  But it's about ensuring that the error is
> appropriately targeted.  If the proxy isn't implementing the protocol
> properly, then the developers of that proxy really do need to pay
> attention.  Protocol errors like this should not occur during normal
> operation.
> >>
> >> Yes, PROTOCOL_ERROR isn't exactly the nicest error (which is partly why
> closing the connection might be preferable, because you get to include a
> reason phrase), but that only suggests that we might want to explore ways
> of making errors clearer or more actionable.
> >>
> >>> I can see where h2 specification around pseudo-headers is specific to
> >>> the h2 protocol.  However, an implementation "could" HPACK-decode the
> >>> entire HEADERS frame into a single string that looks almost identical
> >>> to HTTP/1.1 request headers with the exception of the addition of
> >>> pseudo-headers.  It could then be loosely parsed as HTTP/1.x request
> >>> headers.
> >>
> >> If you are rewriting HTTP/2 into HTTP/1.x, you need all the
> pseudo-fields in order to make sense of the message.  If one is missing,
> then your converter can generate an error (which you can send as a stream
> error).  Same if a pseudo-field arrives late.
> >>
> >>> An implementation might need to fully parse the HEADERS frame
> >>> before being able to determine that a required pseudo-header is
> missing.
> >>> If it has already gotten this far parsing the HTTP request, why should
> >>> the RFC disallow the implementation from returning an HTTP error code?
> >>
> >> The specification requires that pseudo-fields precede real fields, so
> this is not really an option.
> >>
> >
>
>