Re: A potential patch for Bug#60668

Also, I am not sure if php_trim is what we want here. It looks like vrana's initial proposal was to discard everything after CR-LF. This is different from trimming CR/LF/whitespace at the end of the string. I suppose that's a less important issue. I am mainly curious about your opinion as to why we might want to allow CR-LF in the user agent value. On Jan 25, 2012, at 7:33 PM, Chris Stockton wrote: > Hello, > > On Wed, Jan 25, 2012 at 5:43 PM, Kiyoto Tamura <me@ktamura.com> wrote: >> vrana has raise a good point in a potentially dangerous behavior with ini_set() in >> https://bugs.php.net/bug.php?id=60668. >> >> Here is my proposed patch. Feedback is appreciated. Thanks! >> >> Kiyoto Tamura >> >> diff --git a/Zend/zend_ini.c b/Zend/zend_ini.c >> index a7ec5d7..89b1287 100644 >> --- a/Zend/zend_ini.c >> +++ b/Zend/zend_ini.c >> @@ -83,6 +83,23 @@ static int zend_restore_ini_entry_wrapper(zend_ini_entry **ini_entry >> TSRMLS_DC) >> } >> /* }}} */ >> >> +static uint zend_trim_after_carriage_return(char *value, uint value_length) /* {{{ */ >> +{ >> + uint ii; >> + char prev_c = '\0', curr_c; >> + for (ii = 0; ii < value_length; ++ii) { >> + curr_c = *value; >> + if (prev_c == '\r' && curr_c == '\n') { >> + return ii - 1; >> + } >> + prev_c = curr_c; >> + ++value; >> + } >> + >> + return value_length; >> +} >> +/* }}} */ >> + > > To comment specifically on the patch itself; I am not sure you need a > new zend func here, I think it would be more convenient (and safer) to > use php_trim in standard/string.c. Although I must say looking at the > bug, I am not sure this is exactly something that should be "fixed". > It could be a BC break and in my opinion is just a demonstration of > failure to properly sanitize user input as opposed to a security flaw > in php design itself. > > -Chris