[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

[sumeruchat]

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

[davidtruong]

Don't we want to fail the callback since the setUser didn't actually set the user internally?

[sumeruchat]

@davidtruong makes sense I have updated the code and the tests now.

[davidtruong]

This looks fine. We'll want to update the documentation to provide troubleshooting information if the setUser function fails with the callback.