[PATCH 2 of 2] HTTP/2: finalize request as bad if header validation fails

Fri Mar 10 04:42:40 UTC 2023

Hello!

On Thu, Mar 09, 2023 at 02:49:24PM +0400, Roman Arutyunyan wrote:

> On Thu, Mar 09, 2023 at 03:40:26AM +0300, Maxim Dounin wrote:
> > Hello!
> > 
> > On Wed, Mar 08, 2023 at 06:15:18PM +0400, Roman Arutyunyan wrote:
> > 
> > > Hi,
> > > 
> > > On Wed, Feb 22, 2023 at 03:55:28PM +0300, Maxim Dounin wrote:
> > > > # HG changeset patch
> > > > # User Maxim Dounin <mdounin at mdounin.ru>
> > > > # Date 1677070454 -10800
> > > > #      Wed Feb 22 15:54:14 2023 +0300
> > > > # Node ID ec3819e66f40924efad183c291c850d9ccef16e7
> > > > # Parent  61bd779a868c4021c232dddfe7abda7e8ad32575
> > > > HTTP/2: finalize request as bad if header validation fails.
> > > > 
> > > > Similarly to 7192:d5a535774861, this avoids spurious zero statuses
> > > > in access.log, and in line with other header-related errors.
> > > 
> > > This is what RFC 9113, 8.2.1. Field Validity, says about header field
> > > validation:
> > > 
> > >   A request or response that contains a field that violates
> > >   any of these conditions MUST be treated as malformed (Section 8.1.1).
> > > 
> > > Also (the previous RFC 7540 did not have this statement):
> > > 
> > >   When a request message violates one of these requirements, an implementation
> > >   SHOULD generate a 400 (Bad Request) status code (see Section 15.5.1 of [HTTP])
> > > 
> > > Considering this, responding with HTTP 400 is certainly an improvement compared
> > > to the current code.  Also, it allows to set a custom error page handler.
> > > 
> > > However, not resetting the stream as before is a less obvious change.  If we
> > > consider nginx an intermediary, the RFC 9113 prescribes us to send stream reset:
> > > 
> > >   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.
> > > 
> > > This restriction probably makes sense for the intermediaries unable to handle
> > > the error in a more flexible way.
> > > 
> > > I like the change, but I'd like to hear what you think about the reset issue.
> > 
> > We already do exactly the same for quite similar cases, as 
> > introduced in 7192:d5a535774861, for unknown and invalid 
> > pseudo-headers.  This patch simply makes handling of 
> > errors detected by ngx_http_v2_validate_header() equivalent.
> 
> Yes, I have read that discussion.  It's seemed suspicious though that only the
> pseudo-headers error was treated as HTTP 400 by that change, while
> ngx_http_v2_validate_header(), which is just a few lines above, wasn't
> affected.

I think this was mostly because issues with pseudo-headers were 
seen in practice by Ruslan while working on HTTP/2 push, but he 
wasn't in position to further audit various other cases which can 
lead to similar behaviour.

> > I does not look like returning 400 instead of sending RST_STREAM 
> > causes any issues.  Further, given that HTTP/2 error handling is 
> > mostly unimplemented by clients, I tend to think that returning 
> > 400 instead of RST_STREAM is a better option.
> 
> I personally like it too.
> 
> > Note well that nothing in RFC 7540 actually requires to send 
> > RST_STREAM: while it says that (detected) malformed requests "MUST 
> > be treated as a stream error", handling of stream errors does not 
> > require to actually send RST_STREAM, as there is no such normative 
> > requirement in section 5.4.2.  
> 
> Indeed there's no requirement level here, which makes the whole paragraph
> pointless:
> 
>    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.  The RST_STREAM frame includes an error code that
>    indicates the type of error.
> 
> > Further, RFC 9113 explicitly states 
> > that (https://www.rfc-editor.org/rfc/rfc9113#section-8.1.1):
> > 
> >   For malformed requests, a server MAY send an HTTP response prior 
> >   to closing or resetting the stream. 
> > 
> > Note "closing or resetting", which implies that RST_STREAM is not 
> > mandatory.
> 
> If the server is an origin server, and the request is malformed, stream error
> is indeed not mandatory.  But for an intermediary, it is.  It's however not
> clear, whether stream error should trigger RST_STREAM, considering the lack
> of a requirement level in 5.4.2.

Intermediaries "MUST not forward" such requests (and responses).  
The "MUST be treated as a stream error" requirement applies to all 
types of servers and clients, IMO.  The following paragraph 
further clarifies how servers and clients are expected to behave 
(and this includes intermediaries acting as a server or as a 
client).

> > While we might consider changing the code to generate RST_STREAM 
> > after sending the response in such cases, this does not seem to be 
> > needed (and might actually make things worse).  In either case 
> > this doesn't look like something to be addressed in this patch 
> > series.
> 
> The discussion is also relevant to HTTP/3 which may trigger either QUIC or
> HTTP errors for malformed request streams.
> 
> As I said before, I like HTTP errors more since they provide more flexibility
> and are more compatible with the way nginx treats HTTP/1 requests.  Please
> commit the patch.

Pushed to http://mdounin.ru/hg/nginx, relevant test changes are in 
http://mdounin.ru/hg/nginx-tests.

-- 
Maxim Dounin
http://mdounin.ru/