Fix #2670 #2671

Merged
merged 5 commits into from Jan 28, 2014

Projects

None yet

3 participants

@janisto
Contributor
janisto commented Jul 22, 2013

fixes #2670

@samdark samdark and 1 other commented on an outdated diff Jul 22, 2013
framework/web/CHttpRequest.php
@@ -666,7 +666,22 @@ public function getUserAgent()
*/
public function getUserHostAddress()
{
- return isset($_SERVER['REMOTE_ADDR'])?$_SERVER['REMOTE_ADDR']:'127.0.0.1';
+ foreach(array('HTTP_CLIENT_IP','HTTP_X_FORWARDED_FOR','HTTP_X_FORWARDED','HTTP_X_CLUSTER_CLIENT_IP','HTTP_FORWARDED_FOR','HTTP_FORWARDED','REMOTE_ADDR') as $key)
@samdark
samdark Jul 22, 2013 Member

We should not trust HTTP_* because these are easy to forge via HTTP headers.

@janisto
janisto Jul 22, 2013 Contributor

I know, but isn't this better with filter_var?

@samdark
samdark Jul 22, 2013 Member

No, it's not. I can provide valid IP but different from real one. For example, if I'd use such check for Gii and limiting its use to 127.0.0.1 the check would be very easy to pass with just sending "forwarded for" header containing 127.0.0.1

@janisto
janisto Jul 22, 2013 Contributor

Makes sense.

@samdark samdark and 1 other commented on an outdated diff Jul 22, 2013
framework/web/CHttpRequest.php
@@ -666,7 +666,22 @@ public function getUserAgent()
*/
public function getUserHostAddress()
{
- return isset($_SERVER['REMOTE_ADDR'])?$_SERVER['REMOTE_ADDR']:'127.0.0.1';
+ foreach(array('HTTP_CLIENT_IP','HTTP_X_FORWARDED_FOR','HTTP_X_FORWARDED','HTTP_X_CLUSTER_CLIENT_IP','HTTP_FORWARDED_FOR','HTTP_FORWARDED','REMOTE_ADDR') as $key)
+ {
+ if(array_key_exists($key,$_SERVER)===true)
+ {
+ foreach(explode(',',$_SERVER[$key]) as $ip)
+ {
+ $ip=trim($ip);
+ if(version_compare(PHP_VERSION,'5.2.0')>=0)
@samdark
samdark Jul 22, 2013 Member

Minimum requirement for Yii2 is 5.3.

@janisto
janisto Jul 22, 2013 Contributor

This is Yii1.1 ;)

@samdark samdark and 1 other commented on an outdated diff Jul 22, 2013
framework/web/CHttpRequest.php
@@ -666,7 +666,17 @@ public function getUserAgent()
*/
public function getUserHostAddress()
{
- return isset($_SERVER['REMOTE_ADDR'])?$_SERVER['REMOTE_ADDR']:'127.0.0.1';
+ $addr=isset($_SERVER['REMOTE_ADDR'])?$_SERVER['REMOTE_ADDR']:'127.0.0.1';
+ foreach(explode(',',$addr) as $ip)
+ {
+ $ip=trim($ip);
+ if(version_compare(PHP_VERSION,'5.2.0')>=0)
@samdark
samdark Jul 22, 2013 Member

I don't like that for 5.2 it will not validate IPs while for 5.3 it will since it introduces possible errors when developing on 5.3 and then deploying to 5.2.

@janisto
janisto Jul 22, 2013 Contributor

It will validate for versions 5.2.0 and up, but I understand your point.
http://php.net/manual/en/function.filter-var.php

@samdark samdark and 1 other commented on an outdated diff Jul 22, 2013
framework/web/CHttpRequest.php
@@ -666,7 +666,17 @@ public function getUserAgent()
*/
public function getUserHostAddress()
{
- return isset($_SERVER['REMOTE_ADDR'])?$_SERVER['REMOTE_ADDR']:'127.0.0.1';
+ $addr=isset($_SERVER['REMOTE_ADDR'])?$_SERVER['REMOTE_ADDR']:'127.0.0.1';
+ foreach(explode(',',$addr) as $ip)
@samdark
samdark Jul 22, 2013 Member

As I know, $_SERVER['REMOTE_ADDR'] is always a single IP.

@janisto
janisto Jul 22, 2013 Contributor

Hmm, I'm pretty sure there can be multiple IP addresses. I could be wrong.

@samdark
samdark Jul 22, 2013 Member

It can be multiple IPs in HTTP headers such as X-Forwarded-For but in REMOTE_ADDR it's always TCP address and it should be the only one all the time.

@cebe cebe merged commit e8ba2d5 into yiisoft:master Jan 28, 2014
@cebe
Member
cebe commented Jan 28, 2014

Thanks!

@janisto
Contributor
janisto commented Jan 28, 2014

Don't thank me yet. Make sure that this pull request didn't compromize any security issues.

@janisto
Contributor
janisto commented Jan 28, 2014

@samdark let's hear your opinion.

@samdark
Member
samdark commented Jan 29, 2014

I think it should be reverted. Not because of any security issue but because it's basically not needed. $_SERVER['REMOTE_ADDR'] is from TCP/IP so it's always a valid IP.

@cebe
Member
cebe commented Jan 29, 2014

rethought it too and I agree. should be reverted.

@samdark
Member
samdark commented Jan 29, 2014

Can you handle it?

@cebe cebe added a commit that referenced this pull request Jan 29, 2014
@cebe cebe reverted pull request #2671 for issue #2670 9a1eac1
@cebe
Member
cebe commented Jan 29, 2014

done. 9a1eac1

@twiesenthal twiesenthal pushed a commit to twiesenthal/yii that referenced this pull request May 21, 2014
@cebe cebe reverted pull request #2671 for issue #2670 812eb78
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment