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);
     }
     {
         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 === null) {
             return false;
         }

+

         if ($ldapUser['uid'] !== $user->external_auth_id) {
             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 = [];
         foreach ($attrs as $key => $attrText) {
             $newKey = '${' . $key . '}';

-            $newAttrs[$newKey] = $attrText;
+            $newAttrs[$newKey] = $this->ldap->escape($attrText);

         }
         return strtr($filterString, $newAttrs);
     }
         }
         return strtr($filterString, $newAttrs);
     }


@@ -265,7 +266,8 @@ class LdapService
         $baseDn = $this->config['base_dn'];
         $groupsAttr = strtolower($this->config['group_attribute']);
 
         $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 [];
         }
         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
     /**
      * 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
      */
      * @return array
      */

-    protected function groupFilter($ldapSearchReturn)
+    protected function groupFilter(array $userGroupSearchResponse)

     {
         $groupsAttr = strtolower($this->config['group_attribute']);
         $ldapGroups = [];
         $count = 0;
     {
         $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++) {
         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];
             }
         }
             if (!in_array($dnComponents[0], $ldapGroups)) {
                 $ldapGroups[] = $dnComponents[0];
             }
         }

+

         return $ldapGroups;
     }
 
         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();
     }
 
         $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);
     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);
                 'dn' => ['dc=test' . config('services.ldap.base_dn')]
             ]]);
         $this->mockLdap->shouldReceive('bind')->times(6)->andReturn(true);

+        $this->mockEscapes(4);

 
         $this->visit('/login')
             ->see('Username')
 
         $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);
                 'mail' => [$this->mockUser->email]
             ]]);
         $this->mockLdap->shouldReceive('bind')->times(3)->andReturn(true);

+        $this->mockEscapes(2);

 
         $this->visit('/login')
             ->see('Username')
 
         $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);
                 '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')
 
         $this->visit('/login')
             ->see('Username')


@@ -146,7 +163,7 @@ class LdapTest extends BrowserKitTest
             ->dontSee('External Authentication');
     }
 
             ->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']);
     {
         $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->mockLdap->shouldReceive('bind')->times(6)->andReturn(true);

+        $this->mockEscapes(5);
+        $this->mockExplodes(6);

 
         $this->visit('/login')
             ->see('Username')
 
         $this->visit('/login')
             ->see('Username')


@@ -227,6 +246,8 @@ class LdapTest extends BrowserKitTest
                 ]
             ]]);
         $this->mockLdap->shouldReceive('bind')->times(5)->andReturn(true);
                 ]
             ]]);
         $this->mockLdap->shouldReceive('bind')->times(5)->andReturn(true);

+        $this->mockEscapes(4);
+        $this->mockExplodes(2);

 
         $this->visit('/login')
             ->see('Username')
 
         $this->visit('/login')
             ->see('Username')


@@ -279,6 +300,8 @@ class LdapTest extends BrowserKitTest
                 ]
             ]]);
         $this->mockLdap->shouldReceive('bind')->times(5)->andReturn(true);
                 ]
             ]]);
         $this->mockLdap->shouldReceive('bind')->times(5)->andReturn(true);

+        $this->mockEscapes(4);
+        $this->mockExplodes(2);

 
         $this->visit('/login')
             ->see('Username')
 
         $this->visit('/login')
             ->see('Username')


@@ -328,6 +351,8 @@ class LdapTest extends BrowserKitTest
                 ]
             ]]);
         $this->mockLdap->shouldReceive('bind')->times(6)->andReturn(true);
                 ]
             ]]);
         $this->mockLdap->shouldReceive('bind')->times(6)->andReturn(true);

+        $this->mockEscapes(5);
+        $this->mockExplodes(6);

 
         $this->visit('/login')
             ->see('Username')
 
         $this->visit('/login')
             ->see('Username')