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

Outlaw illegal metadata characters
crast · Aug 25, 2015 · 6e86944 · 6e86944
2 parents 3cfb479 + b3e56c4
commit 6e86944
Show file tree

Hide file tree

Showing 13 changed files with 169 additions and 19 deletions.
diff --git a/Makefile b/Makefile
@@ -791,6 +791,7 @@ fling_server: $(BINDIR)/$(CONFIG)/fling_server
 fling_stream_test: $(BINDIR)/$(CONFIG)/fling_stream_test
 fling_test: $(BINDIR)/$(CONFIG)/fling_test
 gen_hpack_tables: $(BINDIR)/$(CONFIG)/gen_hpack_tables
+gen_legal_metadata_characters: $(BINDIR)/$(CONFIG)/gen_legal_metadata_characters
 gpr_cmdline_test: $(BINDIR)/$(CONFIG)/gpr_cmdline_test
 gpr_env_test: $(BINDIR)/$(CONFIG)/gpr_env_test
 gpr_file_test: $(BINDIR)/$(CONFIG)/gpr_file_test
@@ -3384,7 +3385,7 @@ test_python: static_c
 tools: tools_c tools_cxx
 
 
-tools_c: privatelibs_c $(BINDIR)/$(CONFIG)/gen_hpack_tables $(BINDIR)/$(CONFIG)/grpc_create_jwt $(BINDIR)/$(CONFIG)/grpc_fetch_oauth2 $(BINDIR)/$(CONFIG)/grpc_print_google_default_creds_token $(BINDIR)/$(CONFIG)/grpc_verify_jwt
+tools_c: privatelibs_c $(BINDIR)/$(CONFIG)/gen_hpack_tables $(BINDIR)/$(CONFIG)/gen_legal_metadata_characters $(BINDIR)/$(CONFIG)/grpc_create_jwt $(BINDIR)/$(CONFIG)/grpc_fetch_oauth2 $(BINDIR)/$(CONFIG)/grpc_print_google_default_creds_token $(BINDIR)/$(CONFIG)/grpc_verify_jwt
 
 tools_cxx: privatelibs_cxx
 
@@ -7120,6 +7121,35 @@ endif
 endif
 
 
+GEN_LEGAL_METADATA_CHARACTERS_SRC = \
+    tools/codegen/core/gen_legal_metadata_characters.c \
+
+GEN_LEGAL_METADATA_CHARACTERS_OBJS = $(addprefix $(OBJDIR)/$(CONFIG)/, $(addsuffix .o, $(basename $(GEN_LEGAL_METADATA_CHARACTERS_SRC))))
+ifeq ($(NO_SECURE),true)
+
+# You can't build secure targets if you don't have OpenSSL.
+
+$(BINDIR)/$(CONFIG)/gen_legal_metadata_characters: openssl_dep_error
+
+else
+
+$(BINDIR)/$(CONFIG)/gen_legal_metadata_characters: $(GEN_LEGAL_METADATA_CHARACTERS_OBJS)
+	$(E) "[LD]      Linking $@"
+	$(Q) mkdir -p `dirname $@`
+	$(Q) $(LD) $(LDFLAGS) $(GEN_LEGAL_METADATA_CHARACTERS_OBJS) $(LDLIBS) $(LDLIBS_SECURE) -o $(BINDIR)/$(CONFIG)/gen_legal_metadata_characters
+
+endif
+
+$(OBJDIR)/$(CONFIG)/tools/codegen/core/gen_legal_metadata_characters.o: 
+deps_gen_legal_metadata_characters: $(GEN_LEGAL_METADATA_CHARACTERS_OBJS:.o=.dep)
+
+ifneq ($(NO_SECURE),true)
+ifneq ($(NO_DEPS),true)
+-include $(GEN_LEGAL_METADATA_CHARACTERS_OBJS:.o=.dep)
+endif
+endif
+
+
 GPR_CMDLINE_TEST_SRC = \
     test/core/support/cmdline_test.c \
 

diff --git a/build.json b/build.json
@@ -1148,6 +1148,15 @@
         "grpc"
       ]
     },
+    {
+      "name": "gen_legal_metadata_characters",
+      "build": "tool",
+      "language": "c",
+      "src": [
+        "tools/codegen/core/gen_legal_metadata_characters.c"
+      ],
+      "deps": []
+    },
     {
       "name": "gpr_cmdline_test",
       "build": "test",

diff --git 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
@@ -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
@@ -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
@@ -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
@@ -46,8 +46,8 @@
 # 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
@@ -76,7 +76,7 @@ def testEcho(self):
     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
@@ -66,26 +66,26 @@ def destroy_transmitting_links(self, invocation_side_link, service_side_link):
 
   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):
     return None
 
   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):

diff --git a/tools/codegen/core/gen_legal_metadata_characters.c b/tools/codegen/core/gen_legal_metadata_characters.c
@@ -0,0 +1,73 @@
+/*
+ *
+ * Copyright 2015, Google Inc.
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are
+ * met:
+ *
+ *     * Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above
+ * copyright notice, this list of conditions and the following disclaimer
+ * in the documentation and/or other materials provided with the
+ * distribution.
+ *     * Neither the name of Google Inc. nor the names of its
+ * contributors may be used to endorse or promote products derived from
+ * this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ *
+ */
+
+/* generates constant table for metadata.c */
+
+#include <stdio.h>
+#include <string.h>
+
+static unsigned char legal_bits[256 / 8];
+
+static void legal(int x) {
+  int byte = x / 8;
+  int bit = x % 8;
+  legal_bits[byte] |= 1 << bit;
+}
+
+static void dump(void) {
+  int i;
+
+  printf("static const gpr_uint8 legal_header_bits[256/8] = ");
+  for (i = 0; i < 256 / 8; i++)
+    printf("%c 0x%02x", i ? ',' : '{', legal_bits[i]);
+  printf(" };\n");
+}
+
+static void clear(void) { memset(legal_bits, 0, sizeof(legal_bits)); }
+
+int main(void) {
+  int i;
+
+  clear();
+  for (i = 'a'; i <= 'z'; i++) legal(i);
+  for (i = '0'; i <= '9'; i++) legal(i);
+  legal('-');
+  legal('_');
+  dump();
+
+  clear();
+  for (i = 32; i <= 126; i++) legal(i);
+  dump();
+
+  return 0;
+}
diff --git a/tools/run_tests/jobset.py b/tools/run_tests/jobset.py
@@ -174,6 +174,7 @@ def __init__(self, spec, bin_hash, newline_on_success, travis, add_env, xml_repo
     for k, v in add_env.iteritems():
       env[k] = v
     self._start = time.time()
+    print spec.cmdline
     self._process = subprocess.Popen(args=spec.cmdline,
                                      stderr=subprocess.STDOUT,
                                      stdout=self._tempfile,

diff --git a/tools/run_tests/sources_and_headers.json b/tools/run_tests/sources_and_headers.json
@@ -237,6 +237,15 @@
       "tools/codegen/core/gen_hpack_tables.c"
     ]
   }, 
+  {
+    "deps": [], 
+    "headers": [], 
+    "language": "c", 
+    "name": "gen_legal_metadata_characters", 
+    "src": [
+      "tools/codegen/core/gen_legal_metadata_characters.c"
+    ]
+  }, 
   {
     "deps": [
       "gpr", 

diff --git a/vsprojects/Grpc.mak b/vsprojects/Grpc.mak
@@ -183,6 +183,14 @@ gen_hpack_tables: gen_hpack_tables.exe
 	echo Running gen_hpack_tables
 	$(OUT_DIR)\gen_hpack_tables.exe
 
+gen_legal_metadata_characters.exe: $(OUT_DIR)
+	echo Building gen_legal_metadata_characters
+	$(CC) $(CFLAGS) /Fo:$(OUT_DIR)\ $(REPO_ROOT)\tools\codegen\core\gen_legal_metadata_characters.c 
+	$(LINK) $(LFLAGS) /OUT:"$(OUT_DIR)\gen_legal_metadata_characters.exe" $(LIBS) $(OUT_DIR)\gen_legal_metadata_characters.obj 
+gen_legal_metadata_characters: gen_legal_metadata_characters.exe
+	echo Running gen_legal_metadata_characters
+	$(OUT_DIR)\gen_legal_metadata_characters.exe
+
 gpr_cmdline_test.exe: build_gpr_test_util build_gpr $(OUT_DIR)
 	echo Building gpr_cmdline_test
 	$(CC) $(CFLAGS) /Fo:$(OUT_DIR)\ $(REPO_ROOT)\test\core\support\cmdline_test.c