General PHP Structure Question

[influxer]

Hey all,

Since I taught myself PHP and don't have much sophisticated knowledge about coding in it, I was wondering if I could ask a few questions to get more familiar with universal methods of coding.

I run a website service with various features, including uploading files, analyzing page statistics, posting message, etc. The way I have structured the website is as follows:

members/upload
members/stats
members/posting

Is the lack of obscurity a problem to begin with? In each directory I have a home page (upload.php, stats.php, posting.php) and I have a 'test' file to actually run code when they submit the form (upload_test.php, stats_test.php, posting_test.php). In each 'test' file I have various different functions I can execute. For example, in the posting_test.php, function 1 will remove a post, function 2 will add a post, function 3 will remove all posts, etc.

So, when somebody submits a certain form on posting_test.php, here is what the form action tag looks like: <form action="posting_test.php?code=1" method="post"> (for example, this is for removing a post).

So my posting_test.php file is separated into codes (code1 remove post, code2 add post, code3 remove all posts), propagated through the URL (is this a big mistake?) and I simply post the respective code under if($code==1){<<CODE HERE>>} if($code==2){<<CODE HERE>>} etc.

Is there a problem with the way I have this set up?

Thanks!
-influxer

[AaronW]

Not really. Personally I use /processor.php?action=edit or delete or add. As long as someone can't just type "/processor.php?action=delete" into the form's action="" attribute and delete a post at will, you should be fine. Just make sure you check for proper member permissions before doing any of those actions. If you don't have a member system in place, then yes it's a bad idea. There's really no good way to do it without members though.

[influxer]

Would it be sufficient enough to just check and see if the $code in the 'test' files is either number 1, 2, or 3?

something like:

if($code != 1 || $code != 2 || $code != 3)
{
die();
}


Also, on every member page I have a small script to check and see if they are logged in and I have the following code on those pages:


if ($logged_in == 0) //check if user is logged in, if not don't allow access to rest of page
{
echo 'Sorry, either your session has expired or you are not logged in. You may, however, return to the <a href="http://www.website.com">websitehome page</a> and log in or register for a free account from there.';
include($path_prefix.'universal/footer_simple.php');
exit;
}


I should also include this little tidbit of code on the 'test' files too, right? As of now I don't but I just realized that could be very problematic..

Also, is it necessary to name pages index.php in the directories instead of stats.php?

P.S: very nice website =) Pretty cool and unique, never seen much like it before.

[Alkerguitar]

I'm just learning php myself, but I have a feeling that I could write a code on my own site like so:

$logged_in = 1
-blah blah-
<form action="yoursite.url/posting/posting_test.php?code=1" method="post">
-blah blah-

in order to post something, or if I were malicious I could attempt to delete all of your posts using
$logged_in = 1
and
<form action="yoursite.url/posting/posting_test.php?code=3" method="post">
... but that's just me thinking, I don't know what limitations are imposed by Apache that would restrict this.

~TheAlker

[firepages]

Its good that you are looking at your code in this manner , you obviously have already spotted potential issues.

As Alkerguitar notes , with register_globals turned off it may be possible for an external form to be submitted that bypasses your if $logged_in = code.

If you are not already checking for $_SESSION['logged_on'] rather than $logged_on then start doing so now for an instant security boost.

Also look at your functions . I used to write ...e.g

function delete_post( $post_id ){
$sql="DELETE from table WHERE id=$post_id";
//etc
}

these days I would ..

function delete_post( $post_id ){
$sql="DELETE from table WHERE id = $post_id AND user_id = {$_SESSION['auth_user']}";
//etc
}

OK not exactly like that but you see the general idea , all your functions that do anything important should have some idea of `ownership`.
e.g there are still many system's out there where its possible to delete/edit other users files/data simply by altering the query string or a form etc , so think about how you (& you know your code best) would be able to bypass your code & fill those gaps.

[influxer]

As Alkerguitar notes , with register_globals turned off it may be possible for an external form to be submitted that bypasses your if $logged_in = code.

If you are not already checking for $_SESSION['logged_on'] rather than $logged_on then start doing so now for an instant security boost.


Here is how I implement my $logged_in variable. On every single member page, I include(session_login.php) which has the following code in it (along with other stuff, but here is what you need to see):


if (!isset($_SESSION['username']) || !isset($_SESSION['password'])) {
$logged_in = 0;
return;
} else {

if($_SESSION['password'] == $db_pass['password']) {
// valid password for username
$logged_in = 1; // they have correct info
// in session variables.
} else {
$logged_in = 0;
unset($_SESSION['username']);
unset($_SESSION['password']);
// kill incorrect session variables.
}


This will act as a $_SESSION['logged_in']...just a different way of implementing..right? This is more than secure enough..right?

Thanks.

[Alkerguitar]

Once again, if one could view that code, they could get around it by simply sending in a form with the following:

$_SESSION['username'] = "Joe";
$_SESSION['password'] = "Joe's *incorrect* password";
$logged_in = 1;
*insert malicious form action*

Because you're only checking that there are actually values in $_session, rather than that they are correct... then again one may not be able to set $_session variables in this manner, I'd have to do a little reading on session variables in the php manual, but It's 5:30am and I'm leaving to go camping at 6....

Good Luck!
~TheAlker

edit:
OOPS, missed the middle part of your code that checked the password.... nonetheless, I think that would only necessitate a correct password, not a correct password for the correct username?

~TheAlker

[influxer]

How about now...I updated the code:


session_start();

if (!isset($_SESSION['username']) || !isset($_SESSION['password'])) {
$logged_in = 0;
return;
} else {

// remember, $_SESSION['password'] will be encrypted.

if(!get_magic_quotes_gpc()) {
$_SESSION['username'] = addslashes($_SESSION['username']);
}


// addslashes to session username before using in a query.
$pass = $db_object->query("SELECT password FROM users WHERE username = '".$_SESSION['username']."'");

if(DB::isError($pass)) {
$logged_in = 0;
unset($_SESSION['username']);
unset($_SESSION['password']);
// kill incorrect session variables.
}

$db_pass = $pass->fetchRow();

// now we have encrypted pass from DB in
//$db_pass['password'], stripslashes() just incase:

$db_pass['password'] = stripslashes($db_pass['password']);
$_SESSION['password'] = stripslashes($_SESSION['password']);



//compare:



if($_SESSION['password'] == $db_pass['password']) {
// valid password for username
$logged_in = 1; // they have correct info
// in session variables.
} else {
$logged_in = 0;
unset($_SESSION['username']);
unset($_SESSION['password']);
// kill incorrect session variables.
}
}


// clean up
unset($db_pass['password']);

$_SESSION['username'] = stripslashes($_SESSION['username']);

[Alkerguitar]

Lookin good!
Hope it's working for you, and I don't see any huge potential security problems, though others may (I'm just a beginner with php)... keep asking around!

~TheAlker