[RFC] Script only include/require

php.internals

Yasuo Ohgaki

11 years ago
Hi all, I wrote patch and made adjustment in the RFC https://wiki.php.net/rfc/script_only_include https://github.com/php/php-src/pull/1111 Where to check filename extension is subject to be changed. At first, I thought implementing this as PHP code is good, but I've changed my mind. It seems better to be done in Zend code. Opinions are appreciated. This RFC aims to make PHP as secure as other languages with respect to "script inclusion" attacks. Note: File inclusion is not a scope of this RFC. INI Changes: - "php_script" -> "zend.script_extensions" - "Allow all files": "*" -> NULL or "" Open Issues: - Error type - Is it OK to raise E_ERROR/E_RECOVERABLE_ERROR in zend_language_scanner.c? - Vote type - 50%+1 or 2/3 If there is anyone who would like to vote "no" for this RFC, I would like to know the reason and try to address/resolve issue you have. Thank you. Regards,
-- Yasuo Ohgaki yohgaki@ohgaki.net

Yasuo Ohgaki

11 years ago
Hi Dmitry and Nikita, On Mon, Feb 23, 2015 at 6:23 AM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:
> I wrote patch and made adjustment in the RFC > https://wiki.php.net/rfc/script_only_include > https://github.com/php/php-src/pull/1111 > Where to check filename extension is subject to be changed. > At first, I thought implementing this as PHP code is good, but > I've changed my mind. It seems better to be done in Zend code. > Opinions are appreciated. > > This RFC aims to make PHP as secure as other languages > with respect to "script inclusion" attacks. > Note: File inclusion is not a scope of this RFC. > > INI Changes: > - "php_script" -> "zend.script_extensions" > - "Allow all files": "*" -> NULL or "" > > Open Issues: > - Error type - Is it OK to raise E_ERROR/E_RECOVERABLE_ERROR in > zend_language_scanner.c? > - Vote type - 50%+1 or 2/3 > > If there is anyone who would like to vote "no" for this RFC, > I would like to know the reason and try to address/resolve issue you have. > > Thank you. >
We don't have care much about which error is raised from Zend engine, since there will be engine exception. My questions are, is it ok to raise E_ERROR or E_RECOVERABLE_ERROR from zend_language_scanner.c? https://github.com/php/php-src/pull/1111/files#diff-93ad74868f98ff7232ebea00007c8b7fR624 Does engine exception catches error from zend_error_noreturn()? Thank you. Regards,
-- Yasuo Ohgaki yohgaki@ohgaki.net

Dmitry Stogov

11 years ago
On Mon, Feb 23, 2015 at 6:55 AM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:
> Hi Dmitry and Nikita, > > On Mon, Feb 23, 2015 at 6:23 AM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote: > >> I wrote patch and made adjustment in the RFC >> https://wiki.php.net/rfc/script_only_include >> https://github.com/php/php-src/pull/1111 >> Where to check filename extension is subject to be changed. >> At first, I thought implementing this as PHP code is good, but >> I've changed my mind. It seems better to be done in Zend code. >> Opinions are appreciated. >> >> This RFC aims to make PHP as secure as other languages >> with respect to "script inclusion" attacks. >> Note: File inclusion is not a scope of this RFC. >> >> INI Changes: >> - "php_script" -> "zend.script_extensions" >> - "Allow all files": "*" -> NULL or "" >> >> Open Issues: >> - Error type - Is it OK to raise E_ERROR/E_RECOVERABLE_ERROR in >> zend_language_scanner.c? >> - Vote type - 50%+1 or 2/3 >> >> If there is anyone who would like to vote "no" for this RFC, >> I would like to know the reason and try to address/resolve issue you have. >> >> Thank you. >> > > We don't have care much about which error is raised from Zend engine, > since there > will be engine exception. > > My questions are, is it ok to raise E_ERROR or E_RECOVERABLE_ERROR from > zend_language_scanner.c? >
Use E_ERROR.
> > > https://github.com/php/php-src/pull/1111/files#diff-93ad74868f98ff7232ebea00007c8b7fR624 > > Does engine exception catches error from zend_error_noreturn()? >
no. it'll be changed into zend_error(). I'm not a security expert, but I think that adding check for script extension won't add significant level of protection. Thanks. Dmitry.

Yasuo Ohgaki

11 years ago
Hi Dmitry, On Tue, Feb 24, 2015 at 4:00 PM, Dmitry Stogov <dmitry@zend.com> wrote:
> Use E_ERROR. > > >> >> >> https://github.com/php/php-src/pull/1111/files#diff-93ad74868f98ff7232ebea00007c8b7fR624 >> >> Does engine exception catches error from zend_error_noreturn()? >> > > no. it'll be changed into zend_error(). >
Thank you for the comment. I'm not a security expert, but I think that adding check for script
> extension won't add significant level of protection. >
I agree. For developers who have more than average skills, this RFC would not be helpful. File inclusions by readfile()/etc are fatal as well also. Users must be careful anyway. My objective is to reduce risk of server takeover by script inclusions as low as other languages and being nice to new developers. I've audited number of web applications written by various languages, there aren't much difference in programmers' skills. My samples are too few and do not represent actual figures, but we'll have less vulnerable PHP apps by this. IMHO. Regards,
-- Yasuo Ohgaki yohgaki@ohgaki.net

Yasuo Ohgaki

11 years ago
Hi all, On Tue, Feb 24, 2015 at 7:20 PM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:
> On Tue, Feb 24, 2015 at 4:00 PM, Dmitry Stogov <dmitry@zend.com> wrote: > >> Use E_ERROR. >> >> >>> >>> >>> https://github.com/php/php-src/pull/1111/files#diff-93ad74868f98ff7232ebea00007c8b7fR624 >>> >>> Does engine exception catches error from zend_error_noreturn()? >>> >> >> no. it'll be changed into zend_error(). >> > > Thank you for the comment. > > I'm not a security expert, but I think that adding check for script >> extension won't add significant level of protection. >> > > I agree. For developers who have more than average skills, this RFC > would not be helpful. File inclusions by readfile()/etc are fatal as well > also. Users must be careful anyway. > > My objective is to reduce risk of server takeover by script inclusions > as low as other languages and being nice to new developers. I've audited > number of web applications written by various languages, there aren't much > difference in programmers' skills. My samples are too few and do not > represent actual figures, but we'll have less vulnerable PHP apps by this. > IMHO. >
I would like to show one common example that is unique to PHP. https://www.google.co.jp/search?q=Exif+Webshell+Backdoor This RFC prevents this type of attack effectively. All users has to do is "checking file extension is image". Regards,
-- Yasuo Ohgaki yohgaki@ohgaki.net

Pádraic Brady

11 years ago
Hi Dmitry, On 24 February 2015 at 07:00, Dmitry Stogov <dmitry@zend.com> wrote:
> I'm not a security expert, but I think that adding check for script > extension won't add significant level of protection.
Will it add a significant level of protection? No. Does it add protection? Yes. Each time we add some incremental security hardening, we make it a bit harder to create vulnerabilities. In this case, if there were code injection issue, the attacker must a) include a local file (not always useful) or b) upload some other apparently innocent file capable of being included (extremely useful). As such, this patch would lock out an obvious path by restricting the files that can be included to a more limited subset. Enough incremental improvements add up to a significant improvement. Paddy
-- Pádraic Brady http://blog.astrumfutura.com http://www.survivethedeepend.com

Stas Malyshev

11 years ago
Hi!
> Will it add a significant level of protection? No. > > Does it add protection? Yes. > > Each time we add some incremental security hardening, we make it a bit > harder to create vulnerabilities. In this case, if there were code
In this case, it seems not to be much harder than changing an URL a bit or uploading a file under different extension. OTOH, it creates a false sense of security - oh, I'm using the secure settings, now I can forget about caring for LFI! - and also has huge BC break potential. For me, it looks like magic quotes comeback.
> injection issue, the attacker must a) include a local file (not always > useful) or b) upload some other apparently innocent file capable of > being included (extremely useful). As such, this patch would lock out > an obvious path by restricting the files that can be included to a > more limited subset.
Unless you disable phar, you can still include pretty much anything by just using phar includes, as far as I can see. I'm pretty sure there are also other stream tricks possible (data://? zip://?)
> Enough incremental improvements add up to a significant improvement.
If that were always true, safe mode and magic quotes would still be here with us.
-- Stas Malyshev smalyshev@gmail.com

Pádraic Brady

11 years ago
Hi On Tuesday, February 24, 2015, Stanislav Malyshev <smalyshev@gmail.com> wrote:
> Hi! > > > Will it add a significant level of protection? No. > > > > Does it add protection? Yes. > > > > Each time we add some incremental security hardening, we make it a bit > > harder to create vulnerabilities. In this case, if there were code > > In this case, it seems not to be much harder than changing an URL a bit > or uploading a file under different extension. OTOH, it creates a false > sense of security - oh, I'm using the secure settings, now I can forget > about caring for LFI! - and also has huge BC break potential. For me, it > looks like magic quotes comeback.
They'd need to upload with a matching file type. Instead of any file types. Fewer possible types is by definition less than all types. This is not even remotely magic quotes. No input is altered.
> > > injection issue, the attacker must a) include a local file (not always > > useful) or b) upload some other apparently innocent file capable of > > being included (extremely useful). As such, this patch would lock out > > an obvious path by restricting the files that can be included to a > > more limited subset. > > Unless you disable phar, you can still include pretty much anything by > just using phar includes, as far as I can see. I'm pretty sure there are > also other stream tricks possible (data://? zip://?)
None of this detracts from limiting file includes. Other potential weaknesses could be addressed separately if you agree there's more than one addressed not addressed here. One might say...incrementally. Also, we are obviously talking about PHP includes with this RFC...
> > Enough incremental improvements add up to a significant improvement. > > If that were always true, safe mode and magic quotes would still be here > with us. >
You keep mentioning magic quotes. That was never an improvement. It was removed from PHP. Please stop trying to associate two unrelated things to establish bad practice by word proximity. The sentence is obviously true. Paddy
-- -- Pádraic Brady http://blog.astrumfutura.com http://www.survivethedeepend.com Zend Framework Community Review Team Zend Framework PHP-FIG Representative

Yasuo Ohgaki

11 years ago
Hi Stas, On Wed, Feb 25, 2015 at 5:33 AM, Pádraic Brady <padraic.brady@gmail.com> wrote:
> On Tuesday, February 24, 2015, Stanislav Malyshev <smalyshev@gmail.com> > wrote: > >> Hi! >> >> > Will it add a significant level of protection? No. >> > >> > Does it add protection? Yes. >> > >> > Each time we add some incremental security hardening, we make it a bit >> > harder to create vulnerabilities. In this case, if there were code >> >> In this case, it seems not to be much harder than changing an URL a bit >> or uploading a file under different extension. OTOH, it creates a false >> sense of security - oh, I'm using the secure settings, now I can forget >> about caring for LFI! - and also has huge BC break potential. For me, it >> looks like magic quotes comeback. > > > They'd need to upload with a matching file type. Instead of any file > types. Fewer possible types is by definition less than all types. > > This is not even remotely magic quotes. No input is altered. >
I would like to add a note for this. Anti Virus products are detecting this type of files as "PHP malware". No other languages have such malware. According to recent F-Secure blog post, this type of "PHP malware" files are not decreasing but increasing. Other than this type of "PHP malware", "PHP WebShell" is detected as PHP malware by anti virus products. The reason why these has to detected as "PHP malware" is that there are PHP programs vulnerable to script inclusion attacks. Leaving this as it is now would make people think "PHP is insecure than other languages", "Wow, we have many PHP malware. We may be better not to use PHP anymore". If "PHP malware" is found in a server, developers are force to check their code. Or they have to ask costly code check to people like me, even when PHP programs is safe. If this RFC is accepted, developers can prove their PHP programs are safe without code check. This RFC benefits may not be obvious for people on this list, but this RFC eliminates certain type of "PHP malware". PHP's script inclusion is a toy for security researcher and attackers for a long time. Let's take away the toy from them. Regards,
-- Yasuo Ohgaki yohgaki@ohgaki.net

Stas Malyshev

11 years ago
Hi!
> I would like to add a note for this. > Anti Virus products are detecting this type of files as "PHP malware".
It looks like you are trying to convince me that PHP malware exists. I would like to save you time by notifying you I am aware of this. My disagreement is not denying PHP malware exists, it is denying that your proposed change does anything to improve situations with code vulnerable to externally controlled includes. There may be a way to mitigate this problem, but I don't see how requiring that .php would be at the end of the filename would be it.
> No other languages have such malware.
Are you seriously claiming there is no malware written in languages besides PHP? It can not be, I must be misunderstanding what you mean here.
> The reason why these has to detected as "PHP malware" is that there are > PHP programs vulnerable to script inclusion attacks.
No, it's not the reason, at least not the main one. The reason is that: a. PHP is an easy language to write code in and is widely deployed b. Writing a remote control kit in PHP is easier than in C, etc. and there's more guarantee it would work on any random PHP host c. There are lots of vulnerable web hosts that have remote execution vulnerabilities and can be exploited
> Leaving this as it is now would make people think "PHP is insecure than > other languages", "Wow, we have many PHP malware. We may be better > not to use PHP anymore".
People that think that are illogical - the fact that somebody chose to write a remote control toolkit in PHP due to PHP'd high footprint on the web has absolutely nothing to do with PHP being less secure. It's like saying Ford cars are insecure because somebody robbed a bank and then drove away in a Ford car. We should pay absolutely zero attention to the opinion of people that are so confused, and instead educate them about the real situation. Of course, if people run no PHP server at all, PHP-driven remote control kits would not be useful. But if the server is vulnerable, there are many other backdoor kits.
> If "PHP malware" is found in a server, developers are force to check > their code. Or they have to ask costly code check to people like me, > even when PHP programs is safe. If this RFC is accepted, developers > can prove their PHP programs are safe without code check.
I do not see how you change leads to anything like that.
> This RFC benefits may not be obvious for people on this list, but this > RFC eliminates certain type of "PHP malware". PHP's script inclusion
I can't think of any type of PHP malware that would be eliminated. At most, the malware injection protocols have to be slightly modified to work around initial hurdle of not being able to pass files with extension .php through move_upload_file(). With RCE vulnerability its trivial, with RFI one based on uploads it is a little harder, but only insignificantly - if I am not mistaken, in the last email I provided a workaround and it took me less than 5 minutes to come up with it, without being professional exploit writer.
> is a toy for security researcher and attackers for a long time. > Let's take away the toy from them.
It may be worth to take away the toy, but this change just moves the toy couple of centimeters aside. Given the BC break potential, I don't see much point.
-- Stas Malyshev smalyshev@gmail.com

Yasuo Ohgaki

11 years ago
Hi Stas, On Wed, Feb 25, 2015 at 7:26 AM, Stanislav Malyshev <smalyshev@gmail.com> wrote:
> > I would like to add a note for this. > > Anti Virus products are detecting this type of files as "PHP malware". > > It looks like you are trying to convince me that PHP malware exists. I > would like to save you time by notifying you I am aware of this. My > disagreement is not denying PHP malware exists, it is denying that your > proposed change does anything to improve situations with code vulnerable > to externally controlled includes. There may be a way to mitigate this > problem, but I don't see how requiring that .php would be at the end of > the filename would be it. >
I'm presenting the fact that "PHP script embedded malware" exists in the wild and malware vendors detect them as "PHP malware".
> > > No other languages have such malware. > > Are you seriously claiming there is no malware written in languages > besides PHP? It can not be, I must be misunderstanding what you mean here. >
Malwares are written by many languages. It's the fact. As far as I know, PHP is the only language that has this type of malware. (Script embedded images) PHP is the only one malware vendors claims it as "PHP malware". This is the fact.
> The reason why these has to detected as "PHP malware" is that there are > > PHP programs vulnerable to script inclusion attacks. > > No, it's not the reason, at least not the main one. The reason is that: > a. PHP is an easy language to write code in and is widely deployed > b. Writing a remote control kit in PHP is easier than in C, etc. and > there's more guarantee it would work on any random PHP host > c. There are lots of vulnerable web hosts that have remote execution > vulnerabilities and can be exploited >
The reason is other languages are almost safe by default against script inclusion attacks. This RFC makes PHP safe by default just like other languages + move_uploaded_file() protection.
> > > Leaving this as it is now would make people think "PHP is insecure than > > other languages", "Wow, we have many PHP malware. We may be better > > not to use PHP anymore". > > People that think that are illogical - the fact that somebody chose to > write a remote control toolkit in PHP due to PHP'd high footprint on the > web has absolutely nothing to do with PHP being less secure. It's like > saying Ford cars are insecure because somebody robbed a bank and then > drove away in a Ford car. We should pay absolutely zero attention to the > opinion of people that are so confused, and instead educate them about > the real situation. > > Of course, if people run no PHP server at all, PHP-driven remote control > kits would not be useful. But if the server is vulnerable, there are > many other backdoor kits. >
People do not have to be exparts of developing softwares. Managers will choose illogical choice.
> If "PHP malware" is found in a server, developers are force to check > > their code. Or they have to ask costly code check to people like me, > > even when PHP programs is safe. If this RFC is accepted, developers > > can prove their PHP programs are safe without code check. > > I do not see how you change leads to anything like that. >
1. Anti-virus detects "PHP malware" 2. Managers surprises possible attack (Server takeover) 3. Developers are forced to check their code, since current PHP has no effective script inclusion attack prevention With this RFC, developers can explain this type of attacks cannot be done by PHP's feature. i.e. Exploit servers via script embedded images, etc cannot be done.
> This RFC benefits may not be obvious for people on this list, but this > > RFC eliminates certain type of "PHP malware". PHP's script inclusion > > I can't think of any type of PHP malware that would be eliminated. At > most, the malware injection protocols have to be slightly modified to > work around initial hurdle of not being able to pass files with > extension .php through move_upload_file(). With RCE vulnerability its > trivial, with RFI one based on uploads it is a little harder, but only > insignificantly - if I am not mistaken, in the last email I provided a > workaround and it took me less than 5 minutes to come up with it, > without being professional exploit writer. >
Embedded PHP script uploads are prohibited by this RFC by default.
> is a toy for security researcher and attackers for a long time. > > Let's take away the toy from them. > > It may be worth to take away the toy, but this change just moves the toy > couple of centimeters aside. Given the BC break potential, I don't see > much point.
PHP became as secure as other languages with respect to script inclusions. by default. The issue here is "PHP is not being as secure as other language with respect to script inclusion attacks". Statistics shows it. This is our issue. Users may shoot their own foot, this is not our issue. Regards,
-- Yasuo Ohgaki yohgaki@ohgaki.net

Stas Malyshev

11 years ago
Hi!
> As far as I know, PHP is the only language that has this type of malware. > (Script embedded images) PHP is the only one malware vendors claims > it as "PHP malware". This is the fact.
Which type is that? Of course only malware in PHP can be presented as "PHP malware", but I don't understand why it is of any significance.
> The reason is other languages are almost safe by default against script > inclusion attacks.
Many languages just don't use patterns like PHP code does - I don't think I ever seen Python code doing imports based on variables - I'm not even sure it's possible in Python. PHP has more capabilities, but of course you need to use them in the right way.
> This RFC makes PHP safe by default just like other languages + > move_uploaded_file() > protection.
No, it does not - I've shown the example why. I'm sure there are more.
> People do not have to be exparts of developing softwares. Managers will > choose illogical choice.
We should not base our decision on the opinions of people we all understand are ignorant.
> 1. Anti-virus detects "PHP malware" > 2. Managers surprises possible attack (Server takeover) > 3. Developers are forced to check their code, since current PHP has no > effective script inclusion attack prevention > > With this RFC, developers can explain this type of attacks cannot be done > by PHP's feature. i.e. Exploit servers via script embedded images, etc > cannot > be done.
I don't think we need to introduce BC-breaking feature in PHP just because somebody has a manager that can't understand the basics of security.
> Embedded PHP script uploads are prohibited by this RFC by default.
Only certain very narrow cases of it.
> > PHP became as secure as other languages with respect to script inclusions.
You keep repeating that, but it's not the case, and PHP already is as secure as other languages - provided you do not use clearly broken code. Security of the language is a misnomer anyway - language can not be secure (unless it's a language that does nothing useful), only specific code can be. Code that allows user-controlled includes without adequate filtering is insecure, and pretending that we make it secure does not improve security, quite the contrary.
> Users may shoot their own foot, this is not our issue.
But that's exactly what is required for your change to be useful at all!
-- Stas Malyshev smalyshev@gmail.com

Jan Ehrhardt

11 years ago
Yasuo Ohgaki in php.internals (Wed, 25 Feb 2015 07:54:01 +0900):
>On Wed, Feb 25, 2015 at 7:26 AM, Stanislav Malyshev <smalyshev@gmail.com> >wrote: > >> > No other languages have such malware. >> >> Are you seriously claiming there is no malware written in languages >> besides PHP? It can not be, I must be misunderstanding what you mean here. > >Malwares are written by many languages. It's the fact. > >As far as I know, PHP is the only language that has this type of malware. >(Script embedded images) PHP is the only one malware vendors claims >it as "PHP malware". This is the fact.
Google for "java malware" and you'll find things like http://www.javaworld.com/article/2104862/java-security/report-half-of-all-exploits-target-java.html Jan

Pádraic Brady

11 years ago
Hi,
>> This RFC benefits may not be obvious for people on this list, but this >> RFC eliminates certain type of "PHP malware". PHP's script inclusion > > I can't think of any type of PHP malware that would be eliminated. At > most, the malware injection protocols have to be slightly modified to > work around initial hurdle of not being able to pass files with > extension .php through move_upload_file(). With RCE vulnerability its > trivial, with RFI one based on uploads it is a little harder, but only > insignificantly - if I am not mistaken, in the last email I provided a > workaround and it took me less than 5 minutes to come up with it, > without being professional exploit writer.
You might want to carefully read Yasuo's sentence about "certain" types which is not the same as "all" types. You seem to be exaggerating the claimed benefit of the RFC and using those exaggerated claims (and their debunking) as evidence against the RFC. In this, you are seriously off topic. The RFC makes a very simple claim about limiting includes to specific file extensions. It does not validate the files - the implicit assumption is the files are pre-validated so it exists to mop up certain edge cases that may bypass validation. This is just basic defense in depth. Paddy
-- Pádraic Brady http://blog.astrumfutura.com http://www.survivethedeepend.com

Stas Malyshev

11 years ago
Hi!
> They'd need to upload with a matching file type. Instead of any file
Not sure what you mean by that. phar can read tars, etc. AFAIK, can't it? Also, phar archive has no requirement of being named something.phar, afaik can be also named cuteponies.gif. E.g., I just did this: 1. Created file chump.php: <?php include $argv[1]; This is an idealized vulnerable script. 2. Created file pwnd.php <?php echo "pwnd!"; This is an idealized exploit. 3. Put it into an archive: tar cvzf cuteponies.gif pwnd.php 4. Run this: php -dallow_url_include=0 chump.php phar://cuteponies.gif/pwnd.php The output is: pwnd! I'm not sure how this measure would protect from such scenario. Am I missing something here?
> This is not even remotely magic quotes. No input is altered.
Don't be so literal. It's not about altering input, it's about the fact that it breaks stuff and not adds much to security.
> None of this detracts from limiting file includes. Other potential
Not sure what you mean. If you can pull off file include - which is a precondition of this feature being useful - then you can pull off phar include.
> weaknesses could be addressed separately if you agree there's more than > one addressed not addressed here. One might say...incrementally.
The problem is there's no increment there. It's like having a password hardcoded to "password". You can say "oh, it's incremental security, at least we have a password!" but it is not incrementing the actual security.
> You keep mentioning magic quotes. That was never an improvement. It was > removed from PHP. Please stop trying to associate two unrelated things
Yes, it was removed from PHP - exactly because it did not produce the attempted improvement in security. This feature is of the same kind - it tries to produce increase in security but fails. Thinking of it as a security feature would produce nothing but an endless stream of CVEs with PHP name attached to it. Not a good idea.
-- Stas Malyshev smalyshev@gmail.com

Yasuo Ohgaki

11 years ago
Hi Stas, On Wed, Feb 25, 2015 at 7:07 AM, Stanislav Malyshev <smalyshev@gmail.com> wrote:
> > They'd need to upload with a matching file type. Instead of any file > > Not sure what you mean by that. phar can read tars, etc. AFAIK, can't > it? Also, phar archive has no requirement of being named something.phar, > afaik can be also named cuteponies.gif. E.g., I just did this: > > 1. Created file chump.php: > > <?php > > include $argv[1]; > > This is an idealized vulnerable script. > > 2. Created file pwnd.php > <?php > > echo "pwnd!"; > > This is an idealized exploit. > > 3. Put it into an archive: > tar cvzf cuteponies.gif pwnd.php > > 4. Run this: > > php -dallow_url_include=0 chump.php phar://cuteponies.gif/pwnd.php > > The output is: > > pwnd! > > I'm not sure how this measure would protect from such scenario. Am I > missing something here?
I think he means matching file "extension". File extension should represent file type, though. The new RFC check filename extensions. It allows only ".php", ".phar" as PHP script and move_uploaded_file() restricts moving PHP scripts by default. (Old idea was to detect PHP script by contents. New RFC is to restrict PHP script file extension.) Since "pwnd.php" has ".php" extension, move_uploaded_file() refuses to move it to upload dir by default. As long as user uses default and move_uploaded_file(), they are free from script upload attacks including embedded script. Regards,
-- Yasuo Ohgaki yohgaki@ohgaki.net

Stas Malyshev

11 years ago
Hi!
> I think he means matching file "extension". File extension should > represent file type, though.
You can not rely on that. I can name files anything regardless of what's in the file.
> Since "pwnd.php" has ".php" extension, move_uploaded_file() refuses to > move it > to upload dir by default.
There's no pwnd.php. The file that I upload is "cuteponies.gif". Please look at the sequence again carefully.
-- Stas Malyshev smalyshev@gmail.com

Yasuo Ohgaki

11 years ago
Hi Stas, On Wed, Feb 25, 2015 at 7:31 AM, Stanislav Malyshev <smalyshev@gmail.com> wrote:
> > I think he means matching file "extension". File extension should > > represent file type, though. > > You can not rely on that. I can name files anything regardless of what's > in the file. > > > Since "pwnd.php" has ".php" extension, move_uploaded_file() refuses to > > move it > > to upload dir by default. > > There's no pwnd.php. The file that I upload is "cuteponies.gif". Please > look at the sequence again carefully.
require('cuteponies.gif) wouldn't work with this RFC. move_uploaded_files() prohibits uploading PHP script. I noticed that I should forbid destination file extension also by this discussion. I'll add it soon. Thank you. Regards,
-- Yasuo Ohgaki yohgaki@ohgaki.net

Pádraic Brady

11 years ago
Hi, On 24 February 2015 at 22:07, Stanislav Malyshev <smalyshev@gmail.com> wrote:
> Hi! > >> They'd need to upload with a matching file type. Instead of any file > > Not sure what you mean by that. phar can read tars, etc. AFAIK, can't > it? Also, phar archive has no requirement of being named something.phar, > afaik can be also named cuteponies.gif. E.g., I just did this:
Your example omitted the image validation step which would have noticed your attempt to upload a phar immediately. Add that and try again. It's not very fair to create a scenario with a total lack of any security, and then ignore that your code's problem is that gaping hole and NOT the minor extension filter on the far end. The control under debate was already provided with a preventable example by Yasuo pointing out how certain crafted images for file inclusion, which would bypass certain image validation checks, would indeed be preventable by his RFC. Please stick to what the RFC actually claims to do. Paddy
-- Pádraic Brady http://blog.astrumfutura.com http://www.survivethedeepend.com

Stas Malyshev

11 years ago
Hi!
> Your example omitted the image validation step which would have
Ah, right, and if I name it .zip, it'd be zip validation, and if I name it .pdf it'd be pdf validation, and if I name it .lol that would be LOL validation. You'd have to manually validate every type in existence and somehow invent validation for unknown ones. And it's not like I can't also make it a valid GIF/PDF/whatever - that has been done already. So you support this "security measure" by saying you can maybe plug the hole using other measures. Maybe you can, maybe (more probably) not but that does not redeem the insecurity of this "security measure".
> again. It's not very fair to create a scenario with a total lack of > any security, and then ignore that your code's problem is that gaping > hole and NOT the minor extension filter on the far end.
But that is *exactly* what this RFC is doing! The gaping security hole is include $argv[1] and that's where it should be fixed, not introducing temporary patches that prevent 1% of the scenarios and can be overridden within minutes. I don't see how it is not fair since the *only* scenario where it is useful is when your code *already* has the gaping security hole, otherwise this RFC has no utility as your includes are controlled by you so they already are php files.
> indeed be preventable by his RFC. Please stick to what the RFC > actually claims to do.
It claims to protect from file inclusion, by only allowing for include to operate on strings which end in .php, and then banning such files (ending in .php) from being handled by move_uploaded_file(). As I demonstrated (and this is I suspect not the only option) this does not actually offer any protection from LFI/RCE, as the end of the string given to include and the file on disk do not have to be the same. In my eyes, mechanism with such big BC break potential that is overridden in so trivial manner has little value. That even not considering upload doesn't even have to use move_uploaded_file() either.
-- Stas Malyshev smalyshev@gmail.com

Pádraic Brady

11 years ago
Hi On Wednesday, February 25, 2015, Stanislav Malyshev <smalyshev@gmail.com> wrote:
> Hi! > > > Your example omitted the image validation step which would have > > Ah, right, and if I name it .zip, it'd be zip validation, and if I name > it .pdf it'd be pdf validation, and if I name it .lol that would be LOL > validation. You'd have to manually validate every type in existence and > somehow invent validation for unknown ones. And it's not like I can't > also make it a valid GIF/PDF/whatever - that has been done already. So > you support this "security measure" by saying you can maybe plug the > hole using other measures. Maybe you can, maybe (more probably) not but > that does not redeem the insecurity of this "security measure".
Well, you fire those right over and then we'll have a debate worth having ;).
> > again. It's not very fair to create a scenario with a total lack of > > any security, and then ignore that your code's problem is that gaping > > hole and NOT the minor extension filter on the far end. > > But that is *exactly* what this RFC is doing! The gaping security hole > is include $argv[1] and that's where it should be fixed, not introducing > temporary patches that prevent 1% of the scenarios and can be overridden > within minutes.
RFC does not target invalidated uploads. For heavens sake, it's a defense in depth measure not a sentient AI!
> I don't see how it is not fair since the *only* scenario where it is > useful is when your code *already* has the gaping security hole, > otherwise this RFC has no utility as your includes are controlled by you > so they already are php files. >
That is not true! The lack of validation is one of degrees. You are speaking in absolutes and ignoring partial effectiveness. Your example had ZERO validation. Yasuo's clearly targeted successful validation of an image. Not none at all.
> > indeed be preventable by his RFC. Please stick to what the RFC > > actually claims to do. > > It claims to protect from file inclusion, by only allowing for include > to operate on strings which end in .php, and then banning such files > (ending in .php) from being handled by move_uploaded_file(). As I > demonstrated (and this is I suspect not the only option) this does not > actually offer any protection from LFI/RCE, as the end of the string > given to include and the file on disk do not have to be the same. In my > eyes, mechanism with such big BC break potential that is overridden in > so trivial manner has little value. >
No it doesn't! You are misrepresenting this RFC as a magic wand. That is not the case and it is extremely frustrating to see you persist on this. Read my emails and read Yasuo's and then Dan's. Then we can have some sort of intelligible discussion. Paddy
-- -- Pádraic Brady http://blog.astrumfutura.com http://www.survivethedeepend.com Zend Framework Community Review Team Zend Framework PHP-FIG Representative

Dan Ackroyd

11 years ago
On 25 February 2015 at 00:09, Pádraic Brady <padraic.brady@gmail.com> wrote:
> > Your example omitted the image validation step which would have > noticed your attempt to upload a phar immediately. Add that and try > again.
Image validation is no defense against this type of attack: http://php.webtutor.pl/en/2011/05/13/php-code-injection-a-simple-virus-written-in-php-and-carried-in-a-jpeg-image/ As soon as you have any possibility of including a file uploaded by an attacker, you are probably going to lose. cheers Dan

Pádraic Brady

11 years ago
Hi Dan On Wednesday, February 25, 2015, Dan Ackroyd <danack@basereality.com> wrote:
> On 25 February 2015 at 00:09, Pádraic Brady <padraic.brady@gmail.com > <javascript:;>> wrote: > > > > Your example omitted the image validation step which would have > > noticed your attempt to upload a phar immediately. Add that and try > > again. > > Image validation is no defense against this type of attack: > > > http://php.webtutor.pl/en/2011/05/13/php-code-injection-a-simple-virus-written-in-php-and-carried-in-a-jpeg-image/ > > As soon as you have any possibility of including a file uploaded by an > attacker, you are probably going to lose. > >
That was indeed my point as Yasuo has already explained earlier. Image validation would however see a phar a mile off. Paddy
-- -- Pádraic Brady http://blog.astrumfutura.com http://www.survivethedeepend.com Zend Framework Community Review Team Zend Framework PHP-FIG Representative

Yasuo Ohgaki

11 years ago
Hi Dan, On Wed, Feb 25, 2015 at 9:38 AM, Dan Ackroyd <danack@basereality.com> wrote:
> On 25 February 2015 at 00:09, Pádraic Brady <padraic.brady@gmail.com> > wrote: > > > > Your example omitted the image validation step which would have > > noticed your attempt to upload a phar immediately. Add that and try > > again. > > Image validation is no defense against this type of attack: > > > http://php.webtutor.pl/en/2011/05/13/php-code-injection-a-simple-virus-written-in-php-and-carried-in-a-jpeg-image/ > > As soon as you have any possibility of including a file uploaded by an > attacker, you are probably going to lose.
I know, and Padraic knows also, attacker can make image file that cannot remove "embedded PHP script" even with image resize. Even tool called "Image Fight" exists to fight against PHP script embedded images. I proposed to include/require to load specific file extensions, but I've got many objections for the idea. Therefore, I've tried to "detect" embedded "PHP script". However, it's complex and I cannot make sure there isn't embedded "PHP script" in a file. Current RFC is based on the original idea with additional move_uploaded_file() protection. It works well for the objective. Regards,
-- Yasuo Ohgaki yohgaki@ohgaki.net

Lester Caine

11 years ago
On 25/02/15 00:38, Dan Ackroyd wrote:
> As soon as you have any possibility of including a file uploaded by an > attacker, you are probably going to lose.
I think that this is perhaps the key here. My framework for new sites requires a user to log in before they can upload anything. So if your manager is worried about 'all this php malware' and your system allow unmanaged uploads ... all bets are off. The next thing I do with images is to create a thumbnail set, so only if you can get at the original file will there be a problem. I admit that I prefer to leave the file name unmanaged, but the option is to rename it original.xxx is also available. Anything uploaded that is not an image or can't be displayed as a thumbnail gets displayed as an icon, and viewing code as text gets the due diligence it deserves, so even if an approved user tries to add php script it will not be placed in a location where even if they could access it it could be run either directly or as an include file. I totally understand the basis of the RFC, I just don't see that creating a few more smoke and mirror obstacles to practices that are perfectly safe when used correctly but will add more work to 'web admins' who have to get around them even when their code is already safe? The code injection example only works on the basis "Imagine a piece of badly-written PHP code responsible for reading the image from disk and sending it to the browser:" This change does nothing to fix the badly-written code, but it is THAT which needs to be fixed rather than perfectly safe systems that 'disobey' this nannying?
-- Lester Caine - G8HFL ----------------------------- Contact - http://lsces.co.uk/wiki/?page=contact L.S.Caine Electronic Services - http://lsces.co.uk EnquirySolve - http://enquirysolve.com/ Model Engineers Digital Workshop - http://medw.co.uk Rainbow Digital Media - http://rainbowdigitalmedia.co.uk

Yasuo Ohgaki

11 years ago
Hi all, Zend engine experts especially, On Mon, Feb 23, 2015 at 6:23 AM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:
> I wrote patch and made adjustment in the RFC > https://wiki.php.net/rfc/script_only_include > https://github.com/php/php-src/pull/1111 > Where to check filename extension is subject to be changed. > At first, I thought implementing this as PHP code is good, but > I've changed my mind. It seems better to be done in Zend code. > Opinions are appreciated. >
I noticed very strange behavior under ZTS build with this patch. It turned out that compiler_globals is not accessible under ZTS build according to gdb. Is this intended? If so, where should I put script_extensions char array? Thank you.
-- Yasuo Ohgaki yohgaki@ohgaki.net

Stas Malyshev

11 years ago
Hi!
> I noticed very strange behavior under ZTS build with this patch. > It turned out that compiler_globals is not accessible under ZTS build > according to gdb. > > Is this intended? If so, where should I put script_extensions char array?
That doesn't look right. If compiler_globals weren't accessible nothing would work in ZTS. It may be some bug or missing setting in gdb.
-- Stas Malyshev smalyshev@gmail.com

Yasuo Ohgaki

11 years ago
Hi Stas, On Mon, Feb 23, 2015 at 5:02 PM, Stanislav Malyshev <smalyshev@gmail.com> wrote:
> > I noticed very strange behavior under ZTS build with this patch. > > It turned out that compiler_globals is not accessible under ZTS build > > according to gdb. > > > > Is this intended? If so, where should I put script_extensions char array? > > That doesn't look right. If compiler_globals weren't accessible nothing > would work in ZTS. It may be some bug or missing setting in gdb.
Thank you. Now I see compiler globals under ZTS in gdb and it looks OK. The execution steps do not make sense at all in gdb, but simply commenting out new code for move_uploaded_file() makes move_uploaded_file() work again. It seems I have to read machine code to see what's wrong :( Regards,
-- Yasuo Ohgaki yohgaki@ohgaki.net