You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by kw...@apache.org on 2017/12/07 17:49:52 UTC
[3/4] impala git commit: IMPALA-6281: Fix use-after-free in InitAuth()
IMPALA-6281: Fix use-after-free in InitAuth()
Previously, we implicitly create a local string object created from
the char* in argv[0] when calling InitAuth(). This string object goes
out of scope once InitAuth() returns but the pointer of this local
string's buffer is passed to the Sasl library which may reference
it after the local string has been deleted, leading to use-after-free.
This bug is exposed by recent change to enable Kerberos with KRPC as
we now always initialize Sasl even if Kerberos is not enabled.
This change fixes the problem above by making a copy of 'appname'
passed to InitAuth(). Also, the new code enforces that multiple
calls to InitAuth() must use the same 'appname' or it will fail.
Testing done: Verified rpc-mgr-test and thrift-server-test no longer
fail in ASAN build.
Change-Id: I1f29c2396df114264dfc23726b8ba778f50e12e9
Reviewed-on: http://gerrit.cloudera.org:8080/8777
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
Reviewed-by: Lars Volker <lv...@cloudera.com>
Tested-by: Impala Public Jenkins
Project: http://git-wip-us.apache.org/repos/asf/impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/a4916e6d
Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/a4916e6d
Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/a4916e6d
Branch: refs/heads/master
Commit: a4916e6d5f5f3542100af791534bfaf9ed544720
Parents: ed72910
Author: Michael Ho <kw...@cloudera.com>
Authored: Tue Dec 5 23:01:00 2017 -0800
Committer: Impala Public Jenkins <im...@gerrit.cloudera.org>
Committed: Thu Dec 7 09:48:50 2017 +0000
----------------------------------------------------------------------
be/src/rpc/auth-provider.h | 9 ++++-----
be/src/rpc/authentication.cc | 13 ++++++++++++-
be/src/rpc/rpc-mgr-test.cc | 6 +++---
be/src/rpc/thrift-server-test.cc | 6 +++---
be/src/transport/TSasl.h | 8 +++++---
common/thrift/generate_error_codes.py | 4 ++++
6 files changed, 31 insertions(+), 15 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/impala/blob/a4916e6d/be/src/rpc/auth-provider.h
----------------------------------------------------------------------
diff --git a/be/src/rpc/auth-provider.h b/be/src/rpc/auth-provider.h
index f974517..ee3bc5f 100644
--- a/be/src/rpc/auth-provider.h
+++ b/be/src/rpc/auth-provider.h
@@ -181,11 +181,10 @@ class NoAuthProvider : public AuthProvider {
/// The first entry point to the authentication subsystem. Performs initialization
/// of Sasl, the global AuthManager, and the two authentication providers. Appname
-/// should generally be argv[0].
-/// TODO: Calling InitAuth() more than once is not an issue, however, calling InitAuth()
-/// more than once with a different 'appname' can lead to undefined behavior. Also,
-/// 'appname' must live as long as the process does since the cyrus-sasl library holds a
-/// reference to it.
+/// should generally be argv[0]. Normally, InitAuth() should only be called once.
+/// In certain test cases, we may call it more than once. It's important that InitAuth()
+/// is called with the same 'appname' if it's called more than once. Otherwise, error
+/// status will be returned.
Status InitAuth(const std::string& appname);
}
http://git-wip-us.apache.org/repos/asf/impala/blob/a4916e6d/be/src/rpc/authentication.cc
----------------------------------------------------------------------
diff --git a/be/src/rpc/authentication.cc b/be/src/rpc/authentication.cc
index 29fa8f7..d5a1b32 100644
--- a/be/src/rpc/authentication.cc
+++ b/be/src/rpc/authentication.cc
@@ -131,6 +131,11 @@ static vector<sasl_callback_t> KERB_INT_CALLBACKS; // Internal kerberos connect
static vector<sasl_callback_t> KERB_EXT_CALLBACKS; // External kerberos connections
static vector<sasl_callback_t> LDAP_EXT_CALLBACKS; // External LDAP connections
+// The name of the application passed to Sasl which will retain reference to it.
+// This is initialized the first time InitAuth() is called. Future call must pass
+// the same 'appname' or InitAuth() will fail.
+static string APP_NAME;
+
// Path to the file based credential cache that we pass to the KRB5CCNAME environment
// variable.
static const string KRB5CCNAME_PATH = "/tmp/krb5cc_impala_internal";
@@ -561,6 +566,12 @@ void SaslSetMutex() {
Status InitAuth(const string& appname) {
+ if (APP_NAME.empty()) {
+ APP_NAME = appname;
+ } else if (APP_NAME != appname) {
+ return Status(TErrorCode::SASL_APP_NAME_MISMATCH, APP_NAME, appname);
+ }
+
// Setup basic callbacks for Sasl. We initialize SASL always since KRPC expects it to
// be initialized.
// Good idea to have logging everywhere
@@ -655,7 +666,7 @@ Status InitAuth(const string& appname) {
SaslSetMutex();
try {
// We assume all impala processes are both server and client.
- sasl::TSaslServer::SaslInit(GENERAL_CALLBACKS, appname);
+ sasl::TSaslServer::SaslInit(GENERAL_CALLBACKS, APP_NAME.c_str());
sasl::TSaslClient::SaslInit(GENERAL_CALLBACKS);
} catch (sasl::SaslServerImplException& e) {
stringstream err_msg;
http://git-wip-us.apache.org/repos/asf/impala/blob/a4916e6d/be/src/rpc/rpc-mgr-test.cc
----------------------------------------------------------------------
diff --git a/be/src/rpc/rpc-mgr-test.cc b/be/src/rpc/rpc-mgr-test.cc
index 9acd39b..7e3cb25 100644
--- a/be/src/rpc/rpc-mgr-test.cc
+++ b/be/src/rpc/rpc-mgr-test.cc
@@ -54,7 +54,7 @@ DECLARE_string(hostname);
// The path of the current executable file that is required for passing into the SASL
// library as the 'application name'.
-static string current_executable_path;
+static string CURRENT_EXECUTABLE_PATH;
namespace impala {
@@ -127,7 +127,7 @@ class RpcMgrKerberizedTest :
DCHECK(kdc_wrapper_.get() != nullptr);
ASSERT_OK(kdc_wrapper_->SetupAndStartMiniKDC(GetParam()));
- ASSERT_OK(InitAuth(current_executable_path));
+ ASSERT_OK(InitAuth(CURRENT_EXECUTABLE_PATH));
RpcMgrTestBase::SetUp();
}
@@ -311,6 +311,6 @@ int main(int argc, char** argv) {
impala::InitCommonRuntime(argc, argv, false, impala::TestInfo::BE_TEST);
// Fill in the path of the current binary for use by the tests.
- current_executable_path = argv[0];
+ CURRENT_EXECUTABLE_PATH = argv[0];
return RUN_ALL_TESTS();
}
http://git-wip-us.apache.org/repos/asf/impala/blob/a4916e6d/be/src/rpc/thrift-server-test.cc
----------------------------------------------------------------------
diff --git a/be/src/rpc/thrift-server-test.cc b/be/src/rpc/thrift-server-test.cc
index 1e65f8a..f1e0f4b 100644
--- a/be/src/rpc/thrift-server-test.cc
+++ b/be/src/rpc/thrift-server-test.cc
@@ -91,7 +91,7 @@ template <class T> class ThriftTestBase : public T {
// The path of the current executable file that is required for passing into the SASL
// library as the 'application name'.
-static string current_executable_path;
+static string CURRENT_EXECUTABLE_PATH;
class ThriftKerberizedParamsTest :
public ThriftTestBase<testing::TestWithParam<KerberosSwitch> > {
@@ -101,7 +101,7 @@ class ThriftKerberizedParamsTest :
DCHECK(kdc_wrapper_.get() != nullptr);
ASSERT_OK(kdc_wrapper_->SetupAndStartMiniKDC(GetParam()));
- ASSERT_OK(InitAuth(current_executable_path));
+ ASSERT_OK(InitAuth(CURRENT_EXECUTABLE_PATH));
ThriftTestBase::SetUp();
}
@@ -552,6 +552,6 @@ int main(int argc, char** argv) {
impala::InitCommonRuntime(argc, argv, false, impala::TestInfo::BE_TEST);
// Fill in the path of the current binary for use by the tests.
- current_executable_path = argv[0];
+ CURRENT_EXECUTABLE_PATH = argv[0];
return RUN_ALL_TESTS();
}
http://git-wip-us.apache.org/repos/asf/impala/blob/a4916e6d/be/src/transport/TSasl.h
----------------------------------------------------------------------
diff --git a/be/src/transport/TSasl.h b/be/src/transport/TSasl.h
index 06e3755..d4671bb 100644
--- a/be/src/transport/TSasl.h
+++ b/be/src/transport/TSasl.h
@@ -209,10 +209,12 @@ class TSaslServer : public sasl::TSasl {
const std::string& userRealm, unsigned flags, sasl_callback_t* callbacks);
/*
- * This initializes the sasl server library and should be called onece per application
+ * This initializes the sasl server library and should be called onece per application.
+ * Note that the caller needs to ensure the life time of 'callbacks' and 'appname' is
+ * beyond that of this object.
*/
- static void SaslInit(const sasl_callback_t* callbacks, const std::string& appname) {
- int result = sasl_server_init(callbacks, appname.c_str());
+ static void SaslInit(const sasl_callback_t* callbacks, const char* appname) {
+ int result = sasl_server_init(callbacks, appname);
if (result != SASL_OK) {
throw SaslServerImplException(sasl_errstring(result, NULL, NULL));
}
http://git-wip-us.apache.org/repos/asf/impala/blob/a4916e6d/common/thrift/generate_error_codes.py
----------------------------------------------------------------------
diff --git a/common/thrift/generate_error_codes.py b/common/thrift/generate_error_codes.py
index b137b5e..40739e0 100755
--- a/common/thrift/generate_error_codes.py
+++ b/common/thrift/generate_error_codes.py
@@ -344,6 +344,10 @@ error_codes = (
("LZ4_COMPRESSION_INPUT_TOO_LARGE", 113,
"The input size is too large for LZ4 compression: $0"),
+
+ ("SASL_APP_NAME_MISMATCH", 114,
+ "InitAuth() called multiple times with different names. Was called with $0. "
+ "Now using $1.")
)
import sys