Merge pull request #8541 from owncloud/hardenIsSubDirectory

Harden issubdirectory()

lib/base.php

@@ -613,7 +613,7 @@ class OC {
 		if (!is_null(self::$REQUESTEDFILE)) {
 			$subdir = OC_App::getAppPath(OC::$REQUESTEDAPP) . '/' . self::$REQUESTEDFILE;
 			$parent = OC_App::getAppPath(OC::$REQUESTEDAPP);
-			if (!OC_Helper::issubdirectory($subdir, $parent)) {
+			if (!OC_Helper::isSubDirectory($subdir, $parent)) {
 				self::$REQUESTEDFILE = null;
 				header('HTTP/1.0 404 Not Found');
 				exit;

lib/private/helper.php

@@ -732,10 +732,22 @@ class OC_Helper {
 	 * @param string $parent
 	 * @return bool
 	 */
-	public static function issubdirectory($sub, $parent) {
-		if (strpos(realpath($sub), realpath($parent)) === 0) {
+	public static function isSubDirectory($sub, $parent) {
+		$realpathSub = realpath($sub);
+		$realpathParent = realpath($parent);
+
+		// realpath() may return false in case the directory does not exist
+		// since we can not be sure how different PHP versions may behave here
+		// we do an additional check whether realpath returned false
+		if($realpathSub === false ||  $realpathParent === false) {
+			return false;
+		}
+
+		// Check whether $sub is a subdirectory of $parent
+		if (strpos($realpathSub, $realpathParent) === 0) {
 			return true;
 		}
+
 		return false;
 	}
 

lib/private/l10n.php

@@ -134,10 +134,10 @@ class OC_L10N implements \OCP\IL10N {
 			$i18ndir = self::findI18nDir($app);
 			// Localization is in /l10n, Texts are in $i18ndir
 			// (Just no need to define date/time format etc. twice)
-			if((OC_Helper::issubdirectory($i18ndir.$lang.'.php', OC::$SERVERROOT.'/core/l10n/')
-				|| OC_Helper::issubdirectory($i18ndir.$lang.'.php', OC::$SERVERROOT.'/lib/l10n/')
-				|| OC_Helper::issubdirectory($i18ndir.$lang.'.php', OC::$SERVERROOT.'/settings')
-				|| OC_Helper::issubdirectory($i18ndir.$lang.'.php', OC_App::getAppPath($app).'/l10n/')
+			if((OC_Helper::isSubDirectory($i18ndir.$lang.'.php', OC::$SERVERROOT.'/core/l10n/')
+				|| OC_Helper::isSubDirectory($i18ndir.$lang.'.php', OC::$SERVERROOT.'/lib/l10n/')
+				|| OC_Helper::isSubDirectory($i18ndir.$lang.'.php', OC::$SERVERROOT.'/settings')
+				|| OC_Helper::isSubDirectory($i18ndir.$lang.'.php', OC_App::getAppPath($app).'/l10n/')
 				)
 				&& file_exists($i18ndir.$lang.'.php')) {
 				// Include the file, save the data from $CONFIG
@@ -162,7 +162,7 @@ class OC_L10N implements \OCP\IL10N {
 				}
 			}
 
-			if(file_exists(OC::$SERVERROOT.'/core/l10n/l10n-'.$lang.'.php') && OC_Helper::issubdirectory(OC::$SERVERROOT.'/core/l10n/l10n-'.$lang.'.php', OC::$SERVERROOT.'/core/l10n/')) {
+			if(file_exists(OC::$SERVERROOT.'/core/l10n/l10n-'.$lang.'.php') && OC_Helper::isSubDirectory(OC::$SERVERROOT.'/core/l10n/l10n-'.$lang.'.php', OC::$SERVERROOT.'/core/l10n/')) {
 				// Include the file, save the data from $CONFIG
 				include OC::$SERVERROOT.'/core/l10n/l10n-'.$lang.'.php';
 				if(isset($LOCALIZATIONS) && is_array($LOCALIZATIONS)) {

tests/lib/helper.php

@@ -120,15 +120,15 @@ class Test_Helper extends PHPUnit_Framework_TestCase {
 		$this->assertEquals($result, $expected);
 	}
 
-	function testIssubdirectory() {
-		$result = OC_Helper::issubdirectory("./data/", "/anotherDirectory/");
+	function testIsSubDirectory() {
+		$result = OC_Helper::isSubDirectory("./data/", "/anotherDirectory/");
 		$this->assertFalse($result);
 
-		$result = OC_Helper::issubdirectory("./data/", "./data/");
+		$result = OC_Helper::isSubDirectory("./data/", "./data/");
 		$this->assertTrue($result);
 
 		mkdir("data/TestSubdirectory", 0777);
-		$result = OC_Helper::issubdirectory("data/TestSubdirectory/", "data");
+		$result = OC_Helper::isSubDirectory("data/TestSubdirectory/", "data");
 		rmdir("data/TestSubdirectory");
 		$this->assertTrue($result);
 	}