Day, Time Based Content

drawmack

A friend of mine runs an internet radio station and she asked if I could write them up a little script that would allow them to display information on their front page about the dj that was currently broadcasting.

The restrictions were thus.
1) It could need files to be uploaded, but those files have to be html files.
2) It had to know what files to display based on their names, not a secondary file.

I came up with this little baby, let me know what you think.

http://www.enderswebdev.com/test/scheduler.phps

drawmack

Alright I just updated the source file. It now allows for pages to be displayed everyday, every weekday, every weekend day and over a span of days.

MarkR

I hope you are running with register_globals turned off, otherwise it is clear that this could be used to retrieve any file on the system.

Mark

drawmack

Yeah I suppose it could, but constructive criticism is always nice, how do you propose I fix that?

MarkR

Fix 1: disable register_globals.

It is important that all PHP apps have register_globals disabled. This is because otherwise there are HEAPS of possible attacks. Also it makes code clearer, using $GET and $POST rather than globals.

I'd recommend your application test for register_globals (use ini_get), and if it's enabled, throw an error message and exit. May as well do magic_quotes too (magic_quotes are evil)

That will prevent register_globals from accidentally being re-enabled in the future.

Secondly, you should on no account allow user input to directly affect the directory read, or the file read.

In the case where you genuinely want to read the file from the query string, create a list of allowed files and if it isn't one of them, don't allow it.

Check any paths from the user for containing only valid characters using a regular expression. Normally I'd recommend you stick to ^[a-z0-9.]+$

Also check it for presence of strings which could indicate a problem, for instance "..". Or perhaps build the regexp so that it only allows a single .

Finally, don't give users a different error message if a file doesn't exist, or if an input validation error occurs, so they can't use it to explore the rules or test for the existence of files independently.

Mark

drawmack

Originally posted by MarkR
Fix 1: disable register_globals.

It is important that all PHP apps have register_globals disabled. This is because otherwise there are HEAPS of possible attacks. Also it makes code clearer, using $GET and $POST rather than globals.

Yeah I know that but I'm not managing the server this is going to be running on and I'm offering it for download from my site so this is not a viable option.

I'd recommend your application test for register_globals (use ini_get), and if it's enabled, throw an error message and exit. May as well do magic_quotes too (magic_quotes are evil)

That would really upset a lot of people who don't have control over their own php ini file. No need for the magic quotes test and this doesn't take input from $GET or $SET.

Secondly, you should on no account allow user input to directly affect the directory read, or the file read.

In the case where you genuinely want to read the file from the query string, create a list of allowed files and if it isn't one of them, don't allow it.

This script isn't designed for user input. It's designed to be included in another file and used to update the content of the page dynamically based on the time of day and the day or the week.

Check any paths from the user for containing only valid characters using a regular expression. Normally I'd recommend you stick to ^[a-z0-9.]+$

I die if(!is_dir($files_dir)) no need for testing valid characters, that will catch a bog entry.

Also check it for presence of strings which could indicate a problem, for instance "..". Or perhaps build the regexp so that it only allows a single .

So then when someone uses this script and they want to keep the files outside of te web root so they can use another script to manipulate them they can't do that? Not very user friendly.

Finally, don't give users a different error message if a file doesn't exist, or if an input validation error occurs, so they can't use it to explore the rules or test for the existence of files independently.

This is good advice I'll probably update the code but not right now.

MarkR

If those variables are designed to come from another script, you should take measures to ensure that it is only ever executed from another script with those vars set.

You don't want a security compromise if someone invokes it directly.

To do this either place it in a directory which the web server denies access to (directly), or check somehow that it is not being invoked directly.

Ideally do both, but it's still no substitute for disabling register_globals.

Mark

drawmack

Originally posted by MarkR
To do this either place it in a directory which the web server denies access to (directly), or check somehow that it is not being invoked directly.

That's a good idea, I'll be sure to recommend they place it outside the web root in the user's manual. Also the self check is simple:

<?php
if(eregi('scheduler.php',$_SERVER['PHP_SELF']))
    die('This page cannot be loaded directly.');
?>

Ideally do both, but it's still no substitute for disabling register_globals.

Yeah I know that, however I have no control over register globals on my server and many other may be in the same situation.

[edit]Okay I updated the source file to remove the stand alone mode.[/edit]