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;
+}
+/* }}} */
/*
- Startup / shutdown
*/
@@ -288,6 +305,11 @@ ZEND_API int zend_alter_ini_entry_ex(char *name, uint name_length, char new_val
zend_hash_add(EG(modified_ini_directives), name, name_length, &ini_entry, sizeof(zend_ini_entry), NULL);
}
-
// per Bug #60668, truncate the string after /r/n for user_agent for security
-
if (strcmp(name, "user_agent") == 0) {
-
new_value_length = zend_trim_after_carriage_return(new_value, new_value_length); -
}
-
duplicate = estrndup(new_value, new_value_length);
if (!ini_entry->on_modify
@@ -672,6 +694,7 @@ ZEND_API ZEND_INI_MH(OnUpdateStringUnempty) /* {{{ */
p = new_value;
return SUCCESS;
}
/ }}} */
/*
Hello,
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
Thanks for the pointers. While I agree with you that application developers must be cognizant of input sanitization, I am not sure what's the value of allowing CR-LF in the user agent value. Said another way, if the user agent contains CR-LF, it was most definitely not meant to be there. Can you elaborate on the downside of the stricter check advocated by vrana?
Personally, I am ambivalent. I am just curious about the other side of the argument.
Thanks,
Kiyoto Tamura
Hello,
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
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.
Hello,
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
Hello,
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.
Ah I see didn't think enough about it, basically my point is for such
a simple string op there is likely something already to do it,
probably still is a function in strings to take care of it.
As for the "feature" of \r\n working in user-agent init set, my main
point is that is a BC break, since it is slightly advocated to use it
as a hack in the docs here [1]. At the end of the day passing any
user input to literally any php function without sanitization can be
dangerous given the right context. I think this specific one would
fall under the developers hands, but hey it's just my opinion you can
see what the core devs say I might be a bit off base.
-Chris
[1] http://php.net/wrappers.http#wrappers.http.example.custom.headers
Thanks for elaborating on the "BC break" (I googled it to no avail). I guess such a change (discarding everything after CR-LF) would break the code using BC breaks.
Either way, at least I am fully aware of how ini_set behaves ;)
Hello,
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.
Ah I see didn't think enough about it, basically my point is for such
a simple string op there is likely something already to do it,
probably still is a function in strings to take care of it.As for the "feature" of \r\n working in user-agent init set, my main
point is that is a BC break, since it is slightly advocated to use it
as a hack in the docs here [1]. At the end of the day passing any
user input to literally any php function without sanitization can be
dangerous given the right context. I think this specific one would
fall under the developers hands, but hey it's just my opinion you can
see what the core devs say I might be a bit off base.-Chris
[1] http://php.net/wrappers.http#wrappers.http.example.custom.headers
About Kiyoto's patch:
Some servers would read as new headers if the newlines were just \n or \r
(which would be illegal per HTTP spec). I think the characters to ban
are: \n \r \0
Just replace your call to zend_trim_after_carriage_return with:
- strtok(new_value, "\r\n"); // Truncate on \n, \r and \0
- new_value_length = strlen(new_value);
Chris Stockton wrote:
As for the "feature" of \r\n working in user-agent init set, my main
point is that is a BC break, since it is slightly advocated to use it
as a hack in the docs here [1].
Adding a new header by inserting it with user_agent is a really buggy hack.
It should be documented as "in php < 5.4 it used to be possible to add a
new
header modifying the ini user-agent. On newer versions you need to use
stream_context_set_params(, array('header' => $headers) )"
(plus a good example).
The proper stream_context_set_params way has apparently been available at
least since PHP 5.0. It's just hard to find.
At the end of the day passing any
user input to literally any php function without sanitization can be
dangerous given the right context.
Sure, but that's no reason to not improve it, specially if you do it on
a major
release (eg. PHP 5.4).
Even though it works, it should be obvious that it isn't expected to be
used that way.
So there should be little problem with it going away,