🛡️ Sentinel: [HIGH] Fix OAuth CSRF Vulnerability

.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.

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)

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.