From b88c0a22c743a6d09a35d1754f1e125f03a76792 Mon Sep 17 00:00:00 2001 From: Alexander Polcyn Date: Thu, 25 Jul 2019 11:46:32 -0700 Subject: [PATCH] Fix use-after-free in ruby call creds --- grpc.def | 2 + include/grpc/grpc_security.h | 8 + .../lib/security/transport/auth_filters.h | 5 - .../call_credentials_timeout_driver.rb | 153 ++++++++++++++++++ src/ruby/end2end/end2end_common.rb | 5 +- src/ruby/ext/grpc/rb_call_credentials.c | 5 +- src/ruby/ext/grpc/rb_grpc_imports.generated.c | 4 + src/ruby/ext/grpc/rb_grpc_imports.generated.h | 6 + src/ruby/spec/support/services.rb | 14 +- .../core/surface/public_headers_must_be_c89.c | 2 + .../helper_scripts/run_ruby_end2end_tests.sh | 1 + 11 files changed, 193 insertions(+), 12 deletions(-) create mode 100755 src/ruby/end2end/call_credentials_timeout_driver.rb diff --git a/grpc.def b/grpc.def index 2f753b29939..ad0dca2f0d4 100644 --- a/grpc.def +++ b/grpc.def @@ -112,6 +112,8 @@ EXPORTS grpc_access_token_credentials_create grpc_google_iam_credentials_create grpc_sts_credentials_create + grpc_auth_metadata_context_copy + grpc_auth_metadata_context_reset grpc_metadata_credentials_create_from_plugin grpc_secure_channel_create grpc_server_credentials_release diff --git a/include/grpc/grpc_security.h b/include/grpc/grpc_security.h index e910b86596b..99635ea8807 100644 --- a/include/grpc/grpc_security.h +++ b/include/grpc/grpc_security.h @@ -390,6 +390,14 @@ typedef struct { void* reserved; } grpc_auth_metadata_context; +/** Performs a deep copy from \a from to \a to. **/ +GRPCAPI void grpc_auth_metadata_context_copy(grpc_auth_metadata_context* from, + grpc_auth_metadata_context* to); + +/** Releases internal resources held by \a context. **/ +GRPCAPI void grpc_auth_metadata_context_reset( + grpc_auth_metadata_context* context); + /** Maximum number of metadata entries returnable by a credentials plugin via a synchronous return. */ #define GRPC_METADATA_CREDENTIALS_PLUGIN_SYNC_MAX 4 diff --git a/src/core/lib/security/transport/auth_filters.h b/src/core/lib/security/transport/auth_filters.h index 5e73c21cc44..ae31b1d578b 100644 --- a/src/core/lib/security/transport/auth_filters.h +++ b/src/core/lib/security/transport/auth_filters.h @@ -32,9 +32,4 @@ void grpc_auth_metadata_context_build( const grpc_slice& call_method, grpc_auth_context* auth_context, grpc_auth_metadata_context* auth_md_context); -void grpc_auth_metadata_context_copy(grpc_auth_metadata_context* from, - grpc_auth_metadata_context* to); - -void grpc_auth_metadata_context_reset(grpc_auth_metadata_context* context); - #endif /* GRPC_CORE_LIB_SECURITY_TRANSPORT_AUTH_FILTERS_H */ diff --git a/src/ruby/end2end/call_credentials_timeout_driver.rb b/src/ruby/end2end/call_credentials_timeout_driver.rb new file mode 100755 index 00000000000..7618b14b974 --- /dev/null +++ b/src/ruby/end2end/call_credentials_timeout_driver.rb @@ -0,0 +1,153 @@ +#!/usr/bin/env ruby +# +# Copyright 2016 gRPC authors. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +this_dir = File.expand_path(File.dirname(__FILE__)) +protos_lib_dir = File.join(this_dir, 'lib') +grpc_lib_dir = File.join(File.dirname(this_dir), 'lib') +$LOAD_PATH.unshift(grpc_lib_dir) unless $LOAD_PATH.include?(grpc_lib_dir) +$LOAD_PATH.unshift(protos_lib_dir) unless $LOAD_PATH.include?(protos_lib_dir) +$LOAD_PATH.unshift(this_dir) unless $LOAD_PATH.include?(this_dir) + +require 'grpc' +require 'end2end_common' + +def create_channel_creds + test_root = File.join(File.dirname(__FILE__), '..', 'spec', 'testdata') + files = ['ca.pem', 'client.key', 'client.pem'] + creds = files.map { |f| File.open(File.join(test_root, f)).read } + GRPC::Core::ChannelCredentials.new(creds[0], creds[1], creds[2]) +end + +def client_cert + test_root = File.join(File.dirname(__FILE__), '..', 'spec', 'testdata') + cert = File.open(File.join(test_root, 'client.pem')).read + fail unless cert.is_a?(String) + cert +end + +def create_server_creds + test_root = File.join(File.dirname(__FILE__), '..', 'spec', 'testdata') + GRPC.logger.info("test root: #{test_root}") + files = ['ca.pem', 'server1.key', 'server1.pem'] + creds = files.map { |f| File.open(File.join(test_root, f)).read } + GRPC::Core::ServerCredentials.new( + creds[0], + [{ private_key: creds[1], cert_chain: creds[2] }], + true) # force client auth +end + +# Useful to update a value within a do block +class MutableValue + attr_accessor :value + + def initialize(value) + @value = value + end +end + +# rubocop:disable Metrics/AbcSize +# rubocop:disable Metrics/MethodLength +def main + server_runner = ServerRunner.new(EchoServerImpl) + server_runner.server_creds = create_server_creds + server_port = server_runner.run + channel_args = { + GRPC::Core::Channel::SSL_TARGET => 'foo.test.google.fr' + } + token_fetch_attempts = MutableValue.new(0) + token_fetch_attempts_mu = Mutex.new + jwt_aud_uri_extraction_success_count = MutableValue.new(0) + jwt_aud_uri_extraction_success_count_mu = Mutex.new + expected_jwt_aud_uri = 'https://foo.test.google.fr/echo.EchoServer' + jwt_aud_uri_failure_values = [] + times_out_first_time_auth_proc = proc do |args| + # We check the value of jwt_aud_uri not necessarily as a test for + # the correctness of jwt_aud_uri w.r.t. its expected semantics, but + # more for as an indirect way to check for memory corruption. + jwt_aud_uri_extraction_success_count_mu.synchronize do + if args[:jwt_aud_uri] == expected_jwt_aud_uri + jwt_aud_uri_extraction_success_count.value += 1 + else + jwt_aud_uri_failure_values << args[:jwt_aud_uri] + end + end + token_fetch_attempts_mu.synchronize do + old_val = token_fetch_attempts.value + token_fetch_attempts.value += 1 + if old_val.zero? + STDERR.puts 'call creds plugin sleeping for 4 seconds' + sleep 4 + STDERR.puts 'call creds plugin done with 4 second sleep' + raise 'test exception thrown purposely from call creds plugin' + end + end + { 'authorization' => 'fake_val' }.merge(args) + end + channel_creds = create_channel_creds.compose( + GRPC::Core::CallCredentials.new(times_out_first_time_auth_proc)) + stub = Echo::EchoServer::Stub.new("localhost:#{server_port}", + channel_creds, + channel_args: channel_args) + STDERR.puts 'perform a first few RPCs to try to get things into a bad state...' + threads = [] + got_at_least_one_failure = MutableValue.new(false) + 2000.times do + threads << Thread.new do + begin + # 2 seconds is chosen as deadline here because it is less than the 4 second + # sleep that the first call creds user callback does. The idea here is that + # a lot of RPCs will be made concurrently all with 2 second deadlines, and they + # will all queue up onto the call creds user callback thread, and will all + # have to wait for the first 4 second sleep to finish. When the deadlines + # of the associated calls fire ~2 seconds in, some of their C-core data + # will have ownership dropped, and they will hit the user-after-free in + # https://github.com/grpc/grpc/issues/19195 if this isn't handled correctly. + stub.echo(Echo::EchoRequest.new(request: 'hello'), deadline: Time.now + 2) + rescue GRPC::BadStatus + got_at_least_one_failure.value = true + # We don't care if these RPCs succeed or fail. The purpose of these + # RPCs is just to try to induce a specific use-after-free bug, and to get + # the call credentials callback thread into a bad state. + end + end + end + threads.each(&:join) + unless got_at_least_one_failure.value + fail 'expected at least one of the initial RPCs to fail' + end + # Expect three more RPCs to succeed + STDERR.puts 'now perform another RPC and expect OK...' + stub.echo(Echo::EchoRequest.new(request: 'hello'), deadline: Time.now + 10) + STDERR.puts 'now perform another RPC and expect OK...' + stub.echo(Echo::EchoRequest.new(request: 'hello'), deadline: Time.now + 10) + STDERR.puts 'now perform another RPC and expect OK...' + stub.echo(Echo::EchoRequest.new(request: 'hello'), deadline: Time.now + 10) + jwt_aud_uri_extraction_success_count_mu.synchronize do + if jwt_aud_uri_extraction_success_count.value != 2003 + fail "Expected to get jwt_aud_uri:#{expected_jwt_aud_uri} passed to call creds +user callback 2003 times, but it was only passed to the call creds user callback +#{jwt_aud_uri_extraction_success_count.value} times. This suggests that either: +a) the expected jwt_aud_uri value is incorrect +b) there is some corruption of the jwt_aud_uri argument +Here are are the values of the jwt_aud_uri parameter that were passed to the call +creds user callback that did not match #{expected_jwt_aud_uri}: +#{jwt_aud_uri_failure_values}" + end + end + server_runner.stop +end + +main diff --git a/src/ruby/end2end/end2end_common.rb b/src/ruby/end2end/end2end_common.rb index ffbaa1986d0..1bc17066735 100755 --- a/src/ruby/end2end/end2end_common.rb +++ b/src/ruby/end2end/end2end_common.rb @@ -43,14 +43,17 @@ end # ServerRunner starts an "echo server" that test clients can make calls to class ServerRunner + attr_accessor :server_creds + def initialize(service_impl, rpc_server_args: {}) @service_impl = service_impl @rpc_server_args = rpc_server_args + @server_creds = :this_port_is_insecure end def run @srv = new_rpc_server_for_testing(@rpc_server_args) - port = @srv.add_http2_port('0.0.0.0:0', :this_port_is_insecure) + port = @srv.add_http2_port('0.0.0.0:0', @server_creds) @srv.handle(@service_impl) @thd = Thread.new do diff --git a/src/ruby/ext/grpc/rb_call_credentials.c b/src/ruby/ext/grpc/rb_call_credentials.c index 61170cc73b2..a63d5dd5aa3 100644 --- a/src/ruby/ext/grpc/rb_call_credentials.c +++ b/src/ruby/ext/grpc/rb_call_credentials.c @@ -109,6 +109,7 @@ static void grpc_rb_call_credentials_callback_with_gil(void* param) { params->callback(params->user_data, md_ary.metadata, md_ary.count, status, error_details); grpc_rb_metadata_array_destroy_including_entries(&md_ary); + grpc_auth_metadata_context_reset(¶ms->context); gpr_free(params); } @@ -118,9 +119,9 @@ static int grpc_rb_call_credentials_plugin_get_metadata( grpc_metadata creds_md[GRPC_METADATA_CREDENTIALS_PLUGIN_SYNC_MAX], size_t* num_creds_md, grpc_status_code* status, const char** error_details) { - callback_params* params = gpr_malloc(sizeof(callback_params)); + callback_params* params = gpr_zalloc(sizeof(callback_params)); params->get_metadata = (VALUE)state; - params->context = context; + grpc_auth_metadata_context_copy(&context, ¶ms->context); params->user_data = user_data; params->callback = cb; diff --git a/src/ruby/ext/grpc/rb_grpc_imports.generated.c b/src/ruby/ext/grpc/rb_grpc_imports.generated.c index 4e50a5c3ea7..37b864d5be4 100644 --- a/src/ruby/ext/grpc/rb_grpc_imports.generated.c +++ b/src/ruby/ext/grpc/rb_grpc_imports.generated.c @@ -135,6 +135,8 @@ grpc_google_refresh_token_credentials_create_type grpc_google_refresh_token_cred grpc_access_token_credentials_create_type grpc_access_token_credentials_create_import; grpc_google_iam_credentials_create_type grpc_google_iam_credentials_create_import; grpc_sts_credentials_create_type grpc_sts_credentials_create_import; +grpc_auth_metadata_context_copy_type grpc_auth_metadata_context_copy_import; +grpc_auth_metadata_context_reset_type grpc_auth_metadata_context_reset_import; grpc_metadata_credentials_create_from_plugin_type grpc_metadata_credentials_create_from_plugin_import; grpc_secure_channel_create_type grpc_secure_channel_create_import; grpc_server_credentials_release_type grpc_server_credentials_release_import; @@ -407,6 +409,8 @@ void grpc_rb_load_imports(HMODULE library) { grpc_access_token_credentials_create_import = (grpc_access_token_credentials_create_type) GetProcAddress(library, "grpc_access_token_credentials_create"); grpc_google_iam_credentials_create_import = (grpc_google_iam_credentials_create_type) GetProcAddress(library, "grpc_google_iam_credentials_create"); grpc_sts_credentials_create_import = (grpc_sts_credentials_create_type) GetProcAddress(library, "grpc_sts_credentials_create"); + grpc_auth_metadata_context_copy_import = (grpc_auth_metadata_context_copy_type) GetProcAddress(library, "grpc_auth_metadata_context_copy"); + grpc_auth_metadata_context_reset_import = (grpc_auth_metadata_context_reset_type) GetProcAddress(library, "grpc_auth_metadata_context_reset"); grpc_metadata_credentials_create_from_plugin_import = (grpc_metadata_credentials_create_from_plugin_type) GetProcAddress(library, "grpc_metadata_credentials_create_from_plugin"); grpc_secure_channel_create_import = (grpc_secure_channel_create_type) GetProcAddress(library, "grpc_secure_channel_create"); grpc_server_credentials_release_import = (grpc_server_credentials_release_type) GetProcAddress(library, "grpc_server_credentials_release"); diff --git a/src/ruby/ext/grpc/rb_grpc_imports.generated.h b/src/ruby/ext/grpc/rb_grpc_imports.generated.h index cf351a50a19..260d673a668 100644 --- a/src/ruby/ext/grpc/rb_grpc_imports.generated.h +++ b/src/ruby/ext/grpc/rb_grpc_imports.generated.h @@ -380,6 +380,12 @@ extern grpc_google_iam_credentials_create_type grpc_google_iam_credentials_creat typedef grpc_call_credentials*(*grpc_sts_credentials_create_type)(const grpc_sts_credentials_options* options, void* reserved); extern grpc_sts_credentials_create_type grpc_sts_credentials_create_import; #define grpc_sts_credentials_create grpc_sts_credentials_create_import +typedef void(*grpc_auth_metadata_context_copy_type)(grpc_auth_metadata_context* from, grpc_auth_metadata_context* to); +extern grpc_auth_metadata_context_copy_type grpc_auth_metadata_context_copy_import; +#define grpc_auth_metadata_context_copy grpc_auth_metadata_context_copy_import +typedef void(*grpc_auth_metadata_context_reset_type)(grpc_auth_metadata_context* context); +extern grpc_auth_metadata_context_reset_type grpc_auth_metadata_context_reset_import; +#define grpc_auth_metadata_context_reset grpc_auth_metadata_context_reset_import typedef grpc_call_credentials*(*grpc_metadata_credentials_create_from_plugin_type)(grpc_metadata_credentials_plugin plugin, grpc_security_level min_security_level, void* reserved); extern grpc_metadata_credentials_create_from_plugin_type grpc_metadata_credentials_create_from_plugin_import; #define grpc_metadata_credentials_create_from_plugin grpc_metadata_credentials_create_from_plugin_import diff --git a/src/ruby/spec/support/services.rb b/src/ruby/spec/support/services.rb index 438459dfd79..a5d8e7c187b 100644 --- a/src/ruby/spec/support/services.rb +++ b/src/ruby/spec/support/services.rb @@ -17,12 +17,18 @@ require 'spec_helper' # A test message class EchoMsg - def self.marshal(_o) - '' + attr_reader :msg + + def initialize(msg: '') + @msg = msg end - def self.unmarshal(_o) - EchoMsg.new + def self.marshal(o) + o.msg + end + + def self.unmarshal(msg) + EchoMsg.new(msg: msg) end end diff --git a/test/core/surface/public_headers_must_be_c89.c b/test/core/surface/public_headers_must_be_c89.c index b6ba715e247..1b119317969 100644 --- a/test/core/surface/public_headers_must_be_c89.c +++ b/test/core/surface/public_headers_must_be_c89.c @@ -179,6 +179,8 @@ int main(int argc, char **argv) { printf("%lx", (unsigned long) grpc_access_token_credentials_create); printf("%lx", (unsigned long) grpc_google_iam_credentials_create); printf("%lx", (unsigned long) grpc_sts_credentials_create); + printf("%lx", (unsigned long) grpc_auth_metadata_context_copy); + printf("%lx", (unsigned long) grpc_auth_metadata_context_reset); printf("%lx", (unsigned long) grpc_metadata_credentials_create_from_plugin); printf("%lx", (unsigned long) grpc_secure_channel_create); printf("%lx", (unsigned long) grpc_server_credentials_release); diff --git a/tools/run_tests/helper_scripts/run_ruby_end2end_tests.sh b/tools/run_tests/helper_scripts/run_ruby_end2end_tests.sh index fc0759fc836..304814c2553 100755 --- a/tools/run_tests/helper_scripts/run_ruby_end2end_tests.sh +++ b/tools/run_tests/helper_scripts/run_ruby_end2end_tests.sh @@ -35,4 +35,5 @@ time ruby src/ruby/end2end/graceful_sig_stop_driver.rb || EXIT_CODE=1 time ruby src/ruby/end2end/errors_load_before_grpc_lib.rb || EXIT_CODE=1 time ruby src/ruby/end2end/logger_load_before_grpc_lib.rb || EXIT_CODE=1 time ruby src/ruby/end2end/status_codes_load_before_grpc_lib.rb || EXIT_CODE=1 +time ruby src/ruby/end2end/call_credentials_timeout_driver.rb || EXIT_CODE=1 exit $EXIT_CODE