Improvements?

Teach

Hello there guys.
Just wondering if you could help me with this also, thanks for all the previous help.

Is there anything you can suggest I can change, and what to, to secure the following PHP script. This is used for a contact page which then gets staff details from a "staffinfo" directory, which contains 1.html through 11.html which contains the staff info.
Basically, how could I secure this? To make it only load that 1.html through 11.html to stop anything else being loaded, and to only work within the directory ?

Any help is much appreciated.

The code:

<?php
 include('../header.php');

if (is_numeric($_GET['id'])) {

$id = $_GET['id'];

$info = "/home/LAN/public_html/contact/staffinfo/$id.html";

if (file_exists($info)) {
include ($info);
} else {
echo "Sorry, that staff ID does not exist.  Please try again.";
}

}
else {
 ?>

 staff list.

<?php
}
 include('../footer.php');
 ?>

Regards,

superwormy

Why not just check whether $id is greater than or equal to 1, and less than or equal to 11...?

Teach

hi superwormy and thanks.
I Was reading on the php manual, and found that I could incorporate something ilke this into it:

preg_match('!/home/LAN/public_html/contact/staffinfo/[0-9]*?\.html!');

Is that right and whereabouts would I actually use that on the script I've done?

Thank you very much for your help and sorry to be a nuisance.

pyro

This part would be cause of some concern:

[0-9]*?

The * means 0 or more, so there is no need for the ? (0 or 1 of the previous character).

Also, you have not passed it the subject that it is to be looking for the string in.

Teach

Originally posted by pyro
This part would be cause of some concern:

[0-9]*?

The * means 0 or more, so there is no need for the ? (0 or 1 of the previous character).

Also, you have not passed it the subject that it is to be looking for the string in.

How would I go about doing that please?

pyro

Just remove the ?

Teach

Thanks, I see, sorry.
Would it look something like this?

<?php
 include('../header.php');

if (is_numeric($_GET['id'])) {

$id = $_GET['id'];

$info = "/home/LAN/public_html/contact/staffinfo/$id.html";
preg_match('!/home/LAN/public_html/contact/staffinfo/[0-9]*.html!');

if (file_exists($info)) {
include ($info);
} else {
echo "Sorry, that staff ID does not exist.  Please try again.";
}

}
else {
 ?>

staff list.

<?php
}
 include('../footer.php');
 ?>

What did you mean about the string, or something? Sorry to be a pain.

Thank you.

pyro

I think you'll want something more like this:

 if (!preg_match('!/home/LAN/public_html/contact/staffinfo/[0-9]*\\.html!', $info)) {
    die ("<p>That is not a valid ID.</p>");
}

Teach

Ahh okay thank you.
Is this correct now? And do you think that's an improvement on the original, or could it be further "secured"?

Also I wasn't quite sure whether you meant that bit should replace something totally, or whether it should look something like this:

<?php
 include('../header.php');

if (is_numeric($_GET['id'])) {

$id = $_GET['id'];

$info = "/home/LAN/public_html/contact/staffinfo/$id.html";
if (!preg_match('!/home/LAN/public_html/contact/staffinfo/[0-9]*\.html!', $info)) {
    die ("<p>That is not a valid ID.</p>");
}

if (file_exists($info)) {
include ($info);
} else {
echo "Sorry, that staff ID does not exist.  Please try again.";
}

}
else {
 ?>

staff list.

<?php
}
 include('../footer.php');
 ?>

Please correct me if i'm wrong.
Once again, many thanks. Appreciated.

pyro

Looks pretty good to me.

Teach

Okay pyro, all working FAB!!!

The code i'm using:

<?php
 include('../header.php');

if (is_numeric($_GET['id'])) {

$id = $_GET['id'];

$info = "/home/LAN/public_html/contact/staffinfo/$id.html";
if (!preg_match('!/home/LAN/public_html/contact/staffinfo/[0-9]*.html!', $info)) {
    die ("Sorry, that staff ID does not exist.  Please try again.");
}

if (file_exists($info)) {
include ($info);
} else {
echo "Sorry, that staff ID does not exist.  Please try again.";
}

}
else {
 ?>

<div align="center">
<br />
The long staff list here.
<br /></div>

<?php
}
 include('../footer.php');
 ?>

Two quick queries about this, firstly is [0-9]* right to use if i just want numerics (as the only ids are xx.html (xx are numeric only).
Secondly, if say ?id=sometext then the staff list is echoed, and not the error "Sorry, that staff ID does not exist. Please try again." is that right?

Do you think this indeed an improvement over the first one i pasted, or do you have any suggestions/comments on how to further secure it?
Thanks very much for your help & input.

Teach

What do you think about my last thread pyro? thanks.

pyro

There's some superfluous code in there. For example, you are using is_numeric on the GET variable, and also a regex, both to make sure that it is a number. One should do fine. Also, that is why you'd get the staff listing - because the is_numeric fails before it has a chance to run through anything else...

Shrike

You use file_exists() too in matching the filename. The regexp will fail if the file doesn't exist, no need to check again.

Also [0-9]* would accept 0, 12, 123, 1234, 12345, 123456 and to infinity. I think the correct PCRE syntax is [0-9]{2}, not too sure though.

if (is_numeric($_GET['id'])) {
    $info = "/home/LAN/public_html/contact/staffinfo/".$_GET['id'].".html";
    preg_match("[0-9]{2}\\.html$/i", $info, $matches)
        or die ("Sorry, that staff ID does not exist.  Please try again.");
    include ($info);
}

The i modifier sets regexp to be case-insensitive. You need to escape period chars (.), $ denotes end of string - this should help avoid filenames like file.html.html (not that that's very likely). Also some people say that negating entire function calls is bad programming. I don't know why though 😉

hth

Teach

Ah that is wonderful, thank you.
I'll give that a go.
so i'm using:

<?php
 include('../header.php');

if (is_numeric($_GET['id'])) { 
    $info = "/home/LAN/public_html/contact/staffinfo/".$_GET['id'].".html"; 
    preg_match("[0-9]{2}\.html$/i", $info, $matches) 
        or die ("Sorry, that staff ID does not exist.  Please try again."); 
    include ($info); 
}
else {
 ?>

<div align="center">
staff list here.
<br /></div>

<?php
}
 include('../footer.php');
 ?>

right? 🙂 Thank you again, very much appreciated!!! Seems more secure if i'm right...

Shrike

preg_match call is a bit wrong I think (my fault)

preg_match("/[0-9]{2}.html$/i", $info, $matches)

Missed a forward slash 🙂

Shrike

Ah no I'm being dumb, you do need to check that the file exists 🙂

file_exists($info)
  and include ($info);

Or I guess you could open the directory in which the files reside and then use the regexp to check if they exist...ho hum so many ways....guess it comes down to personal preference

Teach

Hi there, yes thank you that's great, - that's stopped that error relating to that line.
The problem is, that it's just not working, no PHP error - it's just saying "Sorry, that staff ID does not exist. Please try again." if i'm giving http://lan.site/contact/index.php?id=1 which should load 1.html - this is just bringing up that error, hmms.
Thanks again for your help with this!

Shrike

My bad on the PCRE syntax, it should be {0,2} meaning between 0 and 2.

Scratch that, RTFM Shrike. It should be {2, 2}. Note the space between is important. The manual reckons that {2} should work, but it doesn't for me 🙂

Teach

Thanks - i've edited that, but it's still not laoding the files, very odd!!!

Here's what i've got - i hope i haven't misunderstood any of the things you've said.

<?php
 include('../header.php');

if (is_numeric($_GET['id'])) {
    $info = "/home/LAN/public_html/contact/staffinfo/".$_GET['id'].".html";
    preg_match("/[0-9]{2, 2}.html$/i", $info, $matches)
        or die ("Sorry, that staff ID does not exist.  Please try again.");
    include ($info);
}
else {
 ?>

<div align="center">
staff list.
<br /></div>

<?php
}
 include('../footer.php');
 ?>