Add implementation of ProviderInstallerImpl

[voidstarstar]

This is an implementation of the method ProviderInstallerImpl to help protect against SSL exploits. A new dependency on conscrypt was added and it should be updated periodically. Minimal testing was done on an unofficial build of CM13, but further testing is welcome.

Fixes #504 and fixes #72.

I tested Hangouts it no longer appears to crash on startup, even without the workaround #72 (comment).

Issue #86 calls this code so it should probably be tested. I have not tested Gmail Sync, but I suspect this only fixes one of multiple problems with it.

Other Google and non-Google apps may be affected as well if they attempted to update the security provider.

Note: This is not a full implementation since a couple things are missing, but it seems to be enough to work. For example, the default HostnameVerifier is not set, which I think Google added to prevent certain attack vectors. Another thing missing is a test for the conscrypt_gmscore_jni version. These should be added in the future for better security and stability.

[F4uzan]

Tested the commit with Hangouts, everything seems to be working a-okay in the app. Notifications went through and sign-in works as expected.

I did the testing in AOSP 8.0, OmniROM 8.1, as well as AOSP 7.0. They all exhibit the same behaviour and I can attest the fix works just as well across different versions of Android.

[mar-v-in]

Hi @ale5000-git, Hi @voidstarstar

There is a good reason why this PR is not going to be merged as is, but I won't share this reason publicly. If any of you want to know the reason, please contact me in private.

My plan is to create a similar change though.

[voidstarstar]

I also sent a PM on XDA.

I wish GitHub still had a PM feature. :(

[voidstarstar]

@mar-v-in

I updated the code to a commonly used one-liner that simply inserts the Conscrypt provider as the provider with highest preference. It is also now using the latest version of Conscrypt as @ale5000-git requested.

I retested and it seems to still work fine, but further testing is always encouraged.

The new library appears to be released under the Apache License 2.0. It should be noted that this library was created by Google, so we we should be extra careful when deciding if it can be safely merged while still adhering to the ethos of the microG project.

[mar-v-in]

Looks good now. Thanks.

[ale5000-git]

@voidstarstar: I have a doubt, how can be all the code that there was before be replaced with a single line of code?

[ale5000-git]

@mar-v-in: I wonder if the eventual "throws" must be catched and if the return value must be logged.

public static int insertProviderAt(Provider provider, int position)
Returns:
    the actual preference position in which the provider was added, or -1 if the provider was not added because it is already installed.
Throws:
    NullPointerException - if provider is null
    SecurityException - if a security manager exists and its SecurityManager.checkSecurityAccess(java.lang.String) method denies access to add a new provider

[mar-v-in]

@ale5000-git both possible exception cases can't happen in our case: provider can't be null (is created just there) and security manager is not used on Android

[ale5000-git]

@mar-v-in: I'm not so used to Java but can't a "new" return null if there isn't enough free RAM?

[mar-v-in]

@ale5000-git no, Java can throw an OutOfMemoryError on object creation, but it would probably be a bad idea to catch those, as the calling app would need to properly act in such cases.

That said, I don't know if OutOfMemoryError happens on Android at all, I guess ART runtime would just panic and crash the app.

[ale5000-git]

In this case is fine but it still would be nice to show return value in logcat (for future debugging of eventual problems).

[Namnodorel]

@mar-v-in As somebody who is developing a resource intensive app, I can confirm that Android very much throws OutOfMemoryExceptions. This ends the thread the exception occours in, not necessarily the whole app.

[ale5000-git]

@mar-v-in: Isn't possible in a different thread to detect if this action has failed and eventually retry later?
Since many apps load automatically on startup I'm afraid that it can fail on really old phones where there are many hardware restrains.

[mar-v-in]

I don't think Google's original code would catch the OOM either, so we don't need to handle it either. If apps want to, they obviously can catch this case themselves.

Also Android will kill background apps and services if necessary for memory usage of foreground apps, so I doubt we'll see any problems with this

[kb-1000]

Hi, can I please know what was the original implementation of this PR and @mar-v-in what were your reasons for not using it?

[voidstarstar]

Would checking the returned value and logging it as an error if it's -1 be sufficient? Let me know if you'd like me to open voidstarstar@bc242ca as a new PR.

This is untested, but very straightforward.

[kb-1000]

I don't think it's as advanced as the original Google code. It checks if the return value is neither 1 nor -1, and throws an exception instead. In the case of 1 it even does some much more advanced things than this. It might be possible though that I looked at an outdated version of the code.

[voidstarstar]

@kaeptmblaubaer1000 A 1 or -1 are the only two possible return values of insertProviderAt since we pass in a position of 1.

The two documented exceptions do not apply. We do not pass a null Provider. As mentioned above, an unchecked exception like OutOfMemoryError may occur, but we shouldn't handle that here. We also should not get a SecurityException since Android does not have a SecurityManager. The documentation states "Application developers can assume that there's no SecurityManager installed"

I'm not sure what you mean by "advanced". Remember that we don't know what the original implementation actually looks like as it's closed source. Any code you're looking at is likely using ugly reflection or obfuscation and probably isn't especially useful or indicative of what good code should look like. Our goal should be to match the API specs as closely as possible, not the implementation.

[ale5000-git]

@mar-v-in: If possible it would be nice to find a way to fix this at microG level (despite the problem affect also Google Play Service).

Look:

Calling ProviderInstaller.installIfNeeded
...
Originally, we thought we were done after doing this step. However, after further testing, we noticed something peculiar. Many devices succeeded in updating the security Provider via installIfNeeded, but were still throwing SSLHandshakeExceptions when the server required TLS 1.2!

Our initial understanding of OkHttp’s HTTPS strategy was that it uses the highest-installed version of TLS, and it falls back to older protocols if newer ones are not installed. This is false.

OkHttp’s default behavior is to use an SSLSocketFactory to create SSLSockets with default parameters, and select from protocols that are enabled for a given SSLSocket. Here’s the kicker: The latest protocols installed on a device are not necessarily enabled on its default SSLSockets!

To fix this, we need to override OkHttp’s SSLSocketFactory to enable TLS 1.2 on all SSLSockets

Ref: https://medium.com/tech-quizlet/working-with-tls-1-2-on-android-4-4-and-lower-f4f5205629a

[kb-1000]

Look at what I did in MGit, it might fix the issue here too...
Yes, the origihal code uses reflection but AFAIK this is needed to access private fields, isn't it?
The most important thing is that it works for MGit at least.

[voidstarstar]

Look at what I did in MGit, it might fix the issue here too...
Yes, the origihal code uses reflection but AFAIK this is needed to access private fields, isn't it?
The most important thing is that it works for MGit at least.

@kaeptmblaubaer1000

For reference

I actually did happen to come across MGit while working on this patch, but I didn't realize you were the one that committed this file (about 1 year ago).

[ale5000-git]

@mar-v-in

See the relevant changes from MGit project:
https://github.com/maks/MGit/pull/302/files
https://github.com/maks/MGit/pull/272/files
https://github.com/maks/MGit/pull/340/files