Skip to content

Commit

Permalink
Fixed secutiry risk in uri filtering.
Browse files Browse the repository at this point in the history
  • Loading branch information
@frankdejonge
frankdejonge committed Sep 13, 2011
1 parent 6e2a4b2 commit b83b52a
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion classes/input.php
View file
Expand Up @@ -132,7 +132,7 @@ public static function uri()
strrchr($uri, '.') === $ext and $uri = substr($uri,0,-strlen($ext));

// Do some final clean up of the uri
static::$detected_uri = \Security::clean_uri(str_replace(array('//', '../'), '/', $uri));
static::$detected_uri = \Security::clean_uri(preg_replace(array("/\.+\//", '/\/+/'), '/', $uri));

return static::$detected_uri;
}
Expand Down

13 comments on commit b83b52a

@WanWizard
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stevewest @frankdejonge Any idea what the rationale behind this change was?

The original removed double slashes and ../ parent directory references from the URI, the new code simply strips all dots when followed by a slash, which I think is not what should happen here. Am I correct?

@emlynwest
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, this is old code. And I am not sure I really see the advantage here, has an issue come up regarding this?

@WanWizard
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, there's an issue posted on the forum, where someone needs to use a URI like /controller/method/param1./param2./param3, and the dots are stripped of the URI segments...

@emlynwest
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case unless @frankdejonge can shed any light on this I'd say we go back to the old behaviour. Either that or the person having an issue has to re-think their design a little.

@WanWizard
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really sure why we need to filter this anyway. Fuel URI's don't point to a position on disk, they are controller method mappings, so having a URI like /controller/../../../../../../etc/passwd would simply not do anything...

Even worse, standard rewriting first checks if the request points to an existing file or directory. So if a URI can be crafted to (in this example) access the password file, the request wouldn't even reach Fuel...

@emlynwest
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose it could be a possible issue if the uri parameter in the route has a greedy regex and the parameter is passed through to something that loads a file. But arguably that's also a bad developer issue.

@WanWizard
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly, then you're talking about either segments as input, or get variables as input. Both should be validated separate from the URI.

@emlynwest
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, in that case I don't think I have any issue with changing this behaviour.

@WanWizard
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@kenjis
Copy link
Contributor

@kenjis kenjis commented on b83b52a Jan 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to avoid string ../, the previous code is not enough.

<?php
$uri = '..../..../etc/passwd';
var_dump(str_replace(array('//', '../'), '/', $uri));
var_dump(preg_replace(array("/\.+\//", '/\/+/'), '/', $uri));

The results:

string(16) "../../etc/passwd"
string(11) "/etc/passwd"

All input data should be validated before using them, even if it comes from URI string.
But Fuel provides URI security, so users may forget the validations for URI data.

If you change the code, it may be BC break.

But arguably that's also a bad developer issue.

Yes. But if there is a bad developer, and if he/she have to check all code that uses URI data and have to add validation for them, it costs much.

@WanWizard
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The preg_replace isn't correct, since that would convert /this./that./ to /this/that/, the dots are stripped, which is why I started by reverting that commit.

I wonder if this is much of an issue, since the URI in Fuel isn't used for directory traversal at all, it is passed as a parameter to index.php.

Even worse, since most people start there rewrite rules with a -D and -F check before rewriting to index.php, a URL with ../ in it that resolves to an existing file will never be passed on to Fuel.

@WanWizard
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You will see that if you try a URL like http://example.org/../../../../etc/passwd, the webserver does a sort of realpath(), and will convert it to http://example.org/etc/passwd which will be rewritten to index.php?etc/passwd.

Unless in your app the controller method Etc::action_passwd() exists, this will lead to a 404 exception.

@WanWizard
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would even say we should completely remove this, since it doesn't bring anything in terms of security, and it potentially strips data, since URI segments are (or can be) used to pass parameters.

Please sign in to comment.