Browse Source

Fixing security issue when downloading a document see #2722, adding check_abs + making more restrict the Document::is_visible function

Julio Montoya 14 years ago
parent
commit
37eee0ff7a
2 changed files with 30 additions and 29 deletions
  1. 19 20
      main/document/download.php
  2. 11 9
      main/inc/lib/document.lib.php

+ 19 - 20
main/document/download.php

@@ -20,6 +20,12 @@ session_cache_limiter('none');
 require_once '../inc/global.inc.php';
 $this_section = SECTION_COURSES;
 
+// Protection
+api_protect_course_script();
+
+if (!isset($_course)) {
+    api_not_allowed(true);
+}
 require_once api_get_path(LIBRARY_PATH).'document.lib.php';
 
 $doc_url = $_GET['doc_url'];
@@ -36,6 +42,9 @@ $doc_url = str_replace(array('../', '\\..', '\\0', '..\\'), array('', '', '', ''
 // The administrator should probably be able to disable this code through admin
 // inteface.
 $refer_script = strrchr($_SERVER["HTTP_REFERER"], '/');
+
+$sys_course_path = api_get_path(SYS_COURSE_PATH).$_course['path'].'/document';
+
 if (substr($refer_script, 0, 15) == '/fillsurvey.php') {
 	$invitation = substr(strstr($refer_script, 'invitationcode='), 15);
 	$course = strstr($refer_script, 'course=');
@@ -44,15 +53,8 @@ if (substr($refer_script, 0, 15) == '/fillsurvey.php') {
 	$_course = check_download_survey($course, $invitation, $doc_url);
 	$_course['path'] = $_course['directory'];
 } else {
-	// Protection
-	api_protect_course_script();
-
-	if (!isset($_course)) {
-		api_not_allowed(true);
-	}
-
 	// If the rewrite rule asks for a directory, we redirect to the document explorer
-	if (is_dir(api_get_path(SYS_COURSE_PATH).$_course['path'].'/document'.$doc_url)) {
+	if (is_dir($sys_course_path.$doc_url)) {
 		// Remove last slash if present
 		// mod_rewrite can change /some/path/ to /some/path// in some cases, so clean them all off (René)
 		while ($doc_url{$dul = strlen($doc_url) - 1} == '/') {
@@ -65,20 +67,17 @@ if (substr($refer_script, 0, 15) == '/fillsurvey.php') {
 		// Redirect
 		header('Location: '.$document_explorer);
 	}
-
 	// Launch event
 	event_download($doc_url);
 }
 
-$sys_course_path = api_get_path(SYS_COURSE_PATH);
-$full_file_name = $sys_course_path.$_course['path'].'/document'.str_replace('+', ' ', $doc_url);
-
-// Check visibility of document and paths
-$is_allowed_to_edit = api_is_allowed_to_edit();
-if (!$is_allowed_to_edit && !DocumentManager::is_visible($doc_url, $_course, api_get_session_id())) {
-	Display::display_error_message(get_lang('ProtectedDocument'));//api_not_allowed backbutton won't work.
-	exit; // You shouldn't be here anyway.
+if (Security::check_abs_path($sys_course_path.$doc_url, $sys_course_path)) {
+    $full_file_name = $sys_course_path.$doc_url;    
+    // Check visibility of document and paths    
+    if (!api_is_allowed_to_edit() && !DocumentManager::is_visible($doc_url, $_course, api_get_session_id())) {
+    	Display::display_error_message(get_lang('ProtectedDocument'));//api_not_allowed backbutton won't work.
+    	exit; // You shouldn't be here anyway.
+    }    
+    DocumentManager::file_send_for_download($full_file_name);
 }
-
-DocumentManager::file_send_for_download($full_file_name);
-exit;
+exit;

+ 11 - 9
main/inc/lib/document.lib.php

@@ -1065,7 +1065,7 @@ class DocumentManager {
     }
 
     /**
-     * return true if the documentpath have visibility=1 as item_property
+     * Return true if the documentpath have visibility=1 as item_property
      *
      * @param string $document_path the relative complete path of the document
      * @param array  $course the _course array info of the document's course
@@ -1073,23 +1073,25 @@ class DocumentManager {
     public static function is_visible($doc_path, $course, $session_id = 0) {
         $docTable  = Database::get_course_table(TABLE_DOCUMENT, $course['dbName']);
         $propTable = Database::get_course_table(TABLE_ITEM_PROPERTY, $course['dbName']);
-        //note the extra / at the end of doc_path to match every path in the
-        // document table that is part of the document path
+        //note the extra / at the end of doc_path to match every path in the document table that is part of the document path
         $doc_path = Database::escape_string($doc_path);
 
         $session_id = intval($session_id);
         $condition = "AND id_session = $session_id";
         // The " d.filetype='file' " let the user see a file even if the folder is hidden see #2198
-        $sql  = "SELECT path FROM $docTable d, $propTable ip " .
-                "WHERE d.id=ip.ref AND ip.tool='".TOOL_DOCUMENT."' AND visibility=0 $condition AND d.filetype='file' AND locate(concat(path,'/'),'".$doc_path."/')=1";
+        $sql  = "SELECT visibility FROM $docTable d, $propTable ip " .
+                "WHERE d.id=ip.ref AND ip.tool='".TOOL_DOCUMENT."' $condition AND d.filetype='file' AND locate(concat(path,'/'),'".$doc_path."/')=1";
         $result = Database::query($sql);
+        $is_visible = false;
         if (Database::num_rows($result) > 0) {
-            $row = Database::fetch_array($result);
-            //echo "$row[0] not visible";
-            return false;
+            $row = Database::fetch_array($result,'ASSOC');
+            if ($row['visibility'] == 1) {
+            	$is_visible = $_SESSION ['is_allowed_in_course'] || api_is_platform_admin();
+            }
         }
         //improved protection of documents viewable directly through the url: incorporates the same protections of the course at the url of documents:	access allowed for the whole world Open, access allowed for users registered on the platform Private access, document accessible only to course members (see the Users list), Completely closed; the document is only accessible to the course admin and teaching assistants.
-        return $_SESSION ['is_allowed_in_course'] || api_is_platform_admin();
+        //return $_SESSION ['is_allowed_in_course'] || api_is_platform_admin();
+        return $is_visible;
     }
 
         /**