[java-interop, Java.Interop] Securely load native libs #691
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
grendello
reviewed
Aug 19, 2020
|
|
||
| JAVA_INTEROP_BEGIN_DECLS | ||
|
|
||
| #define JAVA_INTEROP_LIBRARY_FAILED (-1000) |
There was a problem hiding this comment.
If this header is to be consumed only by C++, could we use constexpr int JAVA_INTEROP_* = VALUE; instead? Strong typing FTW :)
jonpryor
force-pushed
the
jonp-loadlib-search-order
branch
from
4c791f4 to
f0bb80f
Compare
grendello
approved these changes
Aug 20, 2020
|
DO NOT MERGE until this can be merged: dotnet/android#5031 |
jonpryor
added a commit
to jonpryor/xamarin-android
that referenced
this pull request
Aug 20, 2020
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
jonpryor
force-pushed
the
jonp-loadlib-search-order
branch
3 times, most recently
from
4bfe49d to
001a684
Compare
jonpryor
added a commit
to jonpryor/xamarin-android
that referenced
this pull request
Aug 20, 2020
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
grendello
reviewed
Aug 21, 2020
jonpryor
force-pushed
the
jonp-loadlib-search-order
branch
4 times, most recently
from
fe6d479 to
bf2c410
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
jonpryor
added a commit
to jonpryor/xamarin-android
that referenced
this pull request
Aug 21, 2020
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
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
This was referenced Aug 24, 2020
jonpryor
force-pushed
the
jonp-loadlib-search-order
branch
from
594535c to
d410d5e
Compare
jonpryor
added a commit
to jonpryor/xamarin-android
that referenced
this pull request
Aug 24, 2020
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
|
Note: please use PR message when merging, not the commit message. |
radekdoulik
reviewed
Aug 25, 2020
Fixes: dotnet#676 Context: https://liquid.microsoft.com/Web/Object/Read/ms.security/Requirements/Microsoft.Security.SystemsADM.10039#guide The current security guidance is that the [`System.Runtime.InteropServices.DefaultDllImportSearchPathsAttribute`][0] attribute should be placed either on the assembly or on `[DllImport]` methods to control and constrain where [`LoadLibraryEx()`][1] will look for native libraries, in particular to *prevent* looking for native libraries within e.g. the current working directory or `%PATH%` or any other "attacker-controlled" location. Update `Java.Interop.dll` and `Java.Runtime.Environment.dll` so that the [`DllImportSearchPath`][2] values `AssemblyDirectory` and `SafeDirectories` are used: * `AssemblyDirectory`: "include the directory that contains the assembly itself, and search that directory first." * `SafeDirectories`: "Include the application directory, the `%WinDir%\System32` directory, and user directories in the DLL search path. Additionally, update `src/java-interop` so that instead of requiring the use of [**dlopen**(3)][3] on Windows, the following functions are added to support loading native libraries and resolving symbols from those native libraries: void* java_interop_lib_load (const char *path, unsigned int flags, char **error); void* java_interop_lib_symbol (void* library, const char *symbol, char **error); int java_interop_lib_close (void* library, char **error); (Previously, xamarin-android used the [dlfcn-win32/dlfcn-win32][4] library to implement **dlopen**(3), but dlfcn-win32/dlfcn-win32@ef7e412d calls `LoadLibraryEx()` with `LOAD_WITH_ALTERED_SEARCH_PATH`, which doesn't fulfill our internal requirements.) On Windows, `java_interop_lib_load()` will use [`LoadLibraryEx()`][5] to load libraries from a constrained set of directories: * `LOAD_LIBRARY_SEARCH_APPLICATION_DIR`: "the application's installation directory is searched for the DLL and its dependencies" * `LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR`: "the directory that contains the DLL is temporarily added to the beginning of the list of directories that are searched for the DLL's dependencies." * `LOAD_LIBRARY_SEARCH_USER_DIRS`: "directories added using the `AddDllDirectory()` or the `SetDllDirectory()` function are searched for the DLL and its dependencies" In order to simplify the introduction of `java_interop_lib_load()`, start *requiring* the presence of the symbols `mono_thread_get_managed_id` and `mono_thread_get_name_utf8`. These symbols have been present within Mono for ages at this point, and requiring means we don't need to support `dlopen(NULL)` semantics. Update the `@(ClInclude)` item group and `BuildMac` and related targets so that we properly rebuild things when e.g. `java-interop-dlfcn.h` changes, as would "normally" be expected. Finally, the continued use of `MONO_API` and other macros causes "weird" compiler issues when integrating with xamarin-android. Replace `MONO_API`/etc. use with `JAVA_INTEROP_API`/etc. instead. [0]: https://docs.microsoft.com/en-us/dotnet/api/system.runtime.interopservices.defaultdllimportsearchpathsattribute?view=netcore-3.1 [1]: https://docs.microsoft.com/en-us/windows/win32/api/libloaderapi/nf-libloaderapi-loadlibraryexa?redirectedfrom=MSDN [2]: https://docs.microsoft.com/en-us/dotnet/api/system.runtime.interopservices.dllimportsearchpath?view=netcore-3.1 [3]: https://linux.die.net/man/3/dlopen [4]: https://github.com/dlfcn-win32/dlfcn-win32 [5]: https://docs.microsoft.com/en-us/windows/win32/api/libloaderapi/nf-libloaderapi-loadlibraryexa
jonpryor
force-pushed
the
jonp-loadlib-search-order
branch
from
d410d5e to
35953da
Compare
radekdoulik
approved these changes
Aug 25, 2020
jonpryor
added a commit
to jonpryor/xamarin-android
that referenced
this pull request
Aug 25, 2020
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
jonpryor
added a commit
to jonpryor/xamarin-android
that referenced
this pull request
Aug 26, 2020
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
jonpryor
added a commit
to jonpryor/xamarin-android
that referenced
this pull request
Aug 26, 2020
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...dcaf794 * dotnet/java-interop@dcaf794d: [generator] Fix error and warning codes values (dotnet#697) * 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
jonpryor
added a commit
to dotnet/android
that referenced
this pull request
Aug 27, 2020
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...dcaf794 * dotnet/java-interop@dcaf794d: [generator] Fix error and warning codes values (#697) * dotnet/java-interop@08ff4db1: [java-interop, Java.Interop] Securely load native libs (#691) * dotnet/java-interop@09c68911: [generator] Clean up generated whitespace (#692) * dotnet/java-interop@d664e90e: [generator] Make warning and error messages localizable. (#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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: #676
Context: https://liquid.microsoft.com/Web/Object/Read/ms.security/Requirements/Microsoft.Security.SystemsADM.10039#guide
The current security guidance is that the
System.Runtime.InteropServices.DefaultDllImportSearchPathsAttributeattribute should be placed either on the assembly or on
[DllImport]methods to control and constrain where
LoadLibraryEx()will lookfor native libraries, in particular to prevent looking for native
libraries within e.g. the current working directory or
%PATH%or anyother "attacker-controlled" location.
Update
Java.Interop.dllandJava.Runtime.Environment.dllso thatthe
DllImportSearchPathvaluesAssemblyDirectoryandSafeDirectoriesare used:AssemblyDirectory: "include the directory that contains theassembly itself, and search that directory first."
SafeDirectories: "Include the application directory, the%WinDir%\System32directory, and user directories in the DLLsearch path.
Additionally, update
src/java-interopso that instead of requiringthe use of dlopen(3) on Windows, the following functions are
added to support loading native libraries and resolving symbols
from those native libraries:
(Previously, xamarin-android used the dlfcn-win32/dlfcn-win32
library to implement dlopen(3), but
dlfcn-win32/dlfcn-win32@ef7e412d calls
LoadLibraryEx()withLOAD_WITH_ALTERED_SEARCH_PATH, which doesn't fulfill our internalrequirements.)
On Windows,
java_interop_lib_load()will useLoadLibraryEx()to load libraries from a constrained set ofdirectories:
LOAD_LIBRARY_SEARCH_APPLICATION_DIR: "the application'sinstallation directory is searched for the DLL and its dependencies"
LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR: "the directory that containsthe DLL is temporarily added to the beginning of the list of
directories that are searched for the DLL's dependencies."
LOAD_LIBRARY_SEARCH_USER_DIRS: "directories added using theAddDllDirectory()or theSetDllDirectory()function are searchedfor the DLL and its dependencies"
In order to simplify the introduction of
java_interop_lib_load(), start requiring the presence of thesymbols
mono_thread_get_managed_idandmono_thread_get_name_utf8.These symbols have been present within Mono for ages at this point,
and requiring means we don't need to support
dlopen(NULL)semantics.Update the
@(ClInclude)item group andBuildMacand related targetsso that we properly rebuild things when e.g.
java-interop-dlfcn.hchanges, as would "normally" be expected.
Finally, the continued use of
MONO_APIand other macros causes"weird" compiler issues when integrating with xamarin-android.
Replace
MONO_API/etc. use withJAVA_INTEROP_API/etc. instead.