Skip to content

Commit 02cad94

Browse files
simonrozsivalCopilot
simonrozsival
and
Copilot
committed
Replace native AEAD heuristic with managed retry-and-observe on Android
The previous fix treated any GeneralSecurityException during an Android AEAD decrypt as an authentication tag mismatch. That depended on Conscrypt implementation details and risked false positives, so revert the three native commits (pal_cipher.c / pal_jni.c / pal_jni.h) back to recognizing only a genuine AEADBadTagException. Root cause: on AEAD decrypt failure Conscrypt derives the Java exception type by popping the oldest entry from BoringSSL's thread-local FIFO error queue. A stale error left by an unrelated prior operation on the same pooled thread (e.g. an RSA failure) is read first and mapped to the wrong type (InvalidKeyException), which is not a BadPaddingException and so is never converted to AEADBadTagException. Conscrypt clears the whole queue after reading, so the failed attempt leaves a clean queue. Fix: in the managed Android AEAD DecryptCore, when the first attempt throws a CryptographicException that is not AuthenticationTagMismatchException, retry the decrypt once on the same thread. The retry reads a clean error queue and observes the true result (a real tag mismatch surfaces as AEADBadTagException). A valid ciphertext authenticates regardless of the queue, so the retry only ever runs after a genuine failure and can never turn an authentication failure into unauthenticated plaintext. Validated on a physical 32-bit Android device (Samsung Galaxy A16, armeabi-v7a): 5x full System.Security.Cryptography test suite, 12373 tests, 0 failures, including all ChaCha20Poly1305 EncryptTamperAADDecrypt cases. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 2409f7c commit 02cad94

6 files changed

Lines changed: 77 additions & 49 deletions

File tree

‎src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/AesCcm.Android.cs‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,30 @@ private void DecryptCore(
125125
ReadOnlySpan<byte> tag,
126126
Span<byte> plaintext,
127127
ReadOnlySpan<byte> associatedData)
128+
{
129+
try
130+
{
131+
DecryptCoreOnce(nonce, ciphertext, tag, plaintext, associatedData);
132+
}
133+
catch (CryptographicException ex) when (ex is not AuthenticationTagMismatchException)
134+
{
135+
// Work around a transient Android/Conscrypt issue: an AEAD authentication
136+
// failure can surface with the wrong exception type because the provider
137+
// derives it from BoringSSL's thread-local error queue, which may still hold
138+
// a stale error from an unrelated prior operation on the same thread. The
139+
// failed attempt clears that queue, so an immediate same-thread retry observes
140+
// the true result. A valid ciphertext cannot reach this path, so the retry can
141+
// never turn an authentication failure into unauthenticated plaintext.
142+
DecryptCoreOnce(nonce, ciphertext, tag, plaintext, associatedData);
143+
}
144+
}
145+
146+
private void DecryptCoreOnce(
147+
ReadOnlySpan<byte> nonce,
148+
ReadOnlySpan<byte> ciphertext,
149+
ReadOnlySpan<byte> tag,
150+
Span<byte> plaintext,
151+
ReadOnlySpan<byte> associatedData)
128152
{
129153
bool acquired = false;
130154

‎src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/AesGcm.Android.cs‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,30 @@ private partial void DecryptCore(
114114
ReadOnlySpan<byte> tag,
115115
Span<byte> plaintext,
116116
ReadOnlySpan<byte> associatedData)
117+
{
118+
try
119+
{
120+
DecryptCoreOnce(nonce, ciphertext, tag, plaintext, associatedData);
121+
}
122+
catch (CryptographicException ex) when (ex is not AuthenticationTagMismatchException)
123+
{
124+
// Work around a transient Android/Conscrypt issue: an AEAD authentication
125+
// failure can surface with the wrong exception type because the provider
126+
// derives it from BoringSSL's thread-local error queue, which may still hold
127+
// a stale error from an unrelated prior operation on the same thread. The
128+
// failed attempt clears that queue, so an immediate same-thread retry observes
129+
// the true result. A valid ciphertext cannot reach this path, so the retry can
130+
// never turn an authentication failure into unauthenticated plaintext.
131+
DecryptCoreOnce(nonce, ciphertext, tag, plaintext, associatedData);
132+
}
133+
}
134+
135+
private void DecryptCoreOnce(
136+
ReadOnlySpan<byte> nonce,
137+
ReadOnlySpan<byte> ciphertext,
138+
ReadOnlySpan<byte> tag,
139+
Span<byte> plaintext,
140+
ReadOnlySpan<byte> associatedData)
117141
{
118142
if (!Interop.Crypto.CipherSetTagLength(_ctxHandle, tag.Length))
119143
{

‎src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/ChaCha20Poly1305.Android.cs‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,30 @@ private void DecryptCore(
109109
ReadOnlySpan<byte> tag,
110110
Span<byte> plaintext,
111111
ReadOnlySpan<byte> associatedData)
112+
{
113+
try
114+
{
115+
DecryptCoreOnce(nonce, ciphertext, tag, plaintext, associatedData);
116+
}
117+
catch (CryptographicException ex) when (ex is not AuthenticationTagMismatchException)
118+
{
119+
// Work around a transient Android/Conscrypt issue: an AEAD authentication
120+
// failure can surface with the wrong exception type because the provider
121+
// derives it from BoringSSL's thread-local error queue, which may still hold
122+
// a stale error from an unrelated prior operation on the same thread. The
123+
// failed attempt clears that queue, so an immediate same-thread retry observes
124+
// the true result. A valid ciphertext cannot reach this path, so the retry can
125+
// never turn an authentication failure into unauthenticated plaintext.
126+
DecryptCoreOnce(nonce, ciphertext, tag, plaintext, associatedData);
127+
}
128+
}
129+
130+
private void DecryptCoreOnce(
131+
ReadOnlySpan<byte> nonce,
132+
ReadOnlySpan<byte> ciphertext,
133+
ReadOnlySpan<byte> tag,
134+
Span<byte> plaintext,
135+
ReadOnlySpan<byte> associatedData)
112136
{
113137
Interop.Crypto.EvpCipherSetKeyAndIV(
114138
_ctxHandle,

‎src/native/libs/System.Security.Cryptography.Native.Android/pal_cipher.c‎

Lines changed: 5 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ ARGS_NON_NULL_ALL static jobject GetAlgorithmName(JNIEnv* env, CipherInfo* type)
7676
return make_java_string(env, type->name);
7777
}
7878

79-
ARGS_NON_NULL_ALL static bool CheckJNIExceptionsForAuthTagMismatch(JNIEnv* env, int32_t* authTagMismatch, bool printException, bool decryptMode)
79+
ARGS_NON_NULL_ALL static bool CheckJNIExceptionsForAuthTagMismatch(JNIEnv* env, int32_t* authTagMismatch, bool printException)
8080
{
8181
bool result = false;
8282
INIT_LOCALS(loc, ex);
@@ -85,46 +85,9 @@ ARGS_NON_NULL_ALL static bool CheckJNIExceptionsForAuthTagMismatch(JNIEnv* env,
8585
{
8686
result = true;
8787

88-
if (loc[ex] != NULL)
88+
if (loc[ex] != NULL && (*env)->IsInstanceOf(env, loc[ex], g_AEADBadTagExceptionClass))
8989
{
90-
if ((*env)->IsInstanceOf(env, loc[ex], g_AEADBadTagExceptionClass))
91-
{
92-
*authTagMismatch = 1;
93-
}
94-
else if (decryptMode && (*env)->IsInstanceOf(env, loc[ex], g_GeneralSecurityExceptionClass))
95-
{
96-
// Workaround for an Android/Conscrypt issue. When an AEAD decryption fails
97-
// authentication, the provider determines the thrown exception type by reading
98-
// BoringSSL's thread-local error queue. On some devices that queue can still
99-
// contain a stale error from a prior unrelated operation on the same thread
100-
// (e.g. an RSA error), which is read first (FIFO) and mapped to the wrong
101-
// exception type - we have observed java.security.InvalidKeyException instead of
102-
// javax.crypto.AEADBadTagException. For a successfully initialized AEAD cipher in
103-
// decrypt mode, the key and parameters were already validated at Cipher.init, so
104-
// the only legitimate failure at the tag-update/doFinal step is an authentication
105-
// failure. Treat any GeneralSecurityException here as an authentication tag
106-
// mismatch so the correct AuthenticationTagMismatchException is reported.
107-
*authTagMismatch = 1;
108-
109-
// Reaching this fallback means the provider reported the authentication failure
110-
// with an unexpected exception type. Leave a breadcrumb with the original message
111-
// so that a genuine non-authentication failure, if ever reclassified here, can
112-
// still be diagnosed. This does not run for the common AEADBadTagException case.
113-
jstring message = (jstring)(*env)->CallObjectMethod(env, loc[ex], g_ThrowableGetMessage);
114-
const char* messageChars =
115-
(message != NULL && !(*env)->ExceptionCheck(env)) ? (*env)->GetStringUTFChars(env, message, NULL) : NULL;
116-
LOG_DEBUG("AEAD decrypt reported authentication failure via an unexpected GeneralSecurityException; treating as tag mismatch: %s",
117-
messageChars != NULL ? messageChars : "(no message)");
118-
if (messageChars != NULL)
119-
(*env)->ReleaseStringUTFChars(env, message, messageChars);
120-
if (message != NULL)
121-
(*env)->DeleteLocalRef(env, message);
122-
123-
// getMessage()/GetStringUTFChars() are not expected to throw, but make sure we never
124-
// return to the caller with a pending JNI exception - the caller goes on to raise the
125-
// managed AuthenticationTagMismatchException and must see a clean exception state.
126-
TryClearJNIExceptions(env);
127-
}
90+
*authTagMismatch = 1;
12891
}
12992
}
13093

@@ -334,7 +297,7 @@ int32_t AndroidCryptoNative_AeadCipherUpdate(
334297

335298
*outl = 0;
336299
loc[outDataBytes] = (jbyteArray)(*env)->CallObjectMethod(env, ctx->cipher, g_cipherUpdateMethod, loc[inDataBytes]);
337-
if (CheckJNIExceptionsForAuthTagMismatch(env, authTagMismatch, /* printException: */ true, ctx->encMode == CIPHER_DECRYPT_MODE))
300+
if (CheckJNIExceptionsForAuthTagMismatch(env, authTagMismatch, /* printException: */ true))
338301
{
339302
goto cleanup;
340303
}
@@ -394,7 +357,7 @@ int32_t AndroidCryptoNative_AeadCipherFinalEx(CipherCtx* ctx, uint8_t* outm, int
394357

395358
jbyteArray outBytes = (jbyteArray)(*env)->CallObjectMethod(env, ctx->cipher, g_cipherDoFinalMethod);
396359

397-
if (CheckJNIExceptionsForAuthTagMismatch(env, authTagMismatch, /* printException: */ false, ctx->encMode == CIPHER_DECRYPT_MODE))
360+
if (CheckJNIExceptionsForAuthTagMismatch(env, authTagMismatch, /* printException: */ false))
398361
{
399362
return FAIL;
400363
}

‎src/native/libs/System.Security.Cryptography.Native.Android/pal_jni.c‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -91,9 +91,6 @@ jmethodID g_sslCtxGetDefaultSslParamsMethod;
9191
// javax/crypto/spec/AEADBadTagException
9292
jclass g_AEADBadTagExceptionClass;
9393

94-
// java/security/GeneralSecurityException
95-
jclass g_GeneralSecurityExceptionClass;
96-
9794
// javax/crypto/spec/GCMParameterSpec
9895
jclass g_GCMParameterSpecClass;
9996
jmethodID g_GCMParameterSpecCtor;
@@ -741,7 +738,6 @@ jint AndroidCryptoNative_InitLibraryOnLoad (JavaVM *vm, void *reserved)
741738
g_ivPsCtor = GetMethod(env, false, g_ivPsClass, "<init>", "([B)V");
742739

743740
g_AEADBadTagExceptionClass = GetClassGRef(env, "javax/crypto/AEADBadTagException");
744-
g_GeneralSecurityExceptionClass = GetClassGRef(env, "java/security/GeneralSecurityException");
745741

746742
g_GCMParameterSpecClass = GetClassGRef(env, "javax/crypto/spec/GCMParameterSpec");
747743
g_GCMParameterSpecCtor = GetMethod(env, false, g_GCMParameterSpecClass, "<init>", "(I[B)V");

‎src/native/libs/System.Security.Cryptography.Native.Android/pal_jni.h‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -74,9 +74,6 @@ extern jmethodID g_getBlockSizeMethod;
7474
// javax/crypto/spec/AEADBadTagException
7575
extern jclass g_AEADBadTagExceptionClass;
7676

77-
// java/security/GeneralSecurityException
78-
extern jclass g_GeneralSecurityExceptionClass;
79-
8077
// javax/crypto/spec/IvParameterSpec
8178
extern jclass g_ivPsClass;
8279
extern jmethodID g_ivPsCtor;

0 commit comments

Comments
 (0)