Added proper escaping to LDAP filter operations

To cover #1163

diff --git a/app/Auth/Access/Ldap.php b/app/Auth/Access/Ldap.php
index 468c376268d5ae3e884317e641ecf8e4166894b1..843a2f204920e9bd5154efe477c5212f73efa057 100644 (file)
--- 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 d3a177f8e1dffbac095e4d504550b646182eb690..b49ecf129fc1d5b606c60ecb668f2e0c6736eea1 100644 (file)
--- 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 25d8a59064301d618bc2216495383e880bc3bb2a..16ba113587e772d61a545a0cadcaafd6286ff4ca 100644 (file)
--- 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')