From 92fa70c63546d717cc5fc15a403b6dc4263afc56 Mon Sep 17 00:00:00 2001 From: Martin Ortbauer Date: Thu, 2 Dec 2021 22:38:02 +0100 Subject: [PATCH 1/3] add the state parameter to the oidc authentication request * added state paramter to make resilient against CSRF attacks * see issue #3465 for more information --- .../authentication/oauth/OAuthUtils.kt | 3 ++ .../ui/authentication/LoginActivity.kt | 31 +++++++++++++------ .../authentication/AuthenticationConstants.kt | 1 + .../exceptions/StateMismatchException.kt | 24 ++++++++++++++ 4 files changed, 49 insertions(+), 10 deletions(-) create mode 100644 owncloudDomain/src/main/java/com/owncloud/android/domain/exceptions/StateMismatchException.kt diff --git a/owncloudApp/src/main/java/com/owncloud/android/authentication/oauth/OAuthUtils.kt b/owncloudApp/src/main/java/com/owncloud/android/authentication/oauth/OAuthUtils.kt index 9df8365b927..783de262f9f 100644 --- a/owncloudApp/src/main/java/com/owncloud/android/authentication/oauth/OAuthUtils.kt +++ b/owncloudApp/src/main/java/com/owncloud/android/authentication/oauth/OAuthUtils.kt @@ -30,6 +30,7 @@ import com.owncloud.android.data.authentication.QUERY_PARAMETER_CODE_CHALLENGE_M import com.owncloud.android.data.authentication.QUERY_PARAMETER_REDIRECT_URI import com.owncloud.android.data.authentication.QUERY_PARAMETER_RESPONSE_TYPE import com.owncloud.android.data.authentication.QUERY_PARAMETER_SCOPE +import com.owncloud.android.data.authentication.QUERY_PARAMETER_STATE import com.owncloud.android.domain.authentication.oauth.model.ClientRegistrationRequest import java.net.URLEncoder import java.security.MessageDigest @@ -88,6 +89,7 @@ class OAuthUtils { responseType: String, scope: String, codeChallenge: String, + state: String, ): Uri = authorizationEndpoint.buildUpon() .appendQueryParameter(QUERY_PARAMETER_REDIRECT_URI, redirectUri) @@ -96,6 +98,7 @@ class OAuthUtils { .appendQueryParameter(QUERY_PARAMETER_SCOPE, scope) .appendQueryParameter(QUERY_PARAMETER_CODE_CHALLENGE, codeChallenge) .appendQueryParameter(QUERY_PARAMETER_CODE_CHALLENGE_METHOD, CODE_CHALLENGE_METHOD) + .appendQueryParameter(QUERY_PARAMETER_STATE, state) .build() fun buildRedirectUri(context: Context): Uri = diff --git a/owncloudApp/src/main/java/com/owncloud/android/presentation/ui/authentication/LoginActivity.kt b/owncloudApp/src/main/java/com/owncloud/android/presentation/ui/authentication/LoginActivity.kt index b228606ebfa..1dce164f66e 100644 --- a/owncloudApp/src/main/java/com/owncloud/android/presentation/ui/authentication/LoginActivity.kt +++ b/owncloudApp/src/main/java/com/owncloud/android/presentation/ui/authentication/LoginActivity.kt @@ -52,6 +52,7 @@ import com.owncloud.android.domain.authentication.oauth.model.TokenRequest import com.owncloud.android.domain.exceptions.NoNetworkConnectionException import com.owncloud.android.domain.exceptions.OwncloudVersionNotSupportedException import com.owncloud.android.domain.exceptions.ServerNotReachableException +import com.owncloud.android.domain.exceptions.StateMismatchException import com.owncloud.android.domain.exceptions.UnauthorizedException import com.owncloud.android.domain.server.model.AuthenticationMethod import com.owncloud.android.domain.server.model.ServerInfo @@ -77,6 +78,7 @@ import org.koin.android.ext.android.inject import org.koin.androidx.viewmodel.ext.android.viewModel import timber.log.Timber import java.io.File +import java.util.UUID class LoginActivity : AppCompatActivity(), SslUntrustedCertDialog.OnSslUntrustedCertListener, ISecurityEnforced { @@ -90,6 +92,7 @@ class LoginActivity : AppCompatActivity(), SslUntrustedCertDialog.OnSslUntrusted private lateinit var serverBaseUrl: String private var oidcSupported = false + private var oidcState: String = "" private lateinit var binding: AccountSetupBinding @@ -445,13 +448,15 @@ class LoginActivity : AppCompatActivity(), SslUntrustedCertDialog.OnSslUntrusted val customTabsBuilder: CustomTabsIntent.Builder = CustomTabsIntent.Builder() val customTabsIntent: CustomTabsIntent = customTabsBuilder.build() + this.oidcState = UUID.randomUUID().toString().substring(0,15) val authorizationEndpointUri = OAuthUtils.buildAuthorizationRequest( authorizationEndpoint = authorizationEndpoint, redirectUri = OAuthUtils.buildRedirectUri(applicationContext).toString(), clientId = clientId, responseType = ResponseType.CODE.string, scope = if (oidcSupported) OAUTH2_OIDC_SCOPE else "", - codeChallenge = oauthViewModel.codeChallenge + codeChallenge = oauthViewModel.codeChallenge, + state = this.oidcState ) customTabsIntent.launchUrl( @@ -469,18 +474,24 @@ class LoginActivity : AppCompatActivity(), SslUntrustedCertDialog.OnSslUntrusted private fun handleGetAuthorizationCodeResponse(intent: Intent) { val authorizationCode = intent.data?.getQueryParameter("code") + val state = intent.data?.getQueryParameter("state") - if (authorizationCode != null) { - Timber.d("Authorization code received [$authorizationCode]. Let's exchange it for access token") - exchangeAuthorizationCodeForTokens(authorizationCode) + if (state != this.oidcState){ + Timber.e("OAuth request to get authorization code failed. State mismatching, maybe somebody is trying a CSRF attack.") + updateOAuthStatusIconAndText(StateMismatchException()) } else { - val authorizationError = intent.data?.getQueryParameter("error") - val authorizationErrorDescription = intent.data?.getQueryParameter("error_description") + if (authorizationCode != null) { + Timber.d("Authorization code received [$authorizationCode]. Let's exchange it for access token") + exchangeAuthorizationCodeForTokens(authorizationCode) + } else { + val authorizationError = intent.data?.getQueryParameter("error") + val authorizationErrorDescription = intent.data?.getQueryParameter("error_description") - Timber.e("OAuth request to get authorization code failed. Error: [$authorizationError]. Error description: [$authorizationErrorDescription]") - val authorizationException = - if (authorizationError == "access_denied") UnauthorizedException() else Throwable() - updateOAuthStatusIconAndText(authorizationException) + Timber.e("OAuth request to get authorization code failed. Error: [$authorizationError]. Error description: [$authorizationErrorDescription]") + val authorizationException = + if (authorizationError == "access_denied") UnauthorizedException() else Throwable() + updateOAuthStatusIconAndText(authorizationException) + } } } diff --git a/owncloudData/src/main/java/com/owncloud/android/data/authentication/AuthenticationConstants.kt b/owncloudData/src/main/java/com/owncloud/android/data/authentication/AuthenticationConstants.kt index de043b1365a..0090a6edef9 100644 --- a/owncloudData/src/main/java/com/owncloud/android/data/authentication/AuthenticationConstants.kt +++ b/owncloudData/src/main/java/com/owncloud/android/data/authentication/AuthenticationConstants.kt @@ -51,3 +51,4 @@ const val QUERY_PARAMETER_RESPONSE_TYPE = "response_type" const val QUERY_PARAMETER_SCOPE = "scope" const val QUERY_PARAMETER_CODE_CHALLENGE = "code_challenge" const val QUERY_PARAMETER_CODE_CHALLENGE_METHOD = "code_challenge_method" +const val QUERY_PARAMETER_STATE = "state" diff --git a/owncloudDomain/src/main/java/com/owncloud/android/domain/exceptions/StateMismatchException.kt b/owncloudDomain/src/main/java/com/owncloud/android/domain/exceptions/StateMismatchException.kt new file mode 100644 index 00000000000..5df5967bee1 --- /dev/null +++ b/owncloudDomain/src/main/java/com/owncloud/android/domain/exceptions/StateMismatchException.kt @@ -0,0 +1,24 @@ +/** + * ownCloud Android client application + * + * @author David González Verdugo + * Copyright (C) 2020 ownCloud GmbH. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2, + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ + +package com.owncloud.android.domain.exceptions + +import java.lang.Exception + +class StateMismatchException : Exception() From 576c1033bdc576c40d0a21b4dec069fae5fb61df Mon Sep 17 00:00:00 2001 From: Martin Ortbauer Date: Fri, 3 Dec 2021 15:59:40 +0100 Subject: [PATCH 2/3] use SecureRandom instead of UUID --- .../owncloud/android/authentication/oauth/OAuthUtils.kt | 9 +++++++++ .../presentation/ui/authentication/LoginActivity.kt | 3 +-- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/owncloudApp/src/main/java/com/owncloud/android/authentication/oauth/OAuthUtils.kt b/owncloudApp/src/main/java/com/owncloud/android/authentication/oauth/OAuthUtils.kt index 783de262f9f..9366d0ceb49 100644 --- a/owncloudApp/src/main/java/com/owncloud/android/authentication/oauth/OAuthUtils.kt +++ b/owncloudApp/src/main/java/com/owncloud/android/authentication/oauth/OAuthUtils.kt @@ -38,6 +38,14 @@ import java.security.SecureRandom class OAuthUtils { + fun generateRandomState(): String { + val secureRandom = SecureRandom() + val randomBytes = ByteArray(DEFAULT_STATE_ENTROPY) + secureRandom.nextBytes(randomBytes) + val encoding = Base64.NO_WRAP or Base64.NO_PADDING or Base64.URL_SAFE + return Base64.encodeToString(randomBytes, encoding) + } + fun generateRandomCodeVerifier(): String { val secureRandom = SecureRandom() val randomBytes = ByteArray(DEFAULT_CODE_VERIFIER_ENTROPY) @@ -59,6 +67,7 @@ class OAuthUtils { private const val ALGORITHM_SHA_256 = "SHA-256" private const val CODE_CHALLENGE_METHOD = "S256" private const val DEFAULT_CODE_VERIFIER_ENTROPY = 64 + private const val DEFAULT_STATE_ENTROPY = 15 fun buildClientRegistrationRequest( registrationEndpoint: String, diff --git a/owncloudApp/src/main/java/com/owncloud/android/presentation/ui/authentication/LoginActivity.kt b/owncloudApp/src/main/java/com/owncloud/android/presentation/ui/authentication/LoginActivity.kt index 1dce164f66e..10dc2050ea2 100644 --- a/owncloudApp/src/main/java/com/owncloud/android/presentation/ui/authentication/LoginActivity.kt +++ b/owncloudApp/src/main/java/com/owncloud/android/presentation/ui/authentication/LoginActivity.kt @@ -78,7 +78,6 @@ import org.koin.android.ext.android.inject import org.koin.androidx.viewmodel.ext.android.viewModel import timber.log.Timber import java.io.File -import java.util.UUID class LoginActivity : AppCompatActivity(), SslUntrustedCertDialog.OnSslUntrustedCertListener, ISecurityEnforced { @@ -448,7 +447,7 @@ class LoginActivity : AppCompatActivity(), SslUntrustedCertDialog.OnSslUntrusted val customTabsBuilder: CustomTabsIntent.Builder = CustomTabsIntent.Builder() val customTabsIntent: CustomTabsIntent = customTabsBuilder.build() - this.oidcState = UUID.randomUUID().toString().substring(0,15) + this.oidcState = OAuthUtils().generateRandomState() val authorizationEndpointUri = OAuthUtils.buildAuthorizationRequest( authorizationEndpoint = authorizationEndpoint, redirectUri = OAuthUtils.buildRedirectUri(applicationContext).toString(), From b82644404ffd00e8e61060f18ed83ca52282f254 Mon Sep 17 00:00:00 2001 From: Martin Ortbauer Date: Wed, 15 Dec 2021 21:09:15 +0100 Subject: [PATCH 3/3] add the oidcState parameter to OAuthViewModel like the code_challenge --- .../android/presentation/ui/authentication/LoginActivity.kt | 6 ++---- .../android/presentation/viewmodels/oauth/OAuthViewModel.kt | 1 + 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/owncloudApp/src/main/java/com/owncloud/android/presentation/ui/authentication/LoginActivity.kt b/owncloudApp/src/main/java/com/owncloud/android/presentation/ui/authentication/LoginActivity.kt index 10dc2050ea2..4d8856fe33d 100644 --- a/owncloudApp/src/main/java/com/owncloud/android/presentation/ui/authentication/LoginActivity.kt +++ b/owncloudApp/src/main/java/com/owncloud/android/presentation/ui/authentication/LoginActivity.kt @@ -91,7 +91,6 @@ class LoginActivity : AppCompatActivity(), SslUntrustedCertDialog.OnSslUntrusted private lateinit var serverBaseUrl: String private var oidcSupported = false - private var oidcState: String = "" private lateinit var binding: AccountSetupBinding @@ -447,7 +446,6 @@ class LoginActivity : AppCompatActivity(), SslUntrustedCertDialog.OnSslUntrusted val customTabsBuilder: CustomTabsIntent.Builder = CustomTabsIntent.Builder() val customTabsIntent: CustomTabsIntent = customTabsBuilder.build() - this.oidcState = OAuthUtils().generateRandomState() val authorizationEndpointUri = OAuthUtils.buildAuthorizationRequest( authorizationEndpoint = authorizationEndpoint, redirectUri = OAuthUtils.buildRedirectUri(applicationContext).toString(), @@ -455,7 +453,7 @@ class LoginActivity : AppCompatActivity(), SslUntrustedCertDialog.OnSslUntrusted responseType = ResponseType.CODE.string, scope = if (oidcSupported) OAUTH2_OIDC_SCOPE else "", codeChallenge = oauthViewModel.codeChallenge, - state = this.oidcState + state = oauthViewModel.oidcState ) customTabsIntent.launchUrl( @@ -475,7 +473,7 @@ class LoginActivity : AppCompatActivity(), SslUntrustedCertDialog.OnSslUntrusted val authorizationCode = intent.data?.getQueryParameter("code") val state = intent.data?.getQueryParameter("state") - if (state != this.oidcState){ + if (state != oauthViewModel.oidcState){ Timber.e("OAuth request to get authorization code failed. State mismatching, maybe somebody is trying a CSRF attack.") updateOAuthStatusIconAndText(StateMismatchException()) } else { diff --git a/owncloudApp/src/main/java/com/owncloud/android/presentation/viewmodels/oauth/OAuthViewModel.kt b/owncloudApp/src/main/java/com/owncloud/android/presentation/viewmodels/oauth/OAuthViewModel.kt index 601df3ce344..f025f25c9d0 100644 --- a/owncloudApp/src/main/java/com/owncloud/android/presentation/viewmodels/oauth/OAuthViewModel.kt +++ b/owncloudApp/src/main/java/com/owncloud/android/presentation/viewmodels/oauth/OAuthViewModel.kt @@ -45,6 +45,7 @@ class OAuthViewModel( val codeVerifier: String = OAuthUtils().generateRandomCodeVerifier() val codeChallenge: String = OAuthUtils().generateCodeChallenge(codeVerifier) + val oidcState: String = OAuthUtils().generateRandomState() private val _oidcDiscovery = MediatorLiveData>>() val oidcDiscovery: LiveData>> = _oidcDiscovery