gosa-project / gosa-core

GOsa core
GNU General Public License v2.0
16 stars 14 forks source link

Flawed object storage in $_SESSION #28

Open sunweaver opened 5 years ago

sunweaver commented 5 years ago

I investigate an issue observed with gosa on Debian stretch (php7.0) and buster (php7.3).

The observation is exactly what is described earlier in class_systemManagement.inc: https://github.com/gosa-project/gosa-plugins-systems/blob/cf34737977a97e0090e09390b209078dabdc77af/admin/systems/class_systemManagement.inc#L90

See also Debian bug #907815 (esp. msg #31): https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=907815#31

The observation is that class_management rendered listings contain bogus content when switching between different plugins (userManagement, groupManagement, systemManagement, etc.). Reason for this is that filter caching PHP objects get stored in $_SESSION and they don't properly survive from one page load to the next page load in the web browser. In fact, some arbitrary stuff seems to happen, thus unwanted data becomes accessible via $_SESSION.

[I have consulted the Debian security team on this for advice, so that this might be turned into a CVE issue.]

After studying these posts... https://stackoverflow.com/questions/1442177/storing-objects-in-php-session https://stackoverflow.com/questions/132194/php-storing-objects-inside-the-session

..., it becomes evident that gosa needs to serialize/unserialize object data (and possibly also array data) when attaching them to the $_SESSION array. Even better: avoid using $_SESSION for the purpose of caching information entirely (as it puts heavy load on the webserver).

For Debian buster (and also stretch), we will deactivate the filter caching code and re-load the *-filter.xml files from the file system on every class_management based page (re)loads.

The work-around patch we use in Debian is this: https://salsa.debian.org/debian-edu-pkg-team/gosa/blob/259de6a72a56d1b460d5fa1382d0c1b5c91fcde5/debian/patches/1045_dont_use_filter_caching.patch

But a better approach is leaving the filter caching intact and fix session:get_global and session:set_global like this (code snippet contains debug messages):

diff -ur gosa/include/class_session.inc gosa.patched/include/class_session.inc
--- gosa/include/class_session.inc  2010-07-29 15:19:55.000000000 +0200
+++ gosa.patched/include/class_session.inc  2019-04-19 12:12:59.488081000 +0200
@@ -87,6 +87,7 @@

     public static function set($name,$value)
     {
+   echo "session::set -- ".$name."<br>";
         $channel= "";
         if (isset($_POST['_channel_'])){
             $channel= $_POST['_channel_'];
@@ -106,11 +107,21 @@

     public static function global_set($name,$value)
     {
-        $_SESSION[$name] = $value;
+   echo "session::global_set -- ".$name;
+
+   if (is_scalar($value) ) {
+            echo " (not serialized)<br>";
+            $_SESSION[$name] = $value;
+   } else {
+            echo " (serialized)<br>";
+       $_SESSION[$name] = serialize($value);
+       //$_SESSION[$name] = $value;
+   }
     }

     public static function &get($name)
     {
+   echo "session::get -- ".$name."<br>";
         $channel= "";
         if (isset($_POST['_channel_'])){
             $channel= $_POST['_channel_'];
@@ -118,7 +129,7 @@

         /* Global fallback if not set */
         if ($channel == ""){
-            $ret = &$_SESSION[$name];
+            $ret = &session::global_get($name);
             return($ret);
         }

@@ -134,7 +145,17 @@

     public static function &global_get($name)
     {
-        $ret = &$_SESSION[$name];
+   echo "session::global_get -- ".$name;
+   if (is_serialized($_SESSION[$name])) {
+            echo " (serialized)<br>";
+       $_unserialized = unserialize($_SESSION[$name]);
+       var_dump($_unserialized); echo "<br>";
+       $ret = &$_unserialized;
+   } else {
+            echo " (not serialized)<br>";
+            var_dump($_SESSION[$name]);
+       $ret = &$_SESSION[$name];
+        }
         return($ret);
     }

diff -ur gosa/include/functions.inc gosa.patched/include/functions.inc
--- gosa/include/functions.inc  2018-12-12 16:52:38.000000000 +0100
+++ gosa.patched/include/functions.inc  2019-04-19 11:47:54.952081000 +0200
@@ -3959,5 +3959,117 @@
     return(in_array($needle, $haystack, $strict));
 }

+/**
+ * The is_serialized function is free software. It comes without any warranty, to
+ * the extent permitted by applicable law. You can redistribute it
+ * and/or modify it under the terms of the Do What The Fuck You Want
+ * To Public License, Version 2, as published by Sam Hocevar. See
+ * http://sam.zoy.org/wtfpl/COPYING for more details.
+ */ 
+
+/**
+ * Tests if an input is valid PHP serialized string.
+ *
+ * Checks if a string is serialized using quick string manipulation
+ * to throw out obviously incorrect strings. Unserialize is then run
+ * on the string to perform the final verification.
+ *
+ * Valid serialized forms are the following:
+ * <ul>
+ * <li>boolean: <code>b:1;</code></li>
+ * <li>integer: <code>i:1;</code></li>
+ * <li>double: <code>d:0.2;</code></li>
+ * <li>string: <code>s:4:"test";</code></li>
+ * <li>array: <code>a:3:{i:0;i:1;i:1;i:2;i:2;i:3;}</code></li>
+ * <li>object: <code>O:8:"stdClass":0:{}</code></li>
+ * <li>null: <code>N;</code></li>
+ * </ul>
+ *
+ * @author     Chris Smith <code+php@chris.cs278.org>
+ * @copyright  Copyright (c) 2009 Chris Smith (http://www.cs278.org/)
+ * @license        http://sam.zoy.org/wtfpl/ WTFPL
+ * @param      string  $value  Value to test for serialized form
+ * @param      mixed   $result Result of unserialize() of the $value
+ * @return     boolean         True if $value is serialized data, otherwise false
+ */
+function is_serialized($value, &$result = null)
+{
+   // Bit of a give away this one
+   if (!is_string($value))
+   {
+       return false;
+   }
+
+   // Serialized false, return true. unserialize() returns false on an
+   // invalid string or it could return false if the string is serialized
+   // false, eliminate that possibility.
+   if ($value === 'b:0;')
+   {
+       $result = false;
+       return true;
+   }
+
+   $length = strlen($value);
+   $end    = '';
+
+   switch ($value[0])
+   {
+       case 's':
+           if ($value[$length - 2] !== '"')
+           {
+               return false;
+           }
+       case 'b':
+       case 'i':
+       case 'd':
+           // This looks odd but it is quicker than isset()ing
+           $end .= ';';
+       case 'a':
+       case 'O':
+           $end .= '}';
+
+           if ($value[1] !== ':')
+           {
+               return false;
+           }
+
+           switch ($value[2])
+           {
+               case 0:
+               case 1:
+               case 2:
+               case 3:
+               case 4:
+               case 5:
+               case 6:
+               case 7:
+               case 8:
+               case 9:
+               break;
+
+               default:
+                   return false;
+           }
+       case 'N':
+           $end .= ';';
+
+           if ($value[$length - 1] !== $end[0])
+           {
+               return false;
+           }
+       break;
+
+       default:
+           return false;
+   }
+
+   if (($result = @unserialize($value)) === false)
+   {
+       $result = null;
+       return false;
+   }
+   return true;
+}
+
 // vim:tabstop=2:expandtab:shiftwidth=2:filetype=php:syntax:ruler:
 ?>

However, when applying the above patch, administrative users won't be able to login as admins anymore. So, this needs more work.