KEYCLOAK-2082

Cross site scripting issues
keycloak · Nov 26, 2015 · bf4d5f4 · bf4d5f4
1 parent f5326e1
commit bf4d5f4
Show file tree

Hide file tree

Showing 7 changed files with 24 additions and 10 deletions.
diff --git a/forms/common-themes/src/main/resources/theme/base/account/account.ftl b/forms/common-themes/src/main/resources/theme/base/account/account.ftl
@@ -12,7 +12,7 @@
 
     <form action="${url.accountUrl}" class="form-horizontal" method="post">
 
-        <input type="hidden" id="stateChecker" name="stateChecker" value="${stateChecker}">
+        <input type="hidden" id="stateChecker" name="stateChecker" value="${stateChecker?html}">
 
         <div class="form-group ${messagesPerField.printIfExists('username','has-error')}">
             <div class="col-sm-2 col-md-2">

diff --git a/forms/common-themes/src/main/resources/theme/base/account/applications.ftl b/forms/common-themes/src/main/resources/theme/base/account/applications.ftl
@@ -8,8 +8,8 @@
     </div>
 
     <form action="${url.revokeClientUrl}" method="post">
-        <input type="hidden" id="stateChecker" name="stateChecker" value="${stateChecker}">
+        <input type="hidden" id="stateChecker" name="stateChecker" value="${stateChecker?html}">
-        <input type="hidden" id="referrer" name="referrer" value="${stateChecker}">
+        <input type="hidden" id="referrer" name="referrer" value="${stateChecker?html}">
 
         <table class="table table-striped table-bordered">
             <thead>

diff --git a/forms/common-themes/src/main/resources/theme/base/account/password.ftl b/forms/common-themes/src/main/resources/theme/base/account/password.ftl
@@ -23,7 +23,7 @@
             </div>
         </#if>
 
-        <input type="hidden" id="stateChecker" name="stateChecker" value="${stateChecker}">
+        <input type="hidden" id="stateChecker" name="stateChecker" value="${stateChecker?html}">
 
         <div class="form-group">
             <div class="col-sm-2 col-md-2">

diff --git a/forms/common-themes/src/main/resources/theme/base/account/totp.ftl b/forms/common-themes/src/main/resources/theme/base/account/totp.ftl
@@ -41,7 +41,7 @@
         <hr/>
 
         <form action="${url.totpUrl}" class="form-horizontal" method="post">
-            <input type="hidden" id="stateChecker" name="stateChecker" value="${stateChecker}">
+            <input type="hidden" id="stateChecker" name="stateChecker" value="${stateChecker?html}">
             <div class="form-group">
                 <div class="col-sm-2 col-md-2">
                     <label for="totp" class="control-label">${msg("authenticatorCode")}</label>

diff --git a/model/api/src/main/java/org/keycloak/models/utils/KeycloakModelUtils.java b/model/api/src/main/java/org/keycloak/models/utils/KeycloakModelUtils.java
@@ -1,6 +1,7 @@
 package org.keycloak.models.utils;
 
 import org.bouncycastle.openssl.PEMWriter;
+import org.keycloak.common.util.Base64;
 import org.keycloak.common.util.Base64Url;
 import org.keycloak.models.AuthenticationExecutionModel;
 import org.keycloak.models.AuthenticationFlowModel;
@@ -57,6 +58,16 @@ public static String generateId() {
         return UUID.randomUUID().toString();
     }
 
+    public static String generateSecret() {
+        return generateSecret(32);
+    }
+
+    public static String generateSecret(int bytes) {
+        byte[] buf = new byte[bytes];
+        new SecureRandom().nextBytes(buf);
+        return Base64Url.encode(buf);
+    }
+
     public static PublicKey getPublicKey(String publicKeyPem) {
         if (publicKeyPem != null) {
             try {

diff --git a/services/src/main/java/org/keycloak/services/resources/AbstractSecuredLocalService.java b/services/src/main/java/org/keycloak/services/resources/AbstractSecuredLocalService.java
@@ -6,9 +6,11 @@
 import org.keycloak.AbstractOAuthClient;
 import org.keycloak.common.ClientConnection;
 import org.keycloak.OAuth2Constants;
+import org.keycloak.common.util.Base64Url;
 import org.keycloak.models.ClientModel;
 import org.keycloak.models.KeycloakSession;
 import org.keycloak.models.RealmModel;
+import org.keycloak.models.utils.KeycloakModelUtils;
 import org.keycloak.protocol.oidc.OIDCLoginProtocolService;
 import org.keycloak.services.ForbiddenException;
 import org.keycloak.services.managers.AppAuthManager;
@@ -40,6 +42,9 @@
  */
 public abstract class AbstractSecuredLocalService {
     private static final Logger logger = Logger.getLogger(AbstractSecuredLocalService.class);
+
+    private static final String KEYCLOAK_STATE_CHECKER = "KEYCLOAK_STATE_CHECKER";
+
     protected final ClientModel client;
     protected RealmModel realm;
 
@@ -106,14 +111,14 @@ public Response loginRedirect(@QueryParam("code") String code,
     }
 
     protected void updateCsrfChecks() {
-        Cookie cookie = headers.getCookies().get(AccountService.KEYCLOAK_STATE_CHECKER);
+        Cookie cookie = headers.getCookies().get(KEYCLOAK_STATE_CHECKER);
         if (cookie != null) {
             stateChecker = cookie.getValue();
         } else {
-            stateChecker = UUID.randomUUID().toString();
+            stateChecker = KeycloakModelUtils.generateSecret();
             String cookiePath = AuthenticationManager.getRealmCookiePath(realm, uriInfo);
             boolean secureOnly = realm.getSslRequired().isRequired(clientConnection);
-            CookieHelper.addCookie(AccountService.KEYCLOAK_STATE_CHECKER, stateChecker, cookiePath, null, null, -1, secureOnly, true);
+            CookieHelper.addCookie(KEYCLOAK_STATE_CHECKER, stateChecker, cookiePath, null, null, -1, secureOnly, true);
         }
     }
 

diff --git a/services/src/main/java/org/keycloak/services/resources/AccountService.java b/services/src/main/java/org/keycloak/services/resources/AccountService.java
@@ -117,8 +117,6 @@ public class AccountService extends AbstractSecuredLocalService {
         LOG_DETAILS.add(Details.AUTH_METHOD);
     }
 
-    public static final String KEYCLOAK_STATE_CHECKER = "KEYCLOAK_STATE_CHECKER";
-
     // Used when some other context (ie. IdentityBrokerService) wants to forward error to account management and display it here
     public static final String ACCOUNT_MGMT_FORWARDED_ERROR_NOTE = "ACCOUNT_MGMT_FORWARDED_ERROR";