fix serious logic error and potential security issue with phar_compiled_file and

phar_find_in_include_path.  We were allowing data-based phars to be executed, and actually marking phar-based phar archives
without '.phar' in the name as data-based phars, which would allow modifying them even if phar.readonly=0.  Add test for this sinister case

diff --git a/ext/phar/cgidebug b/ext/phar/cgidebug
index db666970ef3ab69bfca19646b0859fc6c7c09676..bacf6910a6561acd284a5a0d6f211814d9d0a883 100755 (executable)
--- a/ext/phar/cgidebug
+++ b/ext/phar/cgidebug
@@ -5,7 +5,7 @@ export SCRIPT_FILENAME=/home/cellog/workspace/php5/ext/phar/tests/frontcontrolle
 export PATH_TRANSLATED=/home/cellog/workspace/php5/ext/phar/tests/frontcontroller34.php
 export REDIRECT_STATUS=1
 export REQUEST_METHOD=GET
-export REQUEST_URI=/frontcontroller29.php/fatalerror.phps
+export REQUEST_URI=/frontcontroller34.php/start/index.php
 cd /home/cellog/workspace/php5/
 ddd sapi/cgi/php-cgi &
 cd /home/cellog/workspace/php5/ext/phar

diff --git a/ext/phar/phar.c b/ext/phar/phar.c
index b668e043e4065d80338670596fb2ea685d5281dd..411decb84eaf9384a8f43c442cb7c4c2a9348f14 100644 (file)
--- a/ext/phar/phar.c
+++ b/ext/phar/phar.c
@@ -378,7 +378,7 @@ int phar_open_loaded(char *fname, int fname_len, char *alias, int alias_len, int
 
                if (!is_data) {
                        /* prevent any ".phar" without a stub getting through */
-                       if (!phar->halt_offset && !phar->is_brandnew) {
+                       if (!phar->halt_offset && !phar->is_brandnew && (phar->is_tar || phar->is_zip)) {
                                if (PHAR_G(readonly) && FAILURE == zend_hash_find(&(phar->manifest), ".phar/stub.php", sizeof(".phar/stub.php")-1, (void **)&stub)) {
                                        if (error) {
                                                spprintf(error, 0, "'%s' is not a phar archive. Use PharData::__construct() for a standard zip or tar archive", fname);
@@ -387,7 +387,7 @@ int phar_open_loaded(char *fname, int fname_len, char *alias, int alias_len, int
                                }
                        }
                }
-               phar->is_data = is_data;
+               phar->is_data = is_data && (phar->is_tar || phar->is_zip);
                if (pphar) {
                        *pphar = phar;
                }
@@ -964,7 +964,9 @@ int phar_open_or_create_filename(char *fname, int fname_len, char *alias, int al
 {
        const char *ext_str, *z;
        int ext_len;
+       phar_archive_data **test, *unused = NULL;
 
+       test = &unused;
        if (error) {
                *error = NULL;
        }
@@ -982,21 +984,25 @@ int phar_open_or_create_filename(char *fname, int fname_len, char *alias, int al
        }
 
 check_file:
-       if (phar_open_loaded(fname, fname_len, alias, alias_len, is_data, options, pphar, 0 TSRMLS_CC) == SUCCESS) {
-               if (is_data && !((*pphar)->is_tar || (*pphar)->is_zip)) {
+       if (phar_open_loaded(fname, fname_len, alias, alias_len, is_data, options, test, 0 TSRMLS_CC) == SUCCESS) {
+               if (pphar) {
+                       *pphar = *test;
+               }
+               if ((*test)->is_data && !(*test)->is_tar && !(*test)->is_zip) {
                        if (error) {
                                spprintf(error, 0, "Cannot open '%s' as a PharData object. Use Phar::__construct() for executable archives", fname);
                        }
+                       return FAILURE;
                }
-               if (PHAR_G(readonly) && !is_data && ((*pphar)->is_tar || (*pphar)->is_zip)) {
+               if (PHAR_G(readonly) && !(*test)->is_data && ((*test)->is_tar || (*test)->is_zip)) {
                        phar_entry_info *stub;
-                       if (FAILURE == zend_hash_find(&((*pphar)->manifest), ".phar/stub.php", sizeof(".phar/stub.php")-1, (void **)&stub)) {
+                       if (FAILURE == zend_hash_find(&((*test)->manifest), ".phar/stub.php", sizeof(".phar/stub.php")-1, (void **)&stub)) {
                                spprintf(error, 0, "'%s' is not a phar archive. Use PharData::__construct() for a standard zip or tar archive", fname);
                                return FAILURE;
                        }
                }
-               if (pphar && (!PHAR_G(readonly) || is_data)) {
-                       (*pphar)->is_writeable = 1;
+               if (!PHAR_G(readonly) || (*test)->is_data) {
+                       (*test)->is_writeable = 1;
                }
                return SUCCESS;
        }
@@ -1044,7 +1050,7 @@ int phar_create_or_parse_filename(char *fname, int fname_len, char *alias, int a
 
        if (fp) {
                if (phar_open_fp(fp, fname, fname_len, alias, alias_len, options, pphar, error TSRMLS_CC) == SUCCESS) {
-                       if (is_data || !PHAR_G(readonly)) {
+                       if ((*pphar)->is_data || !PHAR_G(readonly)) {
                                (*pphar)->is_writeable = 1;
                        }
                        if (actual) {
@@ -1522,7 +1528,7 @@ int phar_detect_phar_fname_ext(const char *filename, int check_length, const cha
        phar_request_initialize(TSRMLS_C);
        /* first check for alias in first segment */
        pos = strchr(filename, '/');
-       if (pos) {
+       if (pos && pos != filename) {
                if (zend_hash_exists(&(PHAR_GLOBALS->phar_alias_map), (char *) filename, pos - filename)) {
                        *ext_str = pos;
                        *ext_len = -1;
@@ -1843,7 +1849,7 @@ int phar_open_compiled_file(char *alias, int alias_len, char **error TSRMLS_DC)
        long halt_offset;
        zval *halt_constant;
        php_stream *fp;
-       int fname_len, is_data = 0;
+       int fname_len;
 
        if (error) {
                *error = NULL;
@@ -1851,11 +1857,7 @@ int phar_open_compiled_file(char *alias, int alias_len, char **error TSRMLS_DC)
        fname = zend_get_executed_filename(TSRMLS_C);
        fname_len = strlen(fname);
 
-       if (!strstr(fname, ".phar")) {
-               is_data = 1;
-       }
-
-       if (phar_open_loaded(fname, fname_len, alias, alias_len, is_data, REPORT_ERRORS, NULL, 0 TSRMLS_CC) == SUCCESS) {
+       if (phar_open_loaded(fname, fname_len, alias, alias_len, 0, REPORT_ERRORS, NULL, 0 TSRMLS_CC) == SUCCESS) {
                return SUCCESS;
        }
 

diff --git a/ext/phar/tests/security.phpt b/ext/phar/tests/security.phpt
new file mode 100644 (file)
index 0000000..2d54db7
--- /dev/null
+++ b/ext/phar/tests/security.phpt
@@ -0,0 +1,36 @@
+--TEST--
+Phar: test to ensure phar.readonly cannot be circumvented
+--SKIPIF--
+<?php if (!extension_loaded("phar")) die("skip");?>
+--INI--
+phar.readonly=0
+--FILE--
+<?php
+$fname = dirname(__FILE__) . '/' . basename(__FILE__, '.php') . '.phar.php';
+$fname2 = dirname(__FILE__) . '/' . basename(__FILE__, '.php') . '.1.php';
+$a = new Phar($fname);
+$a->setStub('<?php
+Phar::mapPhar();
+$phar = new Phar(__FILE__);
+var_dump($phar->isWritable());
+try {
+$phar["b"] = "should not work!";
+} catch (Exception $e) {
+echo $e->getMessage(),"\n";
+}
+__HALT_COMPILER();
+?>');
+$a['hi'] = 'hi';
+unset($a);
+copy($fname, $fname2);
+Phar::unlinkArchive($fname);
+ini_set('phar.readonly', 1);
+include $fname2;
+?>
+===DONE===
+--CLEAN--
+<?php unlink(dirname(__FILE__) . '/' . basename(__FILE__, '.clean.php') . '.1.php'); ?>
+--EXPECT--
+bool(false)
+Write operations disabled by INI setting
+===DONE===
\ No newline at end of file

diff --git a/ext/phar/util.c b/ext/phar/util.c
index 9b0dc279a5174b2086df400db7fa977e0b728d23..f36d9a031c888f2262fcb85696ebe25c0d678bc6 100644 (file)
--- a/ext/phar/util.c
+++ b/ext/phar/util.c
@@ -225,7 +225,7 @@ char *phar_find_in_include_path(char *filename, int filename_len, phar_archive_d
                return phar_save_resolve_path(filename, filename_len TSRMLS_CC);
        }
        fname = zend_get_executed_filename(TSRMLS_C);
-       if (SUCCESS != phar_split_fname(fname, strlen(fname), &arch, &arch_len, &entry, &entry_len, 0, 0 TSRMLS_CC)) {
+       if (SUCCESS != phar_split_fname(fname, strlen(fname), &arch, &arch_len, &entry, &entry_len, 1, 0 TSRMLS_CC)) {
                return phar_save_resolve_path(filename, filename_len TSRMLS_CC);
        }
        if (*filename == '.') {
@@ -267,7 +267,7 @@ char *phar_find_in_include_path(char *filename, int filename_len, phar_archive_d
                ret_len = strlen(ret);
                /* found phar:// */
 
-               if (SUCCESS != phar_split_fname(ret, ret_len, &arch, &arch_len, &entry, &entry_len, 0, 0 TSRMLS_CC)) {
+               if (SUCCESS != phar_split_fname(ret, ret_len, &arch, &arch_len, &entry, &entry_len, 1, 0 TSRMLS_CC)) {
                        return ret;
                }
                zend_hash_find(&(PHAR_GLOBALS->phar_fname_map), arch, arch_len, (void **) &pphar);
@@ -293,7 +293,7 @@ char *phar_find_in_include_path(char *filename, int filename_len, phar_archive_d
                goto doit;
        }
        fname = zend_get_executed_filename(TSRMLS_C);
-       if (SUCCESS != phar_split_fname(fname, strlen(fname), &arch, &arch_len, &entry, &entry_len, 0, 0 TSRMLS_CC)) {
+       if (SUCCESS != phar_split_fname(fname, strlen(fname), &arch, &arch_len, &entry, &entry_len, 1, 0 TSRMLS_CC)) {
                goto doit;
        }
 
@@ -416,7 +416,7 @@ not_stream:
                                                        ret_len = strlen(trypath);
                                                        /* found phar:// */
 
-                                                       if (SUCCESS != phar_split_fname(trypath, ret_len, &arch, &arch_len, &entry, &entry_len, 0, 0 TSRMLS_CC)) {
+                                                       if (SUCCESS != phar_split_fname(trypath, ret_len, &arch, &arch_len, &entry, &entry_len, 1, 0 TSRMLS_CC)) {
                                                                return estrndup(trypath, ret_len);
                                                        }
                                                        zend_hash_find(&(PHAR_GLOBALS->phar_fname_map), arch, arch_len, (void **) &pphar);