-
Notifications
You must be signed in to change notification settings - Fork 131
Fix Android 16 CT enforcement, TrustManagerImpl fallback, and native TLS null pointer crash #178
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -130,6 +130,23 @@ const PINNING_FIXES = { | |
| } | ||
| ], | ||
|
|
||
| // --- Native Conscrypt Certificate Transparency (Android 16+ / API 36+) | ||
| // Android 16 enforces CT verification for all certificates. Proxy-generated | ||
| // MITM certs lack SCTs, causing NOT_ENOUGH_SCTS failures. Since all traffic | ||
| // is routed through the local proxy, CT checks are unnecessary. | ||
|
|
||
| 'com.android.org.conscrypt.ct.CertificateTransparency': [ | ||
| { | ||
| methodName: 'isCTVerificationRequired', | ||
| replacement: () => () => false | ||
| }, | ||
| { | ||
| methodName: 'checkCT', | ||
| overload: '*', | ||
| replacement: () => NO_OP | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is actually already implemented in httptoolkit/frida-interception-and-unpinning#166 (although that hasn't been pulled into this repo yet). The |
||
| } | ||
| ], | ||
|
|
||
| // --- Native pinning configuration loading (used for configuration by many libraries) | ||
|
|
||
| 'android.security.net.config.NetworkSecurityConfig': [ | ||
|
|
||
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -106,6 +106,10 @@ function patchTargetLib(targetModule, targetName) { | |
| let pendingCheckThreads = new Set(); | ||
|
|
||
| const hookedCallback = new NativeCallback(function (ssl, out_alert) { | ||
| if (!ssl || ssl.isNull()) { | ||
| return SSL_VERIFY_INVALID; | ||
| } | ||
|
|
||
| let realResult = false; // False = not yet called, 0/1 = call result | ||
|
|
||
| const threadId = Process.getCurrentThreadId(); | ||
|
|
@@ -123,7 +127,11 @@ function patchTargetLib(targetModule, targetName) { | |
| // 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); | ||
| try { | ||
| realResult = realCallback(ssl, out_alert); | ||
| } catch (e) { | ||
| realResult = SSL_VERIFY_INVALID; | ||
| } | ||
| } | ||
|
|
||
| // Extremely dumb certificate validation: we accept any chain where the *exact* CA cert | ||
|
|
@@ -136,17 +144,36 @@ function patchTargetLib(targetModule, targetName) { | |
| // or leaf certs, and lots of other issues. This is significantly better than nothing, | ||
| // but it is not production-ready TLS verification for general use in untrusted envs! | ||
|
|
||
| const peerCerts = SSL_get0_peer_certificates(ssl); | ||
| let peerCerts; | ||
| try { | ||
| peerCerts = SSL_get0_peer_certificates(ssl); | ||
| } catch (e) { | ||
| if (!alreadyHaveLock) pendingCheckThreads.delete(threadId); | ||
| return (realResult !== false) ? realResult : SSL_VERIFY_INVALID; | ||
| } | ||
|
|
||
| if (!peerCerts || peerCerts.isNull()) { | ||
| if (!alreadyHaveLock) pendingCheckThreads.delete(threadId); | ||
| return (realResult !== false) ? realResult : SSL_VERIFY_INVALID; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Feels like some of this could be better handled with a large try/finally around a larger block. Are you adding these checks blindly to every possible call, or because you've found specific cases where these can fail? It would be useful to know what those cases are if so. |
||
| } | ||
|
|
||
| // Loop through every cert in the chain: | ||
| for (let i = 0; i < sk_num(peerCerts); i++) { | ||
| // For each cert, check if it *exactly* matches our configured CA cert: | ||
| const cert = sk_value(peerCerts, i); | ||
| const certDataLength = crypto_buffer_len(cert).toNumber(); | ||
|
|
||
| let certDataLength; | ||
| try { | ||
| certDataLength = crypto_buffer_len(cert).toNumber(); | ||
| } catch (e) { continue; } | ||
|
|
||
| if (certDataLength !== CERT_DER.byteLength) continue; | ||
|
|
||
| const certPointer = crypto_buffer_data(cert); | ||
| let certPointer; | ||
| try { | ||
| certPointer = crypto_buffer_data(cert); | ||
| } catch (e) { continue; } | ||
|
|
||
| const certData = new Uint8Array(certPointer.readByteArray(certDataLength)); | ||
|
|
||
| if (certData.every((byte, j) => CERT_DER[j] === byte)) { | ||
|
|
@@ -157,7 +184,11 @@ function patchTargetLib(targetModule, targetName) { | |
|
|
||
| // No matched peer - fallback to the provided callback instead: | ||
| if (realResult === false) { // Haven't called it yet | ||
| realResult = realCallback(ssl, out_alert); | ||
| try { | ||
| realResult = realCallback(ssl, out_alert); | ||
| } catch (e) { | ||
| realResult = SSL_VERIFY_INVALID; | ||
| } | ||
| } | ||
|
|
||
| if (!alreadyHaveLock) pendingCheckThreads.delete(threadId); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is for handling cases where we can't see the class or method names at all due to obfuscation. When we do have them like this, we should just put the hook into the normal hooks list in
android-certificate-unpinning.jsinstead.Do you know why the existing
TrustedCertificateIndexpatch isn't handling this case? It would be very useful to look at the Conscrypt code and see what's changed there, as it might be possible to just fix that to handle this instead.