request for review: file reading security

Anonymous Monk has asked for the wisdom of the Perl Monks concerning the following question:

Hi.

I wrote a small index file for a website, it should read the contents from html files, and print them inside of a template.

what I'm concerned with is if the actual file passing method is secure enough:

this is what I have:

my $req = $ENV{QUERY_STRING};

$req =~ s/^.*\///;
$req = 'index' if -e $req;

my $page = "pages/$req.html";

$page = "pages/index.html" unless -e $page;
[download]

the pages are inside a pages/ folder, and the request is such that index.pl?about will give me the about.html page.

do you see any security flaw with this method? like being somehow able to go back in folders and read stuff you shouldn't be reading?

thank you.

Comment on request for review: file reading security Download Code

Replies are listed 'Best First'.
Re: request for review: file reading security by cchampion (Curate) on Sep 05, 2004 at 10:45 UTC
It isn't only the security. Your line `$req = 'index' if -e $req;` will make all requests invoke "index". Your code means " assign to 'index' if a file named $req exists". I am not sure what you wanted to achieve that way, but here is how I would do it. `use strict; use warnings; my $req = $ENV{QUERY_STRING}; # limits the applicability. Only lowercase file names $req = lc $req; # remove all unwanted characters from the beginning of the string. # In this example, everything except alphanumerics # and underscore is removed. $req =~ s/^[^a-z_0-9]+//; # remove an extension, if any $req =~ s/\.html$//; # default value is the index my $page = "pages/index.html"; # if the page exists, then we use it $page = "pages/$req.html" if -e "pages/$req.html" ;` [download] Also, consider using CGI param instead of reading the environment. HTH	[reply] [d/l] [select]
Re^2: request for review: file reading security by Anonymous Monk on Sep 05, 2004 at 14:32 UTC
Your line $req = 'index' if -e $req; will make all requests invoke "index" no it won't. The QUERY_STRING should not match anything, since the filename would be composed as pages/QUERY_STRING.html That's why if it matches any file, it should roll back to a default.	[reply]
Re: request for review: file reading security by Zed_Lopez (Chaplain) on Sep 05, 2004 at 09:37 UTC
Yeah. As written, the user could pass, e.g., `../topsecretpages/index.html` and start looking at the topsecretpages directory that exists at the same level as pages. (Of course, the user would have to guess or learn the name of the directory, and it is to be hoped you don't really have top secret pages lying around under your web server's document root without any protection.) Updated: Like the followups say, the regexp dealt with that. Teach me to answer SoPWs in the middle of the night...	[reply] [d/l]
Re^2: request for review: file reading security by Anonymous Monk on Sep 05, 2004 at 14:33 UTC
that's why there is `$req =~ s/^.*\///;` which should take care of that.	[reply] [d/l]
Re^3: request for review: file reading security by pbeckingham (Parson) on Sep 05, 2004 at 14:58 UTC
~~I believe your code should look for literal periods:~~ ~~`$req =~ s/^\.\.\///;`~~ ~~[download]~~ But that's still poor, because what about: `blah/../../topsecretpages/page.html` [download] or `../../topsecretpages/page.html` [download] Update: Chady is right. I retract. pbeckingham - typist, perishable vertebrate.	[reply] [d/l] [select]
Re^4: request for review: file reading security by Chady (Priest) on Sep 05, 2004 at 15:57 UTC
Re: request for review: file reading security by CountZero (Bishop) on Sep 05, 2004 at 12:39 UTC
Why not using an existing templating system? It probably has already all you try to achieve here. CountZero "If you have four groups working on a compiler, you'll get a 4-pass compiler." - Conway's Law	[reply]


P is for Practical
	PerlMonks