diff --git a/.jules/sentinel.md b/.jules/sentinel.md new file mode 100644 index 0000000..dcfee25 --- /dev/null +++ b/.jules/sentinel.md @@ -0,0 +1,4 @@ +## 2024-05-27 - Implement CSRF Protection for OAuth +**Vulnerability:** OAuth state parameter used a predictable `System.currentTimeMillis()` value, vulnerable to CSRF attacks. +**Learning:** Always use a cryptographically secure random value for OAuth state parameter and persist it across process boundaries using shared preferences or similar mechanisms to verify during the callback. +**Prevention:** Follow OAuth 2.0 security guidelines and utilize `java.security.SecureRandom` or UUIDs to generate state parameters, ensuring they are verified in the callback process. diff --git a/app/src/main/java/com/example/reddittube/utils/RedditOAuthHelper.kt b/app/src/main/java/com/example/reddittube/utils/RedditOAuthHelper.kt index 6d840b2..a1ebf17 100644 --- a/app/src/main/java/com/example/reddittube/utils/RedditOAuthHelper.kt +++ b/app/src/main/java/com/example/reddittube/utils/RedditOAuthHelper.kt @@ -26,6 +26,9 @@ object RedditOAuthHelper { private const val KEY_USER_TOKEN_EXPIRES_AT = "reddit_user_token_expires_at" private const val KEY_USERNAME = "reddit_username" + // Security keys + private const val KEY_OAUTH_STATE = "reddit_oauth_state" + // Custom API Key / Client ID overrides keys private const val KEY_ENABLE_OVERRIDES = "pref_reddit_enable_overrides" private const val KEY_CUSTOM_CLIENT_ID = "pref_reddit_custom_client_id" @@ -140,10 +143,15 @@ object RedditOAuthHelper { fun launchLogin(context: Context) { val clientId = getClientId(context) val redirectUri = getRedirectUri(context) + val state = UUID.randomUUID().toString() + + val prefs = context.getSharedPreferences(PREFS_NAME, Context.MODE_PRIVATE) + prefs.edit().putString(KEY_OAUTH_STATE, state).apply() + val authUrl = "https://www.reddit.com/api/v1/authorize.compact?" + "client_id=$clientId" + "&response_type=code" + - "&state=rdtube_auth_${System.currentTimeMillis()}" + + "&state=$state" + "&redirect_uri=${Uri.encode(redirectUri)}" + "&duration=permanent" + "&scope=identity,read,mysubreddits,history" @@ -156,7 +164,17 @@ object RedditOAuthHelper { suspend fun handleOAuthCallback(context: Context, uri: Uri): Boolean = withContext(Dispatchers.IO) { val code = uri.getQueryParameter("code") ?: return@withContext false + val state = uri.getQueryParameter("state") + val prefs = context.getSharedPreferences(PREFS_NAME, Context.MODE_PRIVATE) + val savedState = prefs.getString(KEY_OAUTH_STATE, null) + prefs.edit().remove(KEY_OAUTH_STATE).apply() + + if (state == null || state != savedState) { + Log.e("RedditOAuth", "OAuth state mismatch or missing, possible CSRF attack. Expected: $savedState, Got: $state") + return@withContext false + } + val clientId = getClientId(context) val userAgent = getUserAgent(context) val redirectUri = getRedirectUri(context) diff --git a/plan.md b/plan.md new file mode 100644 index 0000000..f4153f8 --- /dev/null +++ b/plan.md @@ -0,0 +1,12 @@ +## Security Fix: Improve OAuth State Parameter Security + +**Vulnerability:** The OAuth state parameter in `launchLogin` (in `RedditOAuthHelper.kt`) uses `System.currentTimeMillis()`: +`"&state=rdtube_auth_${System.currentTimeMillis()}"` +This is predictable and doesn't provide adequate protection against Cross-Site Request Forgery (CSRF) during the OAuth flow. A state parameter should ideally be cryptographically secure and hard to guess, and verified in the callback. + +**Proposed Solution:** +1. Generate a cryptographically secure random string using `java.security.SecureRandom` and `UUID.randomUUID()` or `Base64` encoding. +2. Save this state in `SharedPreferences` before initiating the OAuth flow. +3. In `handleOAuthCallback`, extract the `state` from the callback URI, verify it against the saved state from `SharedPreferences`. If they don't match, abort the login process to prevent CSRF attacks. + +This provides standard CSRF protection in OAuth 2.0 flows.