From 2665fb01a639891427481aaf5d323569bfd3e7a6 Mon Sep 17 00:00:00 2001 From: Hynek Mlnarik Date: Fri, 10 Feb 2023 20:48:40 +0100 Subject: [PATCH] File storage: Fix path traversal Fixes: #17029 --- .../map/storage/file/FileMapStorage.java | 166 ++++++++++-------- .../file/FileMapStorageProviderFactory.java | 11 +- .../testsuite/model/role/RoleModelTest.java | 31 +++- 3 files changed, 125 insertions(+), 83 deletions(-) diff --git a/model/map-file/src/main/java/org/keycloak/models/map/storage/file/FileMapStorage.java b/model/map-file/src/main/java/org/keycloak/models/map/storage/file/FileMapStorage.java index 5526c44fe7..6b49943e7b 100644 --- a/model/map-file/src/main/java/org/keycloak/models/map/storage/file/FileMapStorage.java +++ b/model/map-file/src/main/java/org/keycloak/models/map/storage/file/FileMapStorage.java @@ -16,6 +16,7 @@ */ package org.keycloak.models.map.storage.file; +import org.keycloak.common.util.StackUtil; import org.keycloak.models.ClientModel; import org.keycloak.models.KeycloakSession; import org.keycloak.models.ModelDuplicateException; @@ -44,6 +45,7 @@ import java.io.UncheckedIOException; import java.nio.file.FileAlreadyExistsException; import java.nio.file.Files; import java.nio.file.Path; +import java.util.Arrays; import java.util.IdentityHashMap; import java.util.List; import java.util.Map; @@ -51,6 +53,8 @@ import java.util.Objects; import java.util.Optional; import java.util.function.Function; import java.util.function.Predicate; +import java.util.regex.Matcher; +import java.util.regex.Pattern; import java.util.stream.Collectors; import java.util.stream.Stream; import org.jboss.logging.Logger; @@ -149,79 +153,77 @@ public class FileMapStorage imple .ifPresent(key -> this.fieldPredicates.replace(key, (builder, op, params) -> builder)); } - protected Path getPathForSanitizedId(Path sanitizedIdPath) { - final Path dataDirectory = getDataDirectory(); - final Path dataDirectoryWithChildren = dataDirectory.resolve(sanitizedIdPath).getParent(); - - if (! Files.isDirectory(dataDirectoryWithChildren)) { - try { - Files.createDirectories(dataDirectoryWithChildren); - } catch (IOException ex) { - throw new IllegalStateException("Directory does not exist and cannot be created: " + dataDirectory, ex); + protected Path getPathForEscapedId(String[] escapedIdPathArray) { + Path parentDirectory = getDataDirectory(); + Path targetPath = parentDirectory; + for (String path : escapedIdPathArray) { + targetPath = targetPath.resolve(path).normalize(); + if (! targetPath.getParent().equals(parentDirectory)) { + LOG.warnf("Path traversal detected: %s", Arrays.toString(escapedIdPathArray)); + return null; } - } - return dataDirectoryWithChildren.resolve(sanitizedIdPath.getFileName() + FILE_SUFFIX); - } - - protected Path getPathForSanitizedId(String sanitizedId) { - if (sanitizedId == null) { - throw new IllegalStateException("Invalid ID to sanitize"); + parentDirectory = targetPath; } - return getPathForSanitizedId(Path.of(sanitizedId)); + return targetPath.resolveSibling(targetPath.getFileName() + FILE_SUFFIX); } - protected String sanitizeId(String id) { - Objects.requireNonNull(id, "ID must be non-null"); - - // TODO: sanitize -// id = id -// .replaceAll("=", "=e") -// .replaceAll(":", "=c") -// .replaceAll("/", "=s") -// .replaceAll("\\\\", "=b") -// ; - final Path pId = Path.of(id); - - // Do not allow absolute paths - if (pId.isAbsolute()) { - throw new IllegalStateException("Illegal ID requested: " + id); + protected Path getPathForEscapedId(String escapedId) { + if (escapedId == null) { + throw new IllegalStateException("Invalid ID to escape"); } - return id; + String[] escapedIdArray = ID_COMPONENT_SEPARATOR_PATTERN.split(escapedId); + return getPathForEscapedId(escapedIdArray); } - protected String desanitizeId(String sanitizedId) { - if (sanitizedId == null) { + // Percent sign + Unix (/) and https://learn.microsoft.com/en-us/windows/win32/fileio/naming-a-file reserved characters + private static final Pattern RESERVED_CHARACTERS = Pattern.compile("[%<:>\"/\\\\|?*=]"); + private static final String ID_COMPONENT_SEPARATOR = ":"; + private static final String ESCAPING_CHARACTER = "="; + private static final Pattern ID_COMPONENT_SEPARATOR_PATTERN = Pattern.compile(Pattern.quote(ID_COMPONENT_SEPARATOR) + "+"); + + private static String[] escapeId(String[] idArray) { + if (idArray == null || idArray.length == 0 || idArray.length == 1 && idArray[0] == null) { return null; } + return Stream.of(idArray) + .map(Crud::escapeId) + .toArray(String[]::new); + } - return sanitizedId - .replaceAll("=c", ":") - .replaceAll("=s", "/") - .replaceAll("=b", "\\\\") - .replaceAll("=e", "=") - ; + private static String escapeId(String id) { + Objects.requireNonNull(id, "ID must be non-null"); + StringBuilder idEscaped = new StringBuilder(); + Matcher m = RESERVED_CHARACTERS.matcher(id); + while (m.find()) { + m.appendReplacement(idEscaped, String.format(ESCAPING_CHARACTER + "%02x", (int) m.group().charAt(0))); + } + m.appendTail(idEscaped); + final Path pId = Path.of(idEscaped.toString()); + + return pId.toString(); } protected V parse(Path fileName) { final V parsedObject = YamlParser.parse(fileName, new MapEntityContext<>(entityClass)); if (parsedObject == null) { + LOG.debugf("Could not parse %s%s", fileName, StackUtil.getShortStackTrace()); return null; } + String escapedId = determineKeyFromValue(parsedObject, false); final String fileNameStr = fileName.getFileName().toString(); - String id = determineKeyFromValue(parsedObject, false); - final String desanitizedId = desanitizeId(fileNameStr.substring(0, fileNameStr.length() - FILE_SUFFIX.length())); - if (id == null) { - LOG.debugf("Determined ID from filename: %s", desanitizedId); - id = desanitizedId; - } else if (! id.endsWith(desanitizedId)) { - LOG.warnf("Filename \"%s\" does not end with expected id \"%s\". Fix the file name.", fileNameStr, id); + final String idFromFilename = fileNameStr.substring(0, fileNameStr.length() - FILE_SUFFIX.length()); + if (escapedId == null) { + LOG.debugf("Determined ID from filename: %s%s", idFromFilename); + escapedId = idFromFilename; + } else if (! escapedId.endsWith(idFromFilename)) { + LOG.warnf("Id \"%s\" does not conform with filename \"%s\", expected: %s", escapedId, fileNameStr, escapeId(escapedId)); } - parsedObject.setId(id); + parsedObject.setId(escapedId); parsedObject.clearUpdatedFlag(); return parsedObject; @@ -230,21 +232,31 @@ public class FileMapStorage imple @Override public V create(V value) { // TODO: Lock realm directory for changes (e.g. on realm deletion) - // TODO: Sanitize ID - String sanitizedId = sanitizeId(value.getId()); + String escapedId = value.getId(); - writeYamlContents(getPathForSanitizedId(sanitizedId), value); + writeYamlContents(getPathForEscapedId(escapedId), value); return value; } + /** + * Returns escaped ID - relative file name in the file system with path separator {@link #ID_COMPONENT_SEPARATOR}. + * @param value Object + * @param forCreate Whether this is for create operation ({@code true}) or + * @return + */ @Override public String determineKeyFromValue(V value, boolean forCreate) { final boolean randomId; String[] proposedId = suggestedPath.apply(value); if (! forCreate) { - return proposedId == null ? null : String.join("/", proposedId); + String[] escapedProposedId = escapeId(proposedId); + final String res = proposedId == null ? null : String.join(ID_COMPONENT_SEPARATOR, escapedProposedId); + if (LOG.isDebugEnabled()) { + LOG.debugf("determineKeyFromValue: got %s (%s) for %s", res, res == null ? null : String.join(" [/] ", proposedId), value); + } + return res; } if (proposedId == null || proposedId.length == 0) { @@ -254,25 +266,32 @@ public class FileMapStorage imple randomId = false; } - Path sanitizedId = Path.of( - sanitizeId(proposedId[0]), - Stream.of(proposedId).skip(1).map(this::sanitizeId).toArray(String[]::new) - ); + String[] escapedProposedId = escapeId(proposedId); + Path sp = getPathForEscapedId(escapedProposedId); // sp will never be null + + final Path parentDir = sp.getParent(); + if (! Files.isDirectory(parentDir)) { + try { + Files.createDirectories(parentDir); + } catch (IOException ex) { + throw new IllegalStateException("Directory does not exist and cannot be created: " + parentDir, ex); + } + } - Path sp = getPathForSanitizedId(sanitizedId); for (int counter = 0; counter < 100; counter++) { - LOG.tracef("Attempting to create file %s", sp); + LOG.tracef("Attempting to create file %s", sp, StackUtil.getShortStackTrace()); try { touch(sp); - return String.join("/", proposedId); + final String res = String.join(ID_COMPONENT_SEPARATOR, escapedProposedId); + LOG.debugf("determineKeyFromValue: got %s for created %s", res, value); + return res; } catch (FileAlreadyExistsException ex) { if (! randomId) { throw new ModelDuplicateException("File " + sp + " already exists!"); } final String lastComponent = StringKey.INSTANCE.yieldNewUniqueKey(); - proposedId[proposedId.length - 1] = lastComponent; - sanitizedId = sanitizedId.resolveSibling(sanitizeId(lastComponent)); - sp = getPathForSanitizedId(sanitizedId); + escapedProposedId[escapedProposedId.length - 1] = lastComponent; + sp = getPathForEscapedId(escapedProposedId); } catch (IOException ex) { throw new IllegalStateException("Could not create file " + sp, ex); } @@ -283,8 +302,8 @@ public class FileMapStorage imple @Override public V read(String key) { - return Optional.ofNullable(sanitizeId(key)) - .map(this::getPathForSanitizedId) + return Optional.ofNullable(key) + .map(this::getPathForEscapedId) .filter(Files::isReadable) .map(this::parse) .orElse(null); @@ -318,7 +337,8 @@ public class FileMapStorage imple } Stream res = paths.stream() .filter(FileMapStorage::canParseFile) - .map(this::parse).filter(Objects::nonNull); + .map(this::parse) + .filter(Objects::nonNull); MapModelCriteriaBuilder mcb = queryParameters.getModelCriteriaBuilder().flashToModelCriteriaBuilder(createCriteriaBuilder()); @@ -342,10 +362,12 @@ public class FileMapStorage imple @Override public V update(V value) { - String proposedId = value.getId(); - String sanitizedId = sanitizeId(proposedId); + String escapedId = value.getId(); - Path sp = getPathForSanitizedId(sanitizedId); + Path sp = getPathForEscapedId(escapedId); + if (sp == null) { + throw new IllegalArgumentException("Invalid path: " + escapedId); + } // TODO: improve locking synchronized (FileMapStorageProviderFactory.class) { @@ -357,8 +379,8 @@ public class FileMapStorage imple @Override public boolean delete(String key) { - return Optional.ofNullable(sanitizeId(key)) - .map(this::getPathForSanitizedId) + return Optional.ofNullable(key) + .map(this::getPathForEscapedId) .map(this::removeIfExists) .orElse(false); } @@ -384,7 +406,7 @@ public class FileMapStorage imple } private Path getDataDirectory() { - return dataDirectoryFunc.apply(defaultRealmId == null ? null : sanitizeId(defaultRealmId)); + return dataDirectoryFunc.apply(defaultRealmId == null ? null : escapeId(defaultRealmId)); } private void writeYamlContents(Path sp, V value) { diff --git a/model/map-file/src/main/java/org/keycloak/models/map/storage/file/FileMapStorageProviderFactory.java b/model/map-file/src/main/java/org/keycloak/models/map/storage/file/FileMapStorageProviderFactory.java index ef95a0753b..dbfe08f9b6 100644 --- a/model/map-file/src/main/java/org/keycloak/models/map/storage/file/FileMapStorageProviderFactory.java +++ b/model/map-file/src/main/java/org/keycloak/models/map/storage/file/FileMapStorageProviderFactory.java @@ -41,6 +41,7 @@ import org.keycloak.models.map.storage.ModelEntityUtil; import org.keycloak.models.map.user.MapUserEntity; import org.keycloak.provider.EnvironmentDependentProviderFactory; import java.io.File; +import java.nio.file.FileSystem; import java.nio.file.Path; import java.util.ConcurrentModificationException; import java.util.HashMap; @@ -78,10 +79,10 @@ public class FileMapStorageProviderFactory implements AmphibianProviderFactory) v -> new String[] { v.getClientId() })), - entry(MapPolicyEntity.class, ((Function) v -> new String[] { v.getResourceServerId(), "policy", v.getName() })), - entry(MapPermissionTicketEntity.class,((Function) v -> new String[] { v.getResourceServerId(), "ticket", v.getId()})), - entry(MapResourceEntity.class, ((Function) v -> new String[] { v.getResourceServerId(), "resource", v.getName() })), - entry(MapScopeEntity.class, ((Function) v -> new String[] { v.getResourceServerId(), "scope", v.getName() })) + entry(MapPolicyEntity.class, ((Function) v -> new String[] { v.getResourceServerId(), v.getName() })), + entry(MapPermissionTicketEntity.class,((Function) v -> new String[] { v.getResourceServerId(), v.getId()})), + entry(MapResourceEntity.class, ((Function) v -> new String[] { v.getResourceServerId(), v.getName() })), + entry(MapScopeEntity.class, ((Function) v -> new String[] { v.getResourceServerId(), v.getName() })) ); @Override @@ -123,7 +124,7 @@ public class FileMapStorageProviderFactory implements AmphibianProviderFactory { throw new IllegalStateException("Directory for " + areaName + " area not configured."); }; } - String a = areaName.startsWith("authz-") ? "authz" : areaName; + Path a = areaName.startsWith("authz-") ? Path.of("authz", areaName.substring(6)) : Path.of(areaName); return realmId -> { if (realmId == null || FORBIDDEN_CHARACTERS.matcher(realmId).find()) { diff --git a/testsuite/model/src/test/java/org/keycloak/testsuite/model/role/RoleModelTest.java b/testsuite/model/src/test/java/org/keycloak/testsuite/model/role/RoleModelTest.java index 7782e0d53a..64a7bca1f8 100644 --- a/testsuite/model/src/test/java/org/keycloak/testsuite/model/role/RoleModelTest.java +++ b/testsuite/model/src/test/java/org/keycloak/testsuite/model/role/RoleModelTest.java @@ -21,18 +21,24 @@ import java.util.stream.Collectors; import java.util.stream.IntStream; import java.util.stream.Stream; +import org.junit.Assume; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.empty; import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.nullValue; @RequireProvider(RealmProvider.class) @RequireProvider(ClientProvider.class) @RequireProvider(RoleProvider.class) public class RoleModelTest extends KeycloakModelTest { + private static final String MAIN_ROLE_NAME = "main-role"; + private static final String ROLE_PREFIX = "main-role-composite-"; + private static final String CLIENT_NAME = "client-with-roles"; + private String realmId; private String mainRoleId; private static List rolesSubset; @@ -58,15 +64,15 @@ public class RoleModelTest extends KeycloakModelTest { private void createRoles(KeycloakSession session, RealmModel realm) { - RoleModel mainRole = session.roles().addRealmRole(realm, "main-role"); + RoleModel mainRole = session.roles().addRealmRole(realm, MAIN_ROLE_NAME); mainRoleId = mainRole.getId(); - ClientModel clientModel = session.clients().addClient(realm, "client-with-roles"); + ClientModel clientModel = session.clients().addClient(realm, CLIENT_NAME); // Create 10 realm roles that are composites of main role rolesSubset = IntStream.range(0, 10) .boxed() - .map(i -> session.roles().addRealmRole(realm, "main-role-composite-" + i)) + .map(i -> session.roles().addRealmRole(realm, ROLE_PREFIX + i)) .peek(role -> role.setDescription("This is a description for " + role.getName() + " realm role.")) .peek(mainRole::addCompositeRole) .map(RoleModel::getId) @@ -75,7 +81,7 @@ public class RoleModelTest extends KeycloakModelTest { // Create 10 client roles that are composites of main role rolesSubset.addAll(IntStream.range(10, 20) .boxed() - .map(i -> session.roles().addClientRole(clientModel, "main-role-composite-" + i)) + .map(i -> session.roles().addClientRole(clientModel, ROLE_PREFIX + i)) .peek(role -> role.setDescription("This is a description for " + role.getName() + " client role.")) .peek(mainRole::addCompositeRole) .map(RoleModel::getId) @@ -215,7 +221,7 @@ public class RoleModelTest extends KeycloakModelTest { realmRolesByDescription = session.roles().searchForRolesStream(realm, "DESCRIPTION FOR", 3, 9).collect(Collectors.toList()); assertThat(realmRolesByDescription, hasSize(7)); - ClientModel client = session.clients().getClientByClientId(realm, "client-with-roles"); + ClientModel client = session.clients().getClientByClientId(realm, CLIENT_NAME); List clientRolesByDescription = session.roles().searchForClientRolesStream(client, "this is a", 0, 10).collect(Collectors.toList()); assertThat(clientRolesByDescription, hasSize(10)); @@ -283,6 +289,19 @@ public class RoleModelTest extends KeycloakModelTest { }); } + @Test + public void getRolePathTraversal() { + // Only perform this test if realm role ID = role.name and client role ID = client.id + ":" + role.name + Assume.assumeThat(mainRoleId, is(MAIN_ROLE_NAME)); + Assume.assumeTrue(rolesSubset.stream().anyMatch((CLIENT_NAME + ":" + ROLE_PREFIX + "10")::equals)); + + withRealm(realmId, (session, realm) -> { + RoleModel role = session.roles().getRoleById(realm, (CLIENT_NAME + ":" + ROLE_PREFIX + "10") + "/../../" + MAIN_ROLE_NAME); + assertThat(role, nullValue()); + return null; + }); + } + public void testRolesWithIdsPaginationSearchQueries(GetResult resultProvider) { // test all parameters together List result = resultProvider.getResult("1", 4, 3); @@ -291,6 +310,6 @@ public class RoleModelTest extends KeycloakModelTest { } private void assertIndexValues(List roles, Matcher> matcher) { - assertThat(roles.stream().map(RoleModel::getName).map(s -> s.substring("main-role-composite-".length())).map(Integer::parseInt).collect(Collectors.toList()), matcher); + assertThat(roles.stream().map(RoleModel::getName).map(s -> s.substring(ROLE_PREFIX.length())).map(Integer::parseInt).collect(Collectors.toList()), matcher); } }