diff --git a/conf/default.conf.php b/conf/default.conf.php index 3f6c4078d7..093301a991 100644 --- a/conf/default.conf.php +++ b/conf/default.conf.php @@ -648,7 +648,7 @@ return array( 'ldap.search-first' => false, // The attribute to search for if you have to search for a user - 'ldap.username_attribute' => '', + 'ldap.username-attribute' => '', // The attribute(s) to be regarded as 'real name'. // If more then one attribute is supplied the values of the attributes in diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index c4bc3e10e4..424781ef56 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -710,6 +710,7 @@ phutil_register_library_map(array( 'PhabricatorLDAPLoginController' => 'applications/auth/controller/PhabricatorLDAPLoginController.php', 'PhabricatorLDAPProvider' => 'applications/auth/ldap/PhabricatorLDAPProvider.php', 'PhabricatorLDAPRegistrationController' => 'applications/auth/controller/PhabricatorLDAPRegistrationController.php', + 'PhabricatorLDAPUnknownUserException' => 'applications/auth/ldap/PhabricatorLDAPUnknownUserException.php', 'PhabricatorLDAPUnlinkController' => 'applications/auth/controller/PhabricatorLDAPUnlinkController.php', 'PhabricatorLintEngine' => 'infrastructure/lint/PhabricatorLintEngine.php', 'PhabricatorLiskDAO' => 'infrastructure/storage/lisk/PhabricatorLiskDAO.php', @@ -1727,6 +1728,7 @@ phutil_register_library_map(array( 'PhabricatorJavelinLinter' => 'ArcanistLinter', 'PhabricatorLDAPLoginController' => 'PhabricatorAuthController', 'PhabricatorLDAPRegistrationController' => 'PhabricatorAuthController', + 'PhabricatorLDAPUnknownUserException' => 'Exception', 'PhabricatorLDAPUnlinkController' => 'PhabricatorAuthController', 'PhabricatorLintEngine' => 'PhutilLintEngine', 'PhabricatorLiskDAO' => 'LiskDAO', diff --git a/src/applications/auth/ldap/PhabricatorLDAPProvider.php b/src/applications/auth/ldap/PhabricatorLDAPProvider.php index fa1fcda724..f203af44a1 100644 --- a/src/applications/auth/ldap/PhabricatorLDAPProvider.php +++ b/src/applications/auth/ldap/PhabricatorLDAPProvider.php @@ -17,6 +17,10 @@ */ final class PhabricatorLDAPProvider { + // http://www.php.net/manual/en/function.ldap-errno.php#20665 states + // that the number could be 31 or 49, in testing it has always been 49 + const LDAP_INVALID_CREDENTIALS = 49; + private $userData; private $connection; @@ -47,7 +51,7 @@ final class PhabricatorLDAPProvider { } public function getUsernameAttribute() { - return PhabricatorEnv::getEnvConfig('ldap.username_attribute'); + return PhabricatorEnv::getEnvConfig('ldap.username-attribute'); } public function getLDAPVersion() { @@ -116,14 +120,28 @@ final class PhabricatorLDAPProvider { return $this->userData; } + private function invalidLDAPUserErrorMessage($errno, $errmsg) { + return "LDAP Error #".$errno.": ".$errmsg; + } + public function auth($username, PhutilOpaqueEnvelope $password) { if (strlen(trim($username)) == 0) { throw new Exception('Username can not be empty'); } if (PhabricatorEnv::getEnvConfig('ldap.search-first')) { - $user = $this->getUser($this->getUsernameAttribute(), $username); - $username = $user[($this->getSearchAttribute())][0]; + // To protect against people phishing for accounts we catch the + // exception and present the default exception that would be presented + // in the case of a failed bind. + try { + $user = $this->getUser($this->getUsernameAttribute(), $username); + $username = $user[$this->getSearchAttribute()][0]; + } catch (PhabricatorLDAPUnknownUserException $e) { + throw new Exception( + $this->invalidLDAPUserErrorMessage( + self::LDAP_INVALID_CREDENTIALS, + ldap_err2str(self::LDAP_INVALID_CREDENTIALS))); + } } $conn = $this->getConnection(); @@ -149,7 +167,9 @@ final class PhabricatorLDAPProvider { if (!$result) { throw new Exception( - "LDAP Error #".ldap_errno($conn).": ".ldap_error($conn)); + $this->invalidLDAPUserErrorMessage( + ldap_errno($conn), + ldap_error($conn))); } $this->userData = $this->getUser($this->getSearchAttribute(), $username); @@ -183,7 +203,7 @@ final class PhabricatorLDAPProvider { } if ($entries['count'] == 0) { - throw new Exception('Could not find user'); + throw new PhabricatorLDAPUnknownUserException('Could not find user'); } return $entries[0]; diff --git a/src/applications/auth/ldap/PhabricatorLDAPUnknownUserException.php b/src/applications/auth/ldap/PhabricatorLDAPUnknownUserException.php new file mode 100644 index 0000000000..6994025938 --- /dev/null +++ b/src/applications/auth/ldap/PhabricatorLDAPUnknownUserException.php @@ -0,0 +1,20 @@ +