CODEWITHSUNDEEP
phpsecurity
Student of Hogwarts
Student of Hogwarts

Reputation: 1138

Is this PHP code dangerous?

I've just sketched up the main index.php file. It should be the gateway for all the site requests. The reason why I want that is to have clean URLs.

I have split my website into modules. (Example: register, articles etc..)

Then I've included some lines in .htaccess, one is this:

RewriteRule ^([a-zA-Z0-9-]*)[/]?([a-zA-Z0-9-]*)[/]?([a-zA-Z0-9-]*)[/]?([a-zA-Z0-9-]*)[/]?([a-zA-Z0-9-]*)[/]?([a-zA-Z0-9-]*)[/]?([a-zA-Z0-9-]*)[/]?([a-zA-Z0-9-]*)[/]?([a-zA-Z0-9-]*)[/]?$ index.php?1=$1&2=$2&3=$3&4=$4&5=$5&6=$6&7=$7&8=$8&9=$9 [L,NC]

This just maps each "folder" to the right $_GET element... domain/1/hi/3

$_GET['2'] == 'hi'; // TRUE

So I want to run the module based on the first $_GET element. This way I can keep my project organized. And all files associated with a module is inside its folder.

Here is my folder structure:

/
    modules/
        register/
            ajax/
                process.php
            register.php
        articles/
            articles.php
    index.php

And here is the PHP-code to map everything (index.php):

<?php

$basePath = 'modules';

for ($x = 1; $x <= 9; $x++) {
    if (preg_match('/[^a-zA-Z0-9-]/', $_GET[$x])) {
        require_once(__DIR__ . "/$basePath/404/404.php");
        die();
    }
}

$baseModule = $_GET['1'];

if (file_exists(__DIR__ . "/$basePath/$baseModule/$baseModule.php")) {
    require_once(__DIR__ . "/$basePath/$baseModule/$baseModule.php");
} else {
    require_once(__DIR__ . "/$basePath/404/404.php");
}

Is this dangerous code? The reason why I do the regex is to check that the GETs doesn't contain . or / which could be used to do ../ and thus run virtually any file on the server...

Does this still pose a security hole, a potential security hole, or is it in fact, bad practice?

What is the best approach to this problem?

Upvotes: 1

Views: 244

Answers (3)

Student of Hogwarts
Student of Hogwarts

Reputation: 1138

I figured that defining all the options in an array is the best alternative, because it did NOT have a speed impact on my code when validating it with in_array(). The code was insecure because it COULD allow attacks. I have not found any method that would be able to penetrate the security, but I think it was still a bit on the edge this one.

The reason why I think this is the best alternative, is that Apache is slow.

Please vote this answer up or down, and I will accept the answer with the most votes. (I don't get any reputation if I accept this answer anyway.)

Upvotes: 0

Rolice
Rolice

Reputation: 3103

It seems to me yes - read about include injections. In combination with other parts, for example insecure file uploads this could even "kill". See here.

__DIR__ . "/$basePath/$baseModule/$baseModule.php"

Upvotes: 1

AllInOne
AllInOne

Reputation: 1450

RewriteRule ^/article/(.*)$ http://example.com/articles/articles.php

Then in article.php you look at the url segments and handle as you see fit.

@gschwa's approach would work well too.

Upvotes: 0

Related Questions