# is_a fix for 5.4 and HEAD - Thread: https://externals.io/message/55788 - Original archive: https://news-web.php.net/php.internals/55788 - Started by Alan Knowles on 2011-10-13T07:39:02+00:00 - Messages: 13 - 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 --- ## #55788 — Alan Knowles — 2011-10-13T07:39:02+00:00 URL: https://externals.io/message/55788#55788 Can someone apply this to HEAD and PHP_5_4, or let me have karma. https://bugs.php.net/patch-display.php?bug_id=55475&patch=final_patch_for_5_4_and_HEAD&revision=latest Thanks Alan --- ## #55790 — Stas Malyshev — 2011-10-13T15:23:03+00:00 URL: https://externals.io/message/55788#55790 · In reply to: #55788 > Can someone apply this to HEAD and PHP_5_4, or let me have karma. > > https://bugs.php.net/patch-display.php?bug_id=55475&patch=final_patch_for_5_4_and_HEAD&revision=latest > > Thanks > Alan > > +/* {{{ proto bool is_subclass_of(mixed object_or_string, string class_name [, bool allow_string=true]) +/* {{{ proto bool is_a(mixed object_or_string, string class_name [, bool allow_string=false]) This is quite strange - two almost identical functions with completely different default semantics. -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #55800 — Alan Knowles — 2011-10-13T23:54:51+00:00 URL: https://externals.io/message/55788#55800 · In reply to: #55790 > > > […] > +/* {{{ proto bool is_subclass_of(mixed object_or_string, string > class_name [, bool allow_string=true]) > +/* {{{ proto bool is_a(mixed object_or_string, string class_name [, > bool allow_string=false]) > > This is quite strange - two almost identical functions with completely > different default semantics. Yes, and if you can think of a way to depreciate them now so that it does not break BC and could be changed later, please feel free, but I've looked at it quite a bit and could not find any other way. Regards Alan --- ## #55801 — Rasmus Lerdorf — 2011-10-14T00:06:31+00:00 URL: https://externals.io/message/55788#55801 · In reply to: #55800 > > > […] > Yes, and if you can think of a way to depreciate them now so that it > does not break BC and could be changed later, please feel free, but I've > looked at it quite a bit and could not find any other way. Argh! Pet-peeve, it's deprecate. Depreciate means something completely different. I agree that it is slightly messy, but we have painted ourselves into a bit of a corner with the 5.3 mess. Stas, the whole point here is that changing the `is_a()` default in 5.3 caused huge problems, including security ones, so setting allow_string to false by default fixes that BC break in 5.3. Considering the huge amount of code hit by this I think we should keep that default in 5.4. Alan, does `is_subclass_of()` actually need the allow_string flag at all? It has always been documented take a mixed and hit the autoloader, so why would we need an option to disable the string? -Rasmus --- ## #55802 — Alan Knowles — 2011-10-14T02:12:52+00:00 URL: https://externals.io/message/55788#55802 · In reply to: #55801 > > > […] > Argh! Pet-peeve, it's deprecate. Depreciate means something completely > different. Agh to much accounting recently ;) > I agree that it is slightly messy, but we have painted ourselves into a > bit of a corner with the 5.3 mess. Stas, the whole point here is that > changing the `is_a()` default in 5.3 caused huge problems, including > security ones, so setting allow_string to false by default fixes that BC > break in 5.3. Considering the huge amount of code hit by this I think we > should keep that default in 5.4. > > Alan, does `is_subclass_of()` actually need the allow_string flag at all? > It has always been documented take a mixed and hit the autoloader, so > why would we need an option to disable the string? I don't think I've used is_subclass_of for years, the flag is a knock on effect from sharing the same code, and may be useful in some edge cases (if somebody wanted to use subclass_of with user input.. ), since that bit does not affect me, I do not mind either way. To disable for subclass of it is quite trivial, I can get a full patch ready (including updated tests) if we go that way. if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, only_subclass ? "zs" : "zs|b", &obj, &class_name, &class_name_len, &allow_string) == FAILURE) { Regards Alan > -Rasmus --- ## #55809 — Stas Malyshev — 2011-10-16T00:00:19+00:00 URL: https://externals.io/message/55788#55809 · In reply to: #55801 Hi! > I agree that it is slightly messy, but we have painted ourselves into a > bit of a corner with the 5.3 mess. Stas, the whole point here is that > changing the `is_a()` default in 5.3 caused huge problems, including > security ones, so setting allow_string to false by default fixes that BC I've read complaints about is potentially causing security problems, but is there code out there that was OK before and has security problem with this change? I mean, a real-life app? I'm thinking maybe we should have this options - but maybe have both defaults set to true? This way if you have buggy code and you absolutely refuse to move to proper code you can easily fix it by putting false where needed, but at least our API is not broken anymore. -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #55816 — Pierre Joye — 2011-10-16T10:39:15+00:00 URL: https://externals.io/message/55788#55816 · In reply to: #55809 > Hi! > > > > […] > > I've read complaints about is potentially causing security problems, but is > there code out there that was OK before and has security problem with this > change? I mean, a real-life app? There was example codes in previous discussions, here and on security. The document used for the CVE assignment has some as well. http://www.byte.nl/blog/2011/09/23/security-bug-in-is_a-function-in-php-5-3-7-5-3-8/ https://bugs.php.net/bug.php?id=55475 https://bugzilla.redhat.com/show_bug.cgi?id=741020 Now whether this exact use case is used in a real life app, nobody can say it, but it really does not matter (or one can dig codesearch&co to find some). > I'm thinking maybe we should have this options - but maybe have both > defaults set to true? This way if you have buggy code and you absolutely > refuse to move to proper code you can easily fix it by putting false where > needed, but at least our API is not broken anymore. It is not correct. If a code, for whatever reason, was working fine until now then there is no reason one has to change it to make it work again, or even worst in this case, to make it safe again. And yes, I agree that this kind of code is ugly and should not have written that way, but we cannot do anything but be sure that we don't make this code even worst. Cheers, -- Pierre @pierrejoye | http://blog.thepimp.net | http://www.libgd.org --- ## #55823 — Stas Malyshev — 2011-10-16T19:14:10+00:00 URL: https://externals.io/message/55788#55823 · In reply to: #55816 Hi! > There was example codes in previous discussions, here and on security. > The document used for the CVE assignment has some as well. > > http://www.byte.nl/blog/2011/09/23/security-bug-in-is_a-function-in-php-5-3-7-5-3-8/ > https://bugs.php.net/bug.php?id=55475 > https://bugzilla.redhat.com/show_bug.cgi?id=741020 I know about these, this is why I specifically asked for real-life examples. And yes, it does matter - if it's completely invented example, it's one thing, if it's part of live widely deployed code, it's another. > It is not correct. If a code, for whatever reason, was working fine > until now then there is no reason one has to change it to make it work > again, or even worst in this case, to make it safe again. > > And yes, I agree that this kind of code is ugly and should not have > written that way, but we cannot do anything but be sure that we don't > make this code even worst. The problem is not the beauty of such code, but the fact that we are breaking our APIs to match somebody's code that is insecure anyway, should never be written in such a way and could very well be completely imaginary. The reason to change such code is exactly that this code is insecure and broken, and the fact that nobody could break it by one specific way doesn't mean there aren't other ways. Changing APIs after they're public is very hard, I know it. However right now the API we have is bad, and if we say we would never change anything in order so broken code could stay broken, our API would be bad forever. -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #55824 — Pierre Joye — 2011-10-16T21:14:06+00:00 URL: https://externals.io/message/55788#55824 · In reply to: #55823 > Hi! > > > > […] > > I know about these, this is why I specifically asked for real-life examples. > And yes, it does matter - if it's completely invented example, it's one > thing, if it's part of live widely deployed code, it's another. They are not completely invented examples. Or do you ask for any single issue we have to show the real life examples? In short, we do not. > > […] > > The problem is not the beauty of such code, but the fact that we are > breaking our APIs to match somebody's code that is insecure anyway, The fact is that we have many broken APIs we can't fix for BC reasons. We have to live with that in the 5.x serie. > should never be written in such a way and could very well be completely imaginary. could, should, would, it does not change the problem for a single yota. We can't have a variable ways to work with BC and security issues. We simply can't as we have no way, absolutely no way, to be sure that a given case is not used (widely or not). > The reason to change such code is exactly that this code is insecure and > broken, and the fact that nobody could break it by one specific way doesn't > mean there aren't other ways. We have discussed that already on security, I barely see a reason to begin this discussion again. There is a clear possible security problem, clearly identified and not present before this "fix" was applied. It is easy to fix and does not make PHP worst or better than what it is now but only ensure that there is no BC, or new security issues. Cheers, -- Pierre @pierrejoye | http://blog.thepimp.net | http://www.libgd.org --- ## #55827 — Stas Malyshev — 2011-10-16T22:59:56+00:00 URL: https://externals.io/message/55788#55827 · In reply to: #55824 Hi! > We have discussed that already on security, I barely see a reason to > begin this discussion again. There is a clear possible security > problem, clearly identified and not present before this "fix" was > applied. It is easy to fix and does not make PHP worst or better than > what it is now but only ensure that there is no BC, or new security > issues. Yes, the security problem was present before the fix was applied, and we discussed it on security where I repeatedly pointed out that this code has security hole regardless of any changes in PHP, the change only adds one scenario where it can be exploited, but there are many others. It definitely makes PHP worse by propagating inconsistent APIs. -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #55828 — Pierre Joye — 2011-10-16T23:56:02+00:00 URL: https://externals.io/message/55788#55828 · In reply to: #55827 > Yes, the security problem was present before the fix was applied No, it was not. See the examples in the links I pasted earlier. The code was safe, under controlled context, before this change has applied. With the change the code becomes unsafe under uncontrolled context. That's not acceptable and besides the BC break, it introduce a security flaw. As stated many times, by many persons, in the previous discussion(s). Cheers, -- Pierre @pierrejoye | http://blog.thepimp.net | http://www.libgd.org --- ## #55843 — Jonathan Bond-Caron — 2011-10-17T13:22:33+00:00 URL: https://externals.io/message/55788#55843 · In reply to: #55827 > It definitely makes PHP worse by propagating inconsistent APIs. I created a patch against 5.4: https://bugs.php.net/patch-display.php?bug_id=55475&patch=is_a_5.4_alternati ve&revision=latest The patch changes the behavior to: is_a("ab", "b") // false is_a("ab", "b", true) // autoload "ab", autoload "b" -- false is_subclass_of("ab", "b") // false is_subclass_of("ab", "b", true) // autoload "ab", autoload "b" -- false Both class names can be "autoloaded" but not by default Thoughts? --- ## #55866 — Alan Knowles — 2011-10-19T01:18:41+00:00 URL: https://externals.io/message/55788#55866 · In reply to: #55843 Supporting strings 'by default' in `is_a()` has the downside that it produces slightly unpredictable results. If you are accepting 'mixed' arguments, some of which may be strings, there is a slim chance that the string you accept will match the class or it's parent by accident. Personally if I did not believe that the barrier for a BC change has to be alot higher than 'prettier or consistent API', i would have suggested that is_subclass_of should be changed to be 'consistent', and safer. While the old justification for using `is_a()` over instanceof, has almost gone (it was the only way to write code that was portable to PHP4 & PHP5.*, except for the annoying depreciation message). I still consider `is_a()`, especially with negative testing slightly clearer to read than instanceof. Regards Alan > > > […] > I created a patch against 5.4: > https://bugs.php.net/patch-display.php?bug_id=55475&patch=is_a_5.4_alternati > ve&revision=latest > > The patch changes the behavior to: > is_a("ab", "b") // false > is_a("ab", "b", true) // autoload "ab", autoload "b" -- false > > is_subclass_of("ab", "b") // false > is_subclass_of("ab", "b", true) // autoload "ab", autoload "b" -- false > > Both class names can be "autoloaded" but not by default > > Thoughts?