Newsgroups: php.internals Path: news.php.net Xref: news.php.net php.internals:103247 Return-Path: Delivered-To: mailing list internals@lists.php.net Received: (qmail 83813 invoked from network); 24 Sep 2018 15:16:45 -0000 Received: from unknown (HELO mail-pf1-f194.google.com) (209.85.210.194) by pb1.pair.com with SMTP; 24 Sep 2018 15:16:45 -0000 Received: by mail-pf1-f194.google.com with SMTP id j26-v6so8875124pfi.10 for ; Mon, 24 Sep 2018 04:24:22 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=cKQFbV5+PCvyYtXMIAQjNgIloX2k+kUO+BeuG5c26c8=; b=b168gOKcoLBVEjKvkrtN5Nxw/Km1sBYQI3BnFDccSJDiWNb1C90/Js8G6JDqL1q5lr kQSgw//e32Sd9imegKWh1hCHNfWI7qKsvrLWxZhLeozErJxM3R53gL+ytCkRwNaEznLY rQZdoaj1llWGhDj/NRYwR1Tmhq//s6bIJj5QAzQgoGK8SDFGLyDXHKCUEZcc3iZ7hahv 98hBBYJMbMtyOIBmtf/WJk9xobVjn1yzYZSgH2Gppq5flr2ma5sVCIaPUS3Ra3mzjAnw Ctc9kTLZohKo98eKwnmI775M7LzgUZ+tmjFuS5e13HtfIwpnWBdifMTuOAfwqyA9HBPc Feow== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=cKQFbV5+PCvyYtXMIAQjNgIloX2k+kUO+BeuG5c26c8=; b=NJCcq3Y/3AfEF/OMRRY5M7lxA2V0zEz9ucA+DHu27dX6lKqmnrSUVJ8ZMLYFPEQGk9 RErV2h+9xr7mt25nlUxwC4Igk3voqKV+WyTbX9X6guUB0jxIIYudPDzMh9nEwVN18vch eer9/cxpBKM0zQUFKe68UACrSOFpVd2Qb0SbGCul5n0FYKkWekQA2CxXTXikZe2JbcC6 FJKQAWgHXC1YqGXmblXZx3fR2yiekv7jqrTEjmx/erltpsnrfgY57iG6M9WXICqel9Tc kcOWEvstQ4+CE9IuysTZP3hTUNcFybrdb0kJOQVxaV8wZAPsyg4ELqDEuCohZiTetuVv 1MkA== X-Gm-Message-State: ABuFfoh444YuskC27KB706hkYzypRqvqNEXhJnwWUlcXL2o2uVVvK8bm 0b41CEl0SsK+1ipa7cJ0Zqcz+vl9wVVa9FRi/lP1PQ== X-Google-Smtp-Source: ANB0VdYPLHgij6Zu1B03wWdICxuY+AThnlRHGtVOzbWeZ6+QSXb5I3kwsVNjHfurhVJpfl4OHJeyN/tcjNAbDEVsE3k= X-Received: by 2002:a62:1016:: with SMTP id y22-v6mr9969013pfi.109.1537788261419; Mon, 24 Sep 2018 04:24:21 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: Date: Mon, 24 Sep 2018 13:21:20 +0200 Message-ID: To: Andrey Andreev Cc: PHP Internals Content-Type: multipart/alternative; boundary="0000000000000fcf8605769c3ba6" Subject: Re: [PHP-DEV] Add FILTER_VALIDATE_INCLUDE validation filter for variable includes From: arnold.adaniels.nl@gmail.com (Arnold Daniels) --0000000000000fcf8605769c3ba6 Content-Type: text/plain; charset="UTF-8" On Fri, Sep 21, 2018 at 1:36 PM Andrey Andreev wrote: > Hi, > > On Thu, Sep 20, 2018 at 11:30 PM, Arnold Daniels wrote: > > > > > > On Thu, Sep 20, 2018 at 8:50 PM Andrey Andreev wrote: > >> > >> Hi again, > >> > >> > >> On Thu, Sep 20, 2018 at 5:29 PM, Arnold Daniels > 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 --0000000000000fcf8605769c3ba6--