Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions core/src/org/labkey/core/CoreModule.java
Original file line number Diff line number Diff line change
Expand Up @@ -1500,6 +1500,7 @@ public TabDisplayMode getTabDisplayMode()
public @NotNull Set<Class<?>> getUnitTests()
{
return Set.of(
AdminController.FileRootPermissionTestCase.class,
ApiJsonWriter.TestCase.class,
ClassLoaderTestCase.class,
CopyFileRootPipelineJob.TestCase.class,
Expand Down
103 changes: 89 additions & 14 deletions core/src/org/labkey/core/admin/AdminController.java
Original file line number Diff line number Diff line change
Expand Up @@ -6570,22 +6570,47 @@ private static void initiateCopyFilesPipelineJobs(ViewContext ctx, @NotNull List

private static void throwIfUnauthorizedFileRootChange(ViewContext ctx, FileContentService service, FileManagementForm form)
{
// test permissions. only site admins are able to turn on a custom file root for a folder
// this is only relevant if the folder is either being switched to a custom file root,
// or if the file root is changed.
if (!service.isUseDefaultRoot(ctx.getContainer()))
// Only site admins (AdminOperationsPermission) are able to switch a folder to a custom file root, or
// change an existing custom root's path -- doing so lets a folder admin point file storage anywhere the
// server process can read/write. Resubmitting the folder's own current custom root unchanged is a no-op
// and does not require the elevated permission.
boolean hasAdminOpsPermission = ctx.getUser().hasRootPermission(AdminOperationsPermission.class);
boolean isUseDefaultRoot = service.isUseDefaultRoot(ctx.getContainer());
String requestedRoot;
String currentRoot;

if (form.isCloudFileRoot())
{
Path fileRootPath = service.getFileRootPath(ctx.getContainer());
if (null != fileRootPath)
{
String absolutePath = FileUtil.getAbsolutePath(ctx.getContainer(), fileRootPath);
if (Strings.CI.equals(absolutePath, form.getFolderRootPath()))
{
if (!ctx.getUser().hasRootPermission(AdminOperationsPermission.class))
throw new UnauthorizedException("Only site admins can change file roots");
}
}
requestedRoot = form.getCloudRootName();
currentRoot = (!isUseDefaultRoot && service.isCloudRoot(ctx.getContainer())) ? service.getCloudRootName(ctx.getContainer()) : null;
}
else
{
requestedRoot = StringUtils.trimToNull(form.getFolderRootPath());
Path fileRootPath = isUseDefaultRoot ? null : service.getFileRootPath(ctx.getContainer());
currentRoot = null != fileRootPath ? FileUtil.getAbsolutePath(ctx.getContainer(), fileRootPath) : null;
}

if (!isFileRootChangeAuthorized(hasAdminOpsPermission, isUseDefaultRoot, currentRoot, requestedRoot))
throw new UnauthorizedException("Only site admins can change file roots");
}

/**
* Pure decision logic behind {@link #throwIfUnauthorizedFileRootChange}, factored out for unit testing.
* @param hasAdminOpsPermission whether the requesting user holds root AdminOperationsPermission
* @param isUseDefaultRoot whether the target container currently uses the default (inherited) file root
* @param currentRoot the container's existing custom root path/cloud name, or null if there isn't one
* @param requestedRoot the root path/cloud name submitted in the request, or null if none was submitted
*/
static boolean isFileRootChangeAuthorized(boolean hasAdminOpsPermission, boolean isUseDefaultRoot, @Nullable String currentRoot, @Nullable String requestedRoot)
{
if (hasAdminOpsPermission)
return true;
if (null == requestedRoot)
return isUseDefaultRoot || null == currentRoot; // clearing an existing custom root is still a change to it
if (!isUseDefaultRoot &&requestedRoot.equalsIgnoreCase(currentRoot))
return true; // no-op resubmission of the folder's own existing custom root
return false;
}

public static void setEnabledCloudStores(ViewContext ctx, FileManagementForm form, BindException errors)
Expand Down Expand Up @@ -9564,6 +9589,56 @@ public void modulesWithSchemaVersionButNoScripts()
}
}

// Regression coverage for the file root privilege-escalation fix: a folder/project admin without root
// AdminOperationsPermission must never be able to switch a container to a custom file root, or change an
// existing custom root's path.
public static class FileRootPermissionTestCase extends Assert
{
@Test
public void defaultRootRequiresAdminOpsPermissionForNewCustomPath()
{
// This is the case the original (inverted) condition silently skipped: a container on the default
// root, submitting any custom path, from a user without AdminOperationsPermission.
assertFalse(isFileRootChangeAuthorized(false, true, null, "/some/path"));
}

@Test
public void customRootRequiresAdminOpsPermissionForDifferentPath()
{
assertFalse(isFileRootChangeAuthorized(false, false, "/existing/path", "/attacker/path"));
}

@Test
public void customRootResubmissionOfSamePathIsAllowed()
{
assertTrue(isFileRootChangeAuthorized(false, false, "/existing/path", "/existing/path"));
assertTrue(isFileRootChangeAuthorized(false, false, "/Existing/Path", "/existing/path"));
}

@Test
public void noRequestedRootOnDefaultRootIsAllowed()
{
// No custom root requested and none currently exists -- nothing to protect.
assertTrue(isFileRootChangeAuthorized(false, true, null, null));
}

@Test
public void clearingAnExistingCustomRootRequiresAdminOpsPermission()
{
// A request that omits the root (e.g. a non-ops admin's disabled form fields not being submitted)
// must not be able to silently clear an existing custom root back to default.
assertFalse(isFileRootChangeAuthorized(false, false, "/existing/path", null));
assertTrue(isFileRootChangeAuthorized(true, false, "/existing/path", null));
}

@Test
public void adminOpsPermissionIsAlwaysAllowed()
{
assertTrue(isFileRootChangeAuthorized(true, true, null, "/some/path"));
assertTrue(isFileRootChangeAuthorized(true, false, "/existing/path", "/attacker/path"));
}
}

public static class ModuleForm
{
private String _name;
Expand Down