-
Notifications
You must be signed in to change notification settings - Fork 33
[SDK-165] Fix user-controlled bypass of sensitive method (GSRR-709) #957
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
base: master
Are you sure you want to change the base?
Changes from all commits
8e465e2
c443dcf
a5c0943
d169e0e
cba59cc
3e2e4ee
9d0ac90
7bdf8df
fcb9414
1602ae2
bac5d76
1821459
299152f
d1c933f
b102606
c84e94c
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 |
|---|---|---|
|
|
@@ -420,10 +420,42 @@ private void onLogin( | |
| } | ||
|
|
||
| private void completeUserLogin() { | ||
| completeUserLogin(_email, _userId, _authToken); | ||
| } | ||
|
|
||
| /** | ||
| * Completes user login with validated credentials. | ||
| * This method ensures sensitive operations (syncInApp, syncMessages, registerForPush) only execute | ||
| * with server-validated user data, preventing user-controlled bypass attacks. | ||
| * | ||
| * @param email Server-validated email (can be null) | ||
| * @param userId Server-validated userId (can be null) | ||
| * @param authToken Server-validated authToken (must not be null for sensitive operations when JWT auth is enabled) | ||
| */ | ||
| private void completeUserLogin(@Nullable String email, @Nullable String userId, @Nullable String authToken) { | ||
| if (!isInitialized()) { | ||
| return; | ||
| } | ||
|
|
||
| // Only enforce authToken requirement when JWT auth is enabled | ||
| // This prevents user-controlled bypass where unvalidated userId/email from keychain | ||
| // could be used to access another user's data in JWT auth scenarios | ||
| if (config.authHandler != null && authToken == null) { | ||
| IterableLogger.d(TAG, "Skipping sensitive operations - JWT auth enabled but no validated authToken present"); | ||
| if (_setUserSuccessCallbackHandler != null) { | ||
| _setUserSuccessCallbackHandler.onSuccess(new JSONObject()); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| // SECURITY: Update instance fields with validated credentials passed via completion handler. | ||
| // 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. | ||
| _email = email; | ||
| _userId = userId; | ||
| _authToken = authToken; | ||
|
Comment on lines
+455
to
+457
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 not making sense.
Collaborator
Author
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. yeah so the input that is passed is setting the internal state.
Collaborator
Author
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. @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 |
||
|
|
||
| if (config.autoPushRegistration) { | ||
| registerForPush(); | ||
| } else if (_setUserSuccessCallbackHandler != null) { | ||
|
|
@@ -502,10 +534,45 @@ private String getDeviceId() { | |
| return _deviceId; | ||
| } | ||
|
|
||
| /** | ||
| * Completion handler interface for storeAuthData operations. | ||
| * Receives the exact credentials that were stored to keychain. | ||
| */ | ||
| private interface AuthDataStorageHandler { | ||
| void onAuthDataStored(String email, String userId, String authToken); | ||
| } | ||
|
|
||
| private void storeAuthData() { | ||
| storeAuthData(null); | ||
| } | ||
|
|
||
| /** | ||
| * Stores auth data and optionally invokes completion handler with the stored credentials. | ||
| * | ||
| * SECURITY - TOCTOU Protection: | ||
| * When a completion handler is provided, it receives the exact credentials that were stored | ||
| * to keychain. This prevents TOCTOU (Time-Of-Check-Time-Of-Use) attacks where: | ||
| * 1. Credentials are stored to keychain | ||
| * 2. Attacker modifies keychain (between store and read) | ||
| * 3. Sensitive operations use tampered credentials | ||
| * | ||
| * By capturing credentials BEFORE storage and passing them directly via completion handler, | ||
| * we ensure completeUserLogin uses exactly what was stored, not what's currently in keychain. | ||
| * | ||
| * @param completionHandler Optional handler invoked synchronously after storage with the stored credentials | ||
| */ | ||
| private void storeAuthData(AuthDataStorageHandler completionHandler) { | ||
| if (_applicationContext == null) { | ||
| return; | ||
| } | ||
|
|
||
| // SECURITY: Capture current instance field values BEFORE storing to keychain. | ||
| // These captured values will be passed to completion handler, ensuring the caller | ||
| // receives exactly what was stored, not potentially modified keychain data. | ||
| final String storedEmail = _email; | ||
| final String storedUserId = _userId; | ||
| final String storedAuthToken = _authToken; | ||
|
|
||
| IterableKeychain iterableKeychain = getKeychain(); | ||
| if (iterableKeychain != null) { | ||
| iterableKeychain.saveEmail(_email); | ||
|
|
@@ -515,6 +582,11 @@ private void storeAuthData() { | |
| } else { | ||
| IterableLogger.e(TAG, "Shared preference creation failed. "); | ||
| } | ||
|
|
||
| // Invoke completion handler with the captured credentials | ||
| if (completionHandler != null) { | ||
| completionHandler.onAuthDataStored(storedEmail, storedUserId, storedAuthToken); | ||
| } | ||
| } | ||
|
|
||
| private void retrieveEmailAndUserId() { | ||
|
|
@@ -595,10 +667,14 @@ void setAuthToken(String authToken, boolean bypassAuth) { | |
| if (isInitialized()) { | ||
| if ((authToken != null && !authToken.equalsIgnoreCase(_authToken)) || (_authToken != null && !_authToken.equalsIgnoreCase(authToken))) { | ||
| _authToken = authToken; | ||
| storeAuthData(); | ||
| completeUserLogin(); | ||
| // SECURITY: Use completion handler to atomically store and pass validated credentials. | ||
| // The completion handler receives exact values stored to keychain, preventing TOCTOU | ||
| // attacks where keychain could be modified between storage and completeUserLogin execution. | ||
| storeAuthData((email, userId, token) -> completeUserLogin(email, userId, token)); | ||
| } else if (bypassAuth) { | ||
| completeUserLogin(); | ||
| // SECURITY: Pass current credentials directly to completeUserLogin. | ||
| // completeUserLogin will validate authToken presence when JWT auth is enabled. | ||
| completeUserLogin(_email, _userId, _authToken); | ||
| } | ||
| } | ||
| } | ||
|
|
||
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.
Don't we want to fail the callback since the setUser didn't actually set the user internally?