Subject: Re: Permitted characters in HTTP/2 fields

From: "Martin Thomson" <[email-masked]>
Date: Thu, 20 May 2021 14:34:15 +1000

Coming back to this.

Mark points out that if the purpose of these rules is to cut back on ways in which smuggling might occur (not prevent, that's a fool's errand), then we should also prohibit 0x00-0x1f and 0x7f (the ASCII control characters) and 0x20 (SP) from field names.  I think that's reasonable, but I want to check here.  That shouldn't make this harder to implement and it might catch some genuine problems.  (Mark includes citations in discussion on the pull request.)

https://github.com/httpwg/http2-spec/pull/846 has been updated.  I hope, assuming that chairs are OK, this can be merged soon.

On Wed, Apr 28, 2021, at 08:00, Greg Wilkins wrote:
> By all means change the permissible character in fields, but do it with 
> precise ABNF. If there is to be a difference between what SHOULD be 
> sent and what MUST be checked, then have two sets of ABNF.

I don't think that it makes sense to use ABNF for this.  The text seems pretty clear (even if I just wrote it and so prone to missing errors).   That's just my opinion regarding what you seem to be suggesting is an editorial decision.

---

From: Willy Tarreau <[email-masked]>
Date: Thu, 20 May 2021 18:59:10 +0200

Hi Martin,

On Thu, May 20, 2021 at 02:34:15PM +1000, Martin Thomson wrote:
> Coming back to this.
> 
> Mark points out that if the purpose of these rules is to cut back on ways in
> which smuggling might occur (not prevent, that's a fool's errand), then we
> should also prohibit 0x00-0x1f and 0x7f (the ASCII control characters) and
> 0x20 (SP) from field names.  I think that's reasonable, but I want to check
> here.  That shouldn't make this harder to implement and it might catch some
> genuine problems.

I really agree. I don't remember if 0x80 and above are forbidden in H2 but
I'd personally prefer to block them so that we don't needlessly introduce
the risk of aliasing due to different codings being used. Protocol elements
that define how messages should be delimited/routed/etc must be strictly
defined and easy to enforce in implementations and applications.

Cheers,
Willy

---

From: "Poul-Henning Kamp" <[email-masked]>
Date: Thu, 20 May 2021 18:02:06 +0000

--------
> defined and easy to enforce in implementations and applications.

+1

-- 
Poul-Henning Kamp       | UNIX since Zilog Zeus 3.20
[email-masked]         | TCP/IP since RFC 956
FreeBSD committer       | BSD since 4.3-tahoe    
Never attribute to malice what can adequately be explained by incompetence.

---

From: "Martin Thomson" <[email-masked]>
Date: Fri, 21 May 2021 11:19:35 +1000

Hey Willy,

> defined and easy to enforce in implementations and applications.

We never really said before.  I'm happy to extend the 0x7f to 0x7f-0xff if that is what others want.  It's not quite the same as limiting the grammar to what is permitted for field names, but it might be OK.

field-name is "!" / "#" / "$" / "%" / "&" / "'" / "*" / "+" / "-" / "." / "^" / "_" / "`" / "|" / "~" / DIGIT / ALPHA

That amounts to a whole bunch of characters less than %x21-7E (minus ':').  A simpler check for c >= 0x21 && c <= 0x7e && c != ':' seems reasonable to me.  Then we don't have to worry about Unicode field names.  That's not a whole lot different than c >= 0x21 && c != 0x7e && c != ':' as the current PR has.

I had the distinct impression that we DID see Unicode field names in some cases though.

We wanted to avoid backward incompatibility issues that might result from tighter constraints on field *values*, which is why we never said anything before, but names might be easier.

---

From: Greg Wilkins <[email-masked]>
Date: Fri, 21 May 2021 12:36:15 +1000

This conversation started with:

At our last interim, we discussed potential ways in which HTTP/2 was
> probably too strict about characters (octets really) in field names and
> values.
> The conclusion then was to loosen the restriction and mandate only a small
> set of checks.  This should match what implementations already do.

Any chance of describing exactly what those reasons are, because it's lost
on me exactly what problem is being solved here.      If we don't have a
full brief for these changes, then how are we meant to evaluate them or
indeed record the reason for posterity.  Neither #815 nor #846 explain the
problem other than say the text is confusing.  There is no motivation for
why validation should be less than carrying HTTP fields plus pseudo fields.

I don't mind the current text so much, as it says I can validate against
HTTP semantic fields as defined by
https://www.ietf.org/archive/id/draft-ietf-httpbis-semantics-15.html#section-5,
so I will.   I'm just going to reject any other fields and I'm allowed to,
so I'm happy.    But I have no idea why we want to allow implementations to
send non compliant fields around.   Isn't that just asking for problems.
 If it is because some existing implementations are already sending invalid
fields, then they are doing so regardless and unless you say an impl must
accept them, then any impl may reject them as invalid. So changing the spec
to be less strict makes no difference so long as impls are allowed to
actually enforce correct validation.

Finally, when the "Brief" says we should match what implementations already
do, then the question is which implementations are to be matched?   If
there are some implementations that already enforce the precise spec for
HTTP headers, then should we match those imples or are some implementations
more match worthy than others?

On Fri, 21 May 2021 at 11:22, Martin Thomson <[email-masked]> wrote:

> We never really said before.  I'm happy to extend the 0x7f to 0x7f-0xff if
> that is what others want.  It's not quite the same as limiting the grammar
> to what is permitted for field names, but it might be OK.
>
> field-name is "!" / "#" / "$" / "%" / "&" / "'" / "*" / "+" / "-" / "." /
> "^" / "_" / "`" / "|" / "~" / DIGIT / ALPHA
>
> That amounts to a whole bunch of characters less than %x21-7E (minus
> ':').  A simpler check for c >= 0x21 && c <= 0x7e && c != ':' seems
> reasonable to me.  Then we don't have to worry about Unicode field names.
> That's not a whole lot different than c >= 0x21 && c != 0x7e && c != ':' as
> the current PR has.
>
> We wanted to avoid backward incompatibility issues that might result from
> tighter constraints on field *values*, which is why we never said anything
> before, but names might be easier.
>
>

-- 
Greg Wilkins <[email-masked]> CTO http://webtide.com

---

From: Willy Tarreau <[email-masked]>
Date: Fri, 21 May 2021 08:17:17 +0200

Hi Martin,

On Fri, May 21, 2021 at 11:19:35AM +1000, Martin Thomson wrote:
> that is what others want.  It's not quite the same as limiting the grammar to
> what is permitted for field names, but it might be OK.

I was fine as well with limiting to what is permitted (and I personally use
a bit field to match them) but I agree that using a few ranges is even easier
to implement (especially for small & simple implementations that at least want
to make the effort of staying safe).

> That amounts to a whole bunch of characters less than %x21-7E (minus ':').  A
> simpler check for c >= 0x21 && c <= 0x7e && c != ':' seems reasonable to me.
> Then we don't have to worry about Unicode field names.  That's not a whole
> lot different than c >= 0x21 && c != 0x7e && c != ':' as the current PR has.

Yep in terms of computation it's the same and can be simplified to:

   (uint8_t)(c - 0x21) <= (0x7e - 0x21) && c != ':'

> I had the distinct impression that we DID see Unicode field names in some cases though.

This is exactly the horror that worries me. I really don't want to open
that pandora box, or we can say byebye to HTTP as a safe transport protocol
for applications. Just imagine a client emitting a POST to an H2-to-H1
gateway, with "content-length: 1000" and "Tran\xd1\x95fer-encoding: chunked",
the latter passing through a string manipulation function which
transliterates it to "Transfer-encoding: chunked" on the other side...
And that's just a single example, there are so many possibilities that
it could almost be funny if we weren't concerned a bit by security,
especially as by experience we know that if it can happen it will happen :-/

> before, but names might be easier.

I have no problem with field values as basically anything besides
control chars and heading/trailing blanks, can already appear there
in previous HTTP versions. In practice the only header fields that
could cause trouble when reading incorrectly matched tokens are
Connection and Transfer-encoding, and both of these are already
forbidden in H2.

Thanks,
Willy

---

From: Cory Benfield <[email-masked]>
Date: Mon, 24 May 2021 15:27:43 +0100

On Fri, 21 May 2021 at 03:37, Greg Wilkins <[email-masked]> wrote:
>> At our last interim, we discussed potential ways in which HTTP/2 was probably too strict about characters (octets really) in field names and values.
>> The conclusion then was to loosen the restriction and mandate only a small set of checks.  This should match what implementations already do.
>
>
> Any chance of describing exactly what those reasons are, because it's lost on me exactly what problem is being solved here.      If we don't have a full brief for these changes, then how are we meant to evaluate them or indeed record the reason for posterity.  Neither #815 nor #846 explain the problem other than say the text is confusing.  There is no motivation for why validation should be less than carrying HTTP fields plus pseudo fields.
>
> I don't mind the current text so much, as it says I can validate against HTTP semantic fields as defined by https://www.ietf.org/archive/id/draft-ietf-httpbis-semantics-15.html#section-5, so I will.   I'm just going to reject any other fields and I'm allowed to, so I'm happy.    But I have no idea why we want to allow implementations to send non compliant fields around.   Isn't that just asking for problems.   If it is because some existing implementations are already sending invalid fields, then they are doing so regardless and unless you say an impl must accept them, then any impl may reject them as invalid. So changing the spec to be less strict makes no difference so long as impls are allowed to actually enforce correct validation.

Not all implementations are equal. It is not necessarily reasonable to
bind intermediaries to parse the ABNF of the fields they pass around.
Indeed, many do not. However, these proposed rules do bind
intermediaries: they are supposed to enforce the additional rules put
in place by this proposal.

It's worth remembering how semantics actually describes ABNF as
working. Quoting from -semantics §2.2:

> A sender MUST NOT generate protocol elements that do not match the grammar defined by the corresponding ABNF rules.

This rule binds the _sender_, not the receiver. It leaves unspecified
whether a receiver is obligated to error on a mismatch of the grammar.
This is (as far as I know), deliberate: receivers are entitled to
tolerate violations of the ABNF if they feel they can do so, except
where other relevant specifications forbid it.

That's what these rules are for: they are over-and-above the ABNF, and
bind recipients, including intermediaries. The text on line 3750 of
Martin's proposal covers this pretty clearly I think:

> An intermediary can reject fields that contain invalid field names or values for other
> reasons, in particular those that do not conform to the HTTP ABNF grammar from <xref
> target="HTTP" section="5"/>. Intermediaries that do not perform any validation of fields
> other than the minimum required by <xref target="HttpHeaders"/> could forward messages
> that contain invalid field names or values.

>
> Finally, when the "Brief" says we should match what implementations already do, then the question is which implementations are to be matched?   If there are some implementations that already enforce the precise spec for HTTP headers, then should we match those imples or are some implementations more match worthy than others?
>
>
>

---

From: Greg Wilkins <[email-masked]>
Date: Tue, 25 May 2021 08:25:40 +1000

Cory et al,
In totally ok with the principle of be strict in what you generated and
relaxed in what you accept.

I understand the effort here is to try to tighten up that relaxed
acceptance a bit to ensure that security issues are not created as a
result, but not to impose strict requirement for full Field ABNF compliance.

I just find the text is too imprecise and the motivations for it are not
sufficiently well capture either in the text itself or the PR. It may be
clear to those that attended the meeting, but I find there is missing
context.

If we want to allow flexibility but avoid security issue than we need text
that says: implementations SHOULD (or MAY) enforce the full ABNF ; in order
to avoid security issues [insert ref] all implementation MUST NOT send or
accept fields with <insert precise definition here>.

There is now some discussion on exactly what characters should be
allowed/rejected. Clearly starting exactly the security issues that are
being avoided should assist with well defining that set.

---

From: Greg Wilkins <[email-masked]>
Date: Tue, 25 May 2021 09:31:33 +1000

Actually, my comments are a little out of date and the current text is
pretty much in line with what I'm advocating, especially with the last
commit.    A bit more supporting text in the PR would still be good for
recording why.