[svn r21093] Security improvements FS#4261

main/newscorm/learnpath.class.php

@@ -2962,6 +2962,8 @@ class learnpath {
 		$lp_table = Database :: get_course_table(TABLE_LP_MAIN);
 		$lp_item_table = Database :: get_course_table(TABLE_LP_ITEM);
 		$lp_item_view_table = Database :: get_course_table(TABLE_LP_ITEM_VIEW);
+		$item_id = Database::escape_string($item_id); 
+		
 		$sel = "SELECT l.lp_type as ltype, l.path as lpath, li.item_type as litype, li.path as lipath, li.parameters as liparams " .
 			   "FROM $lp_table l, $lp_item_table li WHERE li.id = $item_id AND li.lp_id = l.id";
 		if ($this->debug > 2) {
@@ -3038,6 +3040,7 @@ class learnpath {
 					if ($this->debug > 2) {
 						error_log('New LP - In learnpath::get_link() ' . __LINE__ . ' - Item type: ' . $lp_item_type, 0);
 					}
+					
 					if ($lp_item_type != 'dir') {
 						//Quite complex here:
 						//we want to make sure 'http://' (and similar) links can 
@@ -3752,30 +3755,23 @@ class learnpath {
 	/**
 	 * Saves the last item seen's ID only in case 
 	 */
-	function save_last() {
+	function save_last()
-
+	{
 		if ($this->debug > 0) {
 			error_log('New LP - In learnpath::save_last()', 0);
 		}
-
 		$table = Database :: get_course_table('lp_view');
-
 		if (isset ($this->current)) {
-
 			if ($this->debug > 2) {
 				error_log('New LP - Saving current item (' . $this->current . ') for later review', 0);
 			}
-
+			$sql = "UPDATE $table SET last_item = " . Database::escape_string($this->get_current_item_id()). " " .
-			$sql = "UPDATE $table SET last_item = " . $this->get_current_item_id() . " " .
-
 					"WHERE lp_id = " . $this->get_id() . " AND user_id = " . $this->get_user_id();
 
 			if ($this->debug > 2) {
 				error_log('New LP - Saving last item seen : ' . $sql, 0);
 			}
-
 			$res = api_sql_query($sql, __FILE__, __LINE__);
-
 		}
 
 		//save progress
@@ -3787,18 +3783,13 @@ class learnpath {
 			$progress = (int) $progress;
 
 			$sql = "UPDATE $table SET progress = $progress " .
-
 					"WHERE lp_id = " . $this->get_id() . " AND " .
-
 				  		  "user_id = " . $this->get_user_id();
-
 			$res = api_sql_query($sql, __FILE__, __LINE__); //ignore errors as some tables might not have the progress field just yet
-
 			$this->progress_db = $progress;
-
 		}
-
 	}
+	
 	/**
 	 * Sets the current item ID (checks if valid and authorized first)
 	 * @param	integer	New item ID. If not given or not authorized, defaults to current
@@ -3816,6 +3807,7 @@ class learnpath {
 			if ($this->debug > 2) {
 				error_log('New LP - New current item given is ' . $item_id . '...', 0);
 			}			
+			if (is_numeric($item_id)) {
 				$item_id = $this->escape_string($item_id);
 				//TODO check in database here
 				$this->last = $this->current;
@@ -3830,6 +3822,9 @@ class learnpath {
 				if ($this->debug > 2) {
 					error_log('New LP - set_current_item(' . $item_id . ') done. Index is now : ' . $this->index, 0);
 				}
+			} else {
+				error_log('New LP - set_current_item(' . $item_id . ') failed. Not a numeric value: ', 0);
+			}
 		}
 	}
 	/**

main/newscorm/lp_controller.php

@@ -93,10 +93,7 @@ if(isset($_SESSION['lpobject']))
 
 if($debug>0) error_log('New LP - Passed data remains check',0);
 
-if($lp_found == false
+if($lp_found == false || (!empty($_REQUEST['lp_id']) && $_SESSION['oLP']->get_id() != $_REQUEST['lp_id'])) {
-	|| (!empty($_REQUEST['lp_id']) && $_SESSION['oLP']->get_id() != $_REQUEST['lp_id'])
-	)
-{
 	if($debug>0) error_log('New LP - oLP is not object, has changed or refresh been asked, getting new',0);		
 	//regenerate a new lp object? Not always as some pages don't need the object (like upload?)
 	if(!empty($_REQUEST['lp_id']) || !empty($myrefresh_id)){
@@ -105,15 +102,17 @@ if($lp_found == false
 		//right object
 		$lp_table = Database::get_course_table('lp');
 		if(!empty($_REQUEST['lp_id'])){
-			$lp_id = learnpath::escape_string($_REQUEST['lp_id']);
+			$lp_id = $_REQUEST['lp_id'];
 		} else {
 			$lp_id = $myrefresh_id;
 		}		
+		if (is_numeric($lp_id)) {
+			$lp_id = Database::escape_string($lp_id);
 			$sel = "SELECT * FROM $lp_table WHERE id = $lp_id";			
+				
 			if($debug>0) error_log('New LP - querying '.$sel,0);
 			$res = api_sql_query($sel);
-		if(Database::num_rows($res))
+			if(Database::num_rows($res)) {
-		{
 				$row = Database::fetch_array($res);
 				$type = $row['lp_type'];
 				if($debug>0) error_log('New LP - found row - type '.$type. ' - Calling constructor with '.api_get_course_id().' - '.$lp_id.' - '.api_get_user_id(),0);			
@@ -140,11 +139,15 @@ if($lp_found == false
 						break;
 				}
 			}
+		} else {
+			if($debug>0) error_log('New LP - Request[lp_id] is not numeric',0);
+		}
+		
+		
 	} else {
 		if($debug>0) error_log('New LP - Request[lp_id] and refresh_id were empty',0);
 	}
-	if($lp_found)
+	if($lp_found) {
-	{
 		$_SESSION['oLP'] = $oLP;
 	}
 }