Re: [PHP-DEV] Re: php4 /ext/standard file.c formatted_print.c
iirc the reason why i changed it to unsigned was that actually the zend engine treated it as unsigned everywhere but in that particular struct. i also remember that i discussed that with andi and that he agreed to change this in the ze2 cvs module and that the extensions should be *fixed*. i agree that it doesn't make any sense to mix types. changing it to uint means to fix all the extensions, changing it to int means to fix the engine (and not just to revert my patch). harald. I might be misunderstanding the problem and I didn't have time to read the phrack article, but doesn't this mean that leaving it unsigned is better? It wouldn't pass the length check and thus, memcpy() wouldn't convert a negative number to something huge. The problem is that every single line of existing PHP extensions, both public and non-public, would need to be audited, if we were to switch the type, because 100% of the current code misinterpretes data from the ZE2 API right now. Changing the API does not solve the existing problem, it merely adds to it. For example, you can add a single central check to the engine today which checks string lengths to be at least 0. If the length field was changed to an unsigned type permanently, such a check would be impossible to implement in a portable way, because C does not define how a negative number will appear when cast to an unsigned type. - Sascha -- PHP Development Mailing List http://www.php.net/ To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Re: php4 /ext/standard file.c formatted_print.c
On Wed, Jan 15, 2003 at 06:36:20PM +0100, Harald Radi wrote: iirc the reason why i changed it to unsigned was that actually the zend engine treated it as unsigned everywhere but in that particular struct. i also remember that i discussed that with andi and that he agreed to change this in the ze2 cvs module and that the extensions should be *fixed*. i agree that it doesn't make any sense to mix types. changing it to uint means to fix all the extensions, changing it to int means to fix the engine (and not just to revert my patch). While I think changing the len field to unsigned or size_t itself is a good idea, it is also the case that there're certain security risks that should have been considered. IMHO as long as no one is likely to agree the idea to either modify Z_STRLEN macro or make a new macro Z_SAFE_STRLEN so that it would force the engine to bail out when the length exceeds the maximam value of signed integer (or possibly signed short) like Sascha said, more priority has to be taken on fixing the engine over the former because it hardly seems the change has been known by the numerous extension developers. Moriyoshi -- PHP Development Mailing List http://www.php.net/ To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Re: php4 /ext/standard file.c formatted_print.c
On Wed, 15 Jan 2003, Harald Radi wrote: iirc the reason why i changed it to unsigned was that actually the zend engine treated it as unsigned everywhere but in that particular struct. i also remember that i discussed that with andi and that he agreed to change this in the ze2 cvs module and that the extensions should be *fixed*. Well, fixing the engine is a small, finite task whereas auditing all existing extension on this planet is an open-ended one. I think it is easy to see that. You need to realize that once a certain API has been established, you cannot go around and change it at will. Especially if the breakage is as subtle as in this case. If the compiler dies, because a function takes a new number of arguments, it is something which becomes visible immediately. Signedness issues are usually hidden until someone exploits them. i agree that it doesn't make any sense to mix types. changing it to uint means to fix all the extensions, changing it to int means to fix the engine (and not just to revert my patch). In which areas of the engine did you notice defects? If we had a list, we could start from there. - Sascha -- PHP Development Mailing List http://www.php.net/ To unsubscribe, visit: http://www.php.net/unsub.php
RE: [PHP-DEV] Re: php4 /ext/standard file.c formatted_print.c
From: Sascha Schumann [mailto:[EMAIL PROTECTED]] iirc the reason why i changed it to unsigned was that actually the zend engine treated it as unsigned everywhere but in that particular struct. i also remember that i discussed that with andi and that he agreed to change this in the ze2 cvs module and that the extensions should be *fixed*. Well, fixing the engine is a small, finite task whereas auditing all existing extension on this planet is an open-ended one. I think it is easy to see that. you're propably a bit too optimistic, i can hardly imagine that all existing extensions all over the world compile out of the box against ZE2. i guess only a handful do this. You need to realize that once a certain API has been established, you cannot go around and change it at will. Especially if the breakage is as subtle as in this case. If the compiler dies, because a function takes a new number of arguments, it is something which becomes visible immediately. Signedness issues are usually hidden until someone exploits them. the only thing that should be realized is that compiler warnings are still a bad thing(tm). i don't see any difference in this compared to changing the number of arguments of a function. i agree that it doesn't make any sense to mix types. changing it to uint means to fix all the extensions, changing it to int means to fix the engine (and not just to revert my patch). In which areas of the engine did you notice defects? If we had a list, we could start from there. hmm, that patch is quite old, i can't remember very well. iirc almost everywhere, but i have to look at my commit again. harald. -- PHP Development Mailing List http://www.php.net/ To unsubscribe, visit: http://www.php.net/unsub.php
RE: [PHP-DEV] Re: php4 /ext/standard file.c formatted_print.c
you're propably a bit too optimistic, i can hardly imagine that all existing extensions all over the world compile out of the box against ZE2. i guess only a handful do this. Apparently, most of the extensions PHP ships with seem to work out of the box. I'm under the impression that almost no changes are required, unless you directly manipulate objects. That brings up the question -- do we have some documentation for extension authors which covers necessary changes. the only thing that should be realized is that compiler warnings are still a bad thing(tm). i don't see any difference in this compared to changing the number of arguments of a function. There is an important difference regarding developer awareness: No. of args change: The compiler will always bail out. Developer knows: Aha, I need to fix something. Signedness change: The compiler might issue a warning or not. Developer knows: Usually nothing. There are so many signedness issues in ext/standard alone, that I would not want to run with such warnings enabled. - Sascha -- PHP Development Mailing List http://www.php.net/ To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Re: php4 /ext/standard file.c formatted_print.c
I might be misunderstanding the problem and I didn't have time to read the phrack article, but doesn't this mean that leaving it unsigned is better? It wouldn't pass the length check and thus, memcpy() wouldn't convert a negative number to something huge. The problem is that every single line of existing PHP extensions, both public and non-public, would need to be audited, if we were to switch the type, because 100% of the current code misinterpretes data from the ZE2 API right now. Changing the API does not solve the existing problem, it merely adds to it. For example, you can add a single central check to the engine today which checks string lengths to be at least 0. If the length field was changed to an unsigned type permanently, such a check would be impossible to implement in a portable way, because C does not define how a negative number will appear when cast to an unsigned type. - Sascha -- PHP Development Mailing List http://www.php.net/ To unsubscribe, visit: http://www.php.net/unsub.php
[PHP-DEV] Re: php4 /ext/standard file.c formatted_print.c
Sorry but just a thought, in that line:
if (argc 1 (int)Z_STRLEN_P(return_value) len / 2) {
you're comparating len / 2 to an int, but len / 2 can be a float too no?
Ignore me if I'm wrong...
--
Regards.
M.CHAILLAN Nicolas
[EMAIL PROTECTED]
www.WorldAKT.com Hébergement de sites internets.
Moriyoshi Koizumi [EMAIL PROTECTED] a écrit dans le message de
news: [EMAIL PROTECTED]
moriyoshi Sat Jan 11 17:17:37 2003 EDT
Modified files:
/php4/ext/standard file.c formatted_print.c
Log:
Reduced compiler warnings in ZE2 build
Index: php4/ext/standard/file.c
diff -u php4/ext/standard/file.c:1.290 php4/ext/standard/file.c:1.291
--- php4/ext/standard/file.c:1.290 Thu Jan 9 18:23:32 2003
+++ php4/ext/standard/file.c Sat Jan 11 17:17:37 2003
@@ -21,7 +21,7 @@
+--+
*/
-/* $Id: file.c,v 1.290 2003/01/09 23:23:32 iliaa Exp $ */
+/* $Id: file.c,v 1.291 2003/01/11 22:17:37 moriyoshi Exp $ */
/* Synced with php 3.0 revision 1.218 1999-06-16 [ssb] */
@@ -1376,7 +1376,7 @@
ZVAL_STRINGL(return_value, buf, line_len, 0);
/* resize buffer if it's much larger than the result.
* Only needed if the user requested a buffer size. */
- if (argc 1 Z_STRLEN_P(return_value) len / 2) {
+ if (argc 1 (int)Z_STRLEN_P(return_value) len / 2) {
Z_STRVAL_P(return_value) = erealloc(buf, line_len + 1);
}
}
@@ -1559,7 +1559,7 @@
}
convert_to_string_ex(arg2);
convert_to_long_ex(arg3);
- num_bytes = MIN(Z_LVAL_PP(arg3), Z_STRLEN_PP(arg2));
+ num_bytes = MIN(Z_LVAL_PP(arg3), (int)Z_STRLEN_PP(arg2));
break;
default:
WRONG_PARAM_COUNT;
Index: php4/ext/standard/formatted_print.c
diff -u php4/ext/standard/formatted_print.c:1.61
php4/ext/standard/formatted_print.c:1.62
--- php4/ext/standard/formatted_print.c:1.61 Thu Jan 9 12:29:31 2003
+++ php4/ext/standard/formatted_print.c Sat Jan 11 17:17:37 2003
@@ -16,7 +16,7 @@
+--+
*/
-/* $Id: formatted_print.c,v 1.61 2003/01/09 17:29:31 wez Exp $ */
+/* $Id: formatted_print.c,v 1.62 2003/01/11 22:17:37 moriyoshi Exp $ */
#include math.h /* modf() */
#include php.h
@@ -504,7 +504,7 @@
currarg = 1;
- while (inposZ_STRLEN_PP(args[format_offset])) {
+ while (inpos (int)Z_STRLEN_PP(args[format_offset])) {
int expprec = 0;
PRINTF_DEBUG((sprintf: format[%d]='%c'\n, inpos, format[inpos]));
--
PHP Development Mailing List http://www.php.net/
To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Re: php4 /ext/standard file.c formatted_print.c
On Sat, 11 Jan 2003, Ilia A. wrote:
On January 11, 2003 06:03 pm, Moriyoshi Koizumi wrote:
On Sat, Jan 11, 2003 at 11:38:20PM +0100, [EMAIL PROTECTED] wrote:
Sorry but just a thought, in that line:
if (argc 1 (int)Z_STRLEN_P(return_value) len / 2) {
Does this mean we now always need to cast the result of the
Z_STRLEN_P/Z_STRLEN_PP macros to int? That seems pretty annoying and not to
producing ugly code.
Certainly not.
What kind of warnings was the compiler (which one?) issuing?
- Sascha
--
PHP Development Mailing List http://www.php.net/
To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Re: php4 /ext/standard file.c formatted_print.c
On Sat, Jan 11, 2003 at 05:56:23PM -0500, Ilia A. wrote:
On January 11, 2003 06:03 pm, Moriyoshi Koizumi wrote:
On Sat, Jan 11, 2003 at 11:38:20PM +0100, [EMAIL PROTECTED] wrote:
Sorry but just a thought, in that line:
if (argc 1 (int)Z_STRLEN_P(return_value) len / 2) {
Does this mean we now always need to cast the result of the
Z_STRLEN_P/Z_STRLEN_PP macros to int? That seems pretty annoying and not to
producing ugly code.
That's all due to the change of len field in zvalue_value union.
Do you mean this kind of warnings should be fixed
not by adding ugly casts but by restoring the structure like ZE1?
(ZE1)
typedef union _zvalue_value {
long lval; /* long value */
double dval;/* double value */
struct {
char *val;
int len;
} str;
HashTable *ht; /* hash table value */
zend_object obj;
} zvalue_value;
(ZE2)
typedef union _zvalue_value {
long lval; /* long value */
double dval;/* double value */
struct {
char *val;
zend_uint len;
} str;
HashTable *ht; /* hash table value */
/* struct {
zend_class_entry *ce;
HashTable *properties;
} obj;
*/
zend_object_value obj;
} zvalue_value;
I think uint'ifying len field is a good idea though.
Moriyoshi
--
PHP Development Mailing List http://www.php.net/
To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Re: php4 /ext/standard file.c formatted_print.c
On Sat, Jan 11, 2003 at 11:59:49PM +0100, Sascha Schumann wrote: Does this mean we now always need to cast the result of the Z_STRLEN_P/Z_STRLEN_PP macros to int? That seems pretty annoying and not to producing ugly code. Certainly not. What kind of warnings was the compiler (which one?) issuing? Please look at the win32/ZE2 compile log in http://snaps.php.net/ Moriyoshi -- PHP Development Mailing List http://www.php.net/ To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Re: php4 /ext/standard file.c formatted_print.c
The cause for this is that phanto changed the type of the string length from a signed type to zend_uint without providing any kind of justification (zvalue_value). As many past security advisories have shown, signedness issues are the frequent cause for severe vulnerabilities in software (recent examples include MySQL, OpenBSD kernel). As all existing PHP extensions and other relevant code assumes that the length of strings is denotated by a signed integer type, I hereby propose to revert that commit and to reinstate the old type. Any objections? - Sascha -- PHP Development Mailing List http://www.php.net/ To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Re: php4 /ext/standard file.c formatted_print.c
I imagine that strings greater then 2 billion characters are fairly rare, so I think it would be a good idea to revert the change as per Sascha's suggestion. This is a fairly old limitation and I think no one will miss this feature. Ilia -- PHP Development Mailing List http://www.php.net/ To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Re: php4 /ext/standard file.c formatted_print.c
On Sun, Jan 12, 2003 at 12:12:39AM +0100, Sascha Schumann wrote:
As many past security advisories have shown, signedness
issues are the frequent cause for severe vulnerabilities in
software (recent examples include MySQL, OpenBSD kernel).
Actually codes like below produce vulnerble runtimes because
the length of string is expected to be a positive integer value...
int maxlen;
...
if ((int)Z_STRLEN_P(length) maxlen) {
RETURN_FALSE;
}
memcpy(allocated_buf, Z_STRVAL_P(length), Z_STRLEN_P(length));
Any objections?
No objection from me.
Moriyoshi
--
PHP Development Mailing List http://www.php.net/
To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Re: php4 /ext/standard file.c formatted_print.c
On Sun, 12 Jan 2003, Moriyoshi Koizumi wrote: On Sun, Jan 12, 2003 at 12:12:39AM +0100, Sascha Schumann wrote: As many past security advisories have shown, signedness issues are the frequent cause for severe vulnerabilities in software (recent examples include MySQL, OpenBSD kernel). Actually codes like below produce vulnerble runtimes because the length of string is expected to be a positive integer value... Yes, unfortunately. Basically the same problem as in the OpenBSD kernel and its select syscall: http://www.phrack.org/phrack/60/p60-0x06.txt Quote: Whilst there is a check [1] on the 'nd' argument (nd represents the highest numbered descriptor plus one, in any of the fd_sets), which is checked against the p-p_fd-fd_nfiles (the number of open descriptors that the process is holding), this check is inadequate -- 'nd' is declared as signed [6], so it can be negative, and therefore will pass the greater-than check [1]. Then 'nd' is put through a macro [2], in order to calculate an unsigned integer, 'ni', which will eventually be used as the the length argument for the copyin operation. - Sascha -- PHP Development Mailing List http://www.php.net/ To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Re: php4 /ext/standard file.c formatted_print.c
At 12:38 AM 1/12/2003 +0100, Sascha Schumann wrote: On Sun, 12 Jan 2003, Moriyoshi Koizumi wrote: On Sun, Jan 12, 2003 at 12:12:39AM +0100, Sascha Schumann wrote: As many past security advisories have shown, signedness issues are the frequent cause for severe vulnerabilities in software (recent examples include MySQL, OpenBSD kernel). Actually codes like below produce vulnerble runtimes because the length of string is expected to be a positive integer value... Yes, unfortunately. Basically the same problem as in the OpenBSD kernel and its select syscall: http://www.phrack.org/phrack/60/p60-0x06.txt Quote: Whilst there is a check [1] on the 'nd' argument (nd represents the highest numbered descriptor plus one, in any of the fd_sets), which is checked against the p-p_fd-fd_nfiles (the number of open descriptors that the process is holding), this check is inadequate -- 'nd' is declared as signed [6], so it can be negative, and therefore will pass the greater-than check [1]. Then 'nd' is put through a macro [2], in order to calculate an unsigned integer, 'ni', which will eventually be used as the the length argument for the copyin operation. I might be misunderstanding the problem and I didn't have time to read the phrack article, but doesn't this mean that leaving it unsigned is better? It wouldn't pass the length check and thus, memcpy() wouldn't convert a negative number to something huge. Andi -- PHP Development Mailing List http://www.php.net/ To unsubscribe, visit: http://www.php.net/unsub.php
