From 08090339249dfd9f8c7dc0f1f06ab818ddc6415b Mon Sep 17 00:00:00 2001 From: Johannes Knutsen Date: Wed, 26 Apr 2017 15:53:25 +0200 Subject: [PATCH] KEYCLOAK-4780 Ensure Base64 encoded HMAC secret key is decoded before use --- .../org/keycloak/common/util/KeyUtils.java | 4 ++-- .../keycloak/common/util/KeyUtilsTest.java | 24 +++++++++++++++++++ .../models/utils/KeycloakModelUtils.java | 7 +++--- .../services/managers/ClientSessionCode.java | 3 ++- .../keys/GeneratedHmacKeyProvider.java | 3 ++- .../keys/GeneratedHmacKeyProviderFactory.java | 4 ++-- .../AbstractSecuredLocalService.java | 3 ++- .../services/resources/WelcomeResource.java | 3 ++- 8 files changed, 39 insertions(+), 12 deletions(-) create mode 100644 common/src/test/java/org/keycloak/common/util/KeyUtilsTest.java diff --git a/common/src/main/java/org/keycloak/common/util/KeyUtils.java b/common/src/main/java/org/keycloak/common/util/KeyUtils.java index 4514e4f8f7..37e2b2a319 100644 --- a/common/src/main/java/org/keycloak/common/util/KeyUtils.java +++ b/common/src/main/java/org/keycloak/common/util/KeyUtils.java @@ -40,8 +40,8 @@ public class KeyUtils { private KeyUtils() { } - public static SecretKey loadSecretKey(String secret) { - return new SecretKeySpec(secret.getBytes(), "HmacSHA256"); + public static SecretKey loadSecretKey(byte[] secret) { + return new SecretKeySpec(secret, "HmacSHA256"); } public static KeyPair generateRsaKeyPair(int keysize) { diff --git a/common/src/test/java/org/keycloak/common/util/KeyUtilsTest.java b/common/src/test/java/org/keycloak/common/util/KeyUtilsTest.java new file mode 100644 index 0000000000..5e0abf560f --- /dev/null +++ b/common/src/test/java/org/keycloak/common/util/KeyUtilsTest.java @@ -0,0 +1,24 @@ +package org.keycloak.common.util; + +import org.junit.Test; + +import javax.crypto.SecretKey; +import javax.crypto.spec.SecretKeySpec; +import java.util.concurrent.ThreadLocalRandom; + +import static org.junit.Assert.assertArrayEquals; +import static org.junit.Assert.assertEquals; + +public class KeyUtilsTest { + + @Test + public void loadSecretKey() throws Exception { + byte[] secretBytes = new byte[32]; + ThreadLocalRandom.current().nextBytes(secretBytes); + SecretKeySpec expected = new SecretKeySpec(secretBytes, "HmacSHA256"); + SecretKey actual = KeyUtils.loadSecretKey(secretBytes); + assertEquals(expected.getAlgorithm(), actual.getAlgorithm()); + assertArrayEquals(expected.getEncoded(), actual.getEncoded()); + } + +} \ No newline at end of file diff --git a/server-spi-private/src/main/java/org/keycloak/models/utils/KeycloakModelUtils.java b/server-spi-private/src/main/java/org/keycloak/models/utils/KeycloakModelUtils.java index 695fe38c1b..b454460325 100755 --- a/server-spi-private/src/main/java/org/keycloak/models/utils/KeycloakModelUtils.java +++ b/server-spi-private/src/main/java/org/keycloak/models/utils/KeycloakModelUtils.java @@ -19,7 +19,6 @@ package org.keycloak.models.utils; import org.keycloak.broker.social.SocialIdentityProvider; import org.keycloak.broker.social.SocialIdentityProviderFactory; -import org.keycloak.common.util.Base64Url; import org.keycloak.common.util.CertificateUtils; import org.keycloak.common.util.KeyUtils; import org.keycloak.common.util.PemUtils; @@ -75,14 +74,14 @@ public final class KeycloakModelUtils { return UUID.randomUUID().toString(); } - public static String generateSecret() { + public static byte[] generateSecret() { return generateSecret(32); } - public static String generateSecret(int bytes) { + public static byte[] generateSecret(int bytes) { byte[] buf = new byte[bytes]; new SecureRandom().nextBytes(buf); - return Base64Url.encode(buf); + return buf; } public static PublicKey getPublicKey(String publicKeyPem) { diff --git a/server-spi-private/src/main/java/org/keycloak/services/managers/ClientSessionCode.java b/server-spi-private/src/main/java/org/keycloak/services/managers/ClientSessionCode.java index f98f8fe837..3d536ddf05 100755 --- a/server-spi-private/src/main/java/org/keycloak/services/managers/ClientSessionCode.java +++ b/server-spi-private/src/main/java/org/keycloak/services/managers/ClientSessionCode.java @@ -18,6 +18,7 @@ package org.keycloak.services.managers; import org.jboss.logging.Logger; +import org.keycloak.common.util.Base64Url; import org.keycloak.common.util.Time; import org.keycloak.models.ClientModel; import org.keycloak.models.ClientSessionModel; @@ -227,7 +228,7 @@ public class ClientSessionCode { private static String generateCode(ClientSessionModel clientSession) { try { - String actionId = KeycloakModelUtils.generateSecret(); + String actionId = Base64Url.encode(KeycloakModelUtils.generateSecret()); StringBuilder sb = new StringBuilder(); sb.append(actionId); diff --git a/services/src/main/java/org/keycloak/keys/GeneratedHmacKeyProvider.java b/services/src/main/java/org/keycloak/keys/GeneratedHmacKeyProvider.java index cbd9035436..a989ac3a11 100644 --- a/services/src/main/java/org/keycloak/keys/GeneratedHmacKeyProvider.java +++ b/services/src/main/java/org/keycloak/keys/GeneratedHmacKeyProvider.java @@ -17,6 +17,7 @@ package org.keycloak.keys; +import org.keycloak.common.util.Base64Url; import org.keycloak.common.util.KeyUtils; import org.keycloak.component.ComponentModel; import org.keycloak.jose.jws.AlgorithmType; @@ -47,7 +48,7 @@ public class GeneratedHmacKeyProvider implements HmacKeyProvider { if (model.hasNote(SecretKey.class.getName())) { secretKey = model.getNote(SecretKey.class.getName()); } else { - secretKey = KeyUtils.loadSecretKey(model.get(Attributes.SECRET_KEY)); + secretKey = KeyUtils.loadSecretKey(Base64Url.decode(model.get(Attributes.SECRET_KEY))); model.setNote(SecretKey.class.getName(), secretKey); } } diff --git a/services/src/main/java/org/keycloak/keys/GeneratedHmacKeyProviderFactory.java b/services/src/main/java/org/keycloak/keys/GeneratedHmacKeyProviderFactory.java index 7207eab05b..3a725170bf 100644 --- a/services/src/main/java/org/keycloak/keys/GeneratedHmacKeyProviderFactory.java +++ b/services/src/main/java/org/keycloak/keys/GeneratedHmacKeyProviderFactory.java @@ -81,8 +81,8 @@ public class GeneratedHmacKeyProviderFactory extends AbstractHmacKeyProviderFact private void generateSecret(ComponentModel model, int size) { try { - String secret = KeycloakModelUtils.generateSecret(size); - model.put(Attributes.SECRET_KEY, secret); + byte[] secret = KeycloakModelUtils.generateSecret(size); + model.put(Attributes.SECRET_KEY, Base64Url.encode(secret)); String kid = KeycloakModelUtils.generateId(); model.put(Attributes.KID_KEY, kid); diff --git a/services/src/main/java/org/keycloak/services/resources/AbstractSecuredLocalService.java b/services/src/main/java/org/keycloak/services/resources/AbstractSecuredLocalService.java index f5a1b270c5..cc8abfb685 100755 --- a/services/src/main/java/org/keycloak/services/resources/AbstractSecuredLocalService.java +++ b/services/src/main/java/org/keycloak/services/resources/AbstractSecuredLocalService.java @@ -22,6 +22,7 @@ import org.jboss.resteasy.spi.HttpRequest; import org.keycloak.AbstractOAuthClient; import org.keycloak.OAuth2Constants; import org.keycloak.common.ClientConnection; +import org.keycloak.common.util.Base64Url; import org.keycloak.common.util.KeycloakUriBuilder; import org.keycloak.common.util.UriUtils; import org.keycloak.models.ClientModel; @@ -133,7 +134,7 @@ public abstract class AbstractSecuredLocalService { if (cookie != null) { stateChecker = cookie.getValue(); } else { - stateChecker = KeycloakModelUtils.generateSecret(); + stateChecker = Base64Url.encode(KeycloakModelUtils.generateSecret()); String cookiePath = AuthenticationManager.getRealmCookiePath(realm, uriInfo); boolean secureOnly = realm.getSslRequired().isRequired(clientConnection); CookieHelper.addCookie(KEYCLOAK_STATE_CHECKER, stateChecker, cookiePath, null, null, -1, secureOnly, true); diff --git a/services/src/main/java/org/keycloak/services/resources/WelcomeResource.java b/services/src/main/java/org/keycloak/services/resources/WelcomeResource.java index 48d2650618..87106145cf 100755 --- a/services/src/main/java/org/keycloak/services/resources/WelcomeResource.java +++ b/services/src/main/java/org/keycloak/services/resources/WelcomeResource.java @@ -19,6 +19,7 @@ package org.keycloak.services.resources; import org.jboss.logging.Logger; import org.keycloak.Config; import org.keycloak.common.ClientConnection; +import org.keycloak.common.util.Base64Url; import org.keycloak.common.util.MimeTypeUtil; import org.keycloak.models.BrowserSecurityHeaders; import org.keycloak.models.KeycloakSession; @@ -246,7 +247,7 @@ public class WelcomeResource { if (stateChecker != null) { return stateChecker; } else { - stateChecker = KeycloakModelUtils.generateSecret(); + stateChecker = Base64Url.encode(KeycloakModelUtils.generateSecret()); String cookiePath = uriInfo.getPath(); boolean secureOnly = uriInfo.getRequestUri().getScheme().equalsIgnoreCase("https"); CookieHelper.addCookie(KEYCLOAK_STATE_CHECKER, stateChecker, cookiePath, null, null, -1, secureOnly, true);