CODEWITHSUNDEEP
phpsecuritydynamicinclude
Johnny
Johnny

Reputation:

regex and $_GET

I'm making a little site for a friend, noobfriendly, so she can add pages easy. I think I'm just gonna include everything in index.php. So she can just drop a page in a folder named /pages/ and it's done.

index.php

if (preg_match('/[a-zA-Z]/', $_GET['page'])){
$page = 'pages/'.$_GET['page'].'.php';

if ($page) {
include $page;
} else {
exit;
}

} else {
include('pages/home.php');
}

This is pretty safe right? I'm only allowing letters a-z. I mean it's not gonna be a big site like google. Well I'm just a hobbycoder so I'm asking guys to make sure :)

What do you think?

Upvotes: 0

Views: 452

Answers (6)

reconbot
reconbot

Reputation: 5287

I really don't like reg editing out strings in paths, you never know what quirks of browsers and filesystems can be exploited. What you really should do is not actually use their input but use it to check against valid filenames.

Use glob or scandir to get a list of the files in the pages directory and then use in_array to see if the requested string is a file. If you try to strip out path elements, you leave yourself open to making a mistake.

Upvotes: 1

0scar
0scar

Reputation: 3200

preg_match('/[a-zA-Z]/', $_GET['page'], $match)

if ($match) {
    $page = "pages/$match.php";
    if(file_exists($page){
        include $page;
    } else {
        include "pages/404.php";
    }
} else {
    include "pages/home.php";
}

Maybe? If 'page' is set to 'blabla89349' this will include page 'blabla.php'. I'm not sure if that is what you intended? Otherwise you could be strict about it and

if ($match == $_GET['page']) {
...

Upvotes: 0

Ivan Krechetov
Ivan Krechetov

Reputation: 19220

No. a/../../../ will also match the regexp

Use '/^[a-zA-Z]+$/'

This way you make sure that there's nothing before and after the sequence of the allowed letters.

Upvotes: 1

Fake51
Fake51

Reputation: 525

It would be a great idea to run some checks on the input before you decide to include it. For one thing, you could do an is_file($filename) before including.

One other thing, your regex is set to only allow one character atm - probably not what you want. Another thing: as the previous poster noted, the regex only checks if the input looks sort of ok. After that, you use the _GET variable. You SHOULD use the result from preg_match (you can have it assign results to an array) to avoid the above problem.

Regards Fake

Upvotes: 1

Robert K
Robert K

Reputation: 30328

To secure this, make sure your regex is /^[a-z]+$/i. This checks the whole string (^ is the start, and $ is the end) to be sure that it is alphabetical only. Then your work will be secure (at least in this part).

Upvotes: 2

Ólafur Waage
Ólafur Waage

Reputation: 69991

You could use this.

ctype_alnum($_GET["page"]);

Upvotes: 3

Related Questions