security issue in validators using regular expressions

Hi all.

Several yii validators use regular expressions, such as CEmailValidator, CUrlValidator, CNumberValidator.

The email and url validators contain regular expressions that can be easily exploited for DOS attacks.

The regular expressions in those validators can easily reach the stack overflow and the process will crash with a segmentation fault.

PCRE is a tool with documented limitations, it is not intended to be used for validation of arbitrary data.

http://bugs.exim.org/show_bug.cgi?id=1018

Here is some Code:

CUrlValidator contains this regexp:

$pattern=’/^(http|https):\/\/(([A-Z0-9][A-Z0-9_-])(\.[A-Z0-9][A-Z0-9_-])+)/i’;

Sending the following string of 4Kb length to any script that runs URL validation leads to segfault:

$str = ‘http://w’.str_repeat(’.a’,4000).’.ru’;

One can compose a similar string that will crashs the email validator, which is used everywhere (I don’t want to write exact code here).

I assume, sending 30 simultaneous requests of this kind will bring down a server till the processes are restarted.

Solution:

use filter_var() function - the one intentionally written for this purpose.

class CUrlValidator extends CValidator

{

//…

public function validateValue($value)


{


	return filter_var($value,FILTER_VALIDATE_EMAIL);


}

}

class CEmailValidator extends CValidator

{

//…

public function validateValue($value)


{


	$valid= filter_var($value,FILTER_VALIDATE_EMAIL);

//…

CNumberValidator does not seem to have an easily reachable security issue, but using a filter_var() would be definitely faster and more reliable:

if($this->integerOnly){

if (!filter_var($value,FILTER_VALIDATE_INT)){$this->addError($object,$attribute,$message);}

}else{

if(!filter_var($value,FILTER_VALIDATE_FLOAT,))…

}

filter_var is available from PHP 5.2… and Yii requirements allows PHP 5.1

by default the rules generated by gii don’t have the skipOnError flag and the check will run (as far as I remember), that is enough for sefgault

it’s the authors choice to care or not ;)

5.1 is long time ago deprecated

I would add a check for the filter_input availability at least, it’s just 1 line.

I noticed now that my mesage got truncated a bit…

I wanted to say that you can prevent this problem by checking the length of the entered value… and that Gii does that by default…

there where a suggestion for filter_var for CEmailValidator… here you can see the response from Qiang - http://code.google.com/p/yii/issues/detail?id=1373

Please open again an issue for filter_var to be used in more validators… and post a link to this thread…

I can’t reopen it, but I commented it with a link to this topic.

to be precize, filter_var() uses a regular expression as well,

but before running the regexp it checks the length of the string

php-5.3.6/ext/filter/logical_filters.c:

void php_filter_validate_email(PHP_INPUT_FILTER_PARAM_DECL) /* {{{ */

if (Z_STRLEN_P(value) > 320) {

RETURN_VALIDATION_FAILED

}

you should do that too, at least, if filter_var() is against your religion

I wonder if we can add this strlen() check in our code, too. Emails, Numbers, etc. will hardly exceed, say 1000 characters…

But this issue makes me wonder, if CClientScript could also be affected: It uses a PCRE to insert some tags in header or body. As this is applied on every page, a security issue would have a much bigger impact. I think about users, submitting form content with errors. After validation that content is inserted into the form again (say in a textarea) and could maybe affect the above replace mechanism.

Haven’t thought more about this, so maybe not an issue at all. But what would you say?

It’s much cheaper to add a check then to risk leaving the framework vulnerable by default, imho.

Not all regular expressions are similarily dangerous. The ones containing subpatterns with unlimited multipliers like ()* and ()+ are easy to reach the stack overflow.

Also, not every site uses CClientScript and has user-submitted content.

But email validation is used in every site in a world-accessible page.

Did you experience DOS attacks, lost money on it? The ones who did know the answer. If your sites are just for fun and you won’t loose year bonuses when it’s down, you would not care ofc.

I think you misunderstood: I’m not asking wether a security issue needs to be fixed - of course it does! I was rather trying to figure out, where Yii’s code could be affected.

BTW i’m not responsible for fixing this, i’m just throwing in my thoughts here.

Thank you for bringing up this issue. I just committed a fix for the email and url validators. For other validators, it is better developers add some length validators before them. Please help review the change and let me know if anything further needs to be done. Thanks.

http://code.google.com/p/yii/source/detail?r=3242

looks ok for me

as a general thought, it may be more safe to write (){0,50} instead of ()* and {1,50} instead of + for subpatterns

but I did not test it yet

Mike, yes, I misunderstood you, sorry ;)