diff --git a/doc/todo.md b/doc/todo.md index f71d4d24..3c92fd2e 100644 --- a/doc/todo.md +++ b/doc/todo.md @@ -69,6 +69,13 @@ when a migration plan is written. - On a case-by-case basis, Horde\ActiveSync also owns default implementations as makes sense. These are marked final and reusable code is exposed as traits. - SQL, Mongo or other backends are owned by Horde\Core - Library owns a generic AuthBackendInterface, Core owns an implementation which ties into Horde Auth. +- Certificate validation becomes a validator interface owned by + Horde\ActiveSync; Core owns the implementation and its configuration. + Today `Request_ValidateCert` performs the OpenSSL purpose/trust checks + (and the CRL/chain TODOs) inline in the request handler, which is the + wrong layer for that concern (review note on + [horde/ActiveSync#74](https://github.com/horde/ActiveSync/pull/74), + 2026-07-02). - Interaction with orthogonal subsystems happens via PSR Events. ### Protocol and class layout diff --git a/lib/Horde/ActiveSync/Credentials.php b/lib/Horde/ActiveSync/Credentials.php index 46824cae..9e659235 100644 --- a/lib/Horde/ActiveSync/Credentials.php +++ b/lib/Horde/ActiveSync/Credentials.php @@ -98,8 +98,12 @@ protected function _getCredentials() : (!empty($serverVars['REDIRECT_HTTP_AUTHORIZATION']) ? $serverVars['REDIRECT_HTTP_AUTHORIZATION'] : $serverVars['Authorization']); - $hash = base64_decode(str_replace('Basic ', '', $authorization)); - if (strpos($hash, ':') !== false) { + // Strip only a leading, case-insensitive "Basic " scheme token + // (str_replace() would remove the token anywhere in the string), + // and decode strictly so malformed input yields no credentials + // rather than garbage. + $hash = base64_decode(preg_replace('/^\s*Basic\s+/i', '', $authorization), true); + if ($hash !== false && strpos($hash, ':') !== false) { [$user, $pass] = explode(':', $hash, 2); } } else { diff --git a/lib/Horde/ActiveSync/Request/Autodiscover.php b/lib/Horde/ActiveSync/Request/Autodiscover.php index 33cb6f95..f18b6a57 100644 --- a/lib/Horde/ActiveSync/Request/Autodiscover.php +++ b/lib/Horde/ActiveSync/Request/Autodiscover.php @@ -67,8 +67,27 @@ public function handle(?Horde_Controller_Request $request = null) if (empty($values) && empty($username)) { throw new Horde_Exception_AuthenticationFailure('No username provided.'); } elseif (!empty($values)) { - // Override the username; AUTODISCOVER MUST use email address. - $credentials->username = $values[2]['value']; + // Override the username; AUTODISCOVER MUST use the email address. + // Locate the EMailAddress element by tag name instead of relying on + // a fixed offset ($values[2]), which breaks (and can select the + // wrong node) if a client reorders or adds elements. The wire value + // is always an email address per the Autodiscover protocol; mapping + // it to the backend username (email, AD/LDAP, plain username, ...) + // is handled downstream by the driver's getUsernameFromEmail(). + $email = null; + foreach ($values as $value) { + if (!empty($value['tag']) && $value['tag'] == 'EMAILADDRESS' + && isset($value['value'])) { + $email = trim($value['value']); + break; + } + } + if ($email !== null && $email !== '') { + $credentials->username = $email; + } elseif (empty($username)) { + // No EMailAddress element and no username from the auth header. + throw new Horde_Exception_AuthenticationFailure('No username provided.'); + } } if (!$this->_activeSync->authenticate($credentials)) { @@ -107,6 +126,24 @@ public function handle(?Horde_Controller_Request $request = null) */ protected function _handle() {} + /** + * Escape a value for safe inclusion in the XML responses built below. + * + * The interpolated values (email, display name, schemas echoed back from + * the client request, backend host names, etc.) are otherwise concatenated + * straight into the response markup, which both corrupts the XML for values + * containing metacharacters and allows content injection for the + * client-supplied schema values. + * + * @param mixed $value The value to escape. + * + * @return string The XML-escaped value. + */ + protected function _xmlEscape($value) + { + return htmlspecialchars((string) $value, ENT_QUOTES | ENT_XML1, 'UTF-8'); + } + /** * Build the appropriate response string to send back to the client. * @@ -136,17 +173,17 @@ protected function _buildResponseString($properties) return ' - ' . $properties['culture'] . ' + ' . $this->_xmlEscape($properties['culture']) . ' - ' . $properties['display_name'] . ' - ' . $properties['email'] . ' + ' . $this->_xmlEscape($properties['display_name']) . ' + ' . $this->_xmlEscape($properties['email']) . ' MobileSync - ' . $properties['url'] . ' - ' . $properties['url'] . ' + ' . $this->_xmlEscape($properties['url']) . ' + ' . $this->_xmlEscape($properties['url']) . ' @@ -159,9 +196,9 @@ protected function _buildResponseString($properties) } $xml = ' - + - ' . $properties['display_name'] . ' + ' . $this->_xmlEscape($properties['display_name']) . ' email @@ -170,9 +207,9 @@ protected function _buildResponseString($properties) if (!empty($properties['imap'])) { $xml .= ' IMAP - ' . $properties['imap']['host'] . ' - ' . $properties['imap']['port'] . ' - ' . $properties['username'] . ' + ' . $this->_xmlEscape($properties['imap']['host']) . ' + ' . $this->_xmlEscape($properties['imap']['port']) . ' + ' . $this->_xmlEscape($properties['username']) . ' off off ' . $this->_getEncryptionValue('imap', $properties) . ' @@ -182,9 +219,9 @@ protected function _buildResponseString($properties) if (!empty($properties['pop'])) { $xml .= ' POP3 - ' . $properties['pop']['host'] . ' - ' . $properties['pop']['port'] . ' - ' . $properties['username'] . ' + ' . $this->_xmlEscape($properties['pop']['host']) . ' + ' . $this->_xmlEscape($properties['pop']['port']) . ' + ' . $this->_xmlEscape($properties['username']) . ' off off ' . $this->_getEncryptionValue('pop', $properties) . ' @@ -194,9 +231,9 @@ protected function _buildResponseString($properties) if (!empty($properties['smtp'])) { $xml .= ' SMTP - ' . $properties['smtp']['host'] . ' - ' . $properties['smtp']['port'] . ' - ' . $properties['username'] . ' + ' . $this->_xmlEscape($properties['smtp']['host']) . ' + ' . $this->_xmlEscape($properties['smtp']['port']) . ' + ' . $this->_xmlEscape($properties['username']) . ' off off ' . $this->_getEncryptionValue('smtp', $properties) . ' @@ -218,7 +255,7 @@ protected function _buildResponseString($properties) protected function _getEncryptionValue($type, $properties) { if (!empty($properties[$type]['encryption'])) { - return '' . $properties[$type]['encryption'] . ''; + return '' . $this->_xmlEscape($properties[$type]['encryption']) . ''; } // Older version of autodiscover. if (!empty($properties[$type]['ssl'])) { @@ -244,14 +281,14 @@ protected function _buildFailureResponse($email, $status, $response_schema) { return ' - + en:us - ' . $email . ' + ' . $this->_xmlEscape($email) . ' - ' . $status . ' + ' . $this->_xmlEscape($status) . ' Unable to autoconfigure the supplied email address. MailUser diff --git a/lib/Horde/ActiveSync/Request/ValidateCert.php b/lib/Horde/ActiveSync/Request/ValidateCert.php index 2888f4b2..bb1d3a5f 100644 --- a/lib/Horde/ActiveSync/Request/ValidateCert.php +++ b/lib/Horde/ActiveSync/Request/ValidateCert.php @@ -120,16 +120,21 @@ protected function _handle() // Valid purpose/trusted? // @TODO: CRL support, CHAIN support + // openssl_x509_checkpurpose() returns true (valid + trusted for the + // purpose), false (invalid purpose OR untrusted), or -1 on error. + // Check the -1 error case with a strict comparison first, since -1 + // is loosely truthy and was previously masked by a typo ($results) + // that left this branch dead. $result = openssl_x509_checkpurpose($cert_pem, X509_PURPOSE_SMIME_SIGN, [$this->_activeSync->certPath]); - if ($result === false) { + if ($result === -1) { + // Unspecified error. + $cert_status[$key] = self::STATUS_UNKNOWN; + } elseif ($result === false) { // @TODO: // checkpurpose returns false if either the purpose is invalid OR // the certificate is untrusted, so we should validate the // trust before we send back any errors. $cert_status[$key] = self::STATUS_PURPOSE_INVALID; - } elseif ($results == -1) { - // Unspecified error. - $cert_status[$key] = self::STATUS_UNKNOWN; } else { // If checkpurpose passes, it's valid AND trusted. $cert_status[$key] = self::STATUS_SUCCESS;