Modify sslpinning bypass for okhttp3 library

[kiven7299]

No description provided.

[leonjza]

Thanks for the PR. In thinking about it, I think we need to explicitly also hook certificatePinner.check$okhttp in addition to certificatePinner.check$okhttp.overload("java.lang.String", "u15"). Could you update the code with that?

[kiven7299]

Yes I could do that.
However, I wonder whether we must use overload or not, since there's only 1 check$okhttp method in Okhttp3.CertificatePinner.

• From Okhttp3 source code, link: https://github1s.com/square/okhttp/blob/HEAD/okhttp/src/jvmMain/kotlin/okhttp3/CertificatePinner.kt
  
• From result of android hooking list class_methods okhttp3.CertificatePin:
  

Besides, the overload("java.lang.String", "u15") seems not match with arguments of check$okhttp method (internal fun check(hostname: String, cleanedPeerCertificatesFn: () -> List<X509Certificate>)), refers to the initial issue.

[leonjza]

Right, but in older versions of okhttp the function signatures are different.

[kiven7299]

I've added hook to certificatePinner.check$okhttp.overload("java.lang.String", "u15"). Please review the new commit.

[CDuPlooy]

I only had a quick look at this, but since the hook is just logging (and thus not throwing the exception); It should be easyish to use the information that the Java bridge already provides to hook all overloads, and then make them all do the same thing. I can take a stab at this sometime next week @kiven7299 if you don't want to.

I'm on a quick lunch break, but I think something like the below should work.

const overloads = Java.use("target_class")["target_method"].overloads
ovearloads.forEach(overload => {
     overload.implementation = function(arguments) {
          // do some logging or whatever
     }
})

This is not the only hook where something like this would be useful.

[kiven7299]

Hi @CDuPlooy,
Thanks for your advice, I've changed the code based on that

const okHttp3CertificatePinnerCheckOkHttp = (ident: string): any | undefined => {
  return wrapJavaPerform(() => {
    try {
      const certificatePinner = Java.use("okhttp3.CertificatePinner");
      certificatePinner["check$okhttp"].overloads.forEach((m) => {
          // get the argument types for this overload
          var calleeArgTypes = m.argumentTypes.map((arg) => arg.className);
          send(color_1.colors.blackBright(`Found okhttp3.CertificatePinner.check$okhttp(${calleeArgTypes.join(", ")}), overriding ...`));

          m.implementation = function () {
              helpers_1.qsend(quiet, color_1.colors.blackBright(`[${ident}] `) + `Called check$okhttp ` +
                  color_1.colors.green(`OkHTTP 3.x CertificatePinner.check$okhttp()`) +
                  `, not throwing an exception.`);
          }
      });
      return CertificatePinnerCheckOkHttp;

    } catch (err) {
      if ((err as Error).message.indexOf("ClassNotFoundException") === 0) {
        throw err;
      }
    }
  });
};

Run code:

[CDuPlooy]

My only feedback would be to rename that m variable to overload, such that the code is a bit more readable. Otherwise it looks great! Thank you for adding this :)

I'll test it today with a new and old version of OkHttp

[CDuPlooy]

I tried to use your branch however the TS did not compile. I have two small suggestions.

The first is to change the hook so that it looks like this:

const okHttp3CertificatePinnerCheckOkHttp = (ident: string): any | undefined => {
  // -- Sample Java
  //
  // Example used to test this bypass.
  //
  // String hostname = "swapi.co";
  // CertificatePinner certificatePinner = new CertificatePinner.Builder()
  //         .add(hostname, "sha256/AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=")
  //         .build();
  // OkHttpClient client = new OkHttpClient.Builder()
  //         .certificatePinner(certificatePinner)
  //         .build();
  // Request request = new Request.Builder()
  //         .url("https://swapi.co/api/people/1")
  //         .build();
  // Response response = client.newCall(request).execute();
  return wrapJavaPerform(() => {
    try {
      const certificatePinner = Java.use("okhttp3.CertificatePinner");
      let originalFunctions = []

      certificatePinner["check$okhttp"].overloads.forEach((overload) => {
          // preserve the implementations so that we can create a proper job
          originalFunctions.push(overload);

          // get the argument types for this overload          
          const calleeArgTypes = overload.argumentTypes.map((arg) => arg.className);
          send(c.blackBright(`Found okhttp3.CertificatePinner.check$okhttp(${calleeArgTypes.join(", ")}), overriding ...`));
          overload.implementation = function () {
              qsend(quiet, c.blackBright(`[${ident}] `) + `Called check$okhttp ` +
                  c.green(`OkHTTP 3.x CertificatePinner.check$okhttp()`) +
                  `, not throwing an exception.`);
          }
      });

      return originalFunctions;

    } catch (err) {
      if ((err as Error).message.indexOf("ClassNotFoundException") === 0) {
        throw err;
      }
    }
  });
};

You'll see that for many of the functionality in objection job objects are created, from the TUI you can issue jobs list and jobs kill $TARGET if you want to stop a specific hook from running. This works (on the java-bridge anyway) by setting the implementation to null; That's why these functions return references to the class & method pairs. I changed this a little to return an array of the overloads, which will eventually be called by jobs kill. That routine will try to access each implementation's implementation ie:

    // remove implementation replacements
    if (job.implementations && job.implementations.length > 0) {
      job.implementations.forEach((method) => {
        // TODO: May be racy if the method is currently used.
        method.implementation = null;
      });
    }

So basically we should be pushing back an array of implementations (one for each encountered overload).

The only other change was ensuring that we then concat this array and not push it (otherwise when the js code tries to read .implementation from the job object, it'll be undefined)

That could be done via:

  job.implementations.push(sslContextEmptyTrustManager(job.identifier));
  job.implementations.push(okHttp3CertificatePinnerCheck(job.identifier));
  
// Specifically this 
 job.implementations.concat(okHttp3CertificatePinnerCheckOkHttp(job.identifier));
  
 job.implementations.push(appceleratorTitaniumPinningTrustManager(job.identifier));
  job.implementations.push(trustManagerImplVerifyChainCheck(job.identifier));
  job.implementations.push(trustManagerImplCheckTrustedRecursiveCheck(job.identifier));
  job.implementations.push(phoneGapSSLCertificateChecker(job.identifier));
  jobs.add(job);

@leonjza, I'm not sure this works though, as when I kill the job, the hooks are still active. I don't think this has to do with the racey behaviour, because the dummy app only makes a request if you ask it to

Finally, I've verified that with these changes, the bypass still works on your dummy app, however I'd like to test on something that uses a more recent version of OkHttp which I will do during the course of today :)

The full diff is:

147,158c147,154
<       let originalFunctions = []
< 
<       certificatePinner["check$okhttp"].overloads.forEach((overload) => {
<           // preserve the implementations so that we can create a proper job
<           originalFunctions.push(overload);
< 
<           // get the argument types for this overload          
<           const calleeArgTypes = overload.argumentTypes.map((arg) => arg.className);
<           send(c.blackBright(`Found okhttp3.CertificatePinner.check$okhttp(${calleeArgTypes.join(", ")}), overriding ...`));
<           overload.implementation = function () {
<               qsend(quiet, c.blackBright(`[${ident}] `) + `Called check$okhttp ` +
<                   c.green(`OkHTTP 3.x CertificatePinner.check$okhttp()`) +
---
>       certificatePinner["check$okhttp"].overloads.forEach((m) => {
>           // get the argument types for this overload
>           var calleeArgTypes = m.argumentTypes.map((arg) => arg.className);
>           send(color_1.colors.blackBright(`Found okhttp3.CertificatePinner.check$okhttp(${calleeArgTypes.join(", ")}), overriding ...`));
> 
>           m.implementation = function () {
>               helpers_1.qsend(quiet, color_1.colors.blackBright(`[${ident}] `) + `Called check$okhttp ` +
>                   color_1.colors.green(`OkHTTP 3.x CertificatePinner.check$okhttp()`) +
162,163c158
< 
<       return originalFunctions;
---
>       return CertificatePinnerCheckOkHttp;
329c324
<   job.implementations.concat(okHttp3CertificatePinnerCheckOkHttp(job.identifier));
---
>   job.implementations.push(okHttp3CertificatePinnerCheckOkHttp(job.identifier));
336d330
< 

[kiven7299]

Hi @CDuPlooy,
I know the problems in my latest committed code; I type helpers_1.qsend instead of qsend, and color_1.colors.blackBright instead of c.blackBright.
Your changes are great and it might be useful in some cases. However, normally when we kill bypass sslpinning job, it is expected to kill all the hooks to all libraries. In my opinion, there is barely a need to kill just one override of CertificatePinner.check$okhttp method.

[kiven7299]

I think I've made much more error in the latest commit. My git skill is suck.

[CDuPlooy]

I think there may be some confusion here. In the example I show, okHttp3CertificatePinnerCheckOkHttp returns an array of Java methods. The job kill routine takes the job object, and then performs a series of actions on each Interceptor, implementation and whatever else. Where previously this hook only operated on one overload, it was okay to return a single element - but now that it operates on multiple elements, it needs to return all of those elements.

With the previous change, if you only pushed one of several overloads and run the kill routine, only that overload would be stopped and the remaining hooks would still be in place with no way to kill them.