# [RFC][DISCUSSION] Remove allow_url_include INI - Thread: https://externals.io/message/83957 - Original archive: https://news-web.php.net/php.internals/83957 - Started by Yasuo Ohgaki on 2015-02-27T03:44:06+00:00 - Messages: 16 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #83957 — Yasuo Ohgaki — 2015-02-27T03:44:06+00:00 URL: https://externals.io/message/83957#83957 Hi all, This is RFC for removing "allow_url_include" INI option. [1] During "Script only include" RFC[2] discussion, stream wrapper issue is raised. I was thinking this issue as a separate issue, but it seems others are not. "Script only include" RFC does not cover stream wrapper hole. This RFC addresses the hole created by stream wrappers. Those who may be concerned this hole in "Script only include" RFC may reconsider your votes by this. Without this RFC, "Script only include" RFC may have infinite number of holes. This RFC closes them and make "Script only include" RFC more effective. I don't use phar on regular basis, feedback from phar users are appreciated. If you find yet another hole in [2], please let me know. [1] https://wiki.php.net/rfc/allow_url_include [2] https://wiki.php.net/rfc/script_only_include Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #83964 — Xinchen Hui — 2015-02-27T06:55:17+00:00 URL: https://externals.io/message/83957#83964 · In reply to: #83957 Hey: > Hi all, > > This is RFC for removing "allow_url_include" INI option. [1] > > During "Script only include" RFC[2] discussion, stream wrapper issue is > raised. > I was thinking this issue as a separate issue, but it seems others are not. > > "Script only include" RFC does not cover stream wrapper hole. This RFC > addresses > the hole created by stream wrappers. Those who may be concerned this hole > in "Script > only include" RFC may reconsider your votes by this. > > Without this RFC, "Script only include" RFC may have infinite number of > holes. hmm, does that means, if this RFC won't pass, then script only include RFC should also be rejected? if yes, then maybe you should put them together? thanks > This RFC closes them and make "Script only include" RFC more effective. > > I don't use phar on regular basis, feedback from phar users are appreciated. > If you find yet another hole in [2], please let me know. > > [1] https://wiki.php.net/rfc/allow_url_include > [2] https://wiki.php.net/rfc/script_only_include > > Regards, > > -- > Yasuo Ohgaki > yohgaki@ohgaki.net -- Xinchen Hui @Laruence http://www.laruence.com/ --- ## #83965 — Yasuo Ohgaki — 2015-02-27T06:56:17+00:00 URL: https://externals.io/message/83957#83965 · In reply to: #83957 Hi all, > This is RFC for removing "allow_url_include" INI option. [1] > > During "Script only include" RFC[2] discussion, stream wrapper issue is > raised. > I was thinking this issue as a separate issue, but it seems others are not. > > "Script only include" RFC does not cover stream wrapper hole. This RFC > addresses > the hole created by stream wrappers. Those who may be concerned this hole > in "Script > only include" RFC may reconsider your votes by this. > > Without this RFC, "Script only include" RFC may have infinite number of > holes. > This RFC closes them and make "Script only include" RFC more effective. > > I don't use phar on regular basis, feedback from phar users are > appreciated. > If you find yet another hole in [2], please let me know. > > [1] https://wiki.php.net/rfc/allow_url_include > [2] https://wiki.php.net/rfc/script_only_include > Simpler approach could be specifying the prefix of URL (wrapper) include('http://php.net', 'http://'); Pros: - Requires a lot less code modifications = less BC. - Simple string comparison is enough. - More specific which wrapper is used. (Only specified wrapper may be used) - More flexible when new wrapper is added. (No additional code is needed for this) Cons: - 'http://' looks redundant - 2nd parameter is used solely for specifying wrapper. i.e. Cannot use it for no embedded mode flag, etc. There may be pseudo wrapper like "noembed://", though. Thoughts? BTW, we are better to fix this regardless of https://wiki.php.net/rfc/script_only_include since we opened hole with Phar which looks like tar file that can be executed as PHP script. There are many servers that allow tar file uploads. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #83966 — Yasuo Ohgaki — 2015-02-27T06:59:11+00:00 URL: https://externals.io/message/83957#83966 · In reply to: #83964 Hi Xinchen, > hmm, does that means, if this RFC won't pass, then script only include > RFC should also be rejected? > > if yes, then maybe you should put them together? > Sorry I just sent previous mail before your mail. We need to fix this regardless of https://wiki.php.net/rfc/script_only_include If we have both, we close the door for "arbitrarily script execution". (I mean almost the same as other language level) Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #83968 — Xinchen Hui — 2015-02-27T07:45:48+00:00 URL: https://externals.io/message/83957#83968 · In reply to: #83966 Hey: > Hi Xinchen, > > > > […] > > > Sorry I just sent previous mail before your mail. > > We need to fix this regardless of > https://wiki.php.net/rfc/script_only_include > If we have both, we close the door for "arbitrarily script execution". > (I mean almost the same as other language level) > Sorry, but I am confused by the point, do you want to disable include a remote php file or not? if yes, how about with allow_url_fopen? eval(file_get_contents(http://xxxxxx/)); thanks > Regards, > > -- > Yasuo Ohgaki > yohgaki@ohgaki.net -- Xinchen Hui @Laruence http://www.laruence.com/ --- ## #83986 — Yasuo Ohgaki — 2015-02-27T09:26:28+00:00 URL: https://externals.io/message/83957#83986 · In reply to: #83968 Hi Xinchen, > Sorry, but I am confused by the point, do you want to disable include > a remote php file or not? > > if yes, how about with allow_url_fopen? > > eval(file_get_contents(http://xxxxxx/)); > > thanks > My objective is to disable "local script/file inclusion" by include/require with default usage. We have been added mitigations like allow_url_include=Off and `NULL` byte restriction for filename. Number of vulnerable applications are reduced, but there are many compare to other languages still. (This RFC removes allow_url_include, but introduce more specific/restrictive API and disables include 'phar://phar_file/script.php'; by default) Regarding eval(file_get_contents(http://xxxxxx/)); This kind of operation can be done in other languages also. These codes are not scope of my RFCs. If user specified to do so, it's user's problem. I would like to make PHP as secure as possible with default usage. e.g. Make include($_GET['var') not to read/execute files, but raise proper errors where it is possible as other languages do. This can be done by https://wiki.php.net/rfc/script_only_include and https://wiki.php.net/rfc/allow_url_include If anyone find missing piece, please let me know. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #83988 — Yasuo Ohgaki — 2015-02-27T09:45:30+00:00 URL: https://externals.io/message/83957#83988 · In reply to: #83968 Hi Xinchen, > Sorry, but I am confused by the point, do you want to disable include > a remote php file or not? > I think the RFC title was misleading. I've changed to Precise URL include control Thank you for your feedback. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #83992 — Rowan Collins — 2015-02-27T11:25:00+00:00 URL: https://externals.io/message/83957#83992 · In reply to: #83957 Yasuo Ohgaki wrote on 27/02/2015 03:44: > Hi all, > > This is RFC for removing "allow_url_include" INI option. [1] > > During "Script only include" RFC[2] discussion, stream wrapper issue is > raised. > I was thinking this issue as a separate issue, but it seems others are not. I'm not convinced by the argument that because "phar://blah" looks like a URL, allowing it makes allow_url_include broken. Perhaps it would be better named allow_remote_include, but it corresponds to masking out your PHP_STREAM_REMOTE flag further down, which is the more important protection. If you want to be able to disable phar:// access, you could add something like allow_local_stream_include for that case without breaking BC. I'm also not at all clear what you mean by "caller" and "callee" responsibilities; surely the difference is just between a global option (ini_set()) and a local one (extra argument)? And in what way does Option #2 require more changes than Option #1, since they both require the argument to be present whenever a stream wrapper is used? I do think local options are better than global ini settings in many cases, but include/require/etc are statements, not functions, so giving them extra arguments is awkward - some of your examples are "wrong" in this regard: // Redundant brackets make this look like a function, but it's not: include('phar://phar_file/script.php'); // I can add as many as I like, the parser is just resolving them to a single string expression: include(((('phar://phar_file/script.php')))); // This is the actual syntax: include'phar://phar_file/script.php'; // Implying this for arguments: include'phar://phar_file/script.php', 'phar://'; // You could explicitly allow a "function form" of the statements, so you could parse this: include(('phar://phar_file/' . $script_name), 'phar://'); // But then you've got a subtle BC break, because the interpretation of this changes: include ($foo) . ('.php'); Regards, -- Rowan Collins [IMSoP] --- ## #84064 — Yasuo Ohgaki — 2015-02-28T02:37:04+00:00 URL: https://externals.io/message/83957#84064 · In reply to: #83992 Hi Rowan, On Fri, Feb 27, 2015 at 8:25 PM, Rowan Collins wrote: > Yasuo Ohgaki wrote on 27/02/2015 03:44: > > > […] > > I'm not convinced by the argument that because "phar://blah" looks like a > URL, allowing it makes allow_url_include broken. Perhaps it would be better > named allow_remote_include, but it corresponds to masking out your > PHP_STREAM_REMOTE flag further down, which is the more important > protection. If you want to be able to disable phar:// access, you could add > something like allow_local_stream_include for that case without breaking BC. > allow_local_stream_include is one possible solution. It has the same problem with allow_url_include. Since allor_url_include is global INI, it applies to child scripts. e.g. A.php ----- responsibilities; surely the difference is just between a global option > (ini_set()) and a local one (extra argument)? And in what way does Option > #2 require more changes than Option #1, since they both require the > argument to be present whenever a stream wrapper is used? > First question is answered in previous comment. INI cannot control flags precisely... Option #1 is simple string comparison for filename is passed. It does not care what's the filter at all nor even filter name is valid or not. 2nd parameter is used to make sure filename passed to include() is user's intension. e.g. ----- // somewhere deep in code $module_name = substr($_GET['module'], -4, 4) === '.php' ? $_GET['module'] : ''; // Do not care to much, we have safe .php files. // somewhere far from above code include($module_name); // phar://evil_phar/evil_script.php can be executed. ----- With this RFC, if 2nd parameter is omitted ------ include($module_name); // E_RROR, URL formed include(stream wrapper) is not allowed. ------ to use phar. User must be explicit. ----- include($module_name, 'phar://'); ----- Option #2 will be much more complex than Option #1, since it introduces types of wrapper, force users to have class method return wrapper type and pass flags around functions to handle wrapper types correctly. There may be more. I do think local options are better than global ini settings in many cases, > but include/require/etc are statements, not functions, so giving them extra > arguments is awkward - some of your examples are "wrong" in this regard: > > // Redundant brackets make this look like a function, but it's not: > include('phar://phar_file/script.php'); > // I can add as many as I like, the parser is just resolving them to a > single string expression: > include(((('phar://phar_file/script.php')))); > // This is the actual syntax: > include'phar://phar_file/script.php'; > // Implying this for arguments: > include'phar://phar_file/script.php', 'phar://'; > // You could explicitly allow a "function form" of the statements, so you > could parse this: > include(('phar://phar_file/' . $script_name), 'phar://'); > // But then you've got a subtle BC break, because the interpretation of > this changes: > include ($foo) . ('.php'); > I used include('phar://phar_file/script.php'), but it can be include 'phar://phar_file/script.php'. I'm using () since it seemed harder to distinguish parameters and other text as we can use () for include/require now. I have no intention to change current include/require syntax, except adding 2nd parameter. Thank you for your feedback. I hope I explained well enough. I'm thinking merging this RFC to "Script only include" RFC. I have to start over, but it seems I must do this. I'll use Option #1 since it's much easier/simpler/less BC/more precise. However, ideas are appreciated always. Regards, > -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #84065 — Yasuo Ohgaki — 2015-02-28T02:47:15+00:00 URL: https://externals.io/message/83957#84065 · In reply to: #84064 Hi all, > On Fri, Feb 27, 2015 at 8:25 PM, Rowan Collins > wrote: > > > […] > > allow_local_stream_include is one possible solution. It has the same > problem with allow_url_include. > Since allor_url_include is global INI, it applies to child scripts. e.g. > > A.php > ----- > ini_set('allow_url_include', 'On'); > include('B.php'); > ----- > > B.php > ----- > include($_GET['var']); // Not only local script execution vulnerable, but > also remote script execution is possible because "allow_url_include=On" > here. > ----- > > We need to control these flags more precisely. That's the reason why I > proposed 2nd parameter for include. > If flags are globals, we cannot control them as it should. > > URL and URI is the same for most users, I think. (It differs, but their > usage is mostly the same) Not > many users expect to work something like > > > include('new_wrapper://some_file_with_special_content/some_how_php_script_is_accecible_suddenly'); > > Attackers can write malicious user stream wrapper as back door. Then > upload attack file and takeover > server. The wrapper will be hard to distinguish if it's legitimate or not. > Attack file can be any kind of > format. It's impossible to detect as malicious file. URL formed script > (stream wrapper) is very flexible/handy/ > hard to be detected tool for attackers. > > Remote resource is extremely danger. Local resource is very dangerous > also. Current phar:// wrapper > implementation spoils file extension and ".." (double dot) protection that > is deployed by many PHP codes > already. User wrapper seems allowed for include(), so malicious user > wrapper can help attackers to > exploit PHP servers. > > > I'm also not at all clear what you mean by "caller" and "callee" > > […] > > First question is answered in previous comment. INI cannot control flags > precisely... > > Option #1 is simple string comparison for filename is passed. It does not > care what's the filter at all nor even filter name is valid or not. 2nd > parameter is > used to make sure filename passed to include() is user's intension. e.g. > > ----- > // somewhere deep in code > $module_name = substr($_GET['module'], -4, 4) === '.php' ? > $_GET['module'] : ''; // Do not care to much, we have safe .php files. > > // somewhere far from above code > include($module_name); // phar://evil_phar/evil_script.php can be executed. > ----- > > With this RFC, if 2nd parameter is omitted > > ------ > include($module_name); // E_RROR, URL formed include(stream wrapper) is > not allowed. > ------ > > to use phar. User must be explicit. > > ----- > include($module_name, 'phar://'); > ----- > > Option #2 will be much more complex than Option #1, since it introduces > types of wrapper, force users to have class method return wrapper type and > pass flags around functions to handle wrapper types correctly. There may be > more. > > > I do think local options are better than global ini settings in many > > […] > // I can add as many as I like, the parser is just resolving them to a > > […] > > I used include('phar://phar_file/script.php'), but it can be include > 'phar://phar_file/script.php'. > I'm using () since it seemed harder to distinguish parameters and other > text as we can > use () for include/require now. > > I have no intention to change current include/require syntax, except > adding 2nd parameter. > > Thank you for your feedback. I hope I explained well enough. > I'm thinking merging this RFC to "Script only include" RFC. I have to > start over, but it seems > I must do this. > > I'll use Option #1 since it's much easier/simpler/less BC/more precise. > However, > ideas are appreciated always. > In short, I'm saying we have serious security problem in our current stream wrapper implementation and it's friends including include/require. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #84067 — Stanislav Malyshev — 2015-02-28T04:53:16+00:00 URL: https://externals.io/message/83957#84067 · In reply to: #84064 Hi! > I have no intention to change current include/require syntax, except adding > 2nd parameter. This is a bit misleading since include is not a function, so there's no 1st parameter. Instead, it's a syntax construct. Of course, syntax construct's grammar can be changed, though I'm not sure if that wouldn't lead to unexpected effects in the parser. But maybe not. Bigger issue is that if somebody is knowledgeable enough in security to go and change their code to use this option, why they aren't knowledgeable enough to fix their includes so this option is not required? -- Stas Malyshev smalyshev@gmail.com --- ## #84082 — Rowan Collins — 2015-02-28T18:46:47+00:00 URL: https://externals.io/message/83957#84082 · In reply to: #84064 > Hi Rowan, > > On Fri, Feb 27, 2015 at 8:25 PM, Rowan Collins > > wrote: > > Yasuo Ohgaki wrote on 27/02/2015 03:44: > > Hi all, > > This is RFC for removing "allow_url_include" INI option. [1] > > During "Script only include" RFC[2] discussion, stream wrapper > issue is > raised. > I was thinking this issue as a separate issue, but it seems > others are not. > > > I'm not convinced by the argument that because "phar://blah" looks > like a URL, allowing it makes allow_url_include broken. Perhaps it > would be better named allow_remote_include, but it corresponds to > masking out your PHP_STREAM_REMOTE flag further down, which is the > more important protection. If you want to be able to disable > phar:// access, you could add something like > allow_local_stream_include for that case without breaking BC. > > > allow_local_stream_include is one possible solution. It has the same > problem with allow_url_include. Yes, the global vs local setting issue is distinct from the question of "is phar://blah a URL?" > We need to control these flags more precisely. That's the reason why I > proposed 2nd parameter for include. > If flags are globals, we cannot control them as it should. I agree with this part. Global settings are always harder to work with, and in this case more dangerous, than something more local. > URL and URI is the same for most users, I think. (It differs, but > their usage is mostly the same) Not > many users expect to work something like > > include('new_wrapper://some_file_with_special_content/some_how_php_script_is_accecible_suddenly'); I'm not sure most users would look at that and think "that's a URL" or "that's a URI". They'd think "that's some funky syntax for including a file from inside an archive". > Attackers can write malicious user stream wrapper as back door. Then > upload attack file and takeover > server. The wrapper will be hard to distinguish if it's legitimate or > not. Attack file can be any kind of > format. It's impossible to detect as malicious file. URL formed script > (stream wrapper) is very flexible/handy/ > hard to be detected tool for attackers. I'm not really clear how this is a "back door". At worst, it seems to be a form of obfuscation, like the classic eval(base64_decode(....)) etc, since it makes the intention of the uploaded file less obvious. But you've still got to activate the stream wrapper, upload the file, and convince the script to include it. > Remote resource is extremely danger. Local resource is very dangerous > also. Current phar:// wrapper > implementation spoils file extension and ".." (double dot) protection > that is deployed by many PHP codes > already. User wrapper seems allowed for include(), so malicious user > wrapper can help attackers to > exploit PHP servers. Relying on the fact that no uploaded file can have a name ending ".php", and that therefore any file ending ".php" is safe to include, seems like a very naive check, best remedied with user education. It's also only exploitable if they rely on the include_path setting, rather than prefixing the input with an expected directory (since '/var/www/my_app/modules/phar://foo.phar/bob.php' would not be loadable). Similarly, the only sensible way I know to protect against ".." is to call `realpath()` or similar, which would simply return false for anything beginning "phar://", regardless of what path it then pointed to. > I'm also not at all clear what you mean by "caller" and "callee" > responsibilities; surely the difference is just between a global > option (ini_set()) and a local one (extra argument)? And in what > way does Option #2 require more changes than Option #1, since they > both require the argument to be present whenever a stream wrapper > is used? > > > First question is answered in previous comment. What I don't understand is who is the "caller" and who is the "callee". All I can see in your examples is "global setting enabled at point X, and exploited at point Y"; there is no caller/callee relationship between those points. > Option #2 will be much more complex than Option #1, since it > introduces types of wrapper, force users to have class method return > wrapper type and pass flags around functions to handle wrapper types > correctly. There may be more. Why does the user have to pass flags around? I would have thought the vast majority of cases will be replacing "require 'phar://foo.phar/bar.php';" with either "require 'phar://foo.phar/bar.php', 'phar://'; or "require 'phar://foo.phar/bar.php', PHP_STREAM_LOCAL". The burden of upgrading is basically identical. The only difference would seem to be the fairly rare scenario of implementing a custom stream wrapper, and then using it for script includes, and that would require only a single implementation of `getType()` on the stream wrapper, which doesn't seem that complex. > I used include('phar://phar_file/script.php'), but it can be include > 'phar://phar_file/script.php'. > I'm using () since it seemed harder to distinguish parameters and > other text as we can > use () for include/require now. No, we can't; we can use () for string expressions right now, and include/require expect a single string expression. This is an important distinction, not just a matter of terminology. I actually encountered a bug in real code which looked something like this: @include('/path/to/some/library.php') && call_some_library_function(); This looks like it executes include(...) in a boolean context, and calls the library function if it succeeds; but it actually evaluates '/path/to/some/library.php' in a boolean context (non-empty string => true), and always attempts to call the library function, all *before* attempting to include anything. It is equivalent to this: @include ( '/path/to/some/library.php' && call_some_library_function() ); > I have no intention to change current include/require syntax, except > adding 2nd parameter. Then you need to remove the parentheses from your examples, since they are incompatible with the current syntax, unless you only treat the parentheses specially if there is also a comma, which seems likely to cause confusion: include ('foo') . '.php'; // existing syntax; includes 'foo.php' include ('foo', PHP_STREAM_LOCAL) . '.php'; // new syntax; includes 'foo'? In case you think all my examples are rather unlikely, consider this perfectly reasonable use of parentheses: include ($debug_mode ? 'debug_' : 'production_') . $class_name . '.php'; If the parser starts treating the parentheses as part of the include syntax, this will break completely. Regards, -- Rowan Collins [IMSoP] --- ## #84085 — Yasuo Ohgaki — 2015-02-28T23:41:50+00:00 URL: https://externals.io/message/83957#84085 · In reply to: #84067 Hi Stas, On Sat, Feb 28, 2015 at 1:53 PM, Stanislav Malyshev wrote: > > […] > adding > > […] > > This is a bit misleading since include is not a function, so there's no > 1st parameter. Instead, it's a syntax construct. Of course, syntax > construct's grammar can be changed, though I'm not sure if that wouldn't > lead to unexpected effects in the parser. But maybe not. > Although it is variable length parameters, we already have "echo". I'll check it see if Zend has problem with 2nd parameter when I write patch. > > Bigger issue is that if somebody is knowledgeable enough in security to > go and change their code to use this option, why they aren't > knowledgeable enough to fix their includes so this option is not required? The root cause of the issue here is preciseness of the setting. I think you agree that current "allow_url_include=Off" with `INI_SYSTEM` is not precise at all. We need to consider local and remote wrapper separately. We may better to consider removing all remote wrapper support from include/require. It's rarely used and user can execute remote script easily with PHP. e.g. eval(readfile('http://host/script')). That said, one other possible solution is disallow most local wrappers for include/require and restrict script file extension. AFAIK, phar and user wrappers are the exception for "allow_url_include=Off", but there may be others. If we remove most local wrapper support(php://input, user wrappers, etc) from include/require, we don't need 2nd parameter. i.e. include('phar://phar_file.phar/script_in_phar.php'); // Requires .phar extension to load PHP script from phar. This requires users to change filename. Therefore, I propose to check wrapper type by 2nd parameter, but filename extension check works also. I don't mind to use this option. There may be users that uses local wrapper to encrypt script files, etc. We need research if disabling most wrappers for include/require have problem or not. If there are other ideas, I appreciate it. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #84086 — Yasuo Ohgaki — 2015-03-01T00:37:39+00:00 URL: https://externals.io/message/83957#84086 · In reply to: #84082 Hi Rowan, On Sun, Mar 1, 2015 at 3:46 AM, Rowan Collins wrote: > > > Hi Rowan, > > On Fri, Feb 27, 2015 at 8:25 PM, Rowan Collins > wrote: > > > […] > > allow_local_stream_include is one possible solution. It has the same > problem with allow_url_include. > > > Yes, the global vs local setting issue is distinct from the question of > "is phar://blah a URL?" I think your question is about remote/local resource. Is phar:// local resource? It's yes. Is phar:// URL/URI? I think it depends how people understand URL/URI. file:// is considered URI/URL even if it's local resource. > We need to control these flags more precisely. That's the reason why I > proposed 2nd parameter for include. > If flags are globals, we cannot control them as it should. > > > I agree with this part. Global settings are always harder to work with, > and in this case more dangerous, than something more local. > > > URL and URI is the same for most users, I think. (It differs, but their > usage is mostly the same) Not > many users expect to work something like > > > include('new_wrapper://some_file_with_special_content/some_how_php_script_is_accecible_suddenly'); > > > I'm not sure most users would look at that and think "that's a URL" or > "that's a URI". They'd think "that's some funky syntax for including a file > from inside an archive". > PHP allows too many wrappers for include/require. We may be better to disable almost all wrappers. Phar remains as problematic wrapper. Solution would be adding way to load script precisely. e.g. Restrict filename extension, specify URL/URI prefix. Note: We need to have either, specific file name for scripts or script loader must be specific for wrapper to be used. > Attackers can write malicious user stream wrapper as back door. Then > upload attack file and takeover > server. The wrapper will be hard to distinguish if it's legitimate or not. > Attack file can be any kind of > format. It's impossible to detect as malicious file. URL formed script > (stream wrapper) is very flexible/handy/ > hard to be detected tool for attackers. > > > I'm not really clear how this is a "back door". At worst, it seems to be a > form of obfuscation, like the classic eval(base64_decode(....)) etc, since > it makes the intention of the uploaded file less obvious. But you've still > got to activate the stream wrapper, upload the file, and convince the > script to include it. Nice attribute of attack with wrapper is actual attack code can be hidden in other data files. The data file may have any kind of form. This makes detection a lot harder. > Remote resource is extremely danger. Local resource is very dangerous > also. Current phar:// wrapper > implementation spoils file extension and ".." (double dot) protection > that is deployed by many PHP codes > already. User wrapper seems allowed for include(), so malicious user > wrapper can help attackers to > exploit PHP servers. > > > Relying on the fact that no uploaded file can have a name ending ".php", > and that therefore any file ending ".php" is safe to include, seems like a > very naive check, best remedied with user education. It's also only > exploitable if they rely on the include_path setting, rather than prefixing > the input with an expected directory (since > '/var/www/my_app/modules/phar://foo.phar/bob.php' would not be loadable). > Right. '/var/www/my_app/modules/phar://foo.phar/bob.php' is invalid. However, relative path is allowed... Using Stas' exploit example php chump.php phar://../cuteponies.gif/pwnd.php This is executes attack code. > > Similarly, the only sensible way I know to protect against ".." is to > call `realpath()` or similar, which would simply return false for anything > beginning "phar://", regardless of what path it then pointed to. This is interesting idea for mitigation. Thank you. > I'm also not at all clear what you mean by "caller" and "callee" > > […] > > First question is answered in previous comment. > > > What I don't understand is who is the "caller" and who is the "callee". > All I can see in your examples is "global setting enabled at point X, and > exploited at point Y"; there is no caller/callee relationship between those > points. > > The include() in the child script's condition (allow_url_include) must be controlled by the caller. i.e. ini_set('allow_url_inlcude', 'Off') is needed before include/require if it works. Current PHP does not allow this by making "allow_url_include" INI_SYSTEM. Almost all PHP codes assume "allow_url_include=Off". If caller needs to set "allow_url_include=On" for some script, all of child scripts are affected since there is no proper caller/callee responsibility segregation. There are 2 possible solutions - Make allow_url_include flag a specific flag for each include/require. - Make sure allow_url_include is desired setting before calling include/require. i.e. ini_set('allow_url_include', 'On/Off') for every include/require. Option #2 will be much more complex than Option #1, since it introduces > types of wrapper, force users to have class method return wrapper type and > pass flags around functions to handle wrapper types correctly. There may be > more. > > > Why does the user have to pass flags around? I would have thought the vast > majority of cases will be replacing "require 'phar://foo.phar/bar.php';" > with either "require 'phar://foo.phar/bar.php', 'phar://'; or "require > 'phar://foo.phar/bar.php', PHP_STREAM_LOCAL". The burden of upgrading is > basically identical. > > The only difference would seem to be the fairly rare scenario of > implementing a custom stream wrapper, and then using it for script > includes, and that would require only a single implementation of `getType()` > on the stream wrapper, which doesn't seem that complex. > It's just more complex than 'URL prefix' check. I don't mind adopting/implementing this method. > I used include('phar://phar_file/script.php'), but it can be include > 'phar://phar_file/script.php'. > I'm using () since it seemed harder to distinguish parameters and other > text as we can > use () for include/require now. > > > No, we can't; we can use () for string expressions right now, and > include/require expect a single string expression. This is an important > distinction, not just a matter of terminology. > > I actually encountered a bug in real code which looked something like this: > > @include('/path/to/some/library.php') && call_some_library_function(); > > This looks like it executes include(...) in a boolean context, and calls > the library function if it succeeds; but it actually evaluates > '/path/to/some/library.php' in a boolean context (non-empty string => > true), and always attempts to call the library function, all *before* > attempting to include anything. It is equivalent to this: > > @include ( '/path/to/some/library.php' && call_some_library_function() ); Interesting code example. Thank you. We can use return value from script. i.e. We can use script file as a function call with include/require. I didn't think of this. I'll keep this in my mind. > > I have no intention to change current include/require syntax, except > adding 2nd parameter. > > > Then you need to remove the parentheses from your examples, since they are > incompatible with the current syntax, unless you only treat the parentheses > specially if there is also a comma, which seems likely to cause confusion: > > include ('foo') . '.php'; // existing syntax; includes 'foo.php' > include ('foo', PHP_STREAM_LOCAL) . '.php'; // new syntax; includes 'foo'? > > > In case you think all my examples are rather unlikely, consider this > perfectly reasonable use of parentheses: > > include ($debug_mode ? 'debug_' : 'production_') . $class_name . '.php'; > > If the parser starts treating the parentheses as part of the include > syntax, this will break completely. include ('foo') . '.php'; // existing syntax; includes 'foo.php' This one is another good example, too. Thank you. I think I'm better to merge this RFC to "Script only inclusion" RFC. I'll do this so that there is no confusions. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #84088 — Stanislav Malyshev — 2015-03-01T02:18:18+00:00 URL: https://externals.io/message/83957#84088 · In reply to: #84085 Hi! > The root cause of the issue here is preciseness of the setting. > I think you agree that current "allow_url_include=Off" with `INI_SYSTEM` is > not precise at all. It is precise - it's doing exactly what it meant to do, separate local wrappers from remote ones. > We need to consider local and remote wrapper separately. > We may better to consider removing all remote wrapper support from > include/require. That's exactly what this setting is doing. > It's rarely used and user can execute remote script easily with PHP. > e.g. eval(readfile('http://host/script')). This setting is indeed rarely used and not recommended to enable, but since it's off by default, I assume anybody enabling it knows what they are doing. > for "allow_url_include=Off", but there may be others. If we remove most > local wrapper > support(php://input, user wrappers, etc) from include/require, we don't > need 2nd parameter. i.e. As I previously noted, php://input is considered remote already. As for others, I'm not sure why we would want to remove them. -- Stas Malyshev smalyshev@gmail.com --- ## #84099 — Lester Caine — 2015-03-01T09:40:17+00:00 URL: https://externals.io/message/83957#84099 · In reply to: #84086 > > […] > > I think your question is about remote/local resource. > Is phar:// local resource? It's yes. > Is phar:// URL/URI? I think it depends how people understand URL/URI. > file:// is considered URI/URL even if it's local resource. A URI consists of two parts ... URL (where/what the resource is) URN (the name of the resource) phar: and file: are URL scheme names and yes there is nothing here to designate if the resource is local or remote. So yes allow_url_include is a totally wrong description anyway? Simply because at the time is was added I don't think local url's were even considered? -- 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