From 49772e00eb9606c3192b88f348d9cbcb22eb93c2 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Fri, 21 Aug 2015 08:08:37 -0700 Subject: Outlaw illegal metadata characters --- Makefile | 32 +++++++++- build.json | 10 +++ src/core/channel/compress_filter.h | 2 +- src/core/surface/call.c | 5 +- src/core/transport/metadata.c | 26 +++++++- src/core/transport/metadata.h | 1 + tools/codegen/core/gen_legal_metadata_characters.c | 73 ++++++++++++++++++++++ tools/run_tests/sources_and_headers.json | 12 ++++ tools/run_tests/tests.json | 1 + vsprojects/Grpc.mak | 8 +++ 10 files changed, 163 insertions(+), 7 deletions(-) create mode 100644 tools/codegen/core/gen_legal_metadata_characters.c diff --git a/Makefile b/Makefile index 31628b4412..f7ace3186a 100644 --- a/Makefile +++ b/Makefile @@ -793,6 +793,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 @@ -3386,7 +3387,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 @@ -7122,6 +7123,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) $(LIBDIR)/$(CONFIG)/libgpr.a $(LIBDIR)/$(CONFIG)/libgrpc.a + $(E) "[LD] Linking $@" + $(Q) mkdir -p `dirname $@` + $(Q) $(LD) $(LDFLAGS) $(GEN_LEGAL_METADATA_CHARACTERS_OBJS) $(LIBDIR)/$(CONFIG)/libgpr.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LDLIBS) $(LDLIBS_SECURE) -o $(BINDIR)/$(CONFIG)/gen_legal_metadata_characters + +endif + +$(OBJDIR)/$(CONFIG)/tools/codegen/core/gen_legal_metadata_characters.o: $(LIBDIR)/$(CONFIG)/libgpr.a $(LIBDIR)/$(CONFIG)/libgrpc.a +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 index bd707d2e34..c6a670970b 100644 --- a/build.json +++ b/build.json @@ -1143,6 +1143,16 @@ "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 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..d758351feb 100644 --- a/src/core/transport/metadata.c +++ b/src/core/transport/metadata.c @@ -681,16 +681,36 @@ 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, 0xfe, 0xff, 0xff, + 0x07, 0xfe, 0xff, 0xff, 0x07, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}; + + /* TODO(ctiller): consider caching this, or computing it on construction */ + 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/tools/codegen/core/gen_legal_metadata_characters.c b/tools/codegen/core/gen_legal_metadata_characters.c new file mode 100644 index 0000000000..5e32d91d50 --- /dev/null +++ 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 +#include + +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 = 'A'; i <= 'Z'; i++) legal(i); + for (i = '0'; i <= '9'; i++) legal(i); + legal('-'); + dump(); + + clear(); + for (i = 32; i <= 126; i++) legal(i); + dump(); + + return 0; +} diff --git a/tools/run_tests/sources_and_headers.json b/tools/run_tests/sources_and_headers.json index 50f078586d..142c84901d 100644 --- a/tools/run_tests/sources_and_headers.json +++ b/tools/run_tests/sources_and_headers.json @@ -237,6 +237,18 @@ "tools/codegen/core/gen_hpack_tables.c" ] }, + { + "deps": [ + "gpr", + "grpc" + ], + "headers": [], + "language": "c", + "name": "gen_legal_metadata_characters", + "src": [ + "tools/codegen/core/gen_legal_metadata_characters.c" + ] + }, { "deps": [ "gpr", diff --git a/tools/run_tests/tests.json b/tools/run_tests/tests.json index 6c06d74834..127b1dfc40 100644 --- a/tools/run_tests/tests.json +++ b/tools/run_tests/tests.json @@ -1538,6 +1538,7 @@ "posix", "windows" ], + "exclude_configs": [], "flaky": false, "language": "c++", "name": "status_test", diff --git a/vsprojects/Grpc.mak b/vsprojects/Grpc.mak index 662de784f7..3d853d9c76 100644 --- 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: build_gpr build_grpc $(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" Debug\gpr.lib Debug\grpc.lib $(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 -- cgit v1.2.3