From a919e2f858e0d9f1247dca8e87bab5f8ad6ec09a Mon Sep 17 00:00:00 2001 From: Greg Beaver Date: Thu, 8 May 2008 00:49:37 +0000 Subject: [PATCH] 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 --- ext/phar/cgidebug | 2 +- ext/phar/phar.c | 34 ++++++++++++++++++---------------- ext/phar/tests/security.phpt | 36 ++++++++++++++++++++++++++++++++++++ ext/phar/util.c | 8 ++++---- 4 files changed, 59 insertions(+), 21 deletions(-) create mode 100644 ext/phar/tests/security.phpt diff --git a/ext/phar/cgidebug b/ext/phar/cgidebug index db666970ef..bacf6910a6 100755 --- 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 b668e043e4..411decb84e 100644 --- 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 index 0000000000..2d54db7908 --- /dev/null +++ b/ext/phar/tests/security.phpt @@ -0,0 +1,36 @@ +--TEST-- +Phar: test to ensure phar.readonly cannot be circumvented +--SKIPIF-- + +--INI-- +phar.readonly=0 +--FILE-- +setStub('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-- + +--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 9b0dc279a5..f36d9a031c 100644 --- 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); -- 2.40.0