From 48b4978a3b181f52ccf95bffd894764c23a9f8d2 Mon Sep 17 00:00:00 2001 From: Rene Pfeuffer Date: Fri, 5 Jul 2024 12:24:24 +0200 Subject: [PATCH] Fix privilege escalation in namespaces MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This fixes the following security issue: If a user creates a new repository in a namespace this user had no permission to read any repository from, the user gets OWNER permissions on this namespace and all other permissions are removed from this namespace. Pushed-by: Rene Pfeuffer Co-authored-by: René Pfeuffer Committed-by: René Pfeuffer --- gradle/changelog/namespace_permissions.yaml | 2 + .../scm/it/NamespacePermissionITCase.java | 197 ++++++++++++++++++ .../java/sonia/scm/it/utils/TestData.java | 8 +- .../sonia/scm/it/webapp/RepositoryITUtil.java | 7 +- .../repository/DefaultNamespaceManager.java | 36 ++-- .../DefaultNamespaceManagerTest.java | 73 ++++--- 6 files changed, 274 insertions(+), 49 deletions(-) create mode 100644 gradle/changelog/namespace_permissions.yaml create mode 100644 scm-it/src/test/java/sonia/scm/it/NamespacePermissionITCase.java diff --git a/gradle/changelog/namespace_permissions.yaml b/gradle/changelog/namespace_permissions.yaml new file mode 100644 index 0000000000..d4d5a28a10 --- /dev/null +++ b/gradle/changelog/namespace_permissions.yaml @@ -0,0 +1,2 @@ +- type: security + description: Privilege escalation in namespaces diff --git a/scm-it/src/test/java/sonia/scm/it/NamespacePermissionITCase.java b/scm-it/src/test/java/sonia/scm/it/NamespacePermissionITCase.java new file mode 100644 index 0000000000..e3cb3ccb8a --- /dev/null +++ b/scm-it/src/test/java/sonia/scm/it/NamespacePermissionITCase.java @@ -0,0 +1,197 @@ +/* + * MIT License + * + * Copyright (c) 2020-present Cloudogu GmbH and Contributors + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + */ + +package sonia.scm.it; + +import jakarta.ws.rs.client.Entity; +import jakarta.ws.rs.core.MediaType; +import jakarta.ws.rs.core.Response; +import junit.framework.AssertionFailedError; +import org.apache.http.HttpStatus; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; +import sonia.scm.it.webapp.ScmClient; +import sonia.scm.web.VndMediaType; + +import java.net.URI; +import java.util.List; +import java.util.Map; +import java.util.stream.Collectors; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.fail; +import static sonia.scm.it.utils.RestUtil.createResourceUrl; +import static sonia.scm.it.utils.RestUtil.given; +import static sonia.scm.it.utils.TestData.assignPermissions; +import static sonia.scm.it.utils.TestData.cleanup; +import static sonia.scm.it.utils.TestData.createUser; +import static sonia.scm.it.webapp.IntegrationTestUtil.createAdminClient; +import static sonia.scm.it.webapp.IntegrationTestUtil.createResource; +import static sonia.scm.it.webapp.RepositoryITUtil.createRepository; +import static sonia.scm.it.webapp.RepositoryITUtil.setNamespaceStrategy; + +public class NamespacePermissionITCase { + + @Before + public void setUp() { + } + + @Test + public void shouldKeepNamespacePermissionsForExistingNamespace() { + cleanup(); + + // let admin create repository in namespace 'existing' + ScmClient adminApiClient = createAdminClient(); + setNamespaceStrategy(adminApiClient, "CustomNamespaceStrategy"); + + Response response = + createResource(adminApiClient, "repositories") + .accept("*/*") + .post(Entity.entity(""" + { + "contact": "zaphod.beeblebrox@hitchhiker.com", + "description": "Heart of Gold is the first prototype ship to successfully utilise the revolutionary Infinite Improbability Drive", + "namespace": "existing", + "name": "HeartOfGold-git", + "archived": false, + "type": "git" + } + """, MediaType.valueOf(VndMediaType.REPOSITORY))); + + assertNotNull(response); + assertEquals(201, response.getStatus()); + + URI url = URI.create(response.getHeaders().get("Location").get(0).toString()); + response.close(); + + // create user with 'create repositories' permission only + createUser("dent", "dent", false, "xml", "arthur@example.com"); + assignPermissions("dent", "repository:create"); + + // let new user create a new repository in the existing namespace + ScmClient userClient = new ScmClient("dent", "dent"); + createRepository(userClient, """ + { + "contact": "dent@hitchhiker.com", + "description": "I want it all", + "namespace": "existing", + "name": "Earth", + "archived": false, + "type": "git" + } + """, "CustomNamespaceStrategy"); + + // user should not have permissions on namespace, only on new repository + given(VndMediaType.REPOSITORY, "dent", "dent") + + .when() + .get(url) + + .then() + .statusCode(403); + + // user should have no permissions in namespace + String permissionUrl = getNamespacePermissionUrl("existing"); + Map permissions = given() + .when() + .get(permissionUrl) + .then() + .statusCode(200) + .extract() + .body().jsonPath().getList("_embedded.permissions") + .stream() + .collect(Collectors.toMap( + e -> ((Map) e).get("name").toString(), + e -> ((Map) e).get("role").toString() + )); + + assertNull(permissions.get("dent")); + assertEquals("OWNER", permissions.get("scmadmin")); + } + + @Test + public void shouldCreateNamespacePermissionsForNewNamespace() { + cleanup(); + + ScmClient adminApiClient = createAdminClient(); + setNamespaceStrategy(adminApiClient, "CustomNamespaceStrategy"); + + // create user with 'create repositories' permission only + createUser("dent", "dent", false, "xml", "arthur@example.com"); + assignPermissions("dent", "repository:create"); + + // let new user create a new repository in new namespace + ScmClient userClient = new ScmClient("dent", "dent"); + createRepository(userClient, """ + { + "contact": "dent@hitchhiker.com", + "description": "I want it all", + "namespace": "new", + "name": "Earth", + "archived": false, + "type": "git" + } + """, "CustomNamespaceStrategy"); + + // user should have OWNER permissions in namespace + String permissionUrl = getNamespacePermissionUrl("new"); + Map permissions = given() + .when() + .get(permissionUrl) + .then() + .statusCode(200) + .extract() + .body().jsonPath().getList("_embedded.permissions") + .stream() + .collect(Collectors.toMap( + e -> ((Map) e).get("name").toString(), + e -> ((Map) e).get("role").toString() + )); + + assertEquals("OWNER", permissions.get("dent")); + } + + private String getNamespacePermissionUrl(String namespace) { + List> namespaces = given(VndMediaType.NAMESPACE_COLLECTION) + .when() + .get(createResourceUrl("namespaces")) + .then() + .statusCode(HttpStatus.SC_OK) + .extract() + .body().jsonPath().getList("_embedded.namespaces"); + return ( + (Map) ( + (Map) namespaces + .stream() + .filter(m -> m.get("namespace").equals(namespace)) + .findFirst() + .orElseThrow(() -> new AssertionFailedError("namespace not found: " + namespace)) + .get("_links")) + .get("permissions")) + .get("href").toString(); + } +} diff --git a/scm-it/src/test/java/sonia/scm/it/utils/TestData.java b/scm-it/src/test/java/sonia/scm/it/utils/TestData.java index 3bd789c596..ec3b798574 100644 --- a/scm-it/src/test/java/sonia/scm/it/utils/TestData.java +++ b/scm-it/src/test/java/sonia/scm/it/utils/TestData.java @@ -44,6 +44,7 @@ import java.util.Map; import java.util.stream.Collectors; import static java.util.Arrays.asList; +import static java.util.Arrays.stream; import static sonia.scm.it.utils.RestUtil.createResourceUrl; import static sonia.scm.it.utils.RestUtil.given; import static sonia.scm.it.utils.RestUtil.givenAnonymous; @@ -113,9 +114,14 @@ public class TestData { public static void assignAdminPermissions(String username) { LOG.info("assign admin permissions to user {}", username); + assignPermissions(username, "*"); + } + + public static void assignPermissions(String username, String... permissions) { + String permissionString = stream(permissions).map(permission -> '"' + permission + '"').collect(Collectors.joining(",")); given(VndMediaType.PERMISSION_COLLECTION) .when() - .body("{'permissions': ['*']}".replaceAll("'", "\"")) + .body("{\"permissions\": [" + permissionString + "]}") .put(getPermissionUrl(username)) .then() .statusCode(HttpStatus.SC_NO_CONTENT); diff --git a/scm-it/src/test/java/sonia/scm/it/webapp/RepositoryITUtil.java b/scm-it/src/test/java/sonia/scm/it/webapp/RepositoryITUtil.java index 822d9ecf8c..acb32b33d0 100644 --- a/scm-it/src/test/java/sonia/scm/it/webapp/RepositoryITUtil.java +++ b/scm-it/src/test/java/sonia/scm/it/webapp/RepositoryITUtil.java @@ -39,6 +39,7 @@ import java.net.URI; import static org.junit.Assert.assertNotNull; import static sonia.scm.it.webapp.ConfigUtil.readConfig; import static sonia.scm.it.webapp.IntegrationTestUtil.BASE_URL; +import static sonia.scm.it.webapp.IntegrationTestUtil.createAdminClient; import static sonia.scm.it.webapp.IntegrationTestUtil.createResource; import static sonia.scm.it.webapp.IntegrationTestUtil.getLink; @@ -56,7 +57,11 @@ public final class RepositoryITUtil } public static RepositoryDto createRepository(ScmClient client, String repositoryJson) { - setNamespaceStrategy(client, "UsernameNamespaceStrategy"); + return createRepository(client, repositoryJson, "UsernameNamespaceStrategy"); + } + + public static RepositoryDto createRepository(ScmClient client, String repositoryJson, String namespaceStrategy) { + setNamespaceStrategy(createAdminClient(), namespaceStrategy); Response response = createResource(client, "repositories") diff --git a/scm-webapp/src/main/java/sonia/scm/repository/DefaultNamespaceManager.java b/scm-webapp/src/main/java/sonia/scm/repository/DefaultNamespaceManager.java index 940451b947..3a341da302 100644 --- a/scm-webapp/src/main/java/sonia/scm/repository/DefaultNamespaceManager.java +++ b/scm-webapp/src/main/java/sonia/scm/repository/DefaultNamespaceManager.java @@ -28,6 +28,8 @@ import com.github.legman.Subscribe; import jakarta.inject.Inject; import jakarta.inject.Singleton; import org.apache.shiro.SecurityUtils; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import sonia.scm.HandlerEventType; import sonia.scm.event.ScmEventBus; import sonia.scm.web.security.AdministrationContext; @@ -35,13 +37,13 @@ import sonia.scm.web.security.AdministrationContext; import java.util.Collection; import java.util.Optional; -import static java.util.Collections.singletonList; import static sonia.scm.ContextEntry.ContextBuilder.entity; import static sonia.scm.NotFoundException.notFound; @Singleton public class DefaultNamespaceManager implements NamespaceManager { + private static final Logger log = LoggerFactory.getLogger(DefaultNamespaceManager.class); private final RepositoryManager repositoryManager; private final NamespaceDao dao; private final ScmEventBus eventBus; @@ -99,23 +101,29 @@ public class DefaultNamespaceManager implements NamespaceManager { } private void cleanUpNamespaceIfEmpty(RepositoryEvent repositoryEvent) { - Collection allNamespaces = repositoryManager.getAllNamespaces(); String oldNamespace = getOldNamespace(repositoryEvent); - if (!allNamespaces.contains(oldNamespace)) { - dao.delete(oldNamespace); - } + log.debug("checking whether to delete namespace {} after deletion of repository", oldNamespace); + administrationContext.runAsAdmin(() -> { + Collection allNamespaces = repositoryManager.getAllNamespaces(); + if (!allNamespaces.contains(oldNamespace)) { + log.debug("deleting configuration for namespace {} after deletion of repository", oldNamespace); + dao.delete(oldNamespace); + } + }); } private void initializeIfNeeded(RepositoryEvent repositoryEvent) { - Namespace namespace = createNamespaceForName(repositoryEvent.getItem().getNamespace()); - if (repositoryManager.getAll(r -> r.getNamespace().equals(namespace.getNamespace())).size() == 1) { - String creatingUser = SecurityUtils.getSubject().getPrincipal().toString(); - administrationContext.runAsAdmin(() -> { - namespace.setPermissions(singletonList(new RepositoryPermission(creatingUser, "OWNER", false))); - modify(namespace); - } - ); - } + String creatingUser = SecurityUtils.getSubject().getPrincipal().toString(); + String namespace = repositoryEvent.getItem().getNamespace(); + log.debug("checking whether to set OWNER permissions for user {} in namespace {} after creation of repository", creatingUser, namespace); + administrationContext.runAsAdmin(() -> { + Namespace namespaceInstance = createNamespaceForName(namespace); + if (repositoryManager.getAll(r -> r.getNamespace().equals(namespaceInstance.getNamespace())).size() == 1) { + log.debug("setting OWNER permissions for user {} in namespace {} after creation of repository", creatingUser, namespace); + namespaceInstance.addPermission(new RepositoryPermission(creatingUser, "OWNER", false)); + modify(namespaceInstance); + } + }); } public boolean repositoryRemovedFromNamespace(RepositoryEvent repositoryEvent) { diff --git a/scm-webapp/src/test/java/sonia/scm/repository/DefaultNamespaceManagerTest.java b/scm-webapp/src/test/java/sonia/scm/repository/DefaultNamespaceManagerTest.java index bce4a1c49a..8dc8b19265 100644 --- a/scm-webapp/src/test/java/sonia/scm/repository/DefaultNamespaceManagerTest.java +++ b/scm-webapp/src/test/java/sonia/scm/repository/DefaultNamespaceManagerTest.java @@ -113,46 +113,53 @@ class DefaultNamespaceManagerTest { assertThat(namespace).isEmpty(); } - @Test - void shouldCleanUpPermissionWhenLastRepositoryOfNamespaceWasDeleted() { - when(repositoryManager.getAllNamespaces()).thenReturn(asList("universe", "rest")); + @Nested + class WithAdminContext { + @BeforeEach + void mockAdminContext() { + doAnswer(invocation -> { + invocation.getArgument(0, Runnable.class).run(); + return null; + }).when(administrationContext).runAsAdmin(any(PrivilegedAction.class)); + } - manager.handleRepositoryEvent(new RepositoryEvent(DELETE, new Repository("1", "git", "life", "earth"))); + @Test + void shouldCleanUpPermissionWhenLastRepositoryOfNamespaceWasDeleted() { + when(repositoryManager.getAllNamespaces()).thenReturn(asList("universe", "rest")); - assertThat(dao.get("life")).isEmpty(); - } + manager.handleRepositoryEvent(new RepositoryEvent(DELETE, new Repository("1", "git", "life", "earth"))); - @Test - void shouldCleanUpPermissionWhenLastRepositoryOfNamespaceWasRenamed() { - when(repositoryManager.getAllNamespaces()).thenReturn(asList("universe", "rest", "highway")); + assertThat(dao.get("life")).isEmpty(); + } - manager.handleRepositoryEvent( - new RepositoryModificationEvent( - MODIFY, - new Repository("1", "git", "highway", "earth"), - new Repository("1", "git", "life", "earth"))); + @Test + void shouldCleanUpPermissionWhenLastRepositoryOfNamespaceWasRenamed() { + when(repositoryManager.getAllNamespaces()).thenReturn(asList("universe", "rest", "highway")); - assertThat(dao.get("life")).isEmpty(); - } + manager.handleRepositoryEvent( + new RepositoryModificationEvent( + MODIFY, + new Repository("1", "git", "highway", "earth"), + new Repository("1", "git", "life", "earth"))); - @Test - void shouldCreateOwnerPermissionWhenFirstRepositoryOfNamespaceWasCreated() { - when(subject.getPrincipal()).thenReturn("trillian"); - when(repositoryManager.getAllNamespaces()).thenReturn(asList("rest", "highway", "universe")); - when(repositoryManager.getAll(any())) - .thenAnswer(invocation -> Stream.of(new Repository("1", "git", "universe", "earth")).filter(invocation.getArgument(0, Predicate.class)).toList()); - doAnswer(invocation -> { - invocation.getArgument(0, Runnable.class).run(); - return null; - }).when(administrationContext).runAsAdmin(any(PrivilegedAction.class)); - manager.handleRepositoryEvent( - new RepositoryModificationEvent( - CREATE, - new Repository("1", "git", "universe", "earth"), - null)); + assertThat(dao.get("life")).isEmpty(); + } - assertThat(dao.get("universe")).isNotEmpty(); - assertThat(dao.get("universe").get().getPermissions()).extracting("name").contains("trillian"); + @Test + void shouldCreateOwnerPermissionWhenFirstRepositoryOfNamespaceWasCreated() { + when(subject.getPrincipal()).thenReturn("trillian"); + when(repositoryManager.getAllNamespaces()).thenReturn(asList("rest", "highway", "universe")); + when(repositoryManager.getAll(any())) + .thenAnswer(invocation -> Stream.of(new Repository("1", "git", "universe", "earth")).filter(invocation.getArgument(0, Predicate.class)).toList()); + manager.handleRepositoryEvent( + new RepositoryModificationEvent( + CREATE, + new Repository("1", "git", "universe", "earth"), + null)); + + assertThat(dao.get("universe")).isNotEmpty(); + assertThat(dao.get("universe").get().getPermissions()).extracting("name").contains("trillian"); + } } @Nested