AndroidPlatform.trustManager() not using ClassLoader from delegate object

[stevelilly-tw]

I am seeing a very similar crash to #2323, reproducible on KitKat and Marshmellow, i.e.:

java.lang.IllegalStateException: Unable to extract the trust manager on okhttp3.internal.platform.AndroidPlatform@b1a3a6d8, sslSocketFactory is class com.paypal.android.sdk.cg
                                                                                at okhttp3.OkHttpClient$Builder.sslSocketFactory(OkHttpClient.java:599)
                                                                                at com.paypal.android.sdk.cc.a(Unknown Source)
                                                                                at com.paypal.android.sdk.cm.<init>(Unknown Source)
                                                                                at com.paypal.android.sdk.payments.PayPalService.a(Unknown Source)
                                                                                at com.paypal.android.sdk.payments.PayPalService.onBind(Unknown Source)

From what I can tell, for me the fallback mechanism inside AndroidPlatform.trustManager() is trying to work, i.e.:

@Override public X509TrustManager trustManager(SSLSocketFactory sslSocketFactory) {
    Object context = readFieldOrNull(sslSocketFactory, sslParametersClass, "sslParameters");
    if (context == null) {
      // If that didn't work, try the Google Play Services SSL provider before giving up. This
      // must be loaded by the SSLSocketFactory's class loader.
      try {
        Class<?> gmsSslParametersClass = Class.forName(
            "com.google.android.gms.org.conscrypt.SSLParametersImpl", false,
            sslSocketFactory.getClass().getClassLoader());

However, it seems that the sslSocketFactory class loader can't load that class, however the sslSocketFactory.delegate class loader can load the class.

A little background: my app allows both Google and PayPal login. PayPal login from start is OK. Attempting a Google login first somehow swaps the com.android.org.conscrypt.SSLParametersImpl for a com.google.android.gms.org.conscrypt.SSLParametersImpl under the hood, after which attempting a PayPal login fails to load gmsSslParametersClass and leads to the crash.

The sslSocketFactory is provided by the PayPal SDK and so loaded from my app's classloader. The delegate seems to be able to be loaded from /system/priv-app/PrebuiltGmsCore.apk

Correct me if I'm wrong, but doesn't that code need to cater for the class loader in the delegate case as well?

[swankjesse]

Yeah, what a pain. Thanks for investigating this.

I think the best fix is to ask our friends at PayPal to use the 2-argument sslSocketFactory() method that accepts both an SSLSocketFactory and a TrustManager. The thing we’re doing now that attempts to pull the trust manager out via reflection isn’t very robust.

https://square.github.io/okhttp/3.x/okhttp/okhttp3/OkHttpClient.Builder.html#sslSocketFactory-javax.net.ssl.SSLSocketFactory-javax.net.ssl.X509TrustManager-

Wanna suggest that to them?

[stevelilly-tw]

Thanks Jesse. Yes that does seem safer. I've raised an issue on PayPal SDK here paypal/PayPal-Android-SDK#341

Meantime, I was wondering why the concrete type of the sslParameters object is checked so strictly? It seems we are only really trying to locate sslSocketFactory.(delegate.)*.sslParameters.trustManager, the intermediate types aren't so important, so long as trustManager is an X509TrustManager.class. With that in mind I was considering if this would suffice:

  @Override public X509TrustManager trustManager(SSLSocketFactory sslSocketFactory) {
    Object context = readFieldOrNull(sslSocketFactory, Object.class, "sslParameters");
    if (context == null) {
      return super.trustManager(sslSocketFactory);
    }

    X509TrustManager x509TrustManager = readFieldOrNull(
        context, X509TrustManager.class, "x509TrustManager");
    if (x509TrustManager != null) return x509TrustManager;

    return readFieldOrNull(context, X509TrustManager.class, "trustManager");
  }

What do you think?

[swankjesse]

This reflection code is very fragile and I’m reluctant to make it more open than absolutely necessary. In particular I’m worried that a clever policy might break security expectations.

No further action to take on this.