Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:21046
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Date: Mon, 05 Dec 2005 10:17:52 +0100
To: internals@lists.php.net
Message-ID: <439405C0.4040200@wp-sa.pl>
MIME-version: 1.0
Content-type: multipart/mixed; boundary="Boundary_(ID_tDBRO2cDejSqR/7K/Y87XQ)"
User-Agent: Mozilla Thunderbird 1.0.2-1.3.3 (X11/20050513)
Subject: bugs in glob() function
From: mobara@wp-sa.pl (Marcin Obara)

--Boundary_(ID_tDBRO2cDejSqR/7K/Y87XQ)
Content-type: text/plain; charset=ISO-8859-2; format=flowed
Content-transfer-encoding: 7BIT

1.
glob() function can cause crash in ZTS mode.
For relative path in ZTS we can find code:
  [...]
  cwd_skip = strlen(cwd)+1;
  snprintf(work_pattern, MAXPATHLEN, "%s%c%s", cwd, DEFAULT_SLASH, pattern);
  [...]
  add_next_index_string(return_value, globbuf.gl_pathv[n]+cwd_skip, 1);
  [...]
If pattern contains something like "./../../../../../../../*" ,
globbuf.gl_pathv[n]  could me shorter than  cwd_skip.
In result we can read from illegal area of memory.

2.
in code we can read:
        /* we assume that any glob pattern will match files from one 
directory only
           so checking the dirname of the first match should be 
sufficient */
But it's not true. Glob() can match files from more than one directory.
i.e:  "./*/*" will match files from many directories

About my solution:
- in safe_mode / open_basedir set -- I dont allow use regular expresions 
before last slash in pattern -- so glob will match files only from one 
directory
- pattern is expanded using virtual_file_ex (without using realname of 
course) to eliminate '..' '.' dirs. If pattern 'leaves' current 
directory, will be used as absolute pattern (result will contain full 
names - not relative ones).
- added missing globfree() calls.
- added (missing?) terminating '\0' in line 453

This patch is not so beautiful (my english too -- I'm not native english 
speaker) , but I hope it will help you fix these bugs.

Best regards.
--
Marcin Obara


--Boundary_(ID_tDBRO2cDejSqR/7K/Y87XQ)
Content-type: text/x-patch; name=php-4.4.1-00_glob.patch
Content-transfer-encoding: 7BIT
Content-disposition: inline; filename=php-4.4.1-00_glob.patch

Index: ext/standard/dir.c
===================================================================
RCS file: /repository/php-src/ext/standard/dir.c,v
retrieving revision 1.109.2.18.2.1
diff -u -r1.109.2.18.2.1 dir.c
--- ext/standard/dir.c	12 Jun 2005 01:15:43 -0000	1.109.2.18.2.1
+++ ext/standard/dir.c	5 Dec 2005 07:51:59 -0000
@@ -351,10 +351,8 @@
 {
 	char cwd[MAXPATHLEN];
 	int cwd_skip = 0;
-#ifdef ZTS
 	char work_pattern[MAXPATHLEN];
 	char *result;
-#endif
 	char *pattern = NULL;
 	int pattern_len;
 	long flags = 0;
@@ -364,23 +362,65 @@
 	if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "s|l", &pattern, &pattern_len, &flags) == FAILURE) 
 		return;
 
-#ifdef ZTS 
+	if ((PG(safe_mode) || (PG(open_basedir) && *PG(open_basedir)))
+		&& (pattern_len > 0)
+		&& ((result = strrchr(pattern+1, DEFAULT_SLASH)) != NULL)
+	) {
+		/* we have directories in pattern
+		 * so we have to check if pattern can match many directories */
+		*result = '\0';
+		if (strchr(pattern, '?') || strchr(pattern, '*') || strchr(pattern, '[')) {
+			/* directory part of pattern contains wildcards - unsafe */
+			*result = DEFAULT_SLASH;
+			php_error_docref(NULL TSRMLS_CC, E_WARNING, "safe_mode/open_basedir restriction in efect. Glob pattern can match files from only ONE directory.");
+			RETURN_FALSE;
+		}
+		*result = DEFAULT_SLASH;
+	}
+
 	if (!IS_ABSOLUTE_PATH(pattern, pattern_len)) {
+		cwd_state new_state;
+
 		result = VCWD_GETCWD(cwd, MAXPATHLEN);	
 		if (!result) {
-			cwd[0] = '\0';
-		}
+			/* virtual_file_ex() with  use_realpath == 0
+			 * must have cwd_length > 0 to do anything
+			 * if path is not absolute;
+			 *
+			 * maybe we should RETURN_FALSE here ?
+			 * but I tried to be compatible with previous implementation
+			 */
+			cwd[0] = DEFAULT_SLASH;
+			cwd[1] = '\0';
+			cwd_skip = 1;
+		} else {
+			cwd_skip = strlen(cwd)+1;
 #ifdef PHP_WIN32
-		if (IS_SLASH(*pattern)) {
-			cwd[2] = '\0';
-		}
+			if ((IS_SLASH(*pattern)) && (cwd_skip > 2)) {
+				cwd_skip = 2;
+			}
 #endif
-		cwd_skip = strlen(cwd)+1;
-
-		snprintf(work_pattern, MAXPATHLEN, "%s%c%s", cwd, DEFAULT_SLASH, pattern);
+		}
+		new_state.cwd = strdup(cwd);
+		new_state.cwd_length = strlen(cwd);
+		if (virtual_file_ex(&new_state, pattern, NULL, 0)) {
+			free(new_state.cwd);
+			RETURN_FALSE;
+		}
+		ret = strlen(cwd);
+		n = (new_state.cwd_length > MAXPATHLEN - 1) ? (MAXPATHLEN - 1) : new_state.cwd_length;
+		if ((n < ret) || (strncmp(new_state.cwd, cwd, ret) != 0)) {
+			/* we MUST NOT use cwd_skip if:
+			 * - pattern is shorter
+			 * - pattern matches not current path
+			 */
+			 cwd_skip = 0;
+		}
+		memcpy(work_pattern, new_state.cwd, n);
+		work_pattern[n] = '\0';
+		free(new_state.cwd);
 		pattern = work_pattern;
 	} 
-#endif
 
 	globbuf.gl_offs = 0;
 	if (0 != (ret = glob(pattern, flags & GLOB_FLAGMASK, NULL, &globbuf))) {
@@ -405,11 +445,18 @@
 
 	/* we assume that any glob pattern will match files from one directory only
 	   so checking the dirname of the first match should be sufficient */
+	/* NO, glob can match files from more than one directory
+	 * but we already checked pattern at the beginning of this function
+	 * if safe_mode or open_basedir is set
+	 */
 	strncpy(cwd, globbuf.gl_pathv[0], MAXPATHLEN);
+	cwd[MAXPATHLEN - 1] = '\0';
 	if (PG(safe_mode) && (!php_checkuid(cwd, NULL, CHECKUID_CHECK_FILE_AND_DIR))) {
+		globfree(&globbuf);
 		RETURN_FALSE;
 	}
 	if (php_check_open_basedir(cwd TSRMLS_CC)) {
+		globfree(&globbuf);
 		RETURN_FALSE;
 	}
 

--Boundary_(ID_tDBRO2cDejSqR/7K/Y87XQ)--