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 efec4fa89e5..db45b0b08ca 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 @@ -111,6 +111,8 @@ public final class KeycloakModelUtils { public static final String GROUP_PATH_ESCAPE = "~"; private static final char CLIENT_ROLE_SEPARATOR = '.'; + public static final int MAX_CLIENT_LOOKUPS_DURING_ROLE_RESOLVE = 25; + private KeycloakModelUtils() { } @@ -937,8 +939,10 @@ public final class KeycloakModelUtils { } // Check client roles for all possible splits by dot + int counter = 0; int scopeIndex = roleName.lastIndexOf(CLIENT_ROLE_SEPARATOR); - while (scopeIndex >= 0) { + while (scopeIndex >= 0 && counter < MAX_CLIENT_LOOKUPS_DURING_ROLE_RESOLVE) { + counter++; String appName = roleName.substring(0, scopeIndex); ClientModel client = realm.getClientByClientId(appName); if (client != null) { @@ -948,6 +952,10 @@ public final class KeycloakModelUtils { scopeIndex = roleName.lastIndexOf(CLIENT_ROLE_SEPARATOR, scopeIndex - 1); } + if (counter >= MAX_CLIENT_LOOKUPS_DURING_ROLE_RESOLVE) { + logger.warnf("Not able to retrieve role model from the role name '%s'. Please use shorter role names with the limited amount of dots, roleName", roleName.length() > 100 ? roleName.substring(0, 100) + "..." : roleName); + return null; + } // determine if roleName is a realm role return realm.getRole(roleName); diff --git a/server-spi-private/src/test/java/org/keycloak/models/KeycloakModelUtilsTest.java b/server-spi-private/src/test/java/org/keycloak/models/KeycloakModelUtilsTest.java index 0407257b96b..d78e1d3785f 100644 --- a/server-spi-private/src/test/java/org/keycloak/models/KeycloakModelUtilsTest.java +++ b/server-spi-private/src/test/java/org/keycloak/models/KeycloakModelUtilsTest.java @@ -25,9 +25,13 @@ import static org.junit.Assert.assertNull; import java.util.List; import java.util.Map; +import java.util.concurrent.atomic.AtomicInteger; import java.util.stream.Stream; + +import org.junit.Assert; import org.junit.Test; import org.keycloak.models.utils.KeycloakModelUtils; +import org.keycloak.models.utils.RealmModelDelegate; /** * @author Daniel Fesenmeyer @@ -67,6 +71,30 @@ public class KeycloakModelUtilsTest { assertParsedRoleQualifier(clientIdAndRoleName, "my.client.id", "role-name"); } + // Tests that count of client lookups during KeycloakModelUtils.getRoleFromString is limited (to prevent issues like DoS or OOM in case that incorrect configuration of the role mapper was provided) + @Test + public void testLimitCountOfClientLookupsDuringGetRoleFromString() { + AtomicInteger counter = new AtomicInteger(0); + + RealmModel realm = new RealmModelDelegate(null) { + + @Override + public ClientModel getClientByClientId(String clientId) { + counter.incrementAndGet(); + return null; + } + }; + + String badRoleName = "."; + for (int i = 0 ; i < 16 ; i++) { + badRoleName = badRoleName + badRoleName; + } + Assert.assertEquals(65536, badRoleName.length()); + + Assert.assertNull(KeycloakModelUtils.getRoleFromString(realm, badRoleName)); + Assert.assertEquals(KeycloakModelUtils.MAX_CLIENT_LOOKUPS_DURING_ROLE_RESOLVE, counter.get()); + } + @Test public void testSplitEscapedPath() { assertArrayEquals(new String[]{"parent", "child"}, KeycloakModelUtils.splitPath("/parent/child", true));