Fixing GRPC initial metadata validation by omriz · Pull Request #16414 · envoyproxy/envoy

omriz · 2021-05-10T10:23:52Z

Commit Message: Fixing GRPC initial metadata validation

Additional Description:
According to the GRPC spec https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md
The values in the initial metadata can be any valid ASCII character in
case of a non binary header type.

Risk Level: Low
Testing: Unit Tests
Docs Changes:N/A
Release Notes: Fixing GRPC initial metadata validation for ASCII characters
Platform Specific Features: None

omriz · 2021-05-10T10:25:00Z

@htuch for your review

dmitri-d · 2021-05-10T17:42:47Z

source/common/grpc/async_client_manager_impl.cc

I don't think this check is sufficient: absl::ascii_isascii(ch) returns true if ch < 128 (see here: http://docs.ros.org/en/kinetic/api/abseil_cpp/html/ascii_8h_source.html). GRPC spec? (https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md) defines ascii char as one between %x20-%x7E.

Ok, I can change the check

dmitri-d · 2021-05-10T18:11:15Z

source/common/grpc/async_client_manager_impl.cc

If I'm reading the spec right, header names can be lower-case ascii chars, numericals, _, -, and .. validateGrpcHeaderChars will return true if the parameter is an upper-case char (try running python -c 'print("A".isalnum())'. Abseil's kPropertyBits is generated using python script, according to comments in ascii.c. kPropertyBits is then used to verify if a character is an alpha-numeric one),

So I'm not sure - but according to this: https://github.com/grpc/grpc-go/blob/master/Documentation/grpc-metadata.md#creating-a-new-metadata it sounds like you can define initial metadata to be both lower and upercase but it will be converted to lowercase. If you think it's better to have a stricter check I can do so but I'm not sure it's the correct intent in this case.

FYI, HTTP/2 always sends header names as lower-case on the wire:

https://datatracker.ietf.org/doc/html/rfc7540#section-8.1.2

htuch · 2021-05-11T01:53:17Z

source/common/grpc/async_client_manager_impl.cc

@markdroth can you also take a look at this? Thanks.

If I'm understanding this code right, it looks like this is requiring that the metadata be already base64 encoded. I don't think that's necessary for GOOGLE_GRPC, because the gRPC library will automatically do base64 encoding and decoding for header values if the header name ends with "-bin".

What happens if it is already base64 encoded?

If the gRPC library automatically does encoding/decoding - what check should be done in this case? or is any value allowed?

We don't check to see if the value is already base64-encoded; we just unconditionally base64-encode it. So it would wind up being double-base64-encoded on the wire.

I think this will actually work, but it does add unnecessary overhead.

Then we can probably allow any value for -bin. This check was added in #14129 (CC @asraa) because fuzzers were crashing the gRPC library with inputs that looked like https://github.com/envoyproxy/envoy/pull/14129/files#diff-2a20e9888f070730d0412bf9806728cfae9f9f9269747837f68287ff4fcdb5caR1.

That seems right, any value probably goes for -bin.
Otherwise you would have seen a crash in the grpc library in the test GoogleGrpcIllegalLengthBase64 if you didn't catch with the exception below...

On the PR as a whole, thanks for correcting this check!

omriz · 2021-05-13T20:09:38Z

Updated the code:

Removed the base64 check
Added the correct ASCII range

I did not modify the lower/upper case of the header key

htuch · 2021-05-14T12:54:08Z

source/common/grpc/async_client_manager_impl.cc

Nit: superfluous blank. If you really want you can use return std::all_of style here, but it's a bit of a wash.

htuch · 2021-05-14T12:54:57Z

source/common/grpc/async_client_manager_impl.cc

Nit: you can probably collapse this down to a single if predicate with &&

htuch

LGTM modulo nits.

dmitri-d · 2021-05-14T20:06:00Z

I did not modify the lower/upper case of the header key

I think validateGrpcHeaderChars should enforce lower-case for header names; even the comment above states they will be lower-case. @htuch -- do you have a (strong) opinion on that?

htuch · 2021-05-16T15:22:42Z

@dmitri-d I think it's fine to have input being lower/upper mix; it looks like the C++ gRPC library is robust to this as this was also the previous validation. I'd suspect it just gets forced lower case when put on the wire.

@omriz this need a merge to fix format concerns (see CI). Otherwise ready to ship.

omriz · 2021-05-18T13:16:09Z

@htuch @dmitri-d , pushed (I think) final version according to comments. Please approve.

omriz · 2021-05-18T14:03:45Z

/retest

repokitteh-read-only · 2021-05-18T14:03:48Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16414 (comment) was created by @omriz.

see: more, trace.

According to the GRPC spec https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md The values in the initial metadata can be any valid ASCII character in case of a non binary header type. Signed-off-by: Omri Zohar <omriz@google.com>

omriz · 2021-05-18T19:26:52Z

/retest

repokitteh-read-only · 2021-05-18T19:26:55Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16414 (comment) was created by @omriz.

see: more, trace.

htuch

LGTM, thanks!

@omriz for future it is generally preferable to merge main, avoid rebase and force push. This allows faster validation of deltas between commits.

According to the GRPC spec https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md The values in the initial metadata can be any valid ASCII character in case of a non binary header type. Risk Level: Low Testing: Unit Tests Docs Changes:N/A Release Notes: Fixing GRPC initial metadata validation for ASCII characters Signed-off-by: Omri Zohar <omriz@google.com>

dmitri-d reviewed May 10, 2021

View reviewed changes

htuch reviewed May 11, 2021

View reviewed changes

htuch assigned dmitri-d and markdroth May 11, 2021

omriz force-pushed the metadata_fix branch from d45ae8a to e60da07 Compare May 13, 2021 20:08

omriz force-pushed the metadata_fix branch from e60da07 to 0c7d7fc Compare May 14, 2021 11:48

htuch reviewed May 14, 2021

View reviewed changes

omriz force-pushed the metadata_fix branch from 0c7d7fc to bdb7f8c Compare May 18, 2021 12:20

omriz force-pushed the metadata_fix branch from bdb7f8c to 1486058 Compare May 18, 2021 15:38

htuch approved these changes May 19, 2021

View reviewed changes

Omri Zohar added 2 commits May 19, 2021 13:39

Merge remote-tracking branch 'github/main'

999a495

Merge remote-tracking branch 'github/main'

a06b664

htuch merged commit d304a2f into envoyproxy:main May 20, 2021

Conversation

omriz commented May 10, 2021

Uh oh!

omriz commented May 10, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

omriz commented May 13, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

dmitri-d commented May 14, 2021

Uh oh!

htuch commented May 16, 2021

Uh oh!

omriz commented May 18, 2021

Uh oh!

omriz commented May 18, 2021

Uh oh!

repokitteh-read-only bot commented May 18, 2021

Uh oh!

omriz commented May 18, 2021

Uh oh!

repokitteh-read-only bot commented May 18, 2021

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants