libre.sh/keycloak-scim
3
0
Fork 0
Code Issues 15 Pull requests Releases Packages Activity Actions

File storage: Fix path traversal

Browse source 
Fixes: #17029
This commit is contained in:
Hynek Mlnarik 2023-02-10 20:48:40 +01:00 committed by Hynek Mlnařík
parent 2711606a70
commit 2665fb01a6
3 changed files with 125 additions and 83 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

184
model/map-file/src/main/java/org/keycloak/models/map/storage/file/FileMapStorage.java
View file

@ -16,6 +16,7 @@
*/ */
package org.keycloak.models.map.storage.file; package org.keycloak.models.map.storage.file;
import org.keycloak.common.util.StackUtil;
import org.keycloak.models.ClientModel; import org.keycloak.models.ClientModel;
import org.keycloak.models.KeycloakSession; import org.keycloak.models.KeycloakSession;
import org.keycloak.models.ModelDuplicateException; import org.keycloak.models.ModelDuplicateException;
@ -44,6 +45,7 @@ import java.io.UncheckedIOException;
import java.nio.file.FileAlreadyExistsException; import java.nio.file.FileAlreadyExistsException;
import java.nio.file.Files; import java.nio.file.Files;
import java.nio.file.Path; import java.nio.file.Path;
import java.util.Arrays;
import java.util.IdentityHashMap; import java.util.IdentityHashMap;
import java.util.List; import java.util.List;
import java.util.Map; import java.util.Map;
@ -51,6 +53,8 @@ import java.util.Objects;
import java.util.Optional; import java.util.Optional;
import java.util.function.Function; import java.util.function.Function;
import java.util.function.Predicate; import java.util.function.Predicate;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import java.util.stream.Collectors; import java.util.stream.Collectors;
import java.util.stream.Stream; import java.util.stream.Stream;
import org.jboss.logging.Logger; import org.jboss.logging.Logger;
@ -149,79 +153,77 @@ public class FileMapStorage<V extends AbstractEntity & UpdatableEntity, M> imple
.ifPresent(key -> this.fieldPredicates.replace(key, (builder, op, params) -> builder)); .ifPresent(key -> this.fieldPredicates.replace(key, (builder, op, params) -> builder));
} }
protected Path getPathForSanitizedId(Path sanitizedIdPath) { protected Path getPathForEscapedId(String[] escapedIdPathArray) {
final Path dataDirectory = getDataDirectory(); Path parentDirectory = getDataDirectory();
final Path dataDirectoryWithChildren = dataDirectory.resolve(sanitizedIdPath).getParent(); Path targetPath = parentDirectory;
for (String path : escapedIdPathArray) {
if (! Files.isDirectory(dataDirectoryWithChildren)) { targetPath = targetPath.resolve(path).normalize();
try { if (! targetPath.getParent().equals(parentDirectory)) {
Files.createDirectories(dataDirectoryWithChildren); LOG.warnf("Path traversal detected: %s", Arrays.toString(escapedIdPathArray));
} catch (IOException ex) {
throw new IllegalStateException("Directory does not exist and cannot be created: " + dataDirectory, ex);
}
}
return dataDirectoryWithChildren.resolve(sanitizedIdPath.getFileName() + FILE_SUFFIX);
}
protected Path getPathForSanitizedId(String sanitizedId) {
if (sanitizedId == null) {
throw new IllegalStateException("Invalid ID to sanitize");
}
return getPathForSanitizedId(Path.of(sanitizedId));
}
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);
}
return id;
}
protected String desanitizeId(String sanitizedId) {
if (sanitizedId == null) {
return null; return null;
} }
parentDirectory = targetPath;
}
return sanitizedId return targetPath.resolveSibling(targetPath.getFileName() + FILE_SUFFIX);
.replaceAll("=c", ":") }
.replaceAll("=s", "/")
.replaceAll("=b", "\\\\")
.replaceAll("=e", "=")
;
protected Path getPathForEscapedId(String escapedId) {
if (escapedId == null) {
throw new IllegalStateException("Invalid ID to escape");
}
String[] escapedIdArray = ID_COMPONENT_SEPARATOR_PATTERN.split(escapedId);
return getPathForEscapedId(escapedIdArray);
}
// 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);
}
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) { protected V parse(Path fileName) {
final V parsedObject = YamlParser.parse(fileName, new MapEntityContext<>(entityClass)); final V parsedObject = YamlParser.parse(fileName, new MapEntityContext<>(entityClass));
if (parsedObject == null) { if (parsedObject == null) {
LOG.debugf("Could not parse %s%s", fileName, StackUtil.getShortStackTrace());
return null; return null;
} }
String escapedId = determineKeyFromValue(parsedObject, false);
final String fileNameStr = fileName.getFileName().toString(); final String fileNameStr = fileName.getFileName().toString();
String id = determineKeyFromValue(parsedObject, false); final String idFromFilename = fileNameStr.substring(0, fileNameStr.length() - FILE_SUFFIX.length());
final String desanitizedId = desanitizeId(fileNameStr.substring(0, fileNameStr.length() - FILE_SUFFIX.length())); if (escapedId == null) {
if (id == null) { LOG.debugf("Determined ID from filename: %s%s", idFromFilename);
LOG.debugf("Determined ID from filename: %s", desanitizedId); escapedId = idFromFilename;
id = desanitizedId; } else if (! escapedId.endsWith(idFromFilename)) {
} else if (! id.endsWith(desanitizedId)) { LOG.warnf("Id \"%s\" does not conform with filename \"%s\", expected: %s", escapedId, fileNameStr, escapeId(escapedId));
LOG.warnf("Filename \"%s\" does not end with expected id \"%s\". Fix the file name.", fileNameStr, id);
} }
parsedObject.setId(id); parsedObject.setId(escapedId);
parsedObject.clearUpdatedFlag(); parsedObject.clearUpdatedFlag();
return parsedObject; return parsedObject;
@ -230,21 +232,31 @@ public class FileMapStorage<V extends AbstractEntity & UpdatableEntity, M> imple
@Override @Override
public V create(V value) { public V create(V value) {
// TODO: Lock realm directory for changes (e.g. on realm deletion) // TODO: Lock realm directory for changes (e.g. on realm deletion)
// TODO: Sanitize ID String escapedId = value.getId();
String sanitizedId = sanitizeId(value.getId());
writeYamlContents(getPathForSanitizedId(sanitizedId), value); writeYamlContents(getPathForEscapedId(escapedId), value);
return 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 @Override
public String determineKeyFromValue(V value, boolean forCreate) { public String determineKeyFromValue(V value, boolean forCreate) {
final boolean randomId; final boolean randomId;
String[] proposedId = suggestedPath.apply(value); String[] proposedId = suggestedPath.apply(value);
if (! forCreate) { 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) { if (proposedId == null || proposedId.length == 0) {
@ -254,25 +266,32 @@ public class FileMapStorage<V extends AbstractEntity & UpdatableEntity, M> imple
randomId = false; randomId = false;
} }
Path sanitizedId = Path.of( String[] escapedProposedId = escapeId(proposedId);
sanitizeId(proposedId[0]), Path sp = getPathForEscapedId(escapedProposedId); // sp will never be null
Stream.of(proposedId).skip(1).map(this::sanitizeId).toArray(String[]::new)
); 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++) { 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 { try {
touch(sp); 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) { } catch (FileAlreadyExistsException ex) {
if (! randomId) { if (! randomId) {
throw new ModelDuplicateException("File " + sp + " already exists!"); throw new ModelDuplicateException("File " + sp + " already exists!");
} }
final String lastComponent = StringKey.INSTANCE.yieldNewUniqueKey(); final String lastComponent = StringKey.INSTANCE.yieldNewUniqueKey();
proposedId[proposedId.length - 1] = lastComponent; escapedProposedId[escapedProposedId.length - 1] = lastComponent;
sanitizedId = sanitizedId.resolveSibling(sanitizeId(lastComponent)); sp = getPathForEscapedId(escapedProposedId);
sp = getPathForSanitizedId(sanitizedId);
} catch (IOException ex) { } catch (IOException ex) {
throw new IllegalStateException("Could not create file " + sp, ex); throw new IllegalStateException("Could not create file " + sp, ex);
} }
@ -283,8 +302,8 @@ public class FileMapStorage<V extends AbstractEntity & UpdatableEntity, M> imple
@Override @Override
public V read(String key) { public V read(String key) {
return Optional.ofNullable(sanitizeId(key)) return Optional.ofNullable(key)
.map(this::getPathForSanitizedId) .map(this::getPathForEscapedId)
.filter(Files::isReadable) .filter(Files::isReadable)
.map(this::parse) .map(this::parse)
.orElse(null); .orElse(null);
@ -318,7 +337,8 @@ public class FileMapStorage<V extends AbstractEntity & UpdatableEntity, M> imple
} }
Stream<V> res = paths.stream() Stream<V> res = paths.stream()
.filter(FileMapStorage::canParseFile) .filter(FileMapStorage::canParseFile)
.map(this::parse).filter(Objects::nonNull); .map(this::parse)
.filter(Objects::nonNull);
MapModelCriteriaBuilder<String,V,M> mcb = queryParameters.getModelCriteriaBuilder().flashToModelCriteriaBuilder(createCriteriaBuilder()); MapModelCriteriaBuilder<String,V,M> mcb = queryParameters.getModelCriteriaBuilder().flashToModelCriteriaBuilder(createCriteriaBuilder());
@ -342,10 +362,12 @@ public class FileMapStorage<V extends AbstractEntity & UpdatableEntity, M> imple
@Override @Override
public V update(V value) { public V update(V value) {
String proposedId = value.getId(); String escapedId = value.getId();
String sanitizedId = sanitizeId(proposedId);
Path sp = getPathForSanitizedId(sanitizedId); Path sp = getPathForEscapedId(escapedId);
if (sp == null) {
throw new IllegalArgumentException("Invalid path: " + escapedId);
}
// TODO: improve locking // TODO: improve locking
synchronized (FileMapStorageProviderFactory.class) { synchronized (FileMapStorageProviderFactory.class) {
@ -357,8 +379,8 @@ public class FileMapStorage<V extends AbstractEntity & UpdatableEntity, M> imple
@Override @Override
public boolean delete(String key) { public boolean delete(String key) {
return Optional.ofNullable(sanitizeId(key)) return Optional.ofNullable(key)
.map(this::getPathForSanitizedId) .map(this::getPathForEscapedId)
.map(this::removeIfExists) .map(this::removeIfExists)
.orElse(false); .orElse(false);
} }
@ -384,7 +406,7 @@ public class FileMapStorage<V extends AbstractEntity & UpdatableEntity, M> imple
} }
private Path getDataDirectory() { 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) { private void writeYamlContents(Path sp, V value) {

11
model/map-file/src/main/java/org/keycloak/models/map/storage/file/FileMapStorageProviderFactory.java
View file

@ -41,6 +41,7 @@ import org.keycloak.models.map.storage.ModelEntityUtil;
import org.keycloak.models.map.user.MapUserEntity; import org.keycloak.models.map.user.MapUserEntity;
import org.keycloak.provider.EnvironmentDependentProviderFactory; import org.keycloak.provider.EnvironmentDependentProviderFactory;
import java.io.File; import java.io.File;
import java.nio.file.FileSystem;
import java.nio.file.Path; import java.nio.file.Path;
import java.util.ConcurrentModificationException; import java.util.ConcurrentModificationException;
import java.util.HashMap; import java.util.HashMap;
@ -78,10 +79,10 @@ public class FileMapStorageProviderFactory implements AmphibianProviderFactory<M
// authz // authz
entry(MapResourceServerEntity.class, ((Function<MapResourceServerEntity, String[]>) v -> new String[] { v.getClientId() })), entry(MapResourceServerEntity.class, ((Function<MapResourceServerEntity, String[]>) v -> new String[] { v.getClientId() })),
entry(MapPolicyEntity.class, ((Function<MapPolicyEntity, String[]>) v -> new String[] { v.getResourceServerId(), "policy", v.getName() })), entry(MapPolicyEntity.class, ((Function<MapPolicyEntity, String[]>) v -> new String[] { v.getResourceServerId(), v.getName() })),
entry(MapPermissionTicketEntity.class,((Function<MapPermissionTicketEntity, String[]>) v -> new String[] { v.getResourceServerId(), "ticket", v.getId()})), entry(MapPermissionTicketEntity.class,((Function<MapPermissionTicketEntity, String[]>) v -> new String[] { v.getResourceServerId(), v.getId()})),
entry(MapResourceEntity.class, ((Function<MapResourceEntity, String[]>) v -> new String[] { v.getResourceServerId(), "resource", v.getName() })), entry(MapResourceEntity.class, ((Function<MapResourceEntity, String[]>) v -> new String[] { v.getResourceServerId(), v.getName() })),
entry(MapScopeEntity.class, ((Function<MapScopeEntity, String[]>) v -> new String[] { v.getResourceServerId(), "scope", v.getName() })) entry(MapScopeEntity.class, ((Function<MapScopeEntity, String[]>) v -> new String[] { v.getResourceServerId(), v.getName() }))
); );
@Override @Override
@ -123,7 +124,7 @@ public class FileMapStorageProviderFactory implements AmphibianProviderFactory<M
return p -> { throw new IllegalStateException("Directory for " + areaName + " area not configured."); }; return p -> { 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 -> { return realmId -> {
if (realmId == null || FORBIDDEN_CHARACTERS.matcher(realmId).find()) { if (realmId == null || FORBIDDEN_CHARACTERS.matcher(realmId).find()) {

31
testsuite/model/src/test/java/org/keycloak/testsuite/model/role/RoleModelTest.java
View file

@ -21,18 +21,24 @@ import java.util.stream.Collectors;
import java.util.stream.IntStream; import java.util.stream.IntStream;
import java.util.stream.Stream; import java.util.stream.Stream;
import org.junit.Assume;
import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.contains;
import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.containsInAnyOrder;
import static org.hamcrest.Matchers.empty; import static org.hamcrest.Matchers.empty;
import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.hasSize;
import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.nullValue;
@RequireProvider(RealmProvider.class) @RequireProvider(RealmProvider.class)
@RequireProvider(ClientProvider.class) @RequireProvider(ClientProvider.class)
@RequireProvider(RoleProvider.class) @RequireProvider(RoleProvider.class)
public class RoleModelTest extends KeycloakModelTest { 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 realmId;
private String mainRoleId; private String mainRoleId;
private static List<String> rolesSubset; private static List<String> rolesSubset;
@ -58,15 +64,15 @@ public class RoleModelTest extends KeycloakModelTest {
private void createRoles(KeycloakSession session, RealmModel realm) { 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(); 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 // Create 10 realm roles that are composites of main role
rolesSubset = IntStream.range(0, 10) rolesSubset = IntStream.range(0, 10)
.boxed() .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(role -> role.setDescription("This is a description for " + role.getName() + " realm role."))
.peek(mainRole::addCompositeRole) .peek(mainRole::addCompositeRole)
.map(RoleModel::getId) .map(RoleModel::getId)
@ -75,7 +81,7 @@ public class RoleModelTest extends KeycloakModelTest {
// Create 10 client roles that are composites of main role // Create 10 client roles that are composites of main role
rolesSubset.addAll(IntStream.range(10, 20) rolesSubset.addAll(IntStream.range(10, 20)
.boxed() .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(role -> role.setDescription("This is a description for " + role.getName() + " client role."))
.peek(mainRole::addCompositeRole) .peek(mainRole::addCompositeRole)
.map(RoleModel::getId) .map(RoleModel::getId)
@ -215,7 +221,7 @@ public class RoleModelTest extends KeycloakModelTest {
realmRolesByDescription = session.roles().searchForRolesStream(realm, "DESCRIPTION FOR", 3, 9).collect(Collectors.toList()); realmRolesByDescription = session.roles().searchForRolesStream(realm, "DESCRIPTION FOR", 3, 9).collect(Collectors.toList());
assertThat(realmRolesByDescription, hasSize(7)); assertThat(realmRolesByDescription, hasSize(7));
ClientModel client = session.clients().getClientByClientId(realm, "client-with-roles"); ClientModel client = session.clients().getClientByClientId(realm, CLIENT_NAME);
List<RoleModel> clientRolesByDescription = session.roles().searchForClientRolesStream(client, "this is a", 0, 10).collect(Collectors.toList()); List<RoleModel> clientRolesByDescription = session.roles().searchForClientRolesStream(client, "this is a", 0, 10).collect(Collectors.toList());
assertThat(clientRolesByDescription, hasSize(10)); 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) { public void testRolesWithIdsPaginationSearchQueries(GetResult resultProvider) {
// test all parameters together // test all parameters together
List<RoleModel> result = resultProvider.getResult("1", 4, 3); List<RoleModel> result = resultProvider.getResult("1", 4, 3);
@ -291,6 +310,6 @@ public class RoleModelTest extends KeycloakModelTest {
} }
private void assertIndexValues(List<RoleModel> roles, Matcher<? super Collection<? extends Integer>> matcher) { private void assertIndexValues(List<RoleModel> roles, Matcher<? super Collection<? extends Integer>> 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);
} }
} }