Fix Improper Access Control security issue: add random string to phot…

…o file name
francoisjacquet · Apr 30, 2022 · f8b9f81 · f8b9f81
1 parent 24e4406
commit f8b9f81
Show file tree

Hide file tree

Showing 11 changed files with 154 additions and 93 deletions.
diff --git a/CHANGES.md b/CHANGES.md
@@ -66,6 +66,7 @@ Changes in 9.0
 - Use URLEscape() for img src attribute, program wide
 - Sanitize / escape URL as THEME is often included for button img src attribute in User.fnc.php
 - Better format for "Add another marking period" form in EditReportCardGrades.php
+- Fix Improper Access Control security issue: add random string to photo file name in TipMessage.fnc.php, Transcripts.fnc.php, PrintClassPictures.php, Student.php, User.php & General_Info.inc.php, thanks to @dungtuanha
 
 Changes in 8.9.5
 ----------------

diff --git a/ProgramFunctions/FileUpload.fnc.php b/ProgramFunctions/FileUpload.fnc.php
@@ -9,7 +9,7 @@
 /**
  * File Upload
  *
- * @example FileUpload( 'photo', $StudentPicturesPath . UserSyear() . '/', array( '.jpg', '.jpeg' ), 2, $error, '.jpg', UserStudentID() );
+ * @example FileUpload( 'FILE_ATTACHED', $FileUploadsPath . UserSyear() . '/staff_' . User( 'STAFF_ID' ) . '/', FileExtensionWhiteList(), 0, $error );
  *
  * @global $_FILES
  *
@@ -101,20 +101,20 @@ function FileUpload( $input, $path, $ext_white_list, $size_limit, &$error, $fina
 /**
  * Image File Upload
  *
- * @example ImageUpload( 'photo', array( 'width' => 150, 'height' => '150' ), $StudentPicturesPath . UserSyear() . '/', array(), '.jpg', UserStudentID() );
- * @example ImageUpload( $base64_img, array( 'width' => 640, 'height' => '320' ) );
+ * @example ImageUpload( 'photo', [ 'width' => 150, 'height' => '150' ], $StudentPicturesPath . UserSyear() . '/', [], '.jpg', UserStudentID() . '.' . bin2hex( openssl_random_pseudo_bytes( 16 ) ) );
+ * @example ImageUpload( $base64_img, [ 'width' => 640, 'height' => '320' ] );
  *
  * @since 3.3
  *
  * @uses FileUpload()
  * @uses ImageResizeGD class.
  *
  * @param string $input            Name of the input file field, for example 'photo', or base64 encoded data, src attribute value.
- * @param array  $target_dim       Target dimensions to determine if can be resized. Defaults to array( 'width' => 994, 'height' => 1405 ) (optional).
+ * @param array  $target_dim       Target dimensions to determine if can be resized. Defaults to [ 'width' => 994, 'height' => 1405 ] (optional).
  * @param string $path             Final path with trailing slash, for example $StudentPicturesPath . UserSyear() . '/'. Defaults to "assets/FileUploads/[Syear]/[staff_or_student_ID]/" (optional).
- * @param array  $ext_white_list   Extensions white list, for example array('.jpg', '.jpeg').
+ * @param array  $ext_white_list   Extensions white list, for example ['.jpg', '.jpeg'].
  * @param string $final_ext        Final file extension (useful for .jpg, if .jpeg submitted) (optional).
- * @param string $file_name_no_ext Final file name without extension, for example UserStudentID() (optional).
+ * @param string $file_name_no_ext Final file name without extension, for example UserStudentID() . '.' . bin2hex( openssl_random_pseudo_bytes( 16 ) ) (optional).
  *
  * @return string|boolean Full path to file, or false if error
  */

diff --git a/ProgramFunctions/TipMessage.fnc.php b/ProgramFunctions/TipMessage.fnc.php
@@ -72,14 +72,17 @@ function MakeStudentPhotoTipMessage( $student_id, $title )
 {
 	global $StudentPicturesPath;
 
-	if ( $StudentPicturesPath
-		&& ( file_exists( ( $picture_path = $StudentPicturesPath . UserSyear() . '/' . $student_id . '.jpg' ) )
-			|| file_exists( ( $picture_path = $StudentPicturesPath . ( UserSyear() - 1 ) . '/' . $student_id . '.jpg' ) ) ) )
+    // @since 9.0 Fix Improper Access Control security issue: add random string to photo file name.
+    $picture_path = (array) glob( $StudentPicturesPath . '*/' . $student_id . '.*jpg' );
+
+    $picture_path = end( $picture_path );
+
+	if ( $picture_path )
 	{
 		return MakeTipMessage( '<img src="' . URLEscape( $picture_path ) . '" width="150" />', $title, $title );
 	}
-	else
-		return $title;
+
+	return $title;
 }
 
 
@@ -88,7 +91,7 @@ function MakeStudentPhotoTipMessage( $student_id, $title )
  * Look for current & previous school year Photos
  *
  * @example require_once 'ProgramFunctions/TipMessage.fnc.php';
- *          return MakeUserPhotoTipMessage( $THIS_RET['STAFF_ID'], $full_name );
+ *          return MakeUserPhotoTipMessage( $THIS_RET['STAFF_ID'], $full_name, $THIS_RET['ROLLOVER_ID'] );
  *
  * @since 3.8
  *
@@ -97,23 +100,37 @@ function MakeStudentPhotoTipMessage( $student_id, $title )
  *
  * @see assets/js/tipmessage/
  *
- * @global $UserPicturesPath Student Pictures Path
+ * @global $UserPicturesPath User Pictures Path
  *
- * @param  string $staff_id Staff ID.
- * @param  string $title    Tip title & label.
+ * @param  string $staff_id          Staff ID.
+ * @param  string $title             Tip title & label.
+ * @param  int    $staff_rollover_id Staff Rollover ID (to get last year's photo). Defaults to 0.
  *
  * @return string User Photo Tip Message or $title if no Photo found
  */
-function MakeUserPhotoTipMessage( $staff_id, $title )
+function MakeUserPhotoTipMessage( $staff_id, $title, $staff_rollover_id = 0 )
 {
 	global $UserPicturesPath;
 
-	if ( $UserPicturesPath
-		&& ( file_exists( ( $picture_path = $UserPicturesPath . UserSyear() . '/' . $staff_id . '.jpg' ) )
-			|| file_exists( ( $picture_path = $UserPicturesPath . ( UserSyear() - 1 ) . '/' . $staff_id . '.jpg' ) ) ) )
+    // @since 9.0 Fix Improper Access Control security issue: add random string to photo file name.
+    $picture_path = (array) glob( $UserPicturesPath . UserSyear() . '/' . $staff_id . '.*jpg' );
+
+    $picture_path = end( $picture_path );
+
+    if ( ! $picture_path
+        && $staff_rollover_id )
+    {
+        // Use Last Year's if Missing.
+        // @since 9.0 Fix Improper Access Control security issue: add random string to photo file name.
+        $picture_path = (array) glob( $UserPicturesPath . ( UserSyear() - 1 ) . '/' . $staff_rollover_id . '.*jpg' );
+
+        $picture_path = end( $picture_path );
+    }
+
+	if ( $picture_path )
 	{
 		return MakeTipMessage( '<img src="' . URLEscape( $picture_path ) . '" width="150" />', $title, $title );
 	}
-	else
-		return $title;
+
+	return $title;
 }
diff --git a/assets/StudentPhotos/README b/assets/StudentPhotos/README
@@ -1,11 +1,14 @@
 This directory holds the student photos.
 
-You should create one subdirectory for every school year, and store student pictures in as <student_id>.JPG
+You should create one subdirectory for every school year, and store student pictures in as <student_id>.<random_string>.jpg
+
+Note: The random string makes it impossible to predict the file name so photos cannot be accessed without being logged in.
+
 For example:
 
-2009
-       1.JPG
-       2.JPG
-2010
-       1.JPG
-       2.JPG
+2021/
+       1.d84cb7b5ffad38b086153f82c1bb27ca.jpg
+       2.d5e76d354876d7a137abac5bb19ba5bd.jpg
+2022/
+       1.3f9509cc43cb8e121f50f2a0ef003fdf.jpg
+       2.8302107827639bc1e7411ed52410dc1e.jpg
diff --git a/assets/UserPhotos/README b/assets/UserPhotos/README
@@ -1,11 +1,14 @@
 This directory holds the user photos.
 
-You should create one subdirectory for every school year, and store user pictures in as <user_id>.JPG
+You should create one subdirectory for every school year, and store user pictures in as <user_id>.<random_string>.jpg
+
+Note: The random string makes it impossible to predict the file name so photos cannot be accessed without being logged in.
+
 For example:
 
-2009
-       1.JPG
-       2.JPG
-2010
-       1.JPG
-       2.JPG
+2021/
+       1.d84cb7b5ffad38b086153f82c1bb27ca.jpg
+       2.d5e76d354876d7a137abac5bb19ba5bd.jpg
+2022/
+       3.3f9509cc43cb8e121f50f2a0ef003fdf.jpg
+       4.8302107827639bc1e7411ed52410dc1e.jpg
diff --git a/modules/Grades/includes/Transcripts.fnc.php b/modules/Grades/includes/Transcripts.fnc.php
@@ -521,22 +521,16 @@ function TranscriptPDFHeader( $student, $school_info, $certificate_text = '' )
 		if ( ! empty( $_REQUEST['showstudentpic'] ) )
 		{
 			// Student Photo.
-			$stu_pic = $StudentPicturesPath . Config( 'SYEAR' ) . '/' . $student['ID'] . '.jpg';
-			$stu_pic2 = $StudentPicturesPath . ( Config( 'SYEAR' ) -1 ) . '/' . $student['ID'] . '.jpg';
-			$stu_pic3 = $StudentPicturesPath . $student['SYEAR'] . '/' . $student['ID'] . '.jpg';
+			// @since 9.0 Fix Improper Access Control security issue: add random string to photo file name.
+			$picture_path = (array) glob( $StudentPicturesPath . '*/' . UserStudentID() . '.*jpg' );
+
+			$picture_path = end( $picture_path );
+
 			$picwidth = 120;
 
-			if ( file_exists( $stu_pic ) )
-			{
-				echo '<img src="' . URLEscape( $stu_pic ) . '" width="' . AttrEscape( $picwidth ) . '" />';
-			}
-			elseif ( file_exists( $stu_pic2 ) )
-			{
-				echo '<img src="' . URLEscape( $stu_pic2 ) . '" width="' . AttrEscape( $picwidth ) . '" />';
-			}
-			elseif ( file_exists( $stu_pic3 ) )
+			if ( $picture_path )
 			{
-				echo '<img src="' . URLEscape( $stu_pic3 ) . '" width="' . AttrEscape( $picwidth ) . '" />';
+				echo '<img src="' . URLEscape( $picture_path ) . '" width="' . AttrEscape( $picwidth ) . '" />';
 			}
 			else
 			{

diff --git a/modules/Scheduling/PrintClassPictures.php b/modules/Scheduling/PrintClassPictures.php
@@ -139,17 +139,19 @@
 
 			echo '<tr><td style="vertical-align:bottom;"><table>';
 
-			$picture_path = $UserPicturesPath . UserSyear() . '/' . $teacher_id . '.jpg';
+			// @since 9.0 Fix Improper Access Control security issue: add random string to photo file name.
+			$picture_path = (array) glob( $UserPicturesPath . UserSyear() . '/' . $teacher_id . '.*jpg' );
 
-			if ( ! file_exists( $picture_path ) )
+			$picture_path = end( $picture_path );
+
+			if ( ! $picture_path
+				&& $teacher['ROLLOVER_ID'] )
 			{
 				// Use Last Year's if Missing.
-				$picture_path = $UserPicturesPath . ( UserSyear() - 1 ) . '/' . $teacher['ROLLOVER_ID'] . '.jpg';
+				// @since 9.0 Fix Improper Access Control security issue: add random string to photo file name.
+				$picture_path = (array) glob( $UserPicturesPath . ( UserSyear() - 1 ) . '/' . $teacher['ROLLOVER_ID'] . '.*jpg' );
 
-				if ( ! file_exists( $picture_path ) )
-				{
-					$picture_path = '';
-				}
+				$picture_path = end( $picture_path );
 			}
 
 			if ( $picture_path )
@@ -186,18 +188,10 @@
 
 			echo '<td style="vertical-align:bottom;"><table>';
 
-			$picture_path = $StudentPicturesPath . UserSyear() . '/' . $student_id . '.jpg';
+			// @since 9.0 Fix Improper Access Control security issue: add random string to photo file name.
+			$picture_path = (array) glob( $StudentPicturesPath . '*/' . $student_id . '.*jpg' );
 
-			if ( ! file_exists( $picture_path ) )
-			{
-				// Use Last Year's if Missing.
-				$picture_path = $StudentPicturesPath . ( UserSyear() - 1 ) . '/' . $student_id . '.jpg';
-
-				if ( ! file_exists( $picture_path ) )
-				{
-					$picture_path = '';
-				}
-			}
+			$picture_path = end( $picture_path );
 
 			if ( $picture_path )
 			{

diff --git a/modules/Students/Student.php b/modules/Students/Student.php
@@ -434,11 +434,27 @@
 				$StudentPicturesPath . UserSyear() . '/',
 				[],
 				'.jpg',
-				UserStudentID()
+				// @since 9.0 Fix Improper Access Control security issue: add random string to photo file name.
+				UserStudentID() . '.' . bin2hex( openssl_random_pseudo_bytes( 16 ) )
 			);
 
+			if ( $new_photo_file )
+			{
+				// Remove old photos.
+				$old_photo_files = glob( $StudentPicturesPath . UserSyear() . '/' . UserStudentID() . '.*jpg' );
+
+				foreach ( $old_photo_files as $old_photo_file )
+				{
+					if ( $old_photo_file !== $new_photo_file )
+					{
+						unlink( $old_photo_file );
+					}
+				}
+			}
+
 			// Hook.
-			do_action( 'Students/Student.php|upload_student_photo' );
+			// @since 9.0 Add $new_photo_file argument to action hook.
+			do_action( 'Students/Student.php|upload_student_photo', $new_photo_file );
 		}
 
 		if ( UserStudentID() )

diff --git a/modules/Students/includes/General_Info.inc.php b/modules/Students/includes/General_Info.inc.php
@@ -7,22 +7,26 @@
 	&& ! isset( $_REQUEST['_ROSARIO_PDF'] ) ):
 ?>
 	<a href="#" onclick="$('.user-photo-form,.user-photo').toggle(); return false;"><?php
-echo button( 'add', '', '', 'smaller' ) . '&nbsp;' . _( 'Student Photo' );
+	echo button( 'add', '', '', 'smaller' ) . '&nbsp;' . _( 'Student Photo' );
 ?></a><br />
 	<div class="user-photo-form hide"><?php
-echo FileInput(
-	'photo',
-	_( 'Student Photo' ) . ' (.jpg, .png, .gif)',
-	'accept=".jpg,.jpeg,.png,.gif"'
-);
+	echo FileInput(
+		'photo',
+		_( 'Student Photo' ) . ' (.jpg, .png, .gif)',
+		'accept=".jpg,.jpeg,.png,.gif"'
+	);
 ?></div>
 <?php endif;
 
-if ( $_REQUEST['student_id'] !== 'new' && ( $file = @fopen( $picture_path = $StudentPicturesPath . UserSyear() . '/' . UserStudentID() . '.jpg', 'r' ) ) || ( $file = @fopen( $picture_path = $StudentPicturesPath . ( UserSyear() - 1 ) . '/' . UserStudentID() . '.jpg', 'r' ) ) ):
-	fclose( $file );
-	?>
-			<img src="<?php echo URLEscape( $picture_path . ( ! empty( $new_photo_file ) ? '?cacheKiller=' . rand() : '' ) ); ?>" class="user-photo" alt="<?php echo AttrEscape( _( 'Student Photo' ) ); ?>" />
-		<?php endif;
+// @since 9.0 Fix Improper Access Control security issue: add random string to photo file name.
+$picture_path = (array) glob( $StudentPicturesPath . '*/' . UserStudentID() . '.*jpg' );
+
+$picture_path = end( $picture_path );
+
+if ( $_REQUEST['student_id'] !== 'new' && $picture_path ):
+?>
+	<img src="<?php echo URLEscape( $picture_path ); ?>" class="user-photo" alt="<?php echo AttrEscape( _( 'Student Photo' ) ); ?>" />
+<?php endif;
 // END IMAGE.
 
 echo '</td><td colspan="2">';

diff --git a/modules/Users/User.php b/modules/Users/User.php
@@ -442,11 +442,27 @@
 				$UserPicturesPath . UserSyear() . '/',
 				[],
 				'.jpg',
-				UserStaffID()
+				// @since 9.0 Fix Improper Access Control security issue: add random string to photo file name.
+				UserStaffID() . '.' . bin2hex( openssl_random_pseudo_bytes( 16 ) )
 			);
 
+			if ( $new_photo_file )
+			{
+				// Remove old photos.
+				$old_photo_files = glob( $UserPicturesPath . UserSyear() . '/' . UserStaffID() . '.*jpg' );
+
+				foreach ( $old_photo_files as $old_photo_file )
+				{
+					if ( $old_photo_file !== $new_photo_file )
+					{
+						unlink( $old_photo_file );
+					}
+				}
+			}
+
 			// Hook.
-			do_action( 'Users/User.php|upload_user_photo' );
+			// @since 9.0 Add $new_photo_file argument to action hook.
+			do_action( 'Users/User.php|upload_user_photo', $new_photo_file );
 		}
 
 		if ( UserStaffID() )

diff --git a/modules/Users/includes/General_Info.inc.php b/modules/Users/includes/General_Info.inc.php
@@ -2,27 +2,40 @@
 echo '<table class="general-info width-100p valign-top fixed-col"><tr class="st"><td rowspan="4">';
 
 // IMAGE.
-
 if ( AllowEdit()
 	&& ! isset( $_REQUEST['_ROSARIO_PDF'] ) ):
 ?>
 	<a href="#" onclick="$('.user-photo-form,.user-photo').toggle(); return false;"><?php
-echo button( 'add', '', '', 'smaller' ) . '&nbsp;' . _( 'User Photo' );
+	echo button( 'add', '', '', 'smaller' ) . '&nbsp;' . _( 'User Photo' );
 ?></a><br />
 	<div class="user-photo-form hide"><?php
-echo FileInput(
-	'photo',
-	_( 'User Photo' ) . ' (.jpg, .png, .gif)',
-	'accept=".jpg,.jpeg,.png,.gif"'
-);
+	echo FileInput(
+		'photo',
+		_( 'User Photo' ) . ' (.jpg, .png, .gif)',
+		'accept=".jpg,.jpeg,.png,.gif"'
+	);
 ?></div>
 <?php endif;
 
-if ( $_REQUEST['staff_id'] !== 'new' && ( $file = @fopen( $picture_path = $UserPicturesPath . UserSyear() . '/' . UserStaffID() . '.jpg', 'r' ) ) || ( $file = @fopen( $picture_path = $UserPicturesPath . ( UserSyear() - 1 ) . '/' . UserStaffID() . '.jpg', 'r' ) ) ):
-	fclose( $file );
-	?>
-		<img src="<?php echo URLEscape( $picture_path . ( ! empty( $new_photo_file ) ? '?cacheKiller=' . rand() : '' ) ); ?>" class="user-photo" alt="<?php echo AttrEscape( _( 'User Photo' ) ); ?>" />
-	<?php endif;
+// @since 9.0 Fix Improper Access Control security issue: add random string to photo file name.
+$picture_path = (array) glob( $UserPicturesPath . UserSyear() . '/' . UserStaffID() . '.*jpg' );
+
+$picture_path = end( $picture_path );
+
+if ( ! $picture_path
+	&& $staff['ROLLOVER_ID'] )
+{
+	// Use Last Year's if Missing.
+	// @since 9.0 Fix Improper Access Control security issue: add random string to photo file name.
+	$picture_path = (array) glob( $UserPicturesPath . ( UserSyear() - 1 ) . '/' . $staff['ROLLOVER_ID'] . '.*jpg' );
+
+	$picture_path = end( $picture_path );
+}
+
+if ( $_REQUEST['staff_id'] !== 'new' && $picture_path ):
+?>
+	<img src="<?php echo URLEscape( $picture_path ); ?>" class="user-photo" alt="<?php echo AttrEscape( _( 'User Photo' ) ); ?>" />
+<?php endif;
 // END IMAGE
 
 echo '</td><td colspan="2">';