Skip to content

Implement script only include #1111

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 8 commits into from
Closed

Implement script only include #1111

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a53cfb5
Implement script only include
Feb 22, 2015
f684d16
Fixed global initialization
Feb 22, 2015
392b2b8
Adjust tests
Feb 22, 2015
9e45a00
Fixed build
Feb 22, 2015
2160541
Added allow_script parameter to move_uploaded_file(). Use constant fo…
Feb 22, 2015
65333ed
Fixed build. Compiler should catch this...
Feb 23, 2015
4fd3f29
Do not modify INI value
Feb 23, 2015
aa35890
Use better name for static function
Feb 23, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions Zend/tests/script_extensions-dummy1.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
<?php
/* dummy */
echo __FILE__."\n";
?>
4 changes: 4 additions & 0 deletions Zend/tests/script_extensions-dummy1.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
<?php
/* dummy */
echo __FILE__."\n";
?>
4 changes: 4 additions & 0 deletions Zend/tests/script_extensions-dummy2.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
<?php
/* dummy */
echo __FILE__."\n";
?>
4 changes: 4 additions & 0 deletions Zend/tests/script_extensions-dummy2.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
<?php
/* dummy */
echo __FILE__."\n";
?>
25 changes: 25 additions & 0 deletions Zend/tests/script_extensions001.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
--TEST--
zend.script_extensions under CLI
--INI--
display_errors=Off
--SKIPIF--
<?php
if (PHP_SAPI != 'cli') {
die('Skip - CLI only test');
}
--FILE--
<?php
include('script_extensions-dummy1.inc');
include_once('script_extensions-dummy1.inc');
include_once('script_extensions-dummy1.php');
require('script_extensions-dummy2.inc');
require_once('script_extensions-dummy2.inc');
require_once('script_extensions-dummy2.php');
echo "Done\n";
?>
--EXPECTF--
%sscript_extensions-dummy1.inc
%sscript_extensions-dummy1.php
%sscript_extensions-dummy2.inc
%sscript_extensions-dummy2.php
Done
28 changes: 28 additions & 0 deletions Zend/tests/script_extensions002.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
--TEST--
zend.script_extensions under CGI
--INI--
display_errors=On
display_startup_errors=On
--SKIPIF--
<?php
if (strstr(PHP_SAPI, 'cgi') == FALSE) {
die('Skip - CGI only test');
}
--FILE--
<?php
include_once('script_extensions-dummy1.php');
require_once('script_extensions-dummy2.php');
include('script_extensions-dummy1.inc');
require('script_extensions-dummy2.inc');
include_once('script_extensions-dummy1.inc');
require_once('script_extensions-dummy2.inc');
echo "Done\n";
?>
--EXPECTF--
X-Powered-By: PHP/%s
Content-type: text/html; charset=UTF-8

%sscript_extensions-dummy1.php
%sscript_extensions-dummy2.php

Fatal error: Illegal script extension detected in %s on line %d
43 changes: 43 additions & 0 deletions Zend/zend.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,40 @@ static ZEND_INI_MH(OnUpdateGCEnabled) /* {{{ */
}
/* }}} */


static ZEND_INI_MH(OnUpdateScriptExtensions) /* {{{ */
{
char *tmp, *str, *save_ptr, *token;
int i;

if (new_value && new_value->len) {
tmp = str = estrndup(new_value->val, new_value->len);
for(str = new_value->val, i = 0; ; str = NULL) {
token = strtok_r(str, " ", &save_ptr);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use php_strtok_r

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, also seems save_ptr isn't initialized here.

if (CG(script_extensions)[i]) {
pefree(CG(script_extensions)[i], 1);
}
if (!token) {
break;
}
CG(script_extensions)[i++] = pestrdup(token, 1);
if (i >= ZEND_MAX_SCRIPT_EXTENSIONS) {
efree(tmp);
return FAILURE;
}
}
efree(tmp);
CG(script_extensions)[i] = NULL;
} else {
if (CG(script_extensions)[0]) {
pefree(CG(script_extensions)[0], 1);
}
CG(script_extensions[0]) = NULL;
}
return SUCCESS;
}
/* }}} */

static ZEND_INI_MH(OnUpdateScriptEncoding) /* {{{ */
{
if (!CG(multibyte)) {
Expand All @@ -103,6 +137,7 @@ static ZEND_INI_MH(OnUpdateScriptEncoding) /* {{{ */
ZEND_INI_BEGIN()
ZEND_INI_ENTRY("error_reporting", NULL, ZEND_INI_ALL, OnUpdateErrorReporting)
STD_ZEND_INI_BOOLEAN("zend.enable_gc", "1", ZEND_INI_ALL, OnUpdateGCEnabled, gc_enabled, zend_gc_globals, gc_globals)
ZEND_INI_ENTRY("zend.script_extensions", ".php .phar", ZEND_INI_ALL, OnUpdateScriptExtensions)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

requiring . means if somebody puts inc there (instead of .inc) they are still vulnerable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If someone put ".php .phar .inc" then, "script.inc" is PHP script. However, move_uploaded_file() will be updated not to allow ".php .phar .inc", so users are protected from script inclusion as long as they use consistent setting.
I forgot about move_uploaded_file() part. I'll add it soon.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extension is the part after the dot. 👎 for requiring the dot, also it is redundant.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, but it requires additional code for it. i.e. To check "php", I need to check "." existence before "php". It's simple check. I'm not sure if it worthes it or not. Anyone?

BTW, httpd.conf uses ".php" as matched filename extension.

<FilesMatch ¥.php$>
SetHandler application/x-httpd-php
</FilesMatch>

We shouldn't use AddHandler for PHP script, but

AddHandler application/x-handler .ext

is the way to specify filename extension.

STD_ZEND_INI_BOOLEAN("zend.multibyte", "0", ZEND_INI_PERDIR, OnUpdateBool, multibyte, zend_compiler_globals, compiler_globals)
ZEND_INI_ENTRY("zend.script_encoding", NULL, ZEND_INI_ALL, OnUpdateScriptEncoding)
STD_ZEND_INI_BOOLEAN("zend.detect_unicode", "1", ZEND_INI_ALL, OnUpdateBool, detect_unicode, zend_compiler_globals, compiler_globals)
Expand Down Expand Up @@ -425,6 +460,7 @@ static void compiler_globals_ctor(zend_compiler_globals *compiler_globals) /* {{
} else {
compiler_globals->static_members_table = NULL;
}
memset(compiler_globals->script_extensions, 0, sizeof(char *) * ZEND_MAX_SCRIPT_EXTENSIONS);
compiler_globals->script_encoding_list = NULL;

#ifdef ZTS
Expand All @@ -437,6 +473,8 @@ static void compiler_globals_ctor(zend_compiler_globals *compiler_globals) /* {{

static void compiler_globals_dtor(zend_compiler_globals *compiler_globals) /* {{{ */
{
int i;

if (compiler_globals->function_table != GLOBAL_FUNCTION_TABLE) {
zend_hash_destroy(compiler_globals->function_table);
free(compiler_globals->function_table);
Expand All @@ -455,6 +493,11 @@ static void compiler_globals_dtor(zend_compiler_globals *compiler_globals) /* {{
if (compiler_globals->script_encoding_list) {
pefree((char*)compiler_globals->script_encoding_list, 1);
}
for(i = 0; i < ZEND_MAX_SCRIPT_EXTENSIONS; i++) {
if (compiler_globals->script_extensions[i]) {
pefree(compiler_globals->script_extensions[i], 1);
}
}
compiler_globals->last_static_member = 0;

#ifdef ZTS
Expand Down
4 changes: 3 additions & 1 deletion Zend/zend_globals.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ END_EXTERN_C()
#endif

#define SYMTABLE_CACHE_SIZE 32

#define ZEND_MAX_SCRIPT_EXTENSIONS 32

#include "zend_compile.h"

Expand Down Expand Up @@ -130,6 +130,8 @@ struct _zend_compiler_globals {

HashTable interned_strings;

char *script_extensions[ZEND_MAX_SCRIPT_EXTENSIONS];

const zend_encoding **script_encoding_list;
size_t script_encoding_list_size;
zend_bool multibyte;
Expand Down
Loading