Skip to content

Commit

Permalink
Bump to xamarin/java.interop/master@08ff4db1
Browse files Browse the repository at this point in the history
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

Changes: dotnet/java-interop@007b35b...08ff4db

  * dotnet/java-interop@08ff4db1: [java-interop, Java.Interop] Securely load native libs (dotnet#691)
  * dotnet/java-interop@09c68911: [generator] Clean up generated whitespace (dotnet#692)
  * dotnet/java-interop@d664e90e: [generator] Make warning and error messages localizable. (dotnet#689)

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
dotnet/java-interop@08ff4db1.  These new functions appropriately use
`LoadLibraryEx()` with a constrained set of `LOAD_LIBRARY_SEARCH_*`
flags.

*Non-obvious semantic note*: On Mono for all platforms, P/Invoking
`__Internal` is equivalent to trying to load the path `NULL`, e.g.
on macOS `dlopen(NULL, …)`.  However, only Android allows a custom
"`loader_func`" to intercept the attempted load of `NULL` and "do
something" with it, which xamarin-android *does* intercept, "remapping"
`NULL` to `libxa-internal-api.so`.  On all other platforms,
`loader_func` cannot intercept this invocation.

…which made for an "interesting" interaction: on macOS, if we used:

	api_dso_handle = java_interop_lib_load (dso_path.get (), JAVA_INTEROP_LIB_LOAD_LOCALLY, nullptr);

then the macOS Designer integration tests would fail!

	Renderer >> 4 [monodroid] Calling into managed runtime init
	Renderer (error) >>
	Renderer (error) >> Unhandled Exception:
	Renderer (error) >> System.EntryPointNotFoundException: java_interop_jnienv_get_java_vm assembly:<unknown assembly> type:<unknown type> member:(null)
	Renderer (error) >> at (wrapper managed-to-native) Java.Interop.NativeMethods.java_interop_jnienv_get_java_vm(intptr,intptr&)
	Renderer (error) >> at Java.Interop.JniEnvironment+References.GetJavaVM (System.IntPtr jnienv, System.IntPtr& vm) [0x00000] in <0f003a4904fd44d0a8cc6a63962ab40b>:0
	Renderer (error) >> at Java.Interop.JniEnvironmentInfo.set_EnvironmentPointer (System.IntPtr value) [0x00037] in <0f003a4904fd44d0a8cc6a63962ab40b>:0
	Renderer (error) >> at Java.Interop.JniEnvironmentInfo..ctor (System.IntPtr environmentPointer, Java.Interop.JniRuntime runtime) [0x00006] in <0f003a4904fd44d0a8cc6a63962ab40b>:0
	Renderer (error) >> at Java.Interop.JniRuntime..ctor (Java.Interop.JniRuntime+CreationOptions options) [0x0017b] in <0f003a4904fd44d0a8cc6a63962ab40b>:0
	Renderer (error) >> at Android.Runtime.AndroidRuntime..ctor (System.IntPtr jnienv, System.IntPtr vm, System.Boolean allocNewObjectSupported, System.IntPtr classLoader, System.IntPtr classLoader_loadClass, System.Boolean jniAddNativeMethodRegistrationAttributePresent) [0x00000] in /Users/builder/azdo/_work/4/s/xamarin-android/src/Mono.Android/Android.Runtime/AndroidRuntime.cs:25

In order for the macOS Designer integration tests to succeed,
`libxa-internal-api.dylib` *must* be loaded as `RTLD_GLOBAL`, which is
done via `JAVA_INTEROP_LIB_LOAD_GLOBALLY`.

[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 26, 2020
1 parent 53ec311 commit b5e7a83
Show file tree
Hide file tree
Showing 13 changed files with 65 additions and 70 deletions.
4 changes: 0 additions & 4 deletions .gitmodules
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,6 @@
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
Expand Down
3 changes: 3 additions & 0 deletions build-tools/installers/create-installers.targets
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,8 @@
<_MSBuildFiles Include="$(MSBuildSrcDir)\javadoc-to-mdoc.pdb" Condition=" '$(PackageId)' != 'Microsoft.Android.Sdk' " />
<_MSBuildFiles Include="$(MSBuildSrcDir)\Java.Interop.dll" />
<_MSBuildFiles Include="$(MSBuildSrcDir)\Java.Interop.pdb" />
<_MSBuildFiles Include="$(MSBuildSrcDir)\Java.Interop.Localization.dll" />
<_MSBuildFiles Include="$(MSBuildSrcDir)\Java.Interop.Localization.pdb" />
<_MSBuildFiles Include="$(MSBuildSrcDir)\Java.Interop.dll.config" />
<_MSBuildFiles Include="$(MSBuildSrcDir)\Java.Interop.Export.dll" />
<_MSBuildFiles Include="$(MSBuildSrcDir)\Java.Interop.Export.pdb" />
Expand Down Expand Up @@ -228,6 +230,7 @@
<_MSBuildFiles Include="$(MSBuildSrcDir)\Xamarin.Android.Bindings.JarToXml.targets" Condition=" '$(PackageId)' != 'Microsoft.Android.Sdk' " />
<_MSBuildFiles Include="$(MSBuildSrcDir)\Xamarin.Android.Build.Tasks.dll" />
<_MSBuildFiles Include="$(MSBuildSrcDir)\Xamarin.Android.Build.Tasks.pdb" />
<_MSBuildFiles Include="@(_LocalizationLanguages->'$(MSBuildSrcDir)\%(Identity)\Java.Interop.Localization.resources.dll')" />
<_MSBuildFiles Include="@(_LocalizationLanguages->'$(MSBuildSrcDir)\%(Identity)\Xamarin.Android.Build.Tasks.resources.dll')" />
<_MSBuildFiles Include="$(MSBuildSrcDir)\Xamarin.Android.BuildInfo.txt" />
<_MSBuildFiles Include="$(MSBuildSrcDir)\Xamarin.Android.Cecil.dll" />
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
2 changes: 1 addition & 1 deletion external/Java.Interop
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
20 changes: 12 additions & 8 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 All @@ -39,6 +40,7 @@ namespace xamarin::android::internal {
}
#endif // DEBUG || !ANDROID

using namespace microsoft::java_interop;
using namespace xamarin::android;
using namespace xamarin::android::internal;

Expand Down Expand Up @@ -347,7 +349,7 @@ AndroidSystem::get_full_dso_path (const char *base_dir, const char *dso_path, bo
}

void*
AndroidSystem::load_dso (const char *path, int dl_flags, bool skip_exists_check)
AndroidSystem::load_dso (const char *path, unsigned int dl_flags, bool skip_exists_check)
{
if (path == nullptr || *path == '\0')
return nullptr;
Expand All @@ -358,14 +360,16 @@ 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_lib_load (path, dl_flags, &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;
}

void*
AndroidSystem::load_dso_from_specified_dirs (const char **directories, size_t num_entries, const char *dso_name, int dl_flags)
AndroidSystem::load_dso_from_specified_dirs (const char **directories, size_t num_entries, const char *dso_name, unsigned int dl_flags)
{
assert (directories != nullptr);
if (dso_name == nullptr)
Expand All @@ -386,13 +390,13 @@ AndroidSystem::load_dso_from_specified_dirs (const char **directories, size_t nu
}

void*
AndroidSystem::load_dso_from_app_lib_dirs (const char *name, int dl_flags)
AndroidSystem::load_dso_from_app_lib_dirs (const char *name, unsigned int dl_flags)
{
return load_dso_from_specified_dirs (static_cast<const char**> (app_lib_directories), app_lib_directories_size, name, dl_flags);
}

void*
AndroidSystem::load_dso_from_override_dirs ([[maybe_unused]] const char *name, [[maybe_unused]] int dl_flags)
AndroidSystem::load_dso_from_override_dirs ([[maybe_unused]] const char *name, [[maybe_unused]] unsigned int dl_flags)
{
#ifdef RELEASE
return nullptr;
Expand All @@ -402,7 +406,7 @@ AndroidSystem::load_dso_from_override_dirs ([[maybe_unused]] const char *name, [
}

void*
AndroidSystem::load_dso_from_any_directories (const char *name, int dl_flags)
AndroidSystem::load_dso_from_any_directories (const char *name, unsigned int dl_flags)
{
void *handle = load_dso_from_override_dirs (name, dl_flags);
if (handle == nullptr)
Expand Down
10 changes: 5 additions & 5 deletions src/monodroid/jni/android-system.hh
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,8 @@ namespace xamarin::android::internal
char* get_bundled_app (JNIEnv *env, jstring dir);
int count_override_assemblies ();
long get_gref_gc_threshold ();
void* load_dso (const char *path, int dl_flags, bool skip_exists_check);
void* load_dso_from_any_directories (const char *name, int dl_flags);
void* load_dso (const char *path, unsigned int dl_flags, bool skip_exists_check);
void* load_dso_from_any_directories (const char *name, unsigned int dl_flags);
char* get_full_dso_path_on_disk (const char *dso_name, bool &needs_free);
monodroid_dirent_t* readdir (monodroid_dir_t *dir);

Expand Down Expand Up @@ -118,9 +118,9 @@ namespace xamarin::android::internal
size_t _monodroid_get_system_property_from_file (const char *path, char **value);
#endif
char* get_full_dso_path (const char *base_dir, const char *dso_path, bool &needs_free);
void* load_dso_from_specified_dirs (const char **directories, size_t num_entries, const char *dso_name, int dl_flags);
void* load_dso_from_app_lib_dirs (const char *name, int dl_flags);
void* load_dso_from_override_dirs (const char *name, int dl_flags);
void* load_dso_from_specified_dirs (const char **directories, size_t num_entries, const char *dso_name, unsigned int dl_flags);
void* load_dso_from_app_lib_dirs (const char *name, unsigned int dl_flags);
void* load_dso_from_override_dirs (const char *name, unsigned int dl_flags);
char* get_existing_dso_path_on_disk (const char *base_dir, const char *dso_name, bool &needs_free);

#if defined (WINDOWS)
Expand Down
14 changes: 10 additions & 4 deletions src/monodroid/jni/debug.cc
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,16 @@
#include <ctype.h>
#include <assert.h>
#include <limits.h>
#include <dlfcn.h>
#include <mono/metadata/mono-debug.h>

#ifdef ANDROID
#include <android/log.h>
#endif

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

#include "java-interop-util.h"

#include "monodroid.h"
Expand All @@ -42,6 +45,9 @@
#include "globals.hh"
#include "cpp-util.hh"

#include "java-interop-dlfcn.h"

using namespace microsoft::java_interop;
using namespace xamarin::android;

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

int dlopen_flags = RTLD_LAZY;
unsigned int dlopen_flags = JAVA_INTEROP_LIB_LOAD_LOCALLY;
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);
Expand Down Expand Up @@ -108,7 +114,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_lib_symbol (handle, symbol, nullptr));
log_warn (LOG_DEFAULT, "Looking for profiler init symbol '%s'? %p", symbol, func);

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

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

Expand Down
2 changes: 1 addition & 1 deletion src/monodroid/jni/monodroid-glue-internal.hh
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ namespace xamarin::android::internal
char* get_java_class_name_for_TypeManager (jclass klass);

private:
int convert_dl_flags (int flags);
unsigned int convert_dl_flags (int flags);
#if defined (WINDOWS) || defined (APPLE_OS_X)
static const char* get_my_location ();
#endif // defined(WINDOWS) || defined(APPLE_OS_X)
Expand Down
Loading

0 comments on commit b5e7a83

Please sign in to comment.