# Comments on PHP security - Thread: https://externals.io/message/27420 - Original archive: https://news-web.php.net/php.internals/27420 - Started by Mathieu CARBONNEAUX on 2007-01-13T15:10:49+00:00 - Messages: 35 - Page 1 of 2: messages 1 to 25 (25 per page, add `?page=N` to the URL) - Next page: https://externals.io/message/27420.md?page=2 - 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 --- ## #27420 — Mathieu CARBONNEAUX — 2007-01-13T15:10:49+00:00 URL: https://externals.io/message/27420#27420 > I don't think you've read the section on prepared statements in the > PDO documentation, because it does mention it there, although it > doesn't beat you over the head with the seriousness of the problem. > > http://us2.php.net/manual/en/ref.pdo.php#pdo.prepared-statements Sorry for my buz... This piece of manual say the same i've said... But i repeat, i thing a security (or a good dev practice portal) portal is a good freind of the manual and can be abel to level up the security of php dev app... But is not the only solution... And i think CERT must not alarm in the same statistic pool if are application security bug, or php engine security bug... because actualy there no posibility to filter on cert (i've seen one...) on php engine only... And it the same with module that use lib that have security bug... And is bad for the php image... On this portal i thing also be interesant log php alert in more friendly way... Regards, Mathieu --- ## #27421 — Stefan Esser — 2007-01-13T15:35:35+00:00 URL: https://externals.io/message/27420#27421 · In reply to: #27420 Mathieu CARBONNEAUX schrieb: > > […] > Sorry for my buz... > This piece of manual say the same i've said... > Yeah and it is wrong. The manual claims the following: > The parameters to prepared statements don't need to be quoted; the driver handles it for you. If your application > exclusively uses prepared statements, you can be sure that no SQL injection will occur. (However, if you're still > building up other parts of the query based on untrusted input, you're still at risk). While it is true that the PHP script is not subject to SQL injection when the application does not build parts of the query based on untrusted userinput it is not true that you are not at risk. Ever heard of stored procedures? And precisely of SQL injection in stored procedures? Prepared statements can do nothing about SQL Injection in Stored Procedures. And if you teach people that they only should use prepared statements they will NEVER know that it is dangerous to put userinput into the query. And aside from that they have never learned how to do it correctly if there is ever a situation where they have to do it. > But i repeat, i thing a security (or a good dev practice portal) portal is a good freind of the manual and can be abel to level up the security of php dev app... > There are enough secure programming examples by so called experts around that are vulnerable to different kind of attacks. Do you really believe a community portal by complete strangers can provide reliable security documentation? > And is bad for the php image... > It is very good that bugs in PHP Applications are bad for PHP's image. The majority of servers get hacked because of Remote URL Includes (and not by XSS/SQL Injection). The existance of Remote URL Includes is fully to blame on the PHP engine and is UNIQUE. After years the feature is still not kicked out of PHP. In PHP 5.2.1 there will be a configuration option that activates an (incomplete) blacklist. Everyone should know by now that blacklists are BAD. > On this portal i thing also be interesant log php alert in more friendly way... > You mean like using marketing tricks to make security bugs look less serious? Stefan Esser Hardened-PHP Project --- ## #27422 — Lukas Kahwe Smith — 2007-01-13T15:49:24+00:00 URL: https://externals.io/message/27420#27422 · In reply to: #27421 Stefan Esser wrote: > While it is true that the PHP script is not subject to SQL injection > when the application does not build parts of the query based on > untrusted userinput it is not true that you are not at risk. > Ever heard of stored procedures? And precisely of SQL injection in > stored procedures? Prepared statements can do nothing about SQL > Injection in Stored Procedures. And if you teach people that they only > should use prepared statements they will NEVER know that it is dangerous > to put userinput into the query. And aside from that they have never > learned how to do it correctly if there is ever a situation where they > have to do it. Maybe I am missing something. If you are talking about dynamic query generation in stored routines, then I would think that people who read the advice over dynamic query generation for PHP, that they would hopefully also apply those practices to when they are writing/using stored routines. Also DBA's in general are not all that fond of dynamic query generation in stored routines. Someone also mentioned that LIMIT parameters cannot be prepared: "As of MySQL 5.0.7, placeholders can be used for the arguments of the LIMIT clause when using prepared statements." So at least in MySQL this works. I dont know about PostgreSQL or Firebird. SQLite does not natively support prepared statements anyways. Furthermore there is currently no good way in PHP to really reuse prepared statements across requests in PHP, as such they help you very little in terms of improving performance, unless you reuse the query multiple times in a request. As a matter of fact they may even reduce performance, because they may generate query plans that are inefficient for the given placeholder values. This is because the query plan is either optimized for the most general case or the first set of placeholder values Anyways .. we are moving away from PHP here. regards, Lukas --- ## #27423 — Stefan Esser — 2007-01-13T16:01:11+00:00 URL: https://externals.io/message/27420#27423 · In reply to: #27422 Hello Lukas, > Maybe I am missing something. If you are talking about dynamic query > generation in stored routines, then I would think that people who read > the advice over dynamic query generation for PHP, that they would > hopefully also apply those practices to when they are writing/using > stored routines. Also DBA's in general are not all that fond of > dynamic query generation in stored routines. Well in a team it is quite likely that people writing PHP applications are not those writing Stored Procedures for the SQL Databases. At least all big projects I have seen have their own team for complicated SQL Queries. Additionally your statement assumes that people are reading documentation ;) However, the good thing about the PDO documentation is that it has the warning about dynamically injected userinput in it. Many security experts (and here I speak in general not only PHP) claim that the SQL injection problem is 100% fixed by the use of prepared statements. Additionally I wonder how well prepared statements meanwhile handle queries like this (that are VERY VERY common in PHP applications) SELECT xyz FROM abc WHERE product_id IN ( 1,2,3,4,5) <- the list having dynamic lenght Stefan Esser --- ## #27424 — Lukas Kahwe Smith — 2007-01-13T16:05:38+00:00 URL: https://externals.io/message/27420#27424 · In reply to: #27423 Stefan Esser wrote: > Hello Lukas, > > > […] > Well in a team it is quite likely that people writing PHP applications > are not those writing Stored Procedures for the SQL Databases. At least > all big projects I have seen have their own team for complicated SQL > Queries. Additionally your statement assumes that people are reading > documentation ;) Well if the people on your team writing the stored procedures do not know about how to protect against SQL injection how is that a PHP issue? Its a common issue that all types of programmers have to learn. The key lesson here is that again you need to remember that switching context means that you need to take care about the security implications of this and its best if both sides are aware of this and alert eachother of potential issues. regards, Lukas --- ## #27425 — Lukas Kahwe Smith — 2007-01-13T16:07:24+00:00 URL: https://externals.io/message/27420#27425 · In reply to: #27423 Stefan Esser wrote: > SELECT xyz FROM abc WHERE product_id IN ( 1,2,3,4,5) <- the list having > dynamic lenght They have worked in PEAR::DB and some other DBAL's, as a result a lot of people have come to think of prepared statements as a `sprintf()` replacement. By the nature of prepared statements they only work with single literals, which a list of values isnt. regards, Lukas --- ## #27426 — Rasmus Lerdorf — 2007-01-13T16:25:45+00:00 URL: https://externals.io/message/27420#27426 · In reply to: #27421 Stefan Esser wrote: > It is very good that bugs in PHP Applications are bad for PHP's image. > The majority of servers get hacked because of Remote URL Includes (and > not by XSS/SQL Injection). The existance of Remote URL Includes is fully > to blame on the PHP engine and is UNIQUE. After years the feature is > still not kicked out of PHP. In PHP 5.2.1 there will be a configuration > option that activates an (incomplete) blacklist. Everyone should know by > now that blacklists are BAD. I am not sure I would call it a blacklist. It is a flag in the streams layer that marks stream handlers that could possibly make a network connection as such and there are only a finite set of stream handlers. Assuming we have marked the right streams, which I think we have at this point, the only way to trick it would be to bounce through some sort of file system network mechanism like auto-mounted file systems or perhaps some \\Server\path\foo.php thing on Windows, but at that level I think it is up to the local sysadmin to make sure the PHP scripts can't get to something they shouldn't. -Rasmus --- ## #27427 — Stefan Esser — 2007-01-13T16:59:45+00:00 URL: https://externals.io/message/27420#27427 · In reply to: #27426 > I am not sure I would call it a blacklist. It is a flag in the streams > layer that marks stream handlers that could possibly make a network > connection as such and there are only a finite set of stream handlers. > Unfortunately there is not a finite set of stream handlers. First of all there are userstreams. An application could register a dangerous userstream (that is of course not marked as URL) which is then abused by an include. Then there are PECL extensions that register streams. Just pick a random one: Let's take ext/ssh2. This is not just any pecl extension, but one from a PHP core developer. SSH is obviously a network protocol. The PHP documentation even states that SSH streams are forbidden during allow_url_fopen. However from looking at the code I assume this documentation is wrong, because I see all is_url flags being set to 0. Voila, just install pecl/ssh and you are still vulnerable to remote file includes. That is the problem with opt-in/aka blacklist methods. Stefan --- ## #27428 — Marcus Boerger — 2007-01-13T17:13:30+00:00 URL: https://externals.io/message/27420#27428 · In reply to: #27427 Hello Stefan, i also think something should be done here. The is_url flag does not really help. What we imho need is an ini setting that allows specifying which stream handlers to allow. And that should not include user streams. best regards marcus Saturday, January 13, 2007, 5:59:45 PM, you wrote: > > […] > Unfortunately there is not a finite set of stream handlers. First of all > there are userstreams. An application could register a dangerous > userstream (that is of course not marked as URL) which is then abused by > an include. > Then there are PECL extensions that register streams. > Just pick a random one: Let's take ext/ssh2. This is not just any pecl > extension, but one from a PHP core developer. > SSH is obviously a network protocol. The PHP documentation even states > that SSH streams are forbidden during allow_url_fopen. > However from looking at the code I assume this documentation is wrong, > because I see all is_url flags being set to 0. > Voila, just install pecl/ssh and you are still vulnerable to remote file > includes. > That is the problem with opt-in/aka blacklist methods. > Stefan Best regards, Marcus --- ## #27429 — Ilia Alshanetsky — 2007-01-13T17:22:33+00:00 URL: https://externals.io/message/27420#27429 · In reply to: #27428 Marcus, You want to use an INI setting to specify which streams are local and which are remote? That seems like a recipe for disaster to me, people adjusting this setting many not consider some streams that are remote etc... leading to security holes. There is really no reason why PHP could not effectively use flags internally to identify the difference between the two sources of streams. Ultimately it'll always fall to the extension writer, same as with open_basedir, which author can choose to bypass if they so choose to. The main issue here is I think is that is_url flag is new and there are many extensions providing remote wrapper that have been written prior to its addition and therefor do not have a proper setting in place, which may have been added in a hurry to solve a compilation failure. > Hello Stefan, > > i also think something should be done here. The is_url flag does not > really help. What we imho need is an ini setting that allows > specifying > which stream handlers to allow. And that should not include user > streams. > > best regards > marcus Ilia Alshanetsky --- ## #27430 — Rasmus Lerdorf — 2007-01-13T17:26:01+00:00 URL: https://externals.io/message/27420#27430 · In reply to: #27429 I thought we also decided to default user streams to be marked as insecure with some mechanism for overriding it. -Rasmus Ilia Alshanetsky wrote: > Marcus, > > You want to use an INI setting to specify which streams are local and > which are remote? That seems like a recipe for disaster to me, people > adjusting this setting many not consider some streams that are remote > etc... leading to security holes. There is really no reason why PHP > could not effectively use flags internally to identify the difference > between the two sources of streams. Ultimately it'll always fall to the > extension writer, same as with open_basedir, which author can choose to > bypass if they so choose to. > > The main issue here is I think is that is_url flag is new and there are > many extensions providing remote wrapper that have been written prior to > its addition and therefor do not have a proper setting in place, which > may have been added in a hurry to solve a compilation failure. > > > > > […] > > Ilia Alshanetsky --- ## #27431 — Marcus Boerger — 2007-01-13T17:32:21+00:00 URL: https://externals.io/message/27420#27431 · In reply to: #27429 Hello Ilia, what i wanted it a SYSTEM INI setting that allows which streams work exactly. As opposedto being able to select between all and none while hoping that the switch works (for the reasons you specified). best regards marcus Saturday, January 13, 2007, 6:22:33 PM, you wrote: > Marcus, > You want to use an INI setting to specify which streams are local and > which are remote? That seems like a recipe for disaster to me, people > adjusting this setting many not consider some streams that are remote > etc... leading to security holes. There is really no reason why PHP > could not effectively use flags internally to identify the difference > between the two sources of streams. Ultimately it'll always fall to > the extension writer, same as with open_basedir, which author can > choose to bypass if they so choose to. > The main issue here is I think is that is_url flag is new and there > are many extensions providing remote wrapper that have been written > prior to its addition and therefor do not have a proper setting in > place, which may have been added in a hurry to solve a compilation > failure. > > > […] > Ilia Alshanetsky Best regards, Marcus --- ## #27432 — Stefan Esser — 2007-01-13T17:46:06+00:00 URL: https://externals.io/message/27420#27432 · In reply to: #27431 My 2cents, the actually best would be to completely forbid the usage of URLs inside include/require and introduce a new keyword: include_url that works like the current include would work and rename allow_url_include into allow_dangerous_urls (for include_url only). Basically this would protect everyone from URL includes with no way around and if someone really really wants this dangerous feature he has to explicitly request it via include_url. Stefan Esser --- ## #27433 — Tim Starling — 2007-01-13T23:15:51+00:00 URL: https://externals.io/message/27420#27433 · In reply to: #27421 Stefan Esser wrote: > It is very good that bugs in PHP Applications are bad for PHP's image. > The majority of servers get hacked because of Remote URL Includes (and > not by XSS/SQL Injection). The existance of Remote URL Includes is fully > to blame on the PHP engine and is UNIQUE. After years the feature is > still not kicked out of PHP. In PHP 5.2.1 there will be a configuration > option that activates an (incomplete) blacklist. Everyone should know by > now that blacklists are BAD. Arbitrary local includes are almost as dangerous, because many PHP applications provide an upload feature. Preventing remote includes is only a small step towards security. The reason PHP is particularly vulnerable is because subscripts are often web-executable, and because of register_globals. When you put the two together, you turn code that looks perfectly reasonable to an average developer into a serious security vulnerability. The code looks so reasonable, in fact, that most developers wouldn't realise the problem unless they were told. And the manual doesn't tell them the specific issue, not even in the security section. I would like to see a prominent warning in the manual pages of include(), require(), include_once() and require_once() stating that variables should never be used as a base path for inclusion of subscripts, and that code such as the following is extremely dangerous: My 2cents, > > the actually best would be to completely forbid the usage of URLs inside > include/require and > introduce a new keyword: include_url that works like the current include > would work and > rename allow_url_include into allow_dangerous_urls (for include_url only). > > Basically this would protect everyone from URL includes with no way > around and if someone > really really wants this dangerous feature he has to explicitly request > it via include_url. Hi, Although I think this is a decent idea, I would rather see a way that harmless url wrappers like phar (pecl.php.net/phar and more to the point pear.php.net/PHP_Archive) that only ever access local files can be more easily white-listed while even preventing any accidental network access by pecl extensions. The include_url solution, although effective, introduces another level of confusion to an already confusing situation: include require include_once require_once include_url include_url_once require_url require_url_once I had suggested a while back that it might also be possible to use an internal zend engine flag that is set during a call to include/require and friends that automatically disables all network access functions while the stream wrapper open and read of file contents is being called, prior to parsing the php into opcodes. This would mean a new zend API function (or macro) would need to be created that E_ERRORs out if include/require is being called, and it would need to be inserted in the front of all ftp, curl, pecl/ssh2, pecl_http, pecl/gopher, fsockopen and whatever else uses the so-called "internet" that I've forgotten. Fortunately, the list is finite. In this way, user stream handlers could be trusted, as they would simply not work (uncatchable E_ERROR) if any remote access is attempted. The same is true of pecl stream wrappers. As I recall, the last I heard was Rasmus said on IRC "that actually might work" and then it was forgotten by all parties including me. :) My idea would *not* protect against third-party extensions, but since these are not distributed with PHP and PHP has no control over them anyways, I doubt that this will be a real issue, as large hosting companies are unlikely to install anything third-party except perhaps Zend Optimizer. If "include_url" is introduced, it could be a large headache for PHP_Archive and will be a large headache for pecl/phar. Perhaps I'm the only one with this issue, but I think achieving full no-way internet include is relatively simple without introducing new keywords. I do like using "dangerous" in the ini settings, that is far more descriptive and accurate. Thanks, Greg --- ## #27435 — Wez Furlong — 2007-01-14T09:16:55+00:00 URL: https://externals.io/message/27420#27435 · In reply to: #27422 > SQLite does not natively support prepared statements anyways. Yes it does :) --- ## #27436 — Lukas Kahwe Smith — 2007-01-14T10:34:08+00:00 URL: https://externals.io/message/27420#27436 · In reply to: #27435 Wez Furlong wrote: > > > […] > > Yes it does :) Ah, I got thrown off by the use of the word "precompile" which they also used to describe dumps of the byte code generated by the SQL compiler. The later was useful to be able to ship the SQLite binary with the SQL compiler stripped out to make the binary lean and safe on the compilation process. I guess they now have true prepared statements though he calls them precompiled. Thanks for the correction. regards, Lukas --- ## #27437 — Stanislav Malyshev — 2007-01-14T11:03:04+00:00 URL: https://externals.io/message/27420#27437 · In reply to: #27428 MB>> i also think something should be done here. The is_url flag does not MB>>really help. What we imho need is an ini setting that allows MB>>specifying which stream handlers to allow. And that should not include MB>>user streams. I don't see how user streams have anything to do with it - if you can define user stream, you also can run any code you wanted to run through a stream directly. If you have control over execution engine, you can run any code, stream or not stream. -- Stanislav Malyshev, Zend Products Engineer stas@zend.com http://www.zend.com/ --- ## #27438 — Marcus Boerger — 2007-01-14T11:15:55+00:00 URL: https://externals.io/message/27420#27438 · In reply to: #27437 Hello Stanislav, I always have control of what I program. I alwayshavecontrol of whichfiles I load. Ergo there is no security issue - WOW. best regards marcus Sunday, January 14, 2007, 12:03:04 PM, you wrote: MB>>> i also think something should be done here. The is_url flag does not MB>>>really help. What we imho need is an ini setting that allows MB>>>specifying which stream handlers to allow. And that should not include MB>>>user streams. > I don't see how user streams have anything to do with it - if you can > define user stream, you also can run any code you wanted to run through a > stream directly. If you have control over execution engine, you can run > any code, stream or not stream. Best regards, Marcus --- ## #27439 — Stefan Esser — 2007-01-14T11:18:04+00:00 URL: https://externals.io/message/27420#27439 · In reply to: #27437 Stanislav, you obviously did not get the point... It is not about including URL stream wrappers that youself provide. It is about URL Include vulnerabilities in the application that allow remote attackers to issue attacks against Userstreams of the application. I would not be suprised to see some Wrapper Userstream that actually allows specifying a remote URL (something like php://filter just as userstream). And If I am not completely mistaken here unlike php://filter a userstream will not give the THIS_IS_AN_INCLUDE_FLAG down to a stream itself opens. PS: Don't tell me that userstreams are not available at the time of the include... I have seen enough stuff like include "base.lib.php"; ... include $templatepath."/header.php"; ... include $templatepath."/footer.php"; Stefan Esser --- ## #27440 — Stefan Esser — 2007-01-14T11:23:20+00:00 URL: https://externals.io/message/27420#27440 · In reply to: #27434 Greg, i was not talking about providing all the different versions of include. For me it is a broken application design to include anything out of an URL wrapper. However it would also be fine to just have allow_url_include affect all URLs and to be setable by `ini_set()` and be turned off by default. Insane applications can then turn it on/off for every REMOTE URL they need. Aside from that there could be a setting for the admin to completely disallow URLs marked as is_URL. Stefan Esser --- ## #27441 — Stanislav Malyshev — 2007-01-14T11:38:17+00:00 URL: https://externals.io/message/27420#27441 · In reply to: #27438 MB>> I always have control of what I program. I alwayshavecontrol of MB>>whichfiles I load. Ergo there is no security issue - WOW. I'm afraid you lost me. Could you please explain why you considering user streams to be security threat without resorting to sarcasm? -- Stanislav Malyshev, Zend Products Engineer stas@zend.com http://www.zend.com/ --- ## #27442 — Stanislav Malyshev — 2007-01-14T12:52:05+00:00 URL: https://externals.io/message/27420#27442 · In reply to: #27439 SE>>And If I am not completely mistaken here unlike php://filter a SE>>userstream will not give the THIS_IS_AN_INCLUDE_FLAG down to a stream SE>>itself opens. I think I see what you mean now - i.e. that the user implementation might be tricked into opening URL for include even though direct opening URL for include is not allowed, and since it would do e.g. fopen, it may work around the allow_url_include. I would say in most cases prohibiting anything but plain file wrapper for include might be OK, however I know about a number of instances of legitimate wrappers used for include - e.g. archive files like phar and there are other, custom solutions that I saw that use wrappers as base. Maybe it would be a good idea also to pass a flag to stream_open saying it is used for include - though it won't fix broken code of course. -- Stanislav Malyshev, Zend Products Engineer stas@zend.com http://www.zend.com/ --- ## #27443 — Greg Beaver — 2007-01-14T20:44:13+00:00 URL: https://externals.io/message/27420#27443 · In reply to: #27442 Stanislav Malyshev wrote: > SE>>And If I am not completely mistaken here unlike php://filter a > SE>>userstream will not give the THIS_IS_AN_INCLUDE_FLAG down to a stream > SE>>itself opens. > > I think I see what you mean now - i.e. that the user implementation might > be tricked into opening URL for include even though direct opening URL for > include is not allowed, and since it would do e.g. fopen, it may work > around the allow_url_include. > > I would say in most cases prohibiting anything but plain file wrapper for > include might be OK, however I know about a number of instances of > legitimate wrappers used for include - e.g. archive files like phar and > there are other, custom solutions that I saw that use wrappers as base. > Maybe it would be a good idea also to pass a flag to stream_open saying it > is used for include - though it won't fix broken code of course. Hi, There is just such a flag passed to wrapper_open (at least inside pecl streams, I imagine you're talking about user streams) but that most likely would not be secure at all, as it puts the onus on the streams implementor, which is the problem right now. This is why I suggested disabling all remote access while calling a stream's open and read for include purposes, it centralizes the security access inside the zend engine where it is possible to audit and easier to control. In my opinion, it would be a pity to lose the design benefits of stream wrappers in the effort to gain more security when security can be gained without losing the benefits. I think it would be good to allow disabling all urls as Stefan suggested, but if there is a way to restrict to truly local stream wrappers, I would need to be educated as to how this is less secure than outright disabling urls. Greg --- ## #27444 — Greg Beaver — 2007-01-14T20:53:03+00:00 URL: https://externals.io/message/27420#27444 · In reply to: #27440 Stefan Esser wrote: > Greg, > > i was not talking about providing all the different versions of include. Ah, was this include_url some kind of ini setting you were talking about? Greg --- Next page: https://externals.io/message/27420.md?page=2