From 7a2bb0f01eebd259ae7babbfa55cffa87afa97ad Mon Sep 17 00:00:00 2001 From: Sage Weil Date: Thu, 14 Mar 2019 16:46:21 -0500 Subject: [PATCH 1/3] mon: do not assert on bad auth payload If we get garbage, fail to authenticate--do not assert out and crash. Signed-off-by: Sage Weil --- src/mon/Monitor.cc | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/src/mon/Monitor.cc b/src/mon/Monitor.cc index 70c8b5476ff..cd4cec304e0 100644 --- a/src/mon/Monitor.cc +++ b/src/mon/Monitor.cc @@ -6195,10 +6195,22 @@ int Monitor::handle_auth_request( uint8_t mode; EntityName entity_name; - decode(mode, p); - assert(mode >= AUTH_MODE_MON && mode <= AUTH_MODE_MON_MAX); - decode(entity_name, p); - decode(con->peer_global_id, p); + try { + decode(mode, p); + if (mode < AUTH_MODE_MON || + mode > AUTH_MODE_MON_MAX) { + dout(1) << __func__ << " invalid mode " << (int)mode << dendl; + delete auth_handler; + return -EACCES; + } + assert(mode >= AUTH_MODE_MON && mode <= AUTH_MODE_MON_MAX); + decode(entity_name, p); + decode(con->peer_global_id, p); + } catch (buffer::error& e) { + dout(1) << __func__ << " failed to decode, " << e.what() << dendl; + delete auth_handler; + return -EACCES; + } // supported method? if (entity_name.get_type() == CEPH_ENTITY_TYPE_MON || From 26b848d7c9af3398b4d4f37bb1c345a332a912ec Mon Sep 17 00:00:00 2001 From: Kefu Chai Date: Fri, 15 Mar 2019 11:22:13 +0800 Subject: [PATCH 2/3] msg/async: do not "return std::move(local_var)" it prevents copy elision. and both GCC and Clang warn like warning: moving a local object in a return statement prevents copy elision [-Wpessimizing-move] Signed-off-by: Kefu Chai --- src/msg/async/crypto_onwire.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/msg/async/crypto_onwire.cc b/src/msg/async/crypto_onwire.cc index 0fa35cac409..3858091357d 100644 --- a/src/msg/async/crypto_onwire.cc +++ b/src/msg/async/crypto_onwire.cc @@ -225,7 +225,7 @@ ceph::bufferlist AES128GCM_OnWireRxHandler::authenticated_decrypt_update( ceph::bufferlist outbl; outbl.push_back(std::move(plainnode)); - return std::move(outbl); + return outbl; } From 0500fff76b088f34c3ae81ab4305eed507051027 Mon Sep 17 00:00:00 2001 From: Kefu Chai Date: Fri, 15 Mar 2019 11:27:53 +0800 Subject: [PATCH 3/3] mon: avoid using naked pointer to save our trouble to delete auth_handler in the error handling paths Signed-off-by: Kefu Chai --- src/mon/Monitor.cc | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/src/mon/Monitor.cc b/src/mon/Monitor.cc index cd4cec304e0..7eeef1dad07 100644 --- a/src/mon/Monitor.cc +++ b/src/mon/Monitor.cc @@ -6184,8 +6184,8 @@ int Monitor::handle_auth_request( } // handler? - AuthServiceHandler *auth_handler = get_auth_service_handler( - auth_method, g_ceph_context, &key_server); + unique_ptr auth_handler{get_auth_service_handler( + auth_method, g_ceph_context, &key_server)}; if (!auth_handler) { dout(1) << __func__ << " auth_method " << auth_method << " not supported" << dendl; @@ -6200,7 +6200,6 @@ int Monitor::handle_auth_request( if (mode < AUTH_MODE_MON || mode > AUTH_MODE_MON_MAX) { dout(1) << __func__ << " invalid mode " << (int)mode << dendl; - delete auth_handler; return -EACCES; } assert(mode >= AUTH_MODE_MON && mode <= AUTH_MODE_MON_MAX); @@ -6208,7 +6207,6 @@ int Monitor::handle_auth_request( decode(con->peer_global_id, p); } catch (buffer::error& e) { dout(1) << __func__ << " failed to decode, " << e.what() << dendl; - delete auth_handler; return -EACCES; } @@ -6221,7 +6219,6 @@ int Monitor::handle_auth_request( dout(10) << __func__ << " entity " << entity_name << " method " << auth_method << " not among supported " << auth_cluster_required.get_supported_set() << dendl; - delete auth_handler; return -EOPNOTSUPP; } } else { @@ -6229,7 +6226,6 @@ int Monitor::handle_auth_request( dout(10) << __func__ << " entity " << entity_name << " method " << auth_method << " not among supported " << auth_cluster_required.get_supported_set() << dendl; - delete auth_handler; return -EOPNOTSUPP; } } @@ -6242,7 +6238,6 @@ int Monitor::handle_auth_request( con->peer_global_id = authmon()->_assign_global_id(); if (!con->peer_global_id) { dout(1) << __func__ << " failed to assign global_id" << dendl; - delete auth_handler; return -EBUSY; } dout(10) << __func__ << " assigned global_id " << con->peer_global_id @@ -6251,7 +6246,7 @@ int Monitor::handle_auth_request( // set up partial session s = new MonSession(con); - s->auth_handler = auth_handler; + s->auth_handler = auth_handler.release(); con->set_priv(RefCountedPtr{s, false}); r = s->auth_handler->start_session(