Fixed bug #66827 Session raises E_NOTICE when session name variable is array

[yohgaki]

Fixed possible user specific DoS.

https://bugs.php.net/bug.php?id=66827

[yohgaki]

I have to work on removing offensive cookie. I'll add this later.

[yohgaki]

Cookie spec is broken by design. This is known for years.

This patch removes unwanted E_NOTICE error. Therefore, apps will not catch error and die.

This patch also try to remove offensive cookie. However, to remove offensive cookie"s" completely, it should try to remove all possible combinations, path (including all parents), domain (including all subdomains), secure flag on/off, httponly flat on/off. It only tries to remove cookie with defined attributes.

Failure to remove offensive cookie results in uninitialized session.
(Note: Strict session has the same issue with the same reason. It prevent session module initialize session with attacker provided ID.)

It may be the time to write a function that delete all cookies. Please note that even if I write the function, DoS with offensive cookie is possible still (with path and domain combinations). I'm not sure if it worthes the effort.

[yohgaki]

Hmm. Why travis complains COOKIE_SET_COOKIE is undefined? It's defined in the same file and compiles w/o problem locally.

It seems travis is trying to compile different file, since I have COOKIE_SET_COOKIE in different lines.

[yohgaki]

Travis is trying to compile against master. That's the reason why it fails.

[Tyrael]

because the PR is targeted against master.

[yohgaki]

Thanks. I have branch that branched from local PHP-5.4 branch.
I suppose it doesn't work :(

It should be OK with PHP-5.4 branch. I'll commit this directly to the repository later.

[Tyrael]

you could just create a new pull request with a correct target so we can see the potential test failures before the merge, but I suppose you tested it properly in local.
on a different note: could you explain how this is a DOS vulnerability?
I mean yeah, if the client sends a bogus sid then we won't be able to restore his/her session, and emiting a warning has some computing power, but I fail to see how this is a dos vulnerability.
I also wonder if we would really go into such depth to try to find the offending cookie.

[yohgaki]

Since any offensive cookie is not deleted w/o this patch, session module will be confused by the cookie.

$_COOKIE['PHPSESSID'] = array() will raise array to string conversion E_NOTICE error. $_COOKIE['PHPSESSID'] = array() has forms of setcookie('PHPSESSID[]', 'whatever'). Session module's session ID cookie never overwrite PHPSESSID[] because session module sends PHPSESSID which is a different cookie.

Depending on order of sent cookies, PHPSESSID[] will always overrides PHPSESSID. Therefore, apps that detects errors may results in fatal error always. (I treat any errors and unhandled exceptions as fatal in my apps, for example)

Since PHPSESSID may never set, session module cannot initialize session. Therefore, the user may never login apps.

It's not a system wide DoS, but a user specific DoS.

[yohgaki]

I didn't add error logging for this, since there are only a few places that log errors. However, this kind of malformed data is most certainly an attack, so it would be better to log what is happening. I'll add error logging for this.

[yohgaki@dev github-php-src]$ grep error_log ext//.c
ext/dom/php_dom.c: and the total used memory into apaches error_log /
ext/standard/basic_functions.c:ZEND_BEGIN_ARG_INFO_EX(arginfo_error_log, 0, 0, 1)
ext/standard/basic_functions.c: PHP_FE(error_log, arginfo_error_log)
ext/standard/basic_functions.c: 0 = send to php_error_log (uses syslog or file depending on ini setting)
ext/standard/basic_functions.c:/ {{{ proto bool error_log(string message [, int message_type [, string destination [, string extra_headers]]])
ext/standard/basic_functions.c:PHP_FUNCTION(error_log)
ext/standard/basic_functions.c: if (_php_error_log_ex(opt_err, message, message_len, opt, headers TSRMLS_CC) == FAILURE) {
ext/standard/basic_functions.c:PHPAPI int _php_error_log(int opt_err, char message, char *opt, char *headers TSRMLS_DC) / {{{ _/
ext/standard/basic_functions.c: return _php_error_log_ex(opt_err, message, (opt_err == 3) ? strlen(message) : 0, opt, headers TSRMLS_CC);
ext/standard/basic_functions.c:PHPAPI int php_error_log_ex(int opt_err, char *message, int message_len, char *opt, char *headers TSRMLS_DC) / {{{ */
ext/standard/basic_functions.c: if (!php_mail(opt, "PHP error_log message", message, headers, NULL TSRMLS_CC)) {
ext/standard/basic_functions.c: if (_CHECK_PATH(varname, varname_len, "error_log") ||

[yohgaki]

Oops, there aren't any code that logs error w/o error event. I think this should be discussed on the list.

[datibbaw]

I liked the patch fine without the "cookie seek & destroy" code; doesn't a new cookie value get sent by the server when it gets rejected?

[yohgaki]

New cookie is sent regardless of this patch and cookie is set. It's just ignored by precedence unless offensive cookie is removed.

[yohgaki]

Attack is possible when apps are vulnerable to JavaScript injection. We may simply ignore offensive cookie and document how/why this kind of issue occurs.

[yohgaki]

https://travis-ci.org/yohgaki/php-src/builds/30296976

This is the patch for master. It includes type mismatch E_NOTICE error removal only. I'll commit this to 5.4 and up later.

[yohgaki]

https://travis-ci.org/yohgaki/php-src/builds/30296976
This patch is merged into the repo. Closing.