Merge pull request #3021 from ctiller/reject-the-stuffs

Outlaw illegal metadata characters

7 files changed, 38 insertions, 18 deletions

diff --git a/src/core/channel/compress_filter.h b/src/core/channel/compress_filter.h
index 0917e81ca4..415459bca6 100644
--- a/src/core/channel/compress_filter.h
+++ b/src/core/channel/compress_filter.h
@@ -36,7 +36,7 @@
 
 #include "src/core/channel/channel_stack.h"
 
-#define GRPC_COMPRESS_REQUEST_ALGORITHM_KEY "internal:grpc-encoding-request"
+#define GRPC_COMPRESS_REQUEST_ALGORITHM_KEY "grpc-internal-encoding-request"
 
 /** Compression filter for outgoing data.
  *
diff --git a/src/core/surface/call.c b/src/core/surface/call.c
index 33f277da46..4426bbbce9 100644
--- a/src/core/surface/call.c
+++ b/src/core/surface/call.c
@@ -1046,10 +1046,11 @@ static int prepare_application_metadata(grpc_call *call, size_t count,
                                                (const gpr_uint8 *)md->value,
                                                md->value_length, 1);
     if (!grpc_mdstr_is_legal_header(l->md->key)) {
-      gpr_log(GPR_ERROR, "attempt to send invalid metadata key");
+      gpr_log(GPR_ERROR, "attempt to send invalid metadata key: %s",
+              grpc_mdstr_as_c_string(l->md->key));
       return 0;
     } else if (!grpc_mdstr_is_bin_suffixed(l->md->key) &&
-               !grpc_mdstr_is_legal_header(l->md->value)) {
+               !grpc_mdstr_is_legal_nonbin_header(l->md->value)) {
       gpr_log(GPR_ERROR, "attempt to send invalid metadata value");
       return 0;
     }
diff --git a/src/core/transport/metadata.c b/src/core/transport/metadata.c
index f92e87e9dd..3fd21a2f5d 100644
--- a/src/core/transport/metadata.c
+++ b/src/core/transport/metadata.c
@@ -681,16 +681,34 @@ void grpc_mdctx_locked_mdelem_unref(grpc_mdctx *ctx,
 
 void grpc_mdctx_unlock(grpc_mdctx *ctx) { unlock(ctx); }
 
-int grpc_mdstr_is_legal_header(grpc_mdstr *s) {
-  /* TODO(ctiller): consider caching this, or computing it on construction */
+static int conforms_to(grpc_mdstr *s, const gpr_uint8 *legal_bits) {
   const gpr_uint8 *p = GPR_SLICE_START_PTR(s->slice);
   const gpr_uint8 *e = GPR_SLICE_END_PTR(s->slice);
   for (; p != e; p++) {
-    if (*p < 32 || *p > 126) return 0;
+    int idx = *p;
+    int byte = idx / 8;
+    int bit = idx % 8;
+    if ((legal_bits[byte] & (1 << bit)) == 0) return 0;
   }
   return 1;
 }
 
+int grpc_mdstr_is_legal_header(grpc_mdstr *s) {
+  static const gpr_uint8 legal_header_bits[256 / 8] = {
+      0x00, 0x00, 0x00, 0x00, 0x00, 0x20, 0xff, 0x03, 0x00, 0x00, 0x00,
+      0x80, 0xfe, 0xff, 0xff, 0x07, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+      0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
+  return conforms_to(s, legal_header_bits);
+}
+
+int grpc_mdstr_is_legal_nonbin_header(grpc_mdstr *s) {
+  static const gpr_uint8 legal_header_bits[256 / 8] = {
+      0x00, 0x00, 0x00, 0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+      0xff, 0xff, 0xff, 0xff, 0x7f, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+      0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
+  return conforms_to(s, legal_header_bits);
+}
+
 int grpc_mdstr_is_bin_suffixed(grpc_mdstr *s) {
   /* TODO(ctiller): consider caching this */
   return grpc_is_binary_header((const char *)GPR_SLICE_START_PTR(s->slice),
diff --git a/src/core/transport/metadata.h b/src/core/transport/metadata.h
index a7af49ba55..eb17747be7 100644
--- a/src/core/transport/metadata.h
+++ b/src/core/transport/metadata.h
@@ -154,6 +154,7 @@ void grpc_mdelem_unref(grpc_mdelem *md);
 const char *grpc_mdstr_as_c_string(grpc_mdstr *s);
 
 int grpc_mdstr_is_legal_header(grpc_mdstr *s);
+int grpc_mdstr_is_legal_nonbin_header(grpc_mdstr *s);
 int grpc_mdstr_is_bin_suffixed(grpc_mdstr *s);
 
 /* Batch mode metadata functions.
diff --git a/src/python/grpcio_test/grpc_test/_adapter/_links_test.py b/src/python/grpcio_test/grpc_test/_adapter/_links_test.py
index c4686b327a..4077b8e350 100644
--- a/src/python/grpcio_test/grpc_test/_adapter/_links_test.py
+++ b/src/python/grpcio_test/grpc_test/_adapter/_links_test.py
@@ -46,8 +46,8 @@ _TIMEOUT = 32
 # TODO(nathaniel): End-to-end metadata testing.
 def _transform_metadata(unused_metadata):
   return (
-      ('one unused key', 'one unused value'),
-      ('another unused key', 'another unused value'),
+      ('one_unused_key', 'one unused value'),
+      ('another_unused_key', 'another unused value'),
 )
 
 
diff --git a/src/python/grpcio_test/grpc_test/_cython/adapter_low_test.py b/src/python/grpcio_test/grpc_test/_cython/adapter_low_test.py
index 9bab930e56..f1bec238cf 100644
--- a/src/python/grpcio_test/grpc_test/_cython/adapter_low_test.py
+++ b/src/python/grpcio_test/grpc_test/_cython/adapter_low_test.py
@@ -76,7 +76,7 @@ class InsecureServerInsecureClient(unittest.TestCase):
     CLIENT_METADATA_BIN_VALUE = b'\0'*1000
     SERVER_INITIAL_METADATA_KEY = 'init_me_me_me'
     SERVER_INITIAL_METADATA_VALUE = 'whodawha?'
-    SERVER_TRAILING_METADATA_KEY = 'California_is_in_a_drought'
+    SERVER_TRAILING_METADATA_KEY = 'california_is_in_a_drought'
     SERVER_TRAILING_METADATA_VALUE = 'zomg it is'
     SERVER_STATUS_CODE = _types.StatusCode.OK
     SERVER_STATUS_DETAILS = 'our work is never over'
diff --git a/src/python/grpcio_test/grpc_test/_links/_transmission_test.py b/src/python/grpcio_test/grpc_test/_links/_transmission_test.py
index 02ddd512c2..db011bca66 100644
--- a/src/python/grpcio_test/grpc_test/_links/_transmission_test.py
+++ b/src/python/grpcio_test/grpc_test/_links/_transmission_test.py
@@ -66,9 +66,9 @@ class TransmissionTest(test_cases.TransmissionTest, unittest.TestCase):
 
   def create_invocation_initial_metadata(self):
     return (
-        ('first invocation initial metadata key', 'just a string value'),
-        ('second invocation initial metadata key', '0123456789'),
-        ('third invocation initial metadata key-bin', '\x00\x57' * 100),
+        ('first_invocation_initial_metadata_key', 'just a string value'),
+        ('second_invocation_initial_metadata_key', '0123456789'),
+        ('third_invocation_initial_metadata_key-bin', '\x00\x57' * 100),
     )
 
   def create_invocation_terminal_metadata(self):
@@ -76,16 +76,16 @@ class TransmissionTest(test_cases.TransmissionTest, unittest.TestCase):
 
   def create_service_initial_metadata(self):
     return (
-        ('first service initial metadata key', 'just another string value'),
-        ('second service initial metadata key', '9876543210'),
-        ('third service initial metadata key-bin', '\x00\x59\x02' * 100),
+        ('first_service_initial_metadata_key', 'just another string value'),
+        ('second_service_initial_metadata_key', '9876543210'),
+        ('third_service_initial_metadata_key-bin', '\x00\x59\x02' * 100),
     )
 
   def create_service_terminal_metadata(self):
     return (
-        ('first service terminal metadata key', 'yet another string value'),
-        ('second service terminal metadata key', 'abcdefghij'),
-        ('third service terminal metadata key-bin', '\x00\x37' * 100),
+        ('first_service_terminal_metadata_key', 'yet another string value'),
+        ('second_service_terminal_metadata_key', 'abcdefghij'),
+        ('third_service_terminal_metadata_key-bin', '\x00\x37' * 100),
     )
 
   def create_invocation_completion(self):