From 4cb2d44f6cb62bb36952fd7f8545740bd8661357 Mon Sep 17 00:00:00 2001 From: Tim Perry Date: Wed, 8 Nov 2023 14:26:13 +0100 Subject: [PATCH] Use targetMethod.call(this...) over this.[method](...) This avoids the potential for bugs (calling the method directly does dynamic lookup based on argument types, which may not match the called method in ambiguous cases, e.g. int vs double are indistinguishable in JS) and improves performance (skipping any dynamic method lookup). --- .../android-certificate-unpinning-fallback.js | 197 +++++++++--------- android/android-certificate-unpinning.js | 17 +- 2 files changed, 110 insertions(+), 104 deletions(-) diff --git a/android/android-certificate-unpinning-fallback.js b/android/android-certificate-unpinning-fallback.js index 3378460..e329453 100644 --- a/android/android-certificate-unpinning-fallback.js +++ b/android/android-certificate-unpinning-fallback.js @@ -107,107 +107,109 @@ Java.perform(function () { }; }; - const buildUnhandledErrorPatcher = (errorClassName) => function (errorMessage) { - try { - console.log('\n !!! --- Unexpected TLS failure --- !!!'); + const buildUnhandledErrorPatcher = (errorClassName, originalConstructor) => { + return function (errorMessage) { + try { + console.log('\n !!! --- Unexpected TLS failure --- !!!'); - // Parse the stack trace to work out who threw this error: - const stackTrace = Java.use('java.lang.Thread').currentThread().getStackTrace(); - const exceptionStackIndex = stackTrace.findIndex(stack => - stack.getClassName() === errorClassName - ); - const callingFunctionStack = stackTrace[exceptionStackIndex + 1]; + // Parse the stack trace to work out who threw this error: + const stackTrace = Java.use('java.lang.Thread').currentThread().getStackTrace(); + const exceptionStackIndex = stackTrace.findIndex(stack => + stack.getClassName() === errorClassName + ); + const callingFunctionStack = stackTrace[exceptionStackIndex + 1]; - const className = callingFunctionStack.getClassName(); - const methodName = callingFunctionStack.getMethodName(); - console.log(` Thrown by ${className}->${methodName}`); + const className = callingFunctionStack.getClassName(); + const methodName = callingFunctionStack.getMethodName(); + console.log(` Thrown by ${className}->${methodName}`); - const callingClass = Java.use(className); - const callingMethod = callingClass[methodName]; + const callingClass = Java.use(className); + const callingMethod = callingClass[methodName]; - callingMethod.overloads.forEach((failingMethod) => { - if (failingMethod.implementation) { - console.warn(' Already patched - but still failing!') - return; // Already patched by Frida - skip it - } - - // Try to spot known methods (despite obfuscation) and disable them: - if (isOkHttpCheckMethod(errorMessage, failingMethod)) { - // See okhttp3.CertificatePinner patches in unpinning script: - failingMethod.implementation = () => { - if (DEBUG_MODE) console.log(` => Fallback OkHttp patch`); - }; - console.log(` [+] ${className}->${methodName} (fallback OkHttp patch)`); - } else if (isAppmattusOkHttpInterceptMethod(errorMessage, failingMethod)) { - // See Appmattus CertificateTransparencyInterceptor patch in unpinning script: - const chainType = Java.use(failingMethod.argumentTypes[0].className); - const responseTypeName = failingMethod.returnType.className; - const okHttpChain = matchOkHttpChain(chainType, responseTypeName); - failingMethod.implementation = (chain) => { - if (DEBUG_MODE) console.log(` => Fallback Appmattus+OkHttp patch`); - const proceed = chain[okHttpChain.proceedMethodName].bind(chain); - const request = chain[okHttpChain.requestFieldName].value; - return proceed(request); - }; - console.log(` [+] ${className}->${methodName} (fallback Appmattus+OkHttp patch)`); - } else if (isX509TrustManager(callingClass, methodName)) { - const argumentTypes = failingMethod.argumentTypes.map(t => t.className); - const returnType = failingMethod.returnType.className; - - if ( - argumentTypes.length === 2 && - argumentTypes.every((t, i) => t === BASE_METHOD_ARGUMENTS[i]) && - returnType === 'void' - ) { - // For the base method, just check against the default: - failingMethod.implementation = (certs, authType) => { - if (DEBUG_MODE) console.log(` => Fallback X509TrustManager patch of ${ - className - } base method`); - - const defaultTrustManager = getCustomX509TrustManager(); // Defined in the unpinning script - defaultTrustManager.checkServerTrusted(certs, authType); - }; - console.log(` [+] ${className}->${methodName} (fallback X509TrustManager base patch)`); - } else if ( - argumentTypes.length === 3 && - argumentTypes.every((t, i) => t === EXTENDED_METHOD_ARGUMENTS[i]) && - returnType === 'java.util.List' - ) { - // For the extended method, we just ignore the hostname, and if the certs are good - // (i.e they're ours), then we say the whole chain is good to go: - failingMethod.implementation = function (certs, authType, _hostname) { - if (DEBUG_MODE) console.log(` => Fallback X509TrustManager patch of ${ - className - } extended method`); - - try { - defaultTrustManager.checkServerTrusted(certs, authType); - } catch (e) { - console.error('Default TM threw:', e); - } - return Java.use('java.util.Arrays').asList(certs); - }; - console.log(` [+] ${className}->${methodName} (fallback X509TrustManager ext patch)`); - } else { - console.warn(` [ ] Skipping unrecognized checkServerTrusted signature in class ${ - callingClass.class.getName() - }`); + callingMethod.overloads.forEach((failingMethod) => { + if (failingMethod.implementation) { + console.warn(' Already patched - but still failing!') + return; // Already patched by Frida - skip it } - } else { - console.error(' [ ] Unrecognized TLS error - this must be patched manually'); - return; - // Later we could try to cover other cases here - automatically recognizing other - // OkHttp interceptors for example, or potentially other approaches, but we need - // to do so carefully to avoid disabling TLS checks entirely. - } - }); - } catch (e) { - console.log(' [ ] Failed to automatically patch failure'); - console.warn(e); - } - return this.$init(errorMessage); + // Try to spot known methods (despite obfuscation) and disable them: + if (isOkHttpCheckMethod(errorMessage, failingMethod)) { + // See okhttp3.CertificatePinner patches in unpinning script: + failingMethod.implementation = () => { + if (DEBUG_MODE) console.log(` => Fallback OkHttp patch`); + }; + console.log(` [+] ${className}->${methodName} (fallback OkHttp patch)`); + } else if (isAppmattusOkHttpInterceptMethod(errorMessage, failingMethod)) { + // See Appmattus CertificateTransparencyInterceptor patch in unpinning script: + const chainType = Java.use(failingMethod.argumentTypes[0].className); + const responseTypeName = failingMethod.returnType.className; + const okHttpChain = matchOkHttpChain(chainType, responseTypeName); + failingMethod.implementation = (chain) => { + if (DEBUG_MODE) console.log(` => Fallback Appmattus+OkHttp patch`); + const proceed = chain[okHttpChain.proceedMethodName].bind(chain); + const request = chain[okHttpChain.requestFieldName].value; + return proceed(request); + }; + console.log(` [+] ${className}->${methodName} (fallback Appmattus+OkHttp patch)`); + } else if (isX509TrustManager(callingClass, methodName)) { + const argumentTypes = failingMethod.argumentTypes.map(t => t.className); + const returnType = failingMethod.returnType.className; + + if ( + argumentTypes.length === 2 && + argumentTypes.every((t, i) => t === BASE_METHOD_ARGUMENTS[i]) && + returnType === 'void' + ) { + // For the base method, just check against the default: + failingMethod.implementation = (certs, authType) => { + if (DEBUG_MODE) console.log(` => Fallback X509TrustManager patch of ${ + className + } base method`); + + const defaultTrustManager = getCustomX509TrustManager(); // Defined in the unpinning script + defaultTrustManager.checkServerTrusted(certs, authType); + }; + console.log(` [+] ${className}->${methodName} (fallback X509TrustManager base patch)`); + } else if ( + argumentTypes.length === 3 && + argumentTypes.every((t, i) => t === EXTENDED_METHOD_ARGUMENTS[i]) && + returnType === 'java.util.List' + ) { + // For the extended method, we just ignore the hostname, and if the certs are good + // (i.e they're ours), then we say the whole chain is good to go: + failingMethod.implementation = function (certs, authType, _hostname) { + if (DEBUG_MODE) console.log(` => Fallback X509TrustManager patch of ${ + className + } extended method`); + + try { + defaultTrustManager.checkServerTrusted(certs, authType); + } catch (e) { + console.error('Default TM threw:', e); + } + return Java.use('java.util.Arrays').asList(certs); + }; + console.log(` [+] ${className}->${methodName} (fallback X509TrustManager ext patch)`); + } else { + console.warn(` [ ] Skipping unrecognized checkServerTrusted signature in class ${ + callingClass.class.getName() + }`); + } + } else { + console.error(' [ ] Unrecognized TLS error - this must be patched manually'); + return; + // Later we could try to cover other cases here - automatically recognizing other + // OkHttp interceptors for example, or potentially other approaches, but we need + // to do so carefully to avoid disabling TLS checks entirely. + } + }); + } catch (e) { + console.log(' [ ] Failed to automatically patch failure'); + console.warn(e); + } + + return originalConstructor.call(this, ...arguments); + } }; // These are the exceptions we watch for and attempt to auto-patch out after they're thrown: @@ -217,7 +219,10 @@ Java.perform(function () { ].forEach((errorClassName) => { const ErrorClass = Java.use(errorClassName); ErrorClass.$init.overloads.forEach((overload) => { - overload.implementation = buildUnhandledErrorPatcher(errorClassName, ErrorClass); + overload.implementation = buildUnhandledErrorPatcher( + errorClassName, + overload + ); }); }) diff --git a/android/android-certificate-unpinning.js b/android/android-certificate-unpinning.js index 870d119..9377d26 100644 --- a/android/android-certificate-unpinning.js +++ b/android/android-certificate-unpinning.js @@ -136,13 +136,13 @@ const PINNING_FIXES = { { methodName: '$init', overload: '*', - replacement: () => { + replacement: (targetMethod) => { const PinSet = Java.use('android.security.net.config.PinSet'); const EMPTY_PINSET = PinSet.EMPTY_PINSET.value; return function () { // Always ignore the 2nd 'pins' PinSet argument entirely: arguments[2] = EMPTY_PINSET; - this.$init(...arguments); + targetMethod.call(this, ...arguments); } } } @@ -167,7 +167,7 @@ const PINNING_FIXES = { 'java.util.List', 'java.net.ProxySelector' ], - replacement: () => { + replacement: (targetMethod) => { const defaultHostnameVerifier = Java.use("com.android.okhttp.internal.tls.OkHostnameVerifier") .INSTANCE.value; const defaultCertPinner = Java.use("com.android.okhttp.CertificatePinner") @@ -179,7 +179,7 @@ const PINNING_FIXES = { arguments[5] = defaultHostnameVerifier; arguments[6] = defaultCertPinner; - this.$init(...arguments); + targetMethod.call(this, ...arguments); } } }, @@ -201,7 +201,7 @@ const PINNING_FIXES = { 'java.util.List', 'java.net.ProxySelector' ], - replacement: () => { + replacement: (targetMethod) => { const defaultHostnameVerifier = Java.use("com.android.okhttp.internal.tls.OkHostnameVerifier") .INSTANCE.value; const defaultCertPinner = Java.use("com.android.okhttp.CertificatePinner") @@ -213,7 +213,7 @@ const PINNING_FIXES = { arguments[4] = defaultHostnameVerifier; arguments[5] = defaultCertPinner; - this.$init(...arguments); + targetMethod.call(this, ...arguments); } } } @@ -346,13 +346,14 @@ const PINNING_FIXES = { 'com.silkimen.cordovahttp.CordovaServerTrust': [ { methodName: '$init', - replacement: () => function () { + replacement: (targetMethod) => function () { // Ignore any attempts to set trust to 'pinned'. Default settings will trust // our cert because of the separate system-certificate injection step. if (arguments[0] === 'pinned') { arguments[0] = 'default'; } - return this.$init(...arguments); + + return targetMethod.call(this, ...arguments); } } ],