On Fri, Sep 21, 2018 at 1:36 PM Andrey Andreev <n...@devilix.net> wrote:
> Hi, > > On Thu, Sep 20, 2018 at 11:30 PM, Arnold Daniels <arn...@jasny.net> wrote: > > > > > > On Thu, Sep 20, 2018 at 8:50 PM Andrey Andreev <n...@devilix.net> wrote: > >> > >> Hi again, > >> > >> > >> On Thu, Sep 20, 2018 at 5:29 PM, Arnold Daniels <arn...@jasny.net> > wrote: > >> > > >> > Variable includes have proper purposes, like for a (PSR-4) autoloader. > >> > This > >> > can't be simply replaced with an 'if' statement. Other reasons are > >> > module > >> > inclusion and generated code. > >> > > >> > >> Of course, there are a few valid applications even for the most > >> discouraged practices - in that you are correct. But you should know I > >> meant *user input* variable inclusion in particular. > >> Either way, I don't see how a *user input filter* validator would help > >> PSR-4, generated code and/or whatever you meant by module inclusion. > > > > > > With module inclusion, I mean the way Symfony works with Bundles and > > Wordpress works with plugins. > > > > I'm familiar with neither, sorry. > > > To be honest, few applications seem to have static include statements at > > all. It's all dynamic. > > > > Again, talking about user inputs. Yes, most includes are dynamic, but > still controlled by the developer and not with user-supplied strings. > In fact, since you mentioned PSR-4 which is used for pretty much > everything nowadays, you should know that almost nobody writes > "include" anymore and it's all handled by the autoloader in a > bullet-proof manner. > > >> > >> > >> Just to be clear, in everything I'm saying, I assume you want to solve > >> the following problem: > >> > >> include $_GET['page'].".php"; // <-- vulnerability > >> > >> (simplified, of course) > > > > > > With all the abstraction you see in modern applications, it's difficult > to > > tell if a variable could have been influenced via the HTTP request. It's > > better always to validate the variable before using it in the include. > > > > ... and I noted that was a simplified example. > > But while it is true that many variables are influenced by the HTTP > request, I fail to recall a valid case where they're not already > validated by a router mapping variables to pre-defined classes, > already handled by an autoloader. > > This is getting kind of hard to follow, we may not even be talking > about the same thing at this point. Can you provide an example of a > case where your proposed filter would solve a problem? > > >> > >> > >> > Variable inclusion is already done very often. I don't think this > filter > >> > will persuade people to do it that would otherwise not. This is a > common > >> > security issue. So if variable inclusion isn't disabled in full, > having > >> > a > >> > common way to prevent such issues seem like a good idea to me. > >> > > >> > >> Well, I've got two things to say about this: > >> > >> 1. The developers who intruduce such vulnerabilities are the ones who > >> don't do validation in the first place, so the way I see it, the few > >> poor souls who might benefit from your proposal wouldn't care for it > >> anyway. > > > > > > If there is a proper way to do it, they'll probably be more inclined to > do > > it. It's the same as for SQL injection. There is a right and a wrong way > to > > inject variables. > > > > That's just wishful thinking; I've never seen a developer who doesn't > do validation all of a sudden go "oh, there's this validator, I should > use it!". They don't do it because they don't care about it, not > because tools don't exist ... we already have tools for everything > that the average developer needs. And not that it matters, but SQLi > prevention is quite different. > > >> > >> 2. Reiterating from my previous reply, this would only serve as an > >> excuse for some to say that user input variable inclusion is an OK > >> thing to do, because "see, PHP has a tool specifically for it, so it > >> must be good". > > > > > > There are probably not many scripts where you see direct user input being > > used for `include`. It's always an indirect hack. > > > > If you link the 'filter' extension to request data, perhaps the function > > should not be in this extension. > > > > I don't understand what you're saying here ... I thought you were > trying to solve a security problem, now all of a sudden that's not > related to request data? I'll wait for that example I asked for. > > >> > >> > >> >> Sanitization is more often than not imperfect and there's always the > >> >> potential to bypass it. > >> > > >> > This would be a validation filter and not a sanization filter. Can you > >> > give > >> > an example on how you could bypass it? > >> > > >> > >> Sorry, you started the discussion by mentioning sanitization and I > >> just went with it without noticing the details. > >> > >> Still, even as a validator this can be bypassed depending on the > >> application architecture - sure, you specify a base path, but who is > >> to say I'm not trying to RCE via something within that base path? > > > > > > You should not allow uploading files with `base_path`. Current RFCs > aren't > > solving this issue and I don't see how to solved this, short of > disalowing > > variable for includes completely. But that's unacceptable since most > > applications rely on it. > > > > "Should not" doesn't mean people don't do it, and I wasn't talking > about uploaded files anyway. Either way, that's a moot point ... my > position is that there's no value in the proposed feature, regardless > of whether it can be bypassed or not. > > Cheers, > Andrey. > Please have a look at * https://wiki.php.net/rfc/script_only_include - PHP RFC: Introduce script only include/require * https://wiki.php.net/rfc/allow_url_include - PHP RFC: Precise URL include control Both describe the problem and possible solutions. Also see https://www.exploit-db.com/search/?action=search&q=file+inclusion&platform=31 Using the filter extension is an alternative solution, which doesn't require changing the PHP syntax. - Arnold
