Skip to content

Commit

Permalink
Fix parallel TLS hook issue
Browse files Browse the repository at this point in the history
It's unclear why, but in some scenarios the hook would reliably hit a
null pointer within the native realCallback() call if the call is made
while another is already in progress (even though without Frida that's
presumably happening just fine, and they're different cb pointers etc
etc).

We could use Frida's exclusive scheduling to fix this, but that raises
the risk of a deadlock here a bit in, so instead we do a very simple
locking setup with a polling unlock. Very quick & rough but works
nicely, and allows reentrant locks in a single thread in case some apps
use SSL to verify SSL somehow. Hard to imagine a cross-thread deadlock
here so hopefully that'll be sufficient...
  • Loading branch information
pimterry committed Jun 17, 2024
1 parent e99740b commit 5b0fd45
Showing 1 changed file with 20 additions and 6 deletions.
26 changes: 20 additions & 6 deletions native-tls-hook.js
Original file line number Diff line number Diff line change
Expand Up @@ -103,15 +103,27 @@ function patchTargetLib(targetLib) {
? new NativeFunction(realCallbackAddr, 'int', ['pointer','pointer'])
: () => SSL_VERIFY_INVALID; // Callback can be null - treat as invalid (=our validation only)

let pendingCheckThreads = new Set();

const hookedCallback = new NativeCallback(function (ssl, out_alert) {
let realResult = false;
let realResult = false; // False = not yet called, 0/1 = call result

const threadId = Process.getCurrentThreadId();
const alreadyHaveLock = pendingCheckThreads.has(threadId);

// We try to have only one thread running these checks at a time, as parallel calls
// here on the same underlying callback seem to crash in some specific scenarios
while (pendingCheckThreads.size > 0 && !alreadyHaveLock) {
Thread.sleep(0.01);
}
pendingCheckThreads.add(threadId);

if (targetLib !== 'libboringssl.dylib') {
// Cronet assumes its callback is always called, and crashes if not. iOS's BoringSSL
// meanwhile seems to use some negative checks in its callback, and rejects the
// connection independently of the return value here if it's called with a bad cert.
// End result: we *only sometimes* proactively call the callback.
realResult = realCallback(ssl, out_alert)
realResult = realCallback(ssl, out_alert);
}

// Extremely dumb certificate validation: we accept any chain where the *exact* CA cert
Expand All @@ -138,16 +150,18 @@ function patchTargetLib(targetLib) {
const certData = new Uint8Array(certPointer.readByteArray(certDataLength));

if (certData.every((byte, j) => CERT_DER[j] === byte)) {
if (!alreadyHaveLock) pendingCheckThreads.delete(threadId);
return SSL_VERIFY_OK;
}
}

// No matched peer - fallback to the provided callback instead:
if (realResult !== false) {
return realResult;
} else {
return realCallback(ssl, out_alert);
if (realResult === false) { // Haven't called it yet
realResult = realCallback(ssl, out_alert);
}

if (!alreadyHaveLock) pendingCheckThreads.delete(threadId);
return realResult;
}, 'int', ['pointer','pointer']);

verificationCallbackCache[realCallbackAddr] = hookedCallback;
Expand Down

0 comments on commit 5b0fd45

Please sign in to comment.