[SDK-165] Fix user-controlled bypass of sensitive method (GSRR-709)

[sumeruchat]

🔒 Security Fix: User-Controlled Bypass of Sensitive Methods (PSEC-636)

The Problem (In Simple Terms)

What was the vulnerability?

Our Android SDK had a security gap where a malicious user could potentially access another user's data. Here's how:

1. The Attack Surface: The SDK stores user credentials (email, userId, authToken) in Android's encrypted keychain
2. The Exploit Window: Between storing credentials and using them for sensitive operations, there was a timing gap
3. The Attack: A sophisticated attacker with a custom app could:
   • Reverse engineer the SDK (it's open source)
   • Modify keychain data between storage and usage
   • Inject another user's credentials
   • Trigger sensitive operations with the wrong user's data

What could an attacker do?

If successful, they could:

• ✉️ Send push notifications to another user's device (registerForPush)
• 📱 Access in-app messages meant for another user (syncInApp)
• 💬 Read embedded messages from another user's account (syncMessages)

Why was this possible?

The code had a classic TOCTOU (Time-Of-Check-Time-Of-Use) vulnerability:

// OLD CODE - VULNERABLE
_email = email;
_authToken = authToken;
storeAuthData();           // ⚠️ Stores to keychain

// ⏰ TIMING GAP - keychain could be tampered with here!

completeUserLogin();       // ⚠️ Reads from instance fields that might have changed
  └─> registerForPush()    // Uses _email, _userId, _authToken
  └─> syncInApp()          // Uses _email, _userId, _authToken  
  └─> syncMessages()       // Uses _email, _userId, _authToken

The Solution

We implemented a completion handler pattern to eliminate the timing gap:

✅ Atomic Credential Flow with Completion Handler

// Step 1: Store with completion handler
private void storeAuthData(AuthDataStorageHandler completionHandler) {
    // Capture credentials BEFORE storing
    final String storedEmail = _email;
    final String storedUserId = _userId;
    final String storedAuthToken = _authToken;
    
    // Store to keychain
    iterableKeychain.saveEmail(_email);
    iterableKeychain.saveUserId(_userId);
    iterableKeychain.saveAuthToken(_authToken);
    
    // Synchronously pass captured credentials to handler
    if (completionHandler != null) {
        completionHandler.onAuthDataStored(storedEmail, storedUserId, storedAuthToken);
    }
}

// Step 2: Use completion handler to pass validated credentials
void setAuthToken(String authToken, boolean bypassAuth) {
    _authToken = authToken;
    
    // Store and immediately use captured credentials
    storeAuthData((email, userId, token) -> completeUserLogin(email, userId, token));
}

// Step 3: Only proceed with server-validated credentials
private void completeUserLogin(@Nullable String email, @Nullable String userId, @Nullable String authToken) {
    // Only enforce authToken requirement when JWT auth is enabled
    if (config.authHandler != null && authToken == null) {
        return;  // No sensitive operations without JWT validation
    }
    
    // Execute sensitive operations with validated data
    registerForPush();
    syncInApp();
    syncMessages();
}

What this prevents:

• ✅ No timing gap: Credentials captured → stored → used in one synchronous flow
• ✅ No re-reading from keychain: Completion handler receives the exact values stored
• ✅ JWT auth protection: When JWT auth is enabled, authToken must be present for sensitive ops
• ✅ Clean data flow: Validated credentials flow directly from capture → sensitive operations

Attack Invalidation

Attack Scenario 1: Keychain Tampering Between Store and Use

• ❌ Before: Attacker modifies keychain after storeAuthData(), completeUserLogin() uses tampered data
• ✅ After: Credentials captured and passed synchronously via completion handler - keychain never re-read

Attack Scenario 2: No AuthToken Validation

• ❌ Before: Could potentially trigger operations without server validation
• ✅ After: When JWT auth enabled (config.authHandler != null), authToken required for sensitive ops

References

• JIRA: GSRR-709
• Related: SDK-165
• CodeQL Alerts: #17, #18, #19

[davidtruong]

That new logic looks good to me.

Can we add some unit tests to ensure that the change works and the data can't be changed mid-flight like it was before?

[Ayyanchira]

Not clear to me what exactly are we mitigating a possible security loophole. Need discussion

[Ayyanchira]

This is not making sense.
_email is passed in completeUserLogin method above.
And that is set back to the same variable here

[sumeruchat]

yeah so the input that is passed is setting the internal state.

[sumeruchat]

@Ayyanchira To further clarify these parameters come from storeAuthData's completion handler which captured them at storage time, ensuring we use exactly what was stored and preventing TOCTOU attacks where
keychain data could be tampered between storage and usage.

[sumeruchat]

I have now added unit tests also and updated the comments to explain the fixes :)