Hi Lester,
On Wed, Feb 12, 2014 at 4:15 PM, Lester Caine <[email protected]> wrote:
> Yasuo Ohgaki wrote:
>
>> Have you actually looked at the 'Some recent LFI issues' that are
>> listed? I
>> don't thing any one of them would have been protected from by this
>> change?
>> Providing a php page that can DISPLAY or run any file that it can
>> read is
>> not going to be protected from by switching embedding off?
>>
>> 1st one is file upload vulnerability. I'll replace it something else.
>> 2nd one is
>>
>> http://seclists.org/bugtraq/2012/Apr/53
>> $filepath = "$path_to_citrus/$load.php";
>> if (file_exists($filepath)) {
>> include('./'.$load.'.php');
>>
>> I'm not sure what they are loading. script() prevents from reading
>> /etc/passwd
>> or any other files.
>>
>
> Since it is loading a .php file there has to be a valid .php file to load?
> This is simply another file upload vulnerability?
>
Oops, I didn't care much about details of LFI reported.
You can find many valid examples here or any other similar sites. Unknown
LFI would be countless.
http://packetstormsecurity.com/search/?q=LFI
3 and 4 as far as I can see expected the files being loaded to contain
> embedded php, and so the environment needed to be secured better and the
> application rewritten. I accept simply disabling php would have blocked the
> hole, but it also would stop the whole application working? Certainly
> onefilecms has been completely reworked and is no longer vulnerable ... as
> far as I can see ... I've not looked at the other app now.
>
As an one of security auditor, I would recommend rewrite. They must have
strong input validation in first place. The reason why these are reported
as security vulnerability is nature of include()/require(). Vulnerability
reporters even reports invalid attack vectors for current PHP that requires
null bytes to attack.
Original author of this RFC was tried to resolve this issue by switching
mode. Although, I like the original idea, current RFC is trying to resolve
this issue by script().
> I still don't see how - short of switching off embedding permanently - you
> will stop critics with ulterior motives from calling PHP insecure? Other
> than that one element, php is as at risk as the other languages. Once
> embedding is switched off either by calling the file using 'script()' (
> which I view would transparently switch embedding off so not need any ini
> switch! ) or by the physical ini switch, then yes PHP is any other
> language, so if they are so worried about it ... use something else?
>
Original RFC was aimed to change require()/include() behavior and protect
applications without rewriting most of code, as few as 3 lines. I changed
the RFC not to change require()/include() behavior. The switch exists for
consistency between directly loaded and indirectly loaded scripts.
Consistency matters for languages, especially for new features. IMHO.
I could almost be convinced about the 'script()' option, but I would then
> insist the loaded file HAD <?php at the top and any other php tag would
> flag an error. It was loosing that 'security blanket' in the other rfc that
> grated ... but I don't see the point of the ini switch unless at some point
> you want to disable embedding permanently :(
It could be made raise error. It's valid option. Top "<?php" is allowed for
easier transition, mitigation for script exposure by miss configuration and
consistency.
It may be better to have more options to vote or back to original proposal?
I don't mind changing the RFC to original form.
It's clean, simple, compatible and easy to adopt. Drawback is 2 parsing
mode for include()/require() which could be problematic.
Regards,
--
Yasuo Ohgaki
[email protected]