Yii Framework Forum: security issue in validators using regular expressions - Yii Framework Forum

Jump to content

Page 1 of 1
  • You cannot start a new topic
  • This topic is locked

security issue in validators using regular expressions all servers with yii sites using email/url validators are vulnerable Rate Topic: -----

#1 User is offline   grigori 

  • Junior Member
  • Pip
  • Yii
  • Group: Members
  • Posts: 41
  • Joined: 06-February 11

Posted 25 May 2011 - 07:41 AM

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...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,))...
}
1

#2 User is offline   Maurizio Domba Cerin 

  • Yii - Yesss It Is !!!
  • Yii
  • Group: Yii Dev Team
  • Posts: 4,542
  • Joined: 12-October 09
  • Location:Croatia

Posted 25 May 2011 - 07:48 AM

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

This post has been edited by mdomba: 25 May 2011 - 08:05 AM
Reason for edit: deleted truncated text

Find more about me.... btw. Do you know your WAN IP?
0

#3 User is offline   grigori 

  • Junior Member
  • Pip
  • Yii
  • Group: Members
  • Posts: 41
  • Joined: 06-February 11

Posted 25 May 2011 - 07:57 AM

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.
0

#4 User is offline   Maurizio Domba Cerin 

  • Yii - Yesss It Is !!!
  • Yii
  • Group: Yii Dev Team
  • Posts: 4,542
  • Joined: 12-October 09
  • Location:Croatia

Posted 25 May 2011 - 08:04 AM

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.c.../detail?id=1373

Please open again an issue for filter_var to be used in more validators... and post a link to this thread...
Find more about me.... btw. Do you know your WAN IP?
0

#5 User is offline   grigori 

  • Junior Member
  • Pip
  • Yii
  • Group: Members
  • Posts: 41
  • Joined: 06-February 11

Posted 25 May 2011 - 08:11 AM

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

#6 User is offline   grigori 

  • Junior Member
  • Pip
  • Yii
  • Group: Members
  • Posts: 41
  • Joined: 06-February 11

Posted 25 May 2011 - 10:16 AM

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
0

#7 User is offline   Mike 

  • Elite Member
  • PipPipPipPipPip
  • Yii
  • Group: Members
  • Posts: 3,017
  • Joined: 06-October 08
  • Location:Upper Palatinate

Posted 26 May 2011 - 01:31 AM

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?
0

#8 User is offline   grigori 

  • Junior Member
  • Pip
  • Yii
  • Group: Members
  • Posts: 41
  • Joined: 06-February 11

Posted 28 May 2011 - 02:33 AM

View PostMike, on 26 May 2011 - 01:31 AM, said:

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

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.

Quote

But this issue makes me wonder, if CClientScript could also be affected: It uses a PCRE to insert some tags in header or body.

Quote

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

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.
0

#9 User is offline   Mike 

  • Elite Member
  • PipPipPipPipPip
  • Yii
  • Group: Members
  • Posts: 3,017
  • Joined: 06-October 08
  • Location:Upper Palatinate

Posted 28 May 2011 - 03:36 AM

View Postgrigori, on 28 May 2011 - 02:33 AM, said:

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.
0

#10 User is offline   qiang 

  • Yii Project Lead
  • Yii
  • Group: Yii Dev Team
  • Posts: 5,907
  • Joined: 04-October 08
  • Location:DC, USA

Posted 28 May 2011 - 09:28 AM

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.c...e/detail?r=3242
1

#11 User is offline   grigori 

  • Junior Member
  • Pip
  • Yii
  • Group: Members
  • Posts: 41
  • Joined: 06-February 11

Posted 29 May 2011 - 04:03 AM

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 ;)
0

Share this topic:


Page 1 of 1
  • You cannot start a new topic
  • This topic is locked

1 User(s) are reading this topic
0 members, 1 guests, 0 anonymous users