Fixing GRPC initial metadata validation #16414

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

+    // ASCII header
+    } else {
+      for (auto ch : header.value()) {
+        if (!(absl::ascii_isascii(ch))) {


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 (!validateGrpcHeaderChars(header.key()) || !validateGrpcHeaderChars(header.value())) {
-      throw EnvoyException("Illegal characters in gRPC initial metadata.");
+    // Validate key
+    if (!validateGrpcHeaderChars(header.key())) {


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

+
+    // Validate value
+    // Binary base64 encoded
+    if (::absl::EndsWith(header.key(), "-bin")) {


@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

+    }
+  }
+  return true;
+


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

+    if (::absl::EndsWith(header.key(), "-bin")) {
+      continue;
+    // ASCII header
+    } else {


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

Fixing GRPC initial metadata validation

1486058

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 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing GRPC initial metadata validation #16414

Fixing GRPC initial metadata validation #16414

omriz commented May 10, 2021

omriz commented May 10, 2021

dmitri-d May 10, 2021

omriz May 12, 2021

dmitri-d May 10, 2021

omriz May 12, 2021

markdroth May 12, 2021

htuch May 11, 2021

markdroth May 11, 2021

htuch May 12, 2021

omriz May 12, 2021

markdroth May 12, 2021

htuch May 13, 2021

asraa May 13, 2021

omriz commented May 13, 2021

htuch May 14, 2021

htuch May 14, 2021

htuch left a comment

dmitri-d commented May 14, 2021

htuch commented May 16, 2021

omriz commented May 18, 2021

omriz commented May 18, 2021

repokitteh-read-only bot commented May 18, 2021

omriz commented May 18, 2021

repokitteh-read-only bot commented May 18, 2021

htuch left a comment

Fixing GRPC initial metadata validation #16414

Fixing GRPC initial metadata validation #16414

Conversation

omriz commented May 10, 2021

omriz commented May 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omriz commented May 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

dmitri-d commented May 14, 2021

htuch commented May 16, 2021

omriz commented May 18, 2021

omriz commented May 18, 2021

repokitteh-read-only bot commented May 18, 2021

omriz commented May 18, 2021

repokitteh-read-only bot commented May 18, 2021

htuch left a comment

Choose a reason for hiding this comment