'Slug' Protection?

Hi,

I’ve another quick ‘Security’ question I thought I’d get an about.

I’m using the following to take data from a MySQL table and create a page.

The page.php?category=about&filename=team will search the database for a filename ‘team’ in the ‘about’ category. With some rewrite htaccess madness my URL becomes /about/team/ but I don’t want some schwine doing /about/team-drop * from table etc…

Here’s my code:


<?php // page.php

require_once('../../dbc/mysqlconnect.php');

$category = mysql_real_escape_string(trim($_GET['category']));
$filename = mysql_real_escape_string(trim($_GET['filename']));

$sql = "SELECT * FROM new_web_pages WHERE filename = '$filename' LIMIT 1";
$res = mysql_query($sql);
$row = mysql_fetch_array($res);

$page_title = htmlspecialchars($row['title']);
$descr = htmlspecialchars($row['descr']);
$keywords = htmlspecialchars($row['keywords']);

include('includes/header.php');

echo $row['content'];

include('includes/foot.php');

?>

I just wanted to know if there were any obvious holes in this CODE, or anything I can do to protect my site & db from crackers.

Many thanks for any advice.

Barring the suggestions already made in the previous thread, you should really validate the supplied data.

Unlike handling integers, text becomes a little more complex, a start would be to ensure it only contains characters you would expect.

Some RegEx would do the trick, and to quote our mutual sudo-French friend, ‘fail early’.

Good call.

So perhaps a regular expression to search for A-Z,a-z and hyphens would do the trick?

If it doesn’t pass that, then end the script?

I’m going to have to take another look through the other thread, it gave me a perma headache at one point :slight_smile:

Thanks again

Something like…


<?php
foreach(array('category', 'filename') as $parameter){
    #if is not set or is empty
    if(false === isset($_GET[$parameter]) || true === empty($_GET[$parameter])){
        #error
    }
    #if we find anything that is NOT alpha-numeric a hyphen or an underscore
    if(1 === preg_match('~[^a-z0-9-_]~i', $_GET[$parameter])){
        #error
    }
}

#looks good.

Brilliant!

Do you think it is worth protecting against SQL ‘key words’ like drop, insert etc.?

Thanks again for all the help Anthony, this’ll be the last in a long time, you’ve been very helpful :slight_smile:

Nah, just make sure you properly escape your query elements or use prepared statements and you’ll be fine I reckon.

But what do i know. :slight_smile:

Although, the fact you’re using filename is worrying, it’s not actually a file is it?

Oh lord no, it just contains the value for the record in the table. Not a physical file in sight :slight_smile:

Just a quick question if it’s ok.
This part here:


<?php
foreach(array('category', 'filename') as $parameter){
    #if is not set or is empty
    if(false === isset($_GET[$parameter]) || true === empty($_GET[$parameter])){
        #error
    }
    #if we find anything that is NOT alpha-numeric a hyphen or an underscore
    if(1 === preg_match('~[^a-z0-9-_]~i', $_GET[$parameter])){
        #error
    }
}
# place SQL query here ?

Do I place the SQL query in at the end of this?

And when you say prepared statement(I know I’ve asked a 1000 times, sorry), how would this look?

Yup, that snippet just ensures the user supplied input exists and ‘looks ok’.

With regards to the prepared statements, this would depend in whether you’re using a mysqli or [URL=“http://nl.php.net/manual/en/pdo.prepare.php”]pdo implementation.

Perfect. Makes sense to me now :slight_smile:

Going to look into PDO shortly. Thanks again sir.

also, it’s good to pay attention to RegEx used in htaccess

dont use " (.*) " use speceific description " ([A-Za-z]+) " if only letter … like this if sql keys were provided in Query_String wouldn’t even reach the page.php

ps,
(!isset() || empty())
is redundant because there will never be a case where !isset() is true, but empty() is not true. empty() alone encompasses all the cases where isset() would be false, and more.

Agree with Egyptechno : expect apache to head off anything out of bounds.

Lets say a-z and A-Z (because you are forgiving and people might type your new CoolUris) min 3 chars, max 10 chars only?

The more specific you are about what you allow, the safer you’ll be.

So set your .htaccess rule as he said above.

(a-zA-Z){3,10}

Some might say that’d be enough, but I’d say then validate the variables again in your script, again : trim(), strtolower() if its not chars a-z between say 3 and 10 chars, dump the data and run.

heres some cut and paste regexes I keep around:


$str = "12-3*45"; // numbers and a dash
if( preg_match("#^[0-9-]{2,8}$#", $str) ) echo "all ok";


$str = "128 abc"; // letters or numbers space only
var_dump( preg_match("#^[a-z0-9 ]{3,20}$#i", $str) ) ;

// I think you want:

if( 0 === preg_match("#^[a-z]{3,20}$#", $str) ) echo "goodnight vienna" ;

}


Apache has its part to play but don’t rely on it because bad people can still access your
page.php?category=about&filename=team directly, and of course you might upset your apache config accidentally.

Hmm, but .php?filename= is set and is empty.

My thinking was that the isset and empty would both return true in this case. Whereas, if I had just used empty on an unset query string, I would have had a notice raised regarding the use of an unset index.

If the var was not set it would fail immediately and not bother evaluating the empty call anyway wouldn’t it?

:confused:

Empty just converts to boolean and checks for false, while suppressing notices. So basically, it does !$var

And isset() just does $var !== null, while suppressing notices.

They have added functionality on objects that use some of the magic methods though.

Thanks for taking the time to explain that to me Chris, I’ll go have a little play around with that now. :wink:

Thanks again Cups, tried this this morning and it worked first time.

Is it easy enough to allow ‘hyphens’ too?

Instead of Vienna, I’m redirecting them to the home page and exiting the page.

Thanks.

$str = "12-3*45"; // this should fail
// accepts between 3 and 10 numbers. letters and dashes only
if( preg_match("#^[a-z0-9-]{3,10}$#", $str) ) echo "all ok";

You need to get an interactive regex tool that lets you play with these values and a PHP PCRE Cheat Sheet.

Google for “The Regex Coach” a desktop app, and PCRE Cheat Sheet to pin up on your wall, there are loads others out there - but at least I can use Regex Coach offline.

Bear in mind that there are many standards of regular expressions, the one you use in your apache .htaccess file, that as standard in PHP (PCRE) and that in a tool like Regex Coach may all contain slight differences.

Learning some basic regex is like learning some sql, html, css etc - it goes with the territory.

The regexes I posted can be simplified I am sure, and in some cases the use of filter_var() may well be far better - I posted these on here because I refer to them quite often as the base for more complex regexes.

Hi Paul,

Many thanks for this.

Will look out the PCRE Cheat Sheet and print that sucka off.

I did a little RegExp years back with Javascript, but only use it now and then.

Thanks again for the help, much appreciated.

Just out of interest, I’m already using an .htaccess for the RegExp on the rewrite, so is there much point in validating using PHP?
Am I missing something obvious?

Is it just in case they guess that my main page is default.php and uses those two names for input?

For that to work, you would have to explicitly deny all other routes to the script.