Collision counting against HashDos attacks #1706

bwoebi · 2015-12-30T22:11:17Z

This PR supersedes #1565.

It adds graceful handling with exceptions on necessary places:
array_combine, array_column, array_flip, array_merge(_recursive) and the trivial array modification ($a["key"] = $val;)
and general graceful handling for:
parse_str and json_decode

bukka · 2016-09-22T18:06:34Z

ext/json/json_parser.y

-		if (Z_REFCOUNTED_P(zvalue)) {
-			Z_DELREF_P(zvalue);
-		}
+		zval_dtor(zvalue);


The value is dtored later so we don't need to call ti now. The only thing that we need to do is Z_DELREF_P(zvalue) because there is Z_ADDREF_P. Technically zvalue will be always refcounted so we could just do Z_DELREF_P(zvalue) but zval_dtor is unnecessary.

bukka · 2016-09-22T18:08:00Z

Zend/tests/bug70644_2.phpt

@@ -0,0 +1,18 @@
+--TEST--


this should definitely go to json or there need to be at least skipif json ext is not there

bukka · 2016-09-22T18:15:02Z

ext/json/json_parser.tab.c

 	}
 	zend_string_release(key);

+	if (EG(exception)) {
+		parser->scanner.errcode = PHP_JSON_ERROR_DEPTH;


Thinking about it, I would actually go for a new json error as the depth is for something else so user knows that it failed because of the attack.

In general, adding a new JSON error isn't worth it, as, like Nikita says:

Getting C=1000
collisions for a single hash-value is extremely unlikely until you
explicitly design your keys to trigger this situation.

Thus it's just too unlikely to ever be helpful in legitimate situations.

I think that it would be useful for users that want to identify that there is some attempt to attack their app. Lets say you have got a public API and you would like to identify that someone is sending such json (possibly trying to dos php version without this patch), then you might want to like to possibly log such case specially. That was my thinking when I suggested a new error... :)

bwoebi · 2016-09-22T18:20:24Z

@bukka I'm planning to update the patch soon [It's not perfect yet, it was more of a PoC back then].

bwoebi · 2016-09-25T20:43:04Z

The updated patch also includes support for SOAP, XML and databases.

Also applies suggestions by @bukka.

nikic · 2016-09-29T13:59:28Z

Did you already check the perf impact?

bwoebi · 2016-09-29T14:10:14Z

@nikic Last time I checked (back then in January), the difference was well within margin of error upon running it against wordpress. There should not be any difference in that result now.

nikic · 2016-09-29T14:13:09Z

It would be good to benchmark individual array operations as well. IIRC WordPress results stayed the same even after switching to SipHash, while individual operations regressed a lot. Both numbers are important.

nikic

My main overall concern is that there is a lot of places where the _exception variant is used without explicitly checking the exception condition. In some cases it will even return a success condition from the function in this case. I'm sure this is going to be fine in most cases, but I don't want to review what the exact effect of throwing an exception somewhere deep in mysqlnd is, while also returning a success value.

I'd like to either have additional test coverage, or drop the exception changes where test coverage cannot be easily obtained. Many of those seem rather tenuous anyway, such as column names in database results (generally not attacker controlled).

nikic · 2016-09-29T14:16:50Z

ext/standard/tests/array/bug70644.phpt

+?>
+--EXPECTF--
+
+Fatal error: Uncaught HashCollisionError: Too many collisions in array in %s:%d


For tests, I'd suggest to always catch the HashCollisionError to make sure that there are no leaks.

nikic · 2016-09-29T14:18:55Z

Zend/zend_vm_def.h

 		} else if (EXPECTED(Z_TYPE_P(offset) == IS_LONG)) {
 			hval = Z_LVAL_P(offset);
 ZEND_VM_C_LABEL(num_index):
-			zend_hash_index_update(Z_ARRVAL_P(EX_VAR(opline->result.var)), hval, expr_ptr);
+			zend_hash_index_update_exception(Z_ARRVAL_P(EX_VAR(opline->result.var)), hval, expr_ptr);


A test:

<?php $elems = ''; for ($i = 0; $i < 2048; $i++) { $elems .= $i * (1<<16) . " => 0, "; } $code = <<<PHP \$x = 0; return [\$x, $elems]; PHP; try { eval($code); } catch (HashCollisionError $e) { echo $e->getMessage(), "\n"; }

nikic · 2016-09-29T14:21:49Z

ext/exif/exif.c

-							add_assoc_string(&tmpi, name, val);
+							zval zv;
+							ZVAL_STRING(&zv, val);
+							zend_hash_str_update_exception(Z_ARRVAL(tmpi), name, strlen(name), &zv);


What about the add_assoc_stringl for the cases above?

Or not only above for that matter

They are not taking user-input, hence I couldn't be bothered; I may still add it if you really like to?

To clarify, are you saying that this is the only branch that takes user-input? Not being familiar with exif, they all look the same to me.

Yeah, I followed it through source code; it's the only branch.

nikic · 2016-09-29T14:28:05Z

ext/json/json_parser.y

+		zval_dtor(object);
+		zend_clear_exception();
+		return FAILURE;
+	}


It looks like the exception only covers the array case. Wouldn't it still be possible to trigger a fatal error if decoding is done into objects?

zend_std_write_property should take care of that itself.

Right, I missed that change.

nikic · 2016-09-29T14:32:43Z

ext/mysqlnd/mysqlnd_statistics.c

+		zend_string *name = zend_string_init(names[i].s, names[i].l, 0);
+		ZVAL_STR(&zv, zend_strpprintf(0, MYSQLND_LLU_SPEC, stats->values[i]));
+		zend_hash_update_exception(Z_ARRVAL_P(return_value), name, &zv);
+		zend_string_release(name);


The names used here seem to be hardcoded: https://github.com/bwoebi/php-src/blob/5042212ef50143803262bbeff1a2b3e91867ce5d/ext/mysqlnd/mysqlnd_statistics.c#L29

nikic · 2016-09-29T14:49:55Z

ext/standard/array.c

-				zval_add_ref(zv);
+				zval zv;
+				ZVAL_COPY_VALUE(&zv, src_entry);
+				zval_add_ref(&zv);


add_ref needs to be called after insertion, otherwise it will not behave correctly. You can replace it with some manual rc1 ref deref code.

Are you sure? Note that I'm operating on a value copy here. (Yes, zval_add_ref(src_entry) would break, that's right. But now it's on a copy...?!)

You're right, it's fine.

nikic · 2016-09-29T14:51:39Z

ext/standard/iptc.c

@@ -371,7 +371,7 @@ PHP_FUNCTION(iptcparse)
 		if ((element = zend_hash_str_find(Z_ARRVAL_P(return_value), key, strlen(key))) == NULL) {
 			array_init(&values);

-			element = zend_hash_str_update(Z_ARRVAL_P(return_value), key, strlen(key), &values);
+			element = zend_hash_str_update_exception(Z_ARRVAL_P(return_value), key, strlen(key), &values);


The return value is used below.

nikic · 2016-09-29T14:53:05Z

ext/standard/string.c

+	/* parse_str() tries to parse as much as possible (per treat_data behavior), hence we just ignore possible HashCollisionErrors */
+	if (EG(exception) && EG(exception)->ce == zend_ce_hash_collision_error) {
+		zend_clear_exception();
+	}


Not sure I get why this needs special handling. Imho this should throw like everything else.

When max_input_vars is exceeded, only a warning is emitted and no exceptions either.

Perhaps we should make this function emit a warning in this case too?

Maybe. I'd also like to know what currently happens with this exception if it occurs during request parsing (i.e. max_input_vars > collision limit)

I'm not sure how exactly I'd phpt that? It would somehow involve using CGI? Do you know how that works?

From a quick look tests/basic/027.phpt might help.

Good, added a test tests/basic/bug70644.phpt … it is just fatal-ing here:

Fatal error: Too many collisions in array in Unknown on line 0

which is fine.

nikic · 2016-09-29T14:57:46Z

ext/xml/xml.c

@@ -692,7 +692,7 @@ static void _xml_add_to_info(xml_parser *parser,char *name)
 	if ((element = zend_hash_str_find(Z_ARRVAL(parser->info), name, strlen(name))) == NULL) {
 		zval values;
 		array_init(&values);
-		element = zend_hash_str_update(Z_ARRVAL(parser->info), name, strlen(name), &values);
+		element = zend_hash_str_update_exception(Z_ARRVAL(parser->info), name, strlen(name), &values);


element is used below.

nikic · 2016-09-29T14:58:47Z

ext/xmlrpc/xmlrpc-epi-php.c

@@ -1292,7 +1292,7 @@ int set_zval_xmlrpc_type(zval* value, XMLRPC_VALUE_TYPE newtype) /* {{{ */
 						ZVAL_LONG(&ztimestamp, timestamp);

 						convert_to_object(value);
-						if (zend_hash_str_update(Z_OBJPROP_P(value), OBJECT_TYPE_ATTR, sizeof(OBJECT_TYPE_ATTR) - 1, &type)) {
+						if (zend_hash_str_update_exception(Z_OBJPROP_P(value), OBJECT_TYPE_ATTR, sizeof(OBJECT_TYPE_ATTR) - 1, &type)) {


This will treat reaching the collision limit as a success condition. Intentional?

bwoebi · 2016-09-29T22:49:58Z

There are things like iptc, xmlrpc or mysql protocol etc. where I'd have to manually craft an example - I lack the knowledge of the exact formats and don't really want to bother reading it up in detail... Thus there will be no tests unless someone else provides these...

nikic · 2017-01-06T09:12:00Z

As mentioned on the other PR, we didn't get a consensus on internals about this change, so probably this is going to need an RFC to go forward.

See also #2149 for an alternative approach (with impractical perf impact).

php-pulls · 2018-09-26T21:53:56Z

Comment on behalf of kalle at php.net:

Closing due to inactivity

laruence added Feature Security labels Jan 2, 2016

bukka reviewed Sep 22, 2016

View reviewed changes

bwoebi force-pushed the collisionCount branch 6 times, most recently from 4cbb283 to 5042212 Compare September 25, 2016 19:24

nikic requested changes Sep 29, 2016

View reviewed changes

Count collisions during HT insertion operations

67c405c

bwoebi force-pushed the collisionCount branch from 5042212 to 5103d9e Compare September 29, 2016 22:44

bwoebi force-pushed the collisionCount branch 2 times, most recently from 2ea09f5 to 6e1a29a Compare September 29, 2016 23:18

Use exceptions instead of fatals in well determined places

2d6c5cb

bwoebi force-pushed the collisionCount branch from 6e1a29a to 2d6c5cb Compare September 29, 2016 23:28

krakjoe added the Waiting on Author label Jan 6, 2017

krakjoe assigned nikic Jan 6, 2017

nikic added RFC and removed Feature labels Jan 6, 2017

krakjoe removed the Waiting on Author label Jan 6, 2017

nikic removed their assignment Jan 9, 2017

php-pulls closed this Sep 26, 2018

Collision counting against HashDos attacks #1706

Collision counting against HashDos attacks #1706

Uh oh!

Conversation

bwoebi commented Dec 30, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bwoebi Sep 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bwoebi commented Sep 22, 2016

Uh oh!

bwoebi commented Sep 25, 2016

Uh oh!

nikic commented Sep 29, 2016

Uh oh!

bwoebi commented Sep 29, 2016

Uh oh!

nikic commented Sep 29, 2016

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bwoebi Sep 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bwoebi Sep 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bwoebi Sep 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

bwoebi Sep 22, 2016 •

edited

Loading

bwoebi Sep 29, 2016 •

edited

Loading

bwoebi Sep 29, 2016 •

edited

Loading

bwoebi Sep 29, 2016 •

edited

Loading