From 049249ce7addafaa0bd09480cd8858cd2c54138f Mon Sep 17 00:00:00 2001 From: Andreas Gampe Date: Tue, 19 Aug 2014 22:31:31 -0700 Subject: [PATCH] NativeBridge: Tighten security on libnativebridge Do not allow arbitrary paths for the native bridge - only allow simple names. Do not allow re-setup of the native bridge. Bug: 16404669 (cherry picked from commit cd2ef4c1af69727231b84ebc82864c170ff0e8ad) Change-Id: Ie22de356d2307fe2758f9094a85d44e61a4098a1 --- include/nativebridge/native_bridge.h | 15 +++ libnativebridge/Android.mk | 6 +- libnativebridge/native_bridge.cc | 101 ++++++++++++++++-- libnativebridge/tests/Android.mk | 33 ++++++ .../tests/InvalidCharsNativeBridge_test.cpp | 40 +++++++ libnativebridge/tests/NativeBridgeTest.h | 33 ++++++ .../tests/ReSetupNativeBridge_test.cpp | 32 ++++++ .../tests/UnavailableNativeBridge_test.cpp | 28 +++++ .../tests/ValidNameNativeBridge_test.cpp | 33 ++++++ 9 files changed, 312 insertions(+), 9 deletions(-) create mode 100644 libnativebridge/tests/Android.mk create mode 100644 libnativebridge/tests/InvalidCharsNativeBridge_test.cpp create mode 100644 libnativebridge/tests/NativeBridgeTest.h create mode 100644 libnativebridge/tests/ReSetupNativeBridge_test.cpp create mode 100644 libnativebridge/tests/UnavailableNativeBridge_test.cpp create mode 100644 libnativebridge/tests/ValidNameNativeBridge_test.cpp diff --git a/include/nativebridge/native_bridge.h b/include/nativebridge/native_bridge.h index 2415e6bc5..c588bbc0e 100644 --- a/include/nativebridge/native_bridge.h +++ b/include/nativebridge/native_bridge.h @@ -29,6 +29,10 @@ struct NativeBridgeRuntimeCallbacks; void SetupNativeBridge(const char* native_bridge_library_filename, const NativeBridgeRuntimeCallbacks* runtime_callbacks); +// Check whether a native bridge is available (initialized). Requires a prior call to +// SetupNativeBridge to make sense. +bool NativeBridgeAvailable(); + // Load a shared library that is supported by the native bridge. void* NativeBridgeLoadLibrary(const char* libpath, int flag); @@ -38,6 +42,17 @@ void* NativeBridgeGetTrampoline(void* handle, const char* name, const char* shor // True if native library is valid and is for an ABI that is supported by native bridge. bool NativeBridgeIsSupported(const char* libpath); +// Returns whether we have seen a native bridge error. This could happen because the library +// was not found, rejected, could not be initialized and so on. +// +// This functionality is mainly for testing. +bool NativeBridgeError(); + +// Returns whether a given string is acceptable as a native bridge library filename. +// +// This functionality is exposed mainly for testing. +bool NativeBridgeNameAcceptable(const char* native_bridge_library_filename); + // Native bridge interfaces to runtime. struct NativeBridgeCallbacks { // Initialize native bridge. Native bridge's internal implementation must ensure MT safety and diff --git a/libnativebridge/Android.mk b/libnativebridge/Android.mk index 017ce0248..9403fd2e1 100644 --- a/libnativebridge/Android.mk +++ b/libnativebridge/Android.mk @@ -10,10 +10,11 @@ include $(CLEAR_VARS) LOCAL_MODULE:= libnativebridge LOCAL_SRC_FILES:= $(NATIVE_BRIDGE_COMMON_SRC_FILES) +LOCAL_SHARED_LIBRARIES := liblog LOCAL_CLANG := true LOCAL_CPP_EXTENSION := .cc LOCAL_CFLAGS := -Werror -LOCAL_CPPFLAGS := -std=gnu++11 +LOCAL_CPPFLAGS := -std=gnu++11 -fvisibility=protected LOCAL_LDFLAGS := -ldl LOCAL_MULTILIB := both @@ -26,10 +27,11 @@ include $(CLEAR_VARS) LOCAL_MODULE:= libnativebridge LOCAL_SRC_FILES:= $(NATIVE_BRIDGE_COMMON_SRC_FILES) +LOCAL_SHARED_LIBRARIES := liblog LOCAL_CLANG := true LOCAL_CPP_EXTENSION := .cc LOCAL_CFLAGS := -Werror -LOCAL_CPPFLAGS := -std=gnu++11 +LOCAL_CPPFLAGS := -std=gnu++11 -fvisibility=protected LOCAL_LDFLAGS := -ldl LOCAL_MULTILIB := both diff --git a/libnativebridge/native_bridge.cc b/libnativebridge/native_bridge.cc index ad4ee73d6..2205f453b 100644 --- a/libnativebridge/native_bridge.cc +++ b/libnativebridge/native_bridge.cc @@ -16,6 +16,7 @@ #include "nativebridge/native_bridge.h" +#include #include #include #include "utils/Mutex.h" @@ -28,27 +29,92 @@ static Mutex native_bridge_lock("native bridge lock"); // The symbol name exposed by native-bridge with the type of NativeBridgeCallbacks. static constexpr const char* kNativeBridgeInterfaceSymbol = "NativeBridgeItf"; -// The path of the library we are supposed to load. -static const char* native_bridge_library_path = nullptr; +// The filename of the library we are supposed to load. +static const char* native_bridge_library_filename = nullptr; // Whether a native bridge is available (loaded and ready). static bool available = false; // Whether we have already initialized (or tried to). static bool initialized = false; +// Whether we had an error at some point. +static bool had_error = false; static NativeBridgeCallbacks* callbacks = nullptr; static const NativeBridgeRuntimeCallbacks* runtime_callbacks = nullptr; -void SetupNativeBridge(const char* nb_library_path, +// Characters allowed in a native bridge filename. The first character must +// be in [a-zA-Z] (expected 'l' for "libx"). The rest must be in [a-zA-Z0-9._-]. +static bool CharacterAllowed(char c, bool first) { + if (first) { + return ('a' <= c && c <= 'z') || ('A' <= c && c <= 'Z'); + } else { + return ('a' <= c && c <= 'z') || ('A' <= c && c <= 'Z') || ('0' <= c && c <= '9') || + (c == '.') || (c == '_') || (c == '-'); + } +} + +// We only allow simple names for the library. It is supposed to be a file in +// /system/lib or /vendor/lib. Only allow a small range of characters, that is +// names consisting of [a-zA-Z0-9._-] and starting with [a-zA-Z]. +bool NativeBridgeNameAcceptable(const char* nb_library_filename) { + const char* ptr = nb_library_filename; + if (*ptr == 0) { + // Emptry string. Allowed, means no native bridge. + return true; + } else { + // First character must be [a-zA-Z]. + if (!CharacterAllowed(*ptr, true)) { + // Found an invalid fist character, don't accept. + ALOGE("Native bridge library %s has been rejected for first character %c", nb_library_filename, *ptr); + return false; + } else { + // For the rest, be more liberal. + ptr++; + while (*ptr != 0) { + if (!CharacterAllowed(*ptr, false)) { + // Found an invalid character, don't accept. + ALOGE("Native bridge library %s has been rejected for %c", nb_library_filename, *ptr); + return false; + } + ptr++; + } + } + return true; + } +} + +void SetupNativeBridge(const char* nb_library_filename, const NativeBridgeRuntimeCallbacks* runtime_cbs) { Mutex::Autolock auto_lock(native_bridge_lock); - native_bridge_library_path = nb_library_path; + if (initialized || native_bridge_library_filename != nullptr) { + // Setup has been called before. Ignore this call. + ALOGW("Called SetupNativeBridge for an already set up native bridge."); + // Note: counts as an error, even though the bridge may be functional. + had_error = true; + return; + } + runtime_callbacks = runtime_cbs; - if (native_bridge_library_path == nullptr) { - initialized = true; + if (nb_library_filename == nullptr) { available = false; + initialized = true; + } else { + // Check whether it's an empty string. + if (*nb_library_filename == 0) { + available = false; + initialized = true; + } else if (!NativeBridgeNameAcceptable(nb_library_filename)) { + available = false; + initialized = true; + had_error = true; + } + + if (!initialized) { + // Didn't find a name error or empty string, assign it. + native_bridge_library_filename = nb_library_filename; + } } } @@ -62,7 +128,15 @@ static bool NativeBridgeInitialize() { available = false; - void* handle = dlopen(native_bridge_library_path, RTLD_LAZY); + if (native_bridge_library_filename == nullptr) { + // Called initialize without setup. dlopen has special semantics for nullptr input. + // So just call it a day here. This counts as an error. + initialized = true; + had_error = true; + return false; + } + + void* handle = dlopen(native_bridge_library_filename, RTLD_LAZY); if (handle != nullptr) { callbacks = reinterpret_cast(dlsym(handle, kNativeBridgeInterfaceSymbol)); @@ -72,8 +146,13 @@ static bool NativeBridgeInitialize() { } if (!available) { + // If we fail initialization, this counts as an error. + had_error = true; dlclose(handle); } + } else { + // Being unable to open the library counts as an error. + had_error = true; } initialized = true; @@ -81,6 +160,14 @@ static bool NativeBridgeInitialize() { return available; } +bool NativeBridgeError() { + return had_error; +} + +bool NativeBridgeAvailable() { + return NativeBridgeInitialize(); +} + void* NativeBridgeLoadLibrary(const char* libpath, int flag) { if (NativeBridgeInitialize()) { return callbacks->loadLibrary(libpath, flag); diff --git a/libnativebridge/tests/Android.mk b/libnativebridge/tests/Android.mk new file mode 100644 index 000000000..f58b8f7a9 --- /dev/null +++ b/libnativebridge/tests/Android.mk @@ -0,0 +1,33 @@ +# Build the unit tests. +LOCAL_PATH := $(call my-dir) +include $(CLEAR_VARS) + +# Build the unit tests. +test_src_files := \ + InvalidCharsNativeBridge_test.cpp \ + ReSetupNativeBridge_test.cpp \ + UnavailableNativeBridge_test.cpp \ + ValidNameNativeBridge_test.cpp + +shared_libraries := \ + libnativebridge + +$(foreach file,$(test_src_files), \ + $(eval include $(CLEAR_VARS)) \ + $(eval LOCAL_CLANG := true) \ + $(eval LOCAL_CPPFLAGS := -std=gnu++11) \ + $(eval LOCAL_SHARED_LIBRARIES := $(shared_libraries)) \ + $(eval LOCAL_SRC_FILES := $(file)) \ + $(eval LOCAL_MODULE := $(notdir $(file:%.cpp=%))) \ + $(eval include $(BUILD_NATIVE_TEST)) \ +) + +$(foreach file,$(test_src_files), \ + $(eval include $(CLEAR_VARS)) \ + $(eval LOCAL_CLANG := true) \ + $(eval LOCAL_CPPFLAGS := -std=gnu++11) \ + $(eval LOCAL_SHARED_LIBRARIES := $(shared_libraries)) \ + $(eval LOCAL_SRC_FILES := $(file)) \ + $(eval LOCAL_MODULE := $(notdir $(file:%.cpp=%))) \ + $(eval include $(BUILD_HOST_NATIVE_TEST)) \ +) diff --git a/libnativebridge/tests/InvalidCharsNativeBridge_test.cpp b/libnativebridge/tests/InvalidCharsNativeBridge_test.cpp new file mode 100644 index 000000000..f37e9c158 --- /dev/null +++ b/libnativebridge/tests/InvalidCharsNativeBridge_test.cpp @@ -0,0 +1,40 @@ +/* + * Copyright (C) 2014 The Android Open Source Project + * + * 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. + */ + +#include "NativeBridgeTest.h" + +namespace android { + +static const char* kTestName = "../librandom$@-bridge_not.existing.so"; + +TEST_F(NativeBridgeTest, InvalidChars) { + // Do one test actually calling setup. + EXPECT_EQ(false, NativeBridgeError()); + SetupNativeBridge(kTestName, nullptr); + // This should lead to an error for invalid characters. + EXPECT_EQ(true, NativeBridgeError()); + + // Further tests need to use NativeBridgeNameAcceptable, as the error + // state can't be changed back. + EXPECT_EQ(false, NativeBridgeNameAcceptable(".")); + EXPECT_EQ(false, NativeBridgeNameAcceptable("..")); + EXPECT_EQ(false, NativeBridgeNameAcceptable("_")); + EXPECT_EQ(false, NativeBridgeNameAcceptable("-")); + EXPECT_EQ(false, NativeBridgeNameAcceptable("lib@.so")); + EXPECT_EQ(false, NativeBridgeNameAcceptable("lib$.so")); +} + +} // namespace android diff --git a/libnativebridge/tests/NativeBridgeTest.h b/libnativebridge/tests/NativeBridgeTest.h new file mode 100644 index 000000000..0d731cb11 --- /dev/null +++ b/libnativebridge/tests/NativeBridgeTest.h @@ -0,0 +1,33 @@ +/* + * Copyright (C) 2014 The Android Open Source Project + * + * 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. + */ + +#ifndef NATIVE_BRIDGE_TEST_H_ +#define NATIVE_BRIDGE_TEST_H_ + +#define LOG_TAG "NativeBridge_test" + +#include +#include + +namespace android { + +class NativeBridgeTest : public testing::Test { +}; + +}; // namespace android + +#endif // NATIVE_BRIDGE_H_ + diff --git a/libnativebridge/tests/ReSetupNativeBridge_test.cpp b/libnativebridge/tests/ReSetupNativeBridge_test.cpp new file mode 100644 index 000000000..ef5bfceb8 --- /dev/null +++ b/libnativebridge/tests/ReSetupNativeBridge_test.cpp @@ -0,0 +1,32 @@ +/* + * Copyright (C) 2014 The Android Open Source Project + * + * 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. + */ + +#include "NativeBridgeTest.h" + +namespace android { + +static const char* kTestName = "librandom-bridge_not.existing.so"; + +TEST_F(NativeBridgeTest, ReSetup) { + EXPECT_EQ(false, NativeBridgeError()); + SetupNativeBridge(kTestName, nullptr); + EXPECT_EQ(false, NativeBridgeError()); + SetupNativeBridge(kTestName, nullptr); + // This should lead to an error for trying to re-setup a native bridge. + EXPECT_EQ(true, NativeBridgeError()); +} + +} // namespace android diff --git a/libnativebridge/tests/UnavailableNativeBridge_test.cpp b/libnativebridge/tests/UnavailableNativeBridge_test.cpp new file mode 100644 index 000000000..27d12336c --- /dev/null +++ b/libnativebridge/tests/UnavailableNativeBridge_test.cpp @@ -0,0 +1,28 @@ +/* + * Copyright (C) 2011 The Android Open Source Project + * + * 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. + */ + +#include "NativeBridgeTest.h" + +namespace android { + +TEST_F(NativeBridgeTest, NoNativeBridge) { + EXPECT_EQ(false, NativeBridgeAvailable()); + // This should lead to an error for trying to initialize a not-setup + // native bridge. + EXPECT_EQ(true, NativeBridgeError()); +} + +} // namespace android diff --git a/libnativebridge/tests/ValidNameNativeBridge_test.cpp b/libnativebridge/tests/ValidNameNativeBridge_test.cpp new file mode 100644 index 000000000..3e019232d --- /dev/null +++ b/libnativebridge/tests/ValidNameNativeBridge_test.cpp @@ -0,0 +1,33 @@ +/* + * Copyright (C) 2011 The Android Open Source Project + * + * 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. + */ + +#include + +namespace android { + +static const char* kTestName = "librandom-bridge_not.existing.so"; + +TEST_F(NativeBridgeTest, ValidName) { + EXPECT_EQ(false, NativeBridgeError()); + SetupNativeBridge(kTestName, nullptr); + EXPECT_EQ(false, NativeBridgeError()); + EXPECT_EQ(false, NativeBridgeAvailable()); + // This should lead to an error for trying to initialize a not-existing + // native bridge. + EXPECT_EQ(true, NativeBridgeError()); +} + +} // namespace android