From 03ce9fe879d6f127f9f83ddd1233f9310d5e401e Mon Sep 17 00:00:00 2001 From: FreddleSpl0it <75116288+FreddleSpl0it@users.noreply.github.com> Date: Mon, 9 Nov 2026 12:19:07 +0100 Subject: [PATCH] [Web] Mail policies, groups: check domain access first --- .../MailPolicy/Service/PolicyService.php | 9 ++++---- .../MailboxGroup/Service/GroupService.php | 22 ++++++++++++++----- .../MailPolicy/Service/PolicyServiceTest.php | 4 +++- .../MailboxGroup/Service/GroupServiceTest.php | 9 ++++++++ 4 files changed, 33 insertions(+), 11 deletions(-) diff --git a/data/web/inc/lib/mailcow/MailPolicy/Service/PolicyService.php b/data/web/inc/lib/mailcow/MailPolicy/Service/PolicyService.php index 3ffafc6e9..3d74e68ae 100644 --- a/data/web/inc/lib/mailcow/MailPolicy/Service/PolicyService.php +++ b/data/web/inc/lib/mailcow/MailPolicy/Service/PolicyService.php @@ -168,13 +168,14 @@ class PolicyService /** @throws UserError */ private function domainFor(mixed $value): string { - $domain = strtolower((string)Input::text($value)); + $domain = Input::text($value); + $domain = ($domain === null || $domain === '') ? '' : (string)idn_to_ascii(strtolower($domain), 0, INTL_IDNA_VARIANT_UTS46); + if ($domain !== '' && !$this->context->hasAdminRights() && !$this->context->canAccessDomain($domain)) { + throw new AccessDenied(); + } if ($domain === '' || !$this->repo->isDomain($domain)) { throw new UserError('domain_not_found', [htmlspecialchars((string)Input::text($value))]); } - if (!$this->context->hasAdminRights() && !$this->context->canAccessDomain($domain)) { - throw new AccessDenied(); - } return $domain; } diff --git a/data/web/inc/lib/mailcow/MailboxGroup/Service/GroupService.php b/data/web/inc/lib/mailcow/MailboxGroup/Service/GroupService.php index 9fdf0aa9f..7875dab59 100644 --- a/data/web/inc/lib/mailcow/MailboxGroup/Service/GroupService.php +++ b/data/web/inc/lib/mailcow/MailboxGroup/Service/GroupService.php @@ -195,12 +195,12 @@ class GroupService { $domain = Input::text($value); $domain = ($domain === null || $domain === '') ? '' : (string)idn_to_ascii(strtolower($domain), 0, INTL_IDNA_VARIANT_UTS46); + if ($domain !== '' && !$this->context->hasAdminRights() && !$this->context->canAccessDomain($domain)) { + throw new AccessDenied(); + } if ($domain === '' || !$this->lookup->isDomain($domain)) { throw new UserError('domain_not_found', [htmlspecialchars((string)Input::text($value))]); } - if (!$this->context->hasAdminRights() && !$this->context->canAccessDomain($domain)) { - throw new AccessDenied(); - } return $domain; } @@ -304,6 +304,9 @@ class GroupService $new = array_values(array_diff(array_unique($add), array_map('strval', array_keys($members)))); $regular = $this->lookup->regularMailboxes($new); foreach ($new as $username) { + if ($this->foreignDomain($username)) { + throw new AccessDenied(); + } if (!in_array($username, $regular, true)) { throw new UserError('group_member_invalid', [htmlspecialchars($username)]); } @@ -337,12 +340,12 @@ class GroupService // The group's own address is covered by sender_allowed $valid = ($address !== null && $address !== $group->address && $this->lookup->aliasExists($address)); } + if ($this->foreignDomain($address)) { + throw new AccessDenied(); + } if (!$valid) { throw new UserError('group_send_as_invalid', [htmlspecialchars($value)]); } - if (!$this->context->hasAdminRights() && !$this->context->canAccessDomain(self::domainOf((string)$address))) { - throw new AccessDenied(); - } $sendAs[] = (string)$address; } $sendAs = array_values(array_unique($sendAs)); @@ -505,4 +508,11 @@ class GroupService { return substr($address, strrpos($address, '@') + 1); } + + /** A domain admin names an address or @domain outside their domains: denied before anything is looked up */ + private function foreignDomain(?string $address): bool + { + $domain = ($address !== null && str_contains($address, '@')) ? self::domainOf($address) : ''; + return $domain !== '' && !$this->context->hasAdminRights() && !$this->context->canAccessDomain($domain); + } } diff --git a/tests/php/MailPolicy/Service/PolicyServiceTest.php b/tests/php/MailPolicy/Service/PolicyServiceTest.php index e870b8eb2..f15f71020 100644 --- a/tests/php/MailPolicy/Service/PolicyServiceTest.php +++ b/tests/php/MailPolicy/Service/PolicyServiceTest.php @@ -28,7 +28,7 @@ class PolicyServiceTest extends TestCase protected function setUp(): void { $this->repo = $this->createMock(PolicyRepository::class); - $this->repo->method('isDomain')->willReturnCallback(fn(string $domain): bool => in_array($domain, ['firma.de', 'tochter.de'], true)); + $this->repo->method('isDomain')->willReturnCallback(fn(string $domain): bool => in_array($domain, ['firma.de', 'tochter.de', 'xn--mller-kva.de'], true)); $this->repo->method('mailboxDomain')->willReturnCallback(fn(string $username): ?string => ['azubi@firma.de' => 'firma.de', 'chef@firma.de' => 'firma.de', 'andere@tochter.de' => 'tochter.de'][$username] ?? null); $this->repo->method('groupDomain')->willReturnCallback(function (int $id): ?string { @@ -91,6 +91,7 @@ class PolicyServiceTest extends TestCase ], array_intersect_key($policy->toArray(), array_flip(['domain', 'name', 'description', 'send_mode', 'send_allow', 'send_block', 'receive_mode', 'receive_allow', 'receive_block', 'reject_text', 'active', 'mailboxes', 'groups']))); $this->assertCount(1, $this->saved); + $this->assertSame('xn--mller-kva.de', $this->service()->create(['domain' => 'Müller.de', 'name' => 'Test'])->domain); } public function testUpdateKeepsMissingValuesAndReplacesListGroups(): void @@ -153,6 +154,7 @@ class PolicyServiceTest extends TestCase $this->assertNotNull($this->service($da)->get(3)); // other domains, no ACL, users and read-only keys $this->expectUserError('access_denied', fn() => $this->service($da)->create(['domain' => 'tochter.de', 'name' => 'Test'])); + $this->expectUserError('access_denied', fn() => $this->service($da)->create(['domain' => 'unbekannt.de', 'name' => 'Test'])); $this->expectUserError(['mail_policy_entry_invalid', 'group:9'], fn() => $this->service($da)->create(['domain' => 'firma.de', 'name' => 'Test', 'send_block_groups' => ['9']])); $this->assertNull($this->service(FakeContext::domainAdmin([], ['tochter.de'], ['mail_policies' => '1']))->get(3)); diff --git a/tests/php/MailboxGroup/Service/GroupServiceTest.php b/tests/php/MailboxGroup/Service/GroupServiceTest.php index a9816989e..76c845aeb 100644 --- a/tests/php/MailboxGroup/Service/GroupServiceTest.php +++ b/tests/php/MailboxGroup/Service/GroupServiceTest.php @@ -184,6 +184,15 @@ class GroupServiceTest extends TestCase $this->expectUserError('access_denied', fn() => $service->create(['domain' => 'other.org', 'name' => 'X'])); $this->expectUserError('access_denied', fn() => $service->create(['domain' => 'example.org', 'name' => 'X', 'members' => ['c@other.org']])); $this->expectUserError('access_denied', fn() => $service->create(['domain' => 'example.org', 'name' => 'X', 'send_as' => '@other.org'])); + // unknown names outside their domains look the same as existing ones + $this->expectUserError('access_denied', fn() => $service->create(['domain' => 'nope.org', 'name' => 'X'])); + $this->expectUserError('access_denied', fn() => $service->create(['domain' => 'example.org', 'name' => 'X', 'members' => ['room1@other.org']])); + $this->expectUserError('access_denied', fn() => $service->create(['domain' => 'example.org', 'name' => 'X', 'send_as' => '@nope.org'])); + $this->expectUserError('access_denied', fn() => $service->create(['domain' => 'example.org', 'name' => 'X', 'send_as' => 'nobody@other.org'])); + $this->expectUserError(['group_member_invalid', 'room1@example.org'], + fn() => $service->create(['domain' => 'example.org', 'name' => 'X', 'members' => ['room1@example.org']])); + $this->expectUserError(['group_send_as_invalid', 'nobody@example.org'], + fn() => $service->create(['domain' => 'example.org', 'name' => 'X', 'send_as' => 'nobody@example.org'])); } public function testAdminFieldsAreIgnoredForDomainAdmins(): void