From 26ec1cc3dcd4ebc205ab7746cc4a92603f37ea97 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Thu, 20 Dec 2018 20:04:09 +0000 Subject: [PATCH] Added proper escaping to LDAP filter operations To cover #1163 --- app/Auth/Access/Ldap.php | 23 +++++++++++++++++++++++ app/Auth/Access/LdapService.php | 19 ++++++++++++------- tests/Auth/LdapTest.php | 27 ++++++++++++++++++++++++++- 3 files changed, 61 insertions(+), 8 deletions(-) diff --git a/app/Auth/Access/Ldap.php b/app/Auth/Access/Ldap.php index 468c37626..843a2f204 100644 --- a/app/Auth/Access/Ldap.php +++ b/app/Auth/Access/Ldap.php @@ -92,4 +92,27 @@ class Ldap { return ldap_bind($ldapConnection, $bindRdn, $bindPassword); } + + /** + * Explode a LDAP dn string into an array of components. + * @param string $dn + * @param int $withAttrib + * @return array + */ + public function explodeDn(string $dn, int $withAttrib) + { + return ldap_explode_dn($dn, $withAttrib); + } + + /** + * Escape a string for use in an LDAP filter. + * @param string $value + * @param string $ignore + * @param int $flags + * @return string + */ + public function escape(string $value, string $ignore = "", int $flags = 0) + { + return ldap_escape($value, $ignore, $flags); + } } diff --git a/app/Auth/Access/LdapService.php b/app/Auth/Access/LdapService.php index d3a177f8e..b49ecf129 100644 --- a/app/Auth/Access/LdapService.php +++ b/app/Auth/Access/LdapService.php @@ -107,6 +107,7 @@ class LdapService if ($ldapUser === null) { return false; } + if ($ldapUser['uid'] !== $user->external_auth_id) { return false; } @@ -195,7 +196,7 @@ class LdapService $newAttrs = []; foreach ($attrs as $key => $attrText) { $newKey = '${' . $key . '}'; - $newAttrs[$newKey] = $attrText; + $newAttrs[$newKey] = $this->ldap->escape($attrText); } return strtr($filterString, $newAttrs); } @@ -265,7 +266,8 @@ class LdapService $baseDn = $this->config['base_dn']; $groupsAttr = strtolower($this->config['group_attribute']); - $groups = $this->ldap->searchAndGetEntries($ldapConnection, $baseDn, 'CN='.$groupName, [$groupsAttr]); + $groupFilter = 'CN=' . $this->ldap->escape($groupName); + $groups = $this->ldap->searchAndGetEntries($ldapConnection, $baseDn, $groupFilter, [$groupsAttr]); if ($groups['count'] === 0) { return []; } @@ -277,23 +279,26 @@ class LdapService /** * Filter out LDAP CN and DN language in a ldap search return * Gets the base CN (common name) of the string - * @param string $ldapSearchReturn + * @param array $userGroupSearchResponse * @return array */ - protected function groupFilter($ldapSearchReturn) + protected function groupFilter(array $userGroupSearchResponse) { $groupsAttr = strtolower($this->config['group_attribute']); $ldapGroups = []; $count = 0; - if (isset($ldapSearchReturn[$groupsAttr]['count'])) { - $count = (int) $ldapSearchReturn[$groupsAttr]['count']; + + if (isset($userGroupSearchResponse[$groupsAttr]['count'])) { + $count = (int) $userGroupSearchResponse[$groupsAttr]['count']; } + for ($i=0; $i<$count; $i++) { - $dnComponents = ldap_explode_dn($ldapSearchReturn[$groupsAttr][$i], 1); + $dnComponents = $this->ldap->explodeDn($userGroupSearchResponse[$groupsAttr][$i], 1); if (!in_array($dnComponents[0], $ldapGroups)) { $ldapGroups[] = $dnComponents[0]; } } + return $ldapGroups; } diff --git a/tests/Auth/LdapTest.php b/tests/Auth/LdapTest.php index 25d8a5906..16ba11358 100644 --- a/tests/Auth/LdapTest.php +++ b/tests/Auth/LdapTest.php @@ -31,6 +31,20 @@ class LdapTest extends BrowserKitTest $this->mockUser = factory(User::class)->make(); } + protected function mockEscapes($times = 1) + { + $this->mockLdap->shouldReceive('escape')->times($times)->andReturnUsing(function($val) { + return ldap_escape($val); + }); + } + + protected function mockExplodes($times = 1) + { + $this->mockLdap->shouldReceive('explodeDn')->times($times)->andReturnUsing(function($dn, $withAttrib) { + return ldap_explode_dn($dn, $withAttrib); + }); + } + public function test_login() { $this->mockLdap->shouldReceive('connect')->once()->andReturn($this->resourceId); @@ -44,6 +58,7 @@ class LdapTest extends BrowserKitTest 'dn' => ['dc=test' . config('services.ldap.base_dn')] ]]); $this->mockLdap->shouldReceive('bind')->times(6)->andReturn(true); + $this->mockEscapes(4); $this->visit('/login') ->see('Username') @@ -73,6 +88,7 @@ class LdapTest extends BrowserKitTest 'mail' => [$this->mockUser->email] ]]); $this->mockLdap->shouldReceive('bind')->times(3)->andReturn(true); + $this->mockEscapes(2); $this->visit('/login') ->see('Username') @@ -97,6 +113,7 @@ class LdapTest extends BrowserKitTest 'dn' => ['dc=test' . config('services.ldap.base_dn')] ]]); $this->mockLdap->shouldReceive('bind')->times(3)->andReturn(true, true, false); + $this->mockEscapes(2); $this->visit('/login') ->see('Username') @@ -146,7 +163,7 @@ class LdapTest extends BrowserKitTest ->dontSee('External Authentication'); } - public function test_login_maps_roles_and_retains_existsing_roles() + public function test_login_maps_roles_and_retains_existing_roles() { $roleToReceive = factory(Role::class)->create(['name' => 'ldaptester', 'display_name' => 'LdapTester']); $roleToReceive2 = factory(Role::class)->create(['name' => 'ldaptester-second', 'display_name' => 'LdapTester Second']); @@ -176,6 +193,8 @@ class LdapTest extends BrowserKitTest ] ]]); $this->mockLdap->shouldReceive('bind')->times(6)->andReturn(true); + $this->mockEscapes(5); + $this->mockExplodes(6); $this->visit('/login') ->see('Username') @@ -227,6 +246,8 @@ class LdapTest extends BrowserKitTest ] ]]); $this->mockLdap->shouldReceive('bind')->times(5)->andReturn(true); + $this->mockEscapes(4); + $this->mockExplodes(2); $this->visit('/login') ->see('Username') @@ -279,6 +300,8 @@ class LdapTest extends BrowserKitTest ] ]]); $this->mockLdap->shouldReceive('bind')->times(5)->andReturn(true); + $this->mockEscapes(4); + $this->mockExplodes(2); $this->visit('/login') ->see('Username') @@ -328,6 +351,8 @@ class LdapTest extends BrowserKitTest ] ]]); $this->mockLdap->shouldReceive('bind')->times(6)->andReturn(true); + $this->mockEscapes(5); + $this->mockExplodes(6); $this->visit('/login') ->see('Username')