Security fixes

.env

@@ -49,7 +49,7 @@ AUTH_REALM=SabreDAV
 AUTH_METHOD=Basic
 
 # In case of IMAP Auth, you must specify the url of the mailbox in the following format host[:port].
-IMAP_AUTH_URL=null
+IMAP_AUTH_URL=
 IMAP_ENCRYPTION_METHOD=ssl
 IMAP_CERTIFICATE_VALIDATION=true
 IMAP_AUTH_USER_AUTOCREATE=false

composer.json

@@ -31,6 +31,7 @@
         "symfony/process": "^7.4",
         "symfony/property-access": "^7.4",
         "symfony/property-info": "^7.4",
+        "symfony/rate-limiter": "7.4.*",
         "symfony/runtime": "^7.4",
         "symfony/security-bundle": "^7.4",
         "symfony/serializer": "^7.4",

config/packages/security.yaml

@@ -18,19 +18,24 @@ security:
             custom_authenticators:
                 - App\Security\LoginFormAuthenticator
             provider: admin_user_provider
+            login_throttling:
+                max_attempts: 5
             logout:
                 path: app_logout
                 target: dashboard
                 # Without this, any page the admin visits can log them out with a plain GET
                 enable_csrf: true
 
 
+    # Only the first matching rule applies, so the public endpoints are listed first and the
+    # admin interface is whatever is left. A route added later is protected unless it is added
+    # to this list on purpose.
     access_control:
         - { path: ^/$, roles: PUBLIC_ACCESS }
         - { path: ^/dav, roles: PUBLIC_ACCESS }
-        - { path: ^/dashboard, roles: ROLE_ADMIN, allow_if: "'%env(default:default_admin_auth_bypass:ADMIN_AUTH_BYPASS)%' === 'true'" }
-        - { path: ^/users, roles: ROLE_ADMIN, allow_if: "'%env(default:default_admin_auth_bypass:ADMIN_AUTH_BYPASS)%' === 'true'" }
-        - { path: ^/calendars, roles: ROLE_ADMIN, allow_if: "'%env(default:default_admin_auth_bypass:ADMIN_AUTH_BYPASS)%' === 'true'" }
-        - { path: ^/addressbooks, roles: ROLE_ADMIN, allow_if: "'%env(default:default_admin_auth_bypass:ADMIN_AUTH_BYPASS)%' === 'true'" }
+        - { path: ^/\.well-known/, roles: PUBLIC_ACCESS }
+        - { path: ^/login$, roles: PUBLIC_ACCESS }
+        - { path: ^/logout$, roles: PUBLIC_ACCESS }
         - { path: ^/api/v1/health$, roles: PUBLIC_ACCESS }
         - { path: ^/api, roles: IS_AUTHENTICATED }
+        - { path: ^/, roles: ROLE_ADMIN, allow_if: "'%env(default:default_admin_auth_bypass:ADMIN_AUTH_BYPASS)%' === 'true'" }

config/reference.php

@@ -629,7 +629,7 @@
  *         }>,
  *     },
  *     rate_limiter?: bool|array{ // Rate limiter configuration
- *         enabled?: bool|Param, // Default: false
+ *         enabled?: bool|Param, // Default: true
  *         limiters?: array<string, array{ // Default: []
  *             lock_factory?: scalar|Param|null, // The service ID of the lock factory used by this limiter (or null to disable locking). // Default: "auto"
  *             cache_pool?: scalar|Param|null, // The cache pool to use for storing the current limiter state. // Default: "cache.rate_limiter"

docker/.env

@@ -1,5 +1,6 @@
 # General settings
-APP_ENV=prod # or dev
+# Either prod or dev
+APP_ENV=prod
 
 CALDAV_ENABLED=true
 CARDDAV_ENABLED=true
@@ -26,7 +27,8 @@ ADMIN_PASSWORD=admin
 ADMIN_AUTH_BYPASS=false
 
 # DAV auth settings
-AUTH_METHOD=Basic   # Basic or IMAP or LDAP
+# Either Basic, IMAP or LDAP
+AUTH_METHOD=Basic
 
 # Basic HTTP auth settings
 AUTH_REALM=SabreDAV
@@ -42,7 +44,8 @@ LDAP_AUTH_URL=ldap://127.0.0.1:3890
 LDAP_DN_PATTERN=uid=%u,ou=users,dc=domain,dc=com
 LDAP_MAIL_ATTRIBUTE=mail
 LDAP_AUTH_USER_AUTOCREATE=false
-LDAP_CERTIFICATE_CHECKING_STRATEGY=try # never, hard, demand, try, or allow
+# Either never, hard, demand, try or allow
+LDAP_CERTIFICATE_CHECKING_STRATEGY=try
 
 # WebDAV settings
 WEBDAV_TMP_DIR=/webdav/tmp

src/Entity/AddressBook.php

@@ -23,10 +23,12 @@ class AddressBook
     private $principalUri;
 
     #[ORM\Column(name: 'displayname', type: 'string', length: 255, nullable: true)]
+    #[Assert\Length(max: 255)]
     private $displayName;
 
     #[ORM\Column(type: 'string', length: 255)]
     #[Assert\Regex("/[0-9a-z\-]+/")]
+    #[Assert\Length(max: 255)]
     private $uri;
 
     #[ORM\Column(type: 'text', nullable: true)]

src/Entity/CalendarInstance.php

@@ -37,10 +37,12 @@ public static function getOwnerAccesses(): array
     private $access;
 
     #[ORM\Column(name: 'displayname', type: 'string', length: 255, nullable: true)]
+    #[Assert\Length(max: 255)]
     private $displayName;
 
     #[ORM\Column(type: 'string', length: 255, nullable: true)]
     #[Assert\Regex("/[0-9a-z\-]+/")]
+    #[Assert\Length(max: 255)]
     private $uri;
 
     #[ORM\Column(type: 'text', nullable: true)]
@@ -51,6 +53,7 @@ public static function getOwnerAccesses(): array
 
     #[ORM\Column(name: 'calendarcolor', type: 'string', length: 10, nullable: true)]
     #[Assert\Regex("/\#[0-9A-F]{6}/")]
+    #[Assert\Length(max: 10)]
     private $calendarColor;
 
     #[ORM\Column(type: 'text', nullable: true)]

src/Entity/CalendarObject.php

@@ -88,18 +88,6 @@ public function setCalendar(?Calendar $calendar): self
         return $this;
     }
 
-    public function getLastModifier(): ?int
-    {
-        return $this->lastModifier;
-    }
-
-    public function setLastModifier(?int $lastModifier): self
-    {
-        $this->lastModifier = $lastModifier;
-
-        return $this;
-    }
-
     public function getEtag(): ?string
     {
         return $this->etag;

src/Entity/Principal.php

@@ -24,24 +24,24 @@ class Principal
     private $id;
 
     #[ORM\Column(type: 'string', length: 255, unique: true)]
-    #[Assert\Unique]
     #[Assert\NotBlank]
+    #[Assert\Length(max: 255)]
     private $uri;
 
     #[ORM\Column(type: 'string', length: 255, nullable: true)]
     #[Assert\NotBlank]
     #[Assert\Email(message: "The email '{{ value }}' is not a valid email.")]
+    #[Assert\Length(max: 255)]
     private $email;
 
     #[ORM\Column(name: 'displayname', type: 'string', length: 255, nullable: true)]
+    #[Assert\Length(max: 255)]
     private $displayName;
 
     #[ORM\Column(type: 'boolean')]
-    #[Assert\NotBlank]
     private $isMain;
 
     #[ORM\Column(type: 'boolean')]
-    #[Assert\NotBlank]
     private $isAdmin;
 
     #[ORM\ManyToMany(targetEntity: 'Principal')]

src/Security/LoginFormAuthenticator.php

@@ -7,13 +7,12 @@
 use Symfony\Component\HttpFoundation\Response;
 use Symfony\Component\Routing\Generator\UrlGeneratorInterface;
 use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
-use Symfony\Component\Security\Core\Exception\CustomUserMessageAuthenticationException;
 use Symfony\Component\Security\Csrf\CsrfTokenManagerInterface;
 use Symfony\Component\Security\Http\Authenticator\AbstractLoginFormAuthenticator;
 use Symfony\Component\Security\Http\Authenticator\Passport\Badge\CsrfTokenBadge;
 use Symfony\Component\Security\Http\Authenticator\Passport\Badge\UserBadge;
+use Symfony\Component\Security\Http\Authenticator\Passport\Credentials\CustomCredentials;
 use Symfony\Component\Security\Http\Authenticator\Passport\Passport;
-use Symfony\Component\Security\Http\Authenticator\Passport\SelfValidatingPassport;
 use Symfony\Component\Security\Http\SecurityRequestAttributes;
 use Symfony\Component\Security\Http\Util\TargetPathTrait;
 
@@ -46,31 +45,33 @@ public function supports(Request $request): bool
             && $request->isMethod('POST');
     }
 
+    /**
+     * The credentials are rejected by the passport rather than by throwing from here, because
+     * `login_throttling` only gets to run once a passport exists: throwing earlier means failed
+     * attempts are counted but never blocked.
+     */
     public function authenticate(Request $request): Passport
     {
-        $credentials = [
-            'username' => $request->request->get('_username'),
-            'password' => $request->request->get('_password'),
-            'csrf_token' => $request->request->get('_csrf_token'),
-        ];
-        $request->getSession()->set(
-            SecurityRequestAttributes::LAST_USERNAME,
-            $credentials['username']
-        );
+        $username = $request->request->getString('_username');
+        $password = $request->request->getString('_password');
 
-        if ($credentials['username'] !== $this->adminLogin) {
-            // fail authentication with a custom error
-            throw new CustomUserMessageAuthenticationException('Username could not be found.');
-        }
+        $request->getSession()->set(SecurityRequestAttributes::LAST_USERNAME, $username);
 
-        if ($credentials['password'] !== $this->adminPassword) {
-            // fail authentication with a custom error
-            throw new CustomUserMessageAuthenticationException('Invalid credentials.');
-        }
+        return new Passport(
+            new UserBadge($username),
+            new CustomCredentials(
+                function (string $presentedPassword) use ($username): bool {
+                    // Both halves are compared, and both in constant time, so the response says
+                    // nothing about which one was wrong — an unknown name and a wrong password
+                    // fail identically with "Invalid credentials.".
+                    $loginMatches = hash_equals($this->adminLogin, $username);
+                    $passwordMatches = hash_equals($this->adminPassword, $presentedPassword);
 
-        return new SelfValidatingPassport(
-            new UserBadge($this->adminLogin),
-            [new CsrfTokenBadge('authenticate', $credentials['csrf_token'])]
+                    return $loginMatches && $passwordMatches;
+                },
+                $password
+            ),
+            [new CsrfTokenBadge('authenticate', $request->request->getString('_csrf_token'))]
         );
     }
 

src/Services/IMAPAuth.php

@@ -61,24 +61,27 @@ final class IMAPAuth extends AbstractAuth
 
     public function __construct(ManagerRegistry $doctrine, Utils $utils, string $IMAPAuthUrl, bool $autoCreate, string $IMAPEncryptionMethod, bool $IMAPCertificateValidation)
     {
-        $components = parse_url($IMAPAuthUrl);
-
-        if (!$components) {
-            throw new Exception('IMAP Error (parsing IMAP url "'.$IMAPAuthUrl.'"): '.$e->getMessage());
+        // `.env` shipped `IMAP_AUTH_URL=null` as a placeholder, and a dotenv file has no null
+        // literal, so it arrives as the four-letter string. Left alone it would be taken for a
+        // hostname and every login would fail on name resolution instead of saying it is unset.
+        if ('null' === strtolower(trim($IMAPAuthUrl))) {
+            $IMAPAuthUrl = '';
         }
 
-        $this->IMAPHost = $components['host'] ?? null;
+        // IMAP_AUTH_URL is documented as `host:port`, but parse_url() only reads a host when the
+        // value looks like an authority: a bare `imap.example.com` lands in `path` instead. The
+        // value is reduced to that authority — leading slashes off, scheme off — and then given
+        // the `//` that makes parse_url read it as a host. The scheme carries no meaning here
+        // anyway: encryption comes from IMAP_ENCRYPTION_METHOD.
+        $authority = preg_replace('~^[a-z][a-z0-9+.-]*://~i', '', ltrim($IMAPAuthUrl, '/'));
+        $components = '' === $IMAPAuthUrl ? [] : parse_url('//'.$authority);
 
-        // Trying to choose the best port if it was not provided,
-        // defaulting to 993 (secure)
-        if (isset($components['port'])) {
-            $this->IMAPPort = $components['port'];
-        } elseif (false === $this->IMAPEncryptionMethod) {
-            $this->IMAPPort = 143;
-        } else {
-            $this->IMAPPort = 993;
+        if (false === $components) {
+            throw new \RuntimeException('IMAP_AUTH_URL could not be parsed: "'.$IMAPAuthUrl.'". Expected something like "imap.example.com:993".');
         }
 
+        $this->IMAPHost = $components['host'] ?? null;
+
         // We're making sure that only ssl, tls or 'false' are passed down to the IMAP client,
         // defaulting to SSL
         $IMAPEncryptionMethodCleaned = strtolower($IMAPEncryptionMethod);
@@ -89,6 +92,17 @@ public function __construct(ManagerRegistry $doctrine, Utils $utils, string $IMA
         } else {
             $this->IMAPEncryptionMethod = 'ssl';
         }
+
+        // Trying to choose the best port if it was not provided,
+        // defaulting to 993 (secure)
+        if (isset($components['port'])) {
+            $this->IMAPPort = $components['port'];
+        } elseif (false === $this->IMAPEncryptionMethod) {
+            $this->IMAPPort = 143;
+        } else {
+            $this->IMAPPort = 993;
+        }
+
         $this->IMAPCertificateValidation = $IMAPCertificateValidation;
 
         $this->autoCreate = $autoCreate;
@@ -103,6 +117,14 @@ public function __construct(ManagerRegistry $doctrine, Utils $utils, string $IMA
      */
     protected function imapOpen(string $username, string $password): bool
     {
+        // Reported here rather than from the constructor: this backend is a constructor argument of
+        // the DAV controller, so it is built on every request even when AUTH_METHOD is not IMAP.
+        if (!$this->IMAPHost) {
+            error_log('IMAP Error (configuration): IMAP_AUTH_URL has no host, expected something like "imap.example.com:993".');
+
+            return false;
+        }
+
         $cm = new ClientManager($options = []);
 
         // Create a new instance of the IMAP client manually