Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
TESTING TESTING TESTING TESTING TESTING
DO NOT MERGE DO NOT MERGE DO NOT MERGE

Fixes: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1139578

Context: dotnet/java-interop#691

Context: https://liquid.microsoft.com/Web/Object/Read/ms.security/Requirements/Microsoft.Security.SystemsADM.10039#guide

For improved security, the guidance is that when loading `.dll` files
on Windows, the default [`LoadLibrary()`][0] behavior is to be
*avoided*, as the [Dynamic-Link Library Search Order][1] includes the
"current working directory" and directories listed in `%PATH%`, both
of which are under control of an "attacker" and not the application.

Instead, [`LoadLibraryEx()`][2] should be used, providing a set of
`LOAD_LIBRARY_SEARCH_*` values which constrain the set of directories
that will be searched for the library (if a full path is not used)
and/or the library's dependencies.

Historically, Xamarin.Android hasn't directly called any
`LoadLibrary()` function.  Instead, xamarin-android called
[**dlopen**(3)][3], then used dlfcn-win32/dlfcn-win32@ef7e412d to
implement `dlopen()` in terms of `LoadLibraryEx()`.

Unfortunately, dlfcn-win32/dlfcn-win32@ef7e412d calls `LoadLibraryEx()`
with `LOAD_WITH_ALTERED_SEARCH_PATH`, which while a security
improvement, does not go far enough for our internal requirements.

Migrate away from dlfcn-win32 and instead use the new
`java-interop-dlfcn.h` family of functions added in
TODO(dotnet/java-interop#691).  These new functions
appropriately use `LoadLibraryEx()` with a constrained set of
`LOAD_LIBRARY_SEARCH_*` flags.

[0]: https://docs.microsoft.com/en-us/windows/win32/api/libloaderapi/nf-libloaderapi-loadlibrarya
[1]: https://docs.microsoft.com/en-us/windows/win32/dlls/dynamic-link-library-search-order
[2]: https://docs.microsoft.com/en-us/windows/win32/api/libloaderapi/nf-libloaderapi-loadlibraryexa
[3]: https://linux.die.net/man/3/dlopen
  • Loading branch information
jonpryor committed Aug 20, 2020
1 parent 53ec311 commit f4852b7
Show file tree
Hide file tree
Showing 10 changed files with 47 additions and 57 deletions.
8 changes: 2 additions & 6 deletions .gitmodules
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,10 @@
path = external/debugger-libs
url = https://github.com/mono/debugger-libs
branch = master
[submodule "external/dlfcn-win32"]
path = external/dlfcn-win32
url = https://github.com/dlfcn-win32/dlfcn-win32.git
branch = v1.1.1
[submodule "external/Java.Interop"]
path = external/Java.Interop
url = https://github.com/xamarin/java.interop.git
branch = master
url = https://github.com/jonpryor/java.interop.git
branch = jonp-loadlib-search-order
[submodule "external/lz4"]
path = external/lz4
url = https://github.com/lz4/lz4.git
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ namespace Xamarin.Android.Prepare
class Step_BuildMingwDependencies : Step
{
static readonly SortedDictionary <string, (string description, string libraryName)> dependencies = new SortedDictionary <string, (string description, string libraryName)> (StringComparer.Ordinal) {
{ "dlfcn-win32", (description: "libdl for Windows", libraryName: "libdl.a") },
{ "mman-win32", (description: "mmap for Windows", libraryName: "libmman.a") },
};

Expand Down
21 changes: 0 additions & 21 deletions build-tools/xaprepare/xaprepare/ThirdPartyNotices/dlfcn.cs

This file was deleted.

1 change: 0 additions & 1 deletion build-tools/xaprepare/xaprepare/xaprepare.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,6 @@
<Compile Include="Steps\Step_ThirdPartyNotices.cs" />
<Compile Include="ThirdPartyNotices\aapt2.cs" />
<Compile Include="ThirdPartyNotices\bundletool.cs" />
<Compile Include="ThirdPartyNotices\dlfcn.cs" />
<Compile Include="ThirdPartyNotices\Java.Interop.cs" />
<Compile Include="ThirdPartyNotices\K4os.Compression.LZ4.cs" />
<Compile Include="ThirdPartyNotices\lz4.cs" />
Expand Down
1 change: 0 additions & 1 deletion external/dlfcn-win32
Submodule dlfcn-win32 deleted from ef7e41
9 changes: 6 additions & 3 deletions src/monodroid/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ else()
if(WIN32 OR MINGW)
message(STATUS "Win32 or MinGW")
set(EXTRA_COMPILER_FLAGS "${EXTRA_COMPILER_FLAGS} -DWINDOWS -DNTDDI_VERSION=NTDDI_VISTA -D_WIN32_WINNT=_WIN32_WINNT_VISTA -fomit-frame-pointer")
set(EXTRA_LINKER_FLAGS "${EXTRA_LINKER_FLAGS} -ldl -lmman -static -pthread -dynamic -lmincore -lmswsock -lwsock32 -lshlwapi -lpsapi -lwinmm")
set(EXTRA_LINKER_FLAGS "${EXTRA_LINKER_FLAGS} -lmman -static -pthread -dynamic -lmincore -lmswsock -lwsock32 -lshlwapi -lpsapi -lwinmm")

if (MINGW_TARGET_32)
set(ANDROID_ABI "host-mxe-Win32")
Expand Down Expand Up @@ -298,6 +298,7 @@ set(MONODROID_SOURCES
${JAVA_INTEROP_SRC_PATH}/java-interop.cc
${JAVA_INTEROP_SRC_PATH}/java-interop-mono.cc
${JAVA_INTEROP_SRC_PATH}/java-interop-util.cc
${JAVA_INTEROP_SRC_PATH}/java-interop-dlfcn.cc
)

set(XA_INTERNAL_API_SOURCES
Expand Down Expand Up @@ -392,11 +393,13 @@ else()
set(LINK_LIBS "-lmonosgen-2.0 -lz ${EXTRA_LINKER_FLAGS}")
endif()

set(DEBUG_HELPER_LINK_LIBS "-ldl")
if(NOT MINGW AND NOT WIN32)
set(DEBUG_HELPER_LINK_LIBS "-ldl")
endif()
if(ENABLE_NDK)
set(LINK_LIBS "${LINK_LIBS} -llog")
set(DEBUG_HELPER_LINK_LIBS "${DEBUG_HELPER_LINK_LIBS} -llog")
elseif(NOT ANDROID)
elseif(NOT ANDROID AND NOT MINGW AND NOT WIN32)
set(LINK_LIBS "-pthread ${LINK_LIBS} -ldl")
endif()

Expand Down
9 changes: 6 additions & 3 deletions src/monodroid/jni/android-system.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
#include <errno.h>
#include <assert.h>
#include <ctype.h>
#include <dlfcn.h>
#include <fcntl.h>

#ifdef ANDROID
Expand All @@ -27,6 +26,8 @@
#include "jni-wrappers.hh"
#include "xamarin-app.hh"
#include "cpp-util.hh"
#include "java-interop-dlfcn.h"
#include "java-interop.h"

#if defined (DEBUG) || !defined (ANDROID)
namespace xamarin::android::internal {
Expand Down Expand Up @@ -358,9 +359,11 @@ AndroidSystem::load_dso (const char *path, int dl_flags, bool skip_exists_check)
return nullptr;
}

void *handle = dlopen (path, dl_flags);
char *error = nullptr;
void *handle = java_interop_load_library (path, 0, &error);
if (handle == nullptr && utils.should_log (LOG_ASSEMBLY))
log_info_nocheck (LOG_ASSEMBLY, "Failed to load shared library '%s'. %s", path, dlerror ());
log_info_nocheck (LOG_ASSEMBLY, "Failed to load shared library '%s'. %s", path, error);
java_interop_free (error);
return handle;
}

Expand Down
12 changes: 6 additions & 6 deletions src/monodroid/jni/debug.cc
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
#include <ctype.h>
#include <assert.h>
#include <limits.h>
#include <dlfcn.h>
#include <mono/metadata/mono-debug.h>

#ifdef ANDROID
Expand All @@ -42,6 +41,8 @@
#include "globals.hh"
#include "cpp-util.hh"

#include "java-interop-dlfcn.h"

using namespace xamarin::android;

//
Expand Down Expand Up @@ -79,15 +80,14 @@ Debug::monodroid_profiler_load (const char *libmono_path, const char *desc, cons
}
simple_pointer_guard<char[]> mname (mname_ptr);

int dlopen_flags = RTLD_LAZY;
simple_pointer_guard<char[]> libname (utils.string_concat ("libmono-profiler-", mname.get (), ".so"));
bool found = false;
void *handle = androidSystem.load_dso_from_any_directories (libname, dlopen_flags);
void *handle = androidSystem.load_dso_from_any_directories (libname, 0);
found = load_profiler_from_handle (handle, desc, mname);

if (!found && libmono_path != nullptr) {
simple_pointer_guard<char[]> full_path (utils.path_combine (libmono_path, libname));
handle = androidSystem.load_dso (full_path, dlopen_flags, FALSE);
handle = androidSystem.load_dso (full_path, 0, FALSE);
found = load_profiler_from_handle (handle, desc, mname);
}

Expand All @@ -108,7 +108,7 @@ typedef void (*ProfilerInitializer) (const char*);
bool
Debug::load_profiler (void *handle, const char *desc, const char *symbol)
{
ProfilerInitializer func = reinterpret_cast<ProfilerInitializer> (dlsym (handle, symbol));
ProfilerInitializer func = reinterpret_cast<ProfilerInitializer> (java_interop_get_symbol_address (handle, symbol, nullptr));
log_warn (LOG_DEFAULT, "Looking for profiler init symbol '%s'? %p", symbol, func);

if (func != nullptr) {
Expand All @@ -129,7 +129,7 @@ Debug::load_profiler_from_handle (void *dso_handle, const char *desc, const char

if (result)
return true;
dlclose (dso_handle);
java_interop_close_library (dso_handle, nullptr);
return false;
}

Expand Down
40 changes: 26 additions & 14 deletions src/monodroid/jni/monodroid-glue.cc
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,10 @@
#include <errno.h>
#include <limits.h>

#if defined (APPLE_OS_X)
#include <dlfcn.h>
#endif // def APPLE_OX_X

#include <fcntl.h>
#include <unistd.h>
#include <stdint.h>
Expand Down Expand Up @@ -82,6 +85,8 @@

#include "cpp-util.hh"

#include "java-interop-dlfcn.h"

using namespace xamarin::android;
using namespace xamarin::android::internal;

Expand Down Expand Up @@ -151,12 +156,11 @@ MonodroidRuntime::setup_bundled_app (const char *dso_name)
if (!application_config.is_a_bundled_app)
return;

static int dlopen_flags = RTLD_LAZY;
void *libapp = nullptr;

if (androidSystem.is_embedded_dso_mode_enabled ()) {
log_info (LOG_DEFAULT, "bundle app: embedded DSO mode");
libapp = androidSystem.load_dso_from_any_directories (dso_name, dlopen_flags);
libapp = androidSystem.load_dso_from_any_directories (dso_name, 0);
} else {
bool needs_free = false;
log_info (LOG_DEFAULT, "bundle app: normal mode");
Expand All @@ -165,7 +169,7 @@ MonodroidRuntime::setup_bundled_app (const char *dso_name)
if (bundle_path == nullptr)
return;
log_info (LOG_BUNDLE, "Attempting to load bundled app from %s", bundle_path);
libapp = androidSystem.load_dso (bundle_path, dlopen_flags, true);
libapp = androidSystem.load_dso (bundle_path, 0, true);
if (needs_free)
delete[] bundle_path;
}
Expand All @@ -181,11 +185,11 @@ MonodroidRuntime::setup_bundled_app (const char *dso_name)
}
}

mono_mkbundle_initialize_mono_api = reinterpret_cast<mono_mkbundle_initialize_mono_api_ptr> (dlsym (libapp, "initialize_mono_api"));
mono_mkbundle_initialize_mono_api = reinterpret_cast<mono_mkbundle_initialize_mono_api_ptr> (java_interop_get_symbol_address (libapp, "initialize_mono_api", nullptr));
if (!mono_mkbundle_initialize_mono_api)
log_error (LOG_BUNDLE, "Missing initialize_mono_api in the application");

mono_mkbundle_init = reinterpret_cast<mono_mkbundle_init_ptr> (dlsym (libapp, "mono_mkbundle_init"));
mono_mkbundle_init = reinterpret_cast<mono_mkbundle_init_ptr> (java_interop_get_symbol_address (libapp, "mono_mkbundle_init", nullptr));
if (!mono_mkbundle_init)
log_error (LOG_BUNDLE, "Missing mono_mkbundle_init in the application");
log_info (LOG_BUNDLE, "Bundled app loaded: %s", dso_name);
Expand Down Expand Up @@ -1067,13 +1071,17 @@ setup_gc_logging (void)
force_inline int
MonodroidRuntime::convert_dl_flags (int flags)
{
#if 0
int lflags = flags & static_cast<int> (MONO_DL_LOCAL) ? 0: RTLD_GLOBAL;

if (flags & static_cast<int> (MONO_DL_LAZY))
lflags |= RTLD_LAZY;
else
lflags |= RTLD_NOW;
return lflags;
#else
return 0;
#endif
}

force_inline void
Expand All @@ -1099,7 +1107,7 @@ MonodroidRuntime::init_internal_api_dso (void *handle)

std::lock_guard<std::mutex> lock (api_init_lock);
if (api_dso_handle != nullptr) {
auto api_shutdown = reinterpret_cast<external_api_shutdown_fn> (dlsym (api_dso_handle, MonoAndroidInternalCalls::SHUTDOWN_FUNCTION_NAME));
auto api_shutdown = reinterpret_cast<external_api_shutdown_fn> (java_interop_get_symbol_address (api_dso_handle, MonoAndroidInternalCalls::SHUTDOWN_FUNCTION_NAME, nullptr));
if (api_shutdown == nullptr) {
// We COULD ignore this situation, but if the function is missing it means we messed something up and thus
// it *is* a fatal error.
Expand All @@ -1111,7 +1119,7 @@ MonodroidRuntime::init_internal_api_dso (void *handle)

api_dso_handle = handle;
auto api = new MonoAndroidInternalCalls_Impl ();
auto api_init = reinterpret_cast<external_api_init_fn>(dlsym (handle, MonoAndroidInternalCalls::INIT_FUNCTION_NAME));
auto api_init = reinterpret_cast<external_api_init_fn>(java_interop_get_symbol_address (handle, MonoAndroidInternalCalls::INIT_FUNCTION_NAME, nullptr));
if (api_init == nullptr) {
log_fatal (LOG_DEFAULT, "Unable to initialize Internal API library, init function '%s' not found in the module", MonoAndroidInternalCalls::INIT_FUNCTION_NAME);
exit (FATAL_EXIT_MONO_MISSING_SYMBOLS);
Expand Down Expand Up @@ -1190,11 +1198,15 @@ void*
MonodroidRuntime::monodroid_dlsym (void *handle, const char *name, char **err, [[maybe_unused]] void *user_data)
{
void *s;
char *e;

s = dlsym (handle, name);
s = java_interop_get_symbol_address (handle, name, &e);

if (!s && err) {
*err = utils.monodroid_strdup_printf ("Could not find symbol '%s'.", name);
*err = utils.monodroid_strdup_printf ("Could not find symbol '%s': %s", name, e);
}
if (e) {
java_interop_free (e);
}

return s;
Expand Down Expand Up @@ -1370,9 +1382,9 @@ MonodroidRuntime::disable_external_signal_handlers (void)
if (!androidSystem.is_mono_llvm_enabled ())
return;

void *llvm = androidSystem.load_dso ("libLLVM.so", RTLD_LAZY, TRUE);
void *llvm = androidSystem.load_dso ("libLLVM.so", 0, TRUE);
if (llvm) {
bool *disable_signals = reinterpret_cast<bool*> (dlsym (llvm, "_ZN4llvm23DisablePrettyStackTraceE"));
bool *disable_signals = reinterpret_cast<bool*> (java_interop_get_symbol_address (llvm, "_ZN4llvm23DisablePrettyStackTraceE", nullptr));
if (disable_signals) {
*disable_signals = true;
log_info (LOG_DEFAULT, "Disabled LLVM signal trapping");
Expand Down Expand Up @@ -1605,7 +1617,7 @@ MonodroidRuntime::Java_mono_android_Runtime_initInternal (JNIEnv *env, jclass kl
if (my_location != nullptr) {
simple_pointer_guard<char, false> dso_path (utils.path_combine (my_location, API_DSO_NAME));
log_info (LOG_DEFAULT, "Attempting to load %s", dso_path.get ());
api_dso_handle = dlopen (dso_path.get (), RTLD_NOW);
api_dso_handle = java_interop_load_library (dso_path.get (), 0, nullptr);
#if defined (APPLE_OS_X)
delete[] my_location;
#else // !defined(APPLE_OS_X)
Expand All @@ -1615,11 +1627,11 @@ MonodroidRuntime::Java_mono_android_Runtime_initInternal (JNIEnv *env, jclass kl

if (api_dso_handle == nullptr) {
log_info (LOG_DEFAULT, "Attempting to load %s with \"bare\" dlopen", API_DSO_NAME);
api_dso_handle = dlopen (API_DSO_NAME, RTLD_NOW);
api_dso_handle = java_interop_load_library (API_DSO_NAME, 0, nullptr);
}
#endif // defined(WINDOWS) || defined(APPLE_OS_X)
if (api_dso_handle == nullptr)
api_dso_handle = androidSystem.load_dso_from_any_directories (API_DSO_NAME, RTLD_NOW);
api_dso_handle = androidSystem.load_dso_from_any_directories (API_DSO_NAME, 0);
init_internal_api_dso (api_dso_handle);

mono_dl_fallback_register (monodroid_dlopen, monodroid_dlsym, nullptr, nullptr);
Expand Down

0 comments on commit f4852b7

Please sign in to comment.