Page 1 of 2 12 Last
  • Jump to page:
    #1
  1. No Profile Picture
    Contributing User
    Devshed Newbie (0 - 499 posts)

    Join Date
    Sep 2004
    Location
    Trondheim, Norway
    Posts
    251
    Rep Power
    10

    Validating navigation code


    Hi.

    I have the following code in my index.php file.
    PHP Code:
    if(empty($_GET["id"])) {
     include(
    "home.php");
    }
    else {
     include(
    "pages.php");

    It includes all other files into the index.php file as long as I link like this index.php?id=some_file.

    Now I am reading this page about the importance of validation when including like I do.
    http://forums.devshed.com/php-faqs-and-stickies-167/everyone-must-read-security-notes-20525.html

    However if I write
    PHP Code:
    if( in_array(empty($_GET["id"]))) 
    the home.php pages is not included. Other than that it seems to work.

    Any ideas what I can do to validate my code below and make it secure but still have it work?
    Last edited by christdi; May 21st, 2013 at 04:27 AM.
    http://chrisdee.bandcamp.com
  2. #2
  3. --
    Devshed Expert (3500 - 3999 posts)

    Join Date
    Jul 2012
    Posts
    3,920
    Rep Power
    1045
    Hi,

    the code makes no sense. The in_array() function does exactly what its name suggests: It checks whether a particular value exists in a particular array. But you neither pass an array nor a value for testing to the function. You send it a boolean: in_array(true) or in_array(false). What is that supposed to do?

    How exactly does your code work? Do you pass actual file names in the URL and include those files in the code? That would be a terrible idea. There's a huge risk of remote file inclusion, and letting arbitrary website visitors tell you what file to include is conceptually wrong. Which files you have on your server and when you include them is none of the user's business. That's your job.

    Of course you can include different files depending on the URL. For example, when a user visits index.php?page=contact, you may include the PHP file contact.php. But the user must never pass an actual file name or path. See the difference?

    This "page" stuff would be done by making an array of valid pages ("contact" etc.). When a user makes a requests, you check if the page is indeed in this array. If it is, you build the actual path (e. g. /var/www/mycoolsite/pages/contact.php).
    The 6 worst sins of security ē How to (properly) access a MySQL database with PHP

    Why canít I use certain words like "drop" as part of my Security Question answers?
    There are certain words used by hackers to try to gain access to systems and manipulate data; therefore, the following words are restricted: "select," "delete," "update," "insert," "drop" and "null".
  4. #3
  5. No Profile Picture
    Contributing User
    Devshed Newbie (0 - 499 posts)

    Join Date
    Sep 2004
    Location
    Trondheim, Norway
    Posts
    251
    Rep Power
    10
    I don't include files but mysql table rows.
    Here is the whole script :

    PHP Code:
    <?php
    mysql_connect
    ("localhost""username""password") or die(mysql_error());
    mysql_select_db("mydb") or die(mysql_error());

    if(empty(
    $_GET['id'])) {
     
    $query mysql_query("SELECT * FROM articles") or die(mysql_error());
    } else {
     
    $query mysql_query("SELECT * FROM articles WHERE id=".$_GET['id']."") or die(mysql_error());
    }
    $fetch mysql_fetch_array($query) or die(mysql_error());

    $title $fetch["title"];
    $content $fetch["content"];
    $date $fetch["date"];
    $category $fetch["category"];

    $result mysql_query("SELECT id, title, category FROM articles order by category, title");

    echo 
    "<select onchange='window.location.href=this.options[this.selectedIndex].value; this.selectedIndex=0;' size=12";
    echo 
    "<option>Select Page</option>";

    while (
    $row mysql_fetch_array($result)) {
     if(
    $row[0]==80 or $row[0]==89 or $row[0]==84 or $row[0]==85 or $row[0]==199 or $row[0]==87 or $row[0]==82) {} else {
      print 
    "<option value=index.php?id=$row[0]><span class=code><i>$row[2] | $row[1]</i></span></option>";
    }}
    echo 
    "</select>";

    if(empty(
    $_GET["id"])) {
     include(
    "home.php");
    }
    else {
     include(
    "pages.php");
    }
    ?>
    http://chrisdee.bandcamp.com
  6. #4
  7. No Profile Picture
    Contributing User
    Devshed Frequenter (2500 - 2999 posts)

    Join Date
    Dec 2004
    Posts
    2,868
    Rep Power
    368
    Jacques is going to have a heart attack. Your query is open to injection!

    also for performance sakes you should do this check at the top of the page:

    if(empty($_GET["id"])) {
    include("home.php");
    }
    else {
    //do all your queries etc
    }
  8. #5
  9. --
    Devshed Expert (3500 - 3999 posts)

    Join Date
    Jul 2012
    Posts
    3,920
    Rep Power
    1045
    Hi,

    the numerous security vulnerabilities are only one of many problems. Whatever online tutorial you got this code from: Delete it from your browser history and never visit it again. If you got it from a book, throw it away.

    This code is ancient. If we lived in the 90s and people were just beginning to write their first home pages, I would probably say: "Cool code, dude. Now add some <blink>". However, this is the 21. century, and web programming has changed since. Many tutorials haven't kept up to that, but you should.

    Don't copy and paste code from some dubious online tutorial you find somewhere on the internet. They're usually garbage. Instead, write your own code based on modern PHP. A good way of checking whether a particular function is still alive is to check the official function reference. For example, all of your "mysql_" functions have a big fat red warning on top begging you to stop using them. The page also tells you what to use instead and how to use it.
    The 6 worst sins of security ē How to (properly) access a MySQL database with PHP

    Why canít I use certain words like "drop" as part of my Security Question answers?
    There are certain words used by hackers to try to gain access to systems and manipulate data; therefore, the following words are restricted: "select," "delete," "update," "insert," "drop" and "null".
  10. #6
  11. No Profile Picture
    Contributing User
    Devshed Newbie (0 - 499 posts)

    Join Date
    Sep 2004
    Location
    Trondheim, Norway
    Posts
    251
    Rep Power
    10
    Originally Posted by Jacques1
    Don't copy and paste code from some dubious online tutorial you find somewhere on the internet. They're usually garbage. Instead, write your own code based on modern PHP.
    Thank you for your answer.

    Actually I have written from the "ground" up with the help of php.net and some of the tutorials here. I may have gotten the "if(empty($_GET["id"])) {" part from some site originally. But as you point out I have not keept up the last years. I remember trying msqli, but I could not make it work as I wanted. I guess there is more to it than just changing to mysqli ?

    From what you say it sound to me I should throw away my ancient code and start new.

    So my first question is about the breadcrum navigation code.
    After doing a search after navigation here on the forum I can only find postings from 2007 and earlier.

    So, what is a secure and modern way of creating a easy navigation code? Should I avoid including all files/ database rows through the index.php (like index.php?id=some_file) ?
    Last edited by christdi; May 22nd, 2013 at 06:53 AM.
    http://chrisdee.bandcamp.com
  12. #7
  13. No Profile Picture
    Lost in code
    Devshed Supreme Being (6500+ posts)

    Join Date
    Dec 2004
    Posts
    8,301
    Rep Power
    7170
    It's OK to base your include or query on the id parameter, you just need to validate it before you do so.

    For example, if you're looking up the ID in a database, you need to escape the value before using it in your query.

    If you want to use the ID for file inclusion, you need to limit it to certain files. You don't need to pre-define the list of files in your code, although that is one way of doing it. You should have a directory dedicated to holding files that are allowed to be included, and you should restrict the characters used in those file names to something very safe, like a-z and 0-9.

    In most frameworks URL routing is a fairly complicated topic. It is possible to write fairly simple routing code that is flexible enough for small sites though. For example, if you create a directory 'pages', and put all of your accessible pages into that directory, then you could load them using an ID parameter by doing:
    PHP Code:
    $id = isset($_GET['id']) ? $_GET['id'] : 'default-page';
    if(!
    preg_match('#^[a-z0-9_-]+$#'$id) || !file_exists('pages/' $id '.php')) {
      die(
    '404: page not found');
    }
    require(
    'pages/' $id '.php'); 
    This is a secure way, but not really a modern way.

    A modern way would be to use URL rewriting with some structure for mapping that to controller files. The exact structure varies from PHP framework to framework. If you want to go this route, your best bet would be to choose an existing PHP framework and use the routing structure that it defines for you.
    PHP FAQ

    Originally Posted by Spad
    Ah USB, the only rectangular connector where you have to make 3 attempts before you get it the right way around
  14. #8
  15. No Profile Picture
    Contributing User
    Devshed Newbie (0 - 499 posts)

    Join Date
    Sep 2004
    Location
    Trondheim, Norway
    Posts
    251
    Rep Power
    10
    Thanks for your answer.

    Url rewriting and controller files mapping sounds complicated.
    Am not familiare with either.

    I think i will first try to build on your example and use mysqli and se if i can make that work.

    I'll post my new code when i hopefully get it to work.
    http://chrisdee.bandcamp.com
  16. #9
  17. No Profile Picture
    Contributing User
    Devshed Newbie (0 - 499 posts)

    Join Date
    Sep 2004
    Location
    Trondheim, Norway
    Posts
    251
    Rep Power
    10
    I have done a little reading about url rewriting from this page.
    http://www.addedbytes.com/articles/f...for-beginners/

    Actually it doesn't look so complicated anyway. It seems to involve using rules with regular expressions in htaccess files wich am somewhat familiar with.
    Last edited by christdi; May 25th, 2013 at 02:44 AM.
    http://chrisdee.bandcamp.com
  18. #10
  19. No Profile Picture
    Contributing User
    Devshed Newbie (0 - 499 posts)

    Join Date
    Sep 2004
    Location
    Trondheim, Norway
    Posts
    251
    Rep Power
    10
    Hi again. I just post here instead of making a new thread.

    I'm in the process of updating my mysql/php code using PDO. I wonder if the code below is more or less secure and modern compared to the old mysql wich is about to be depricated ?

    A short explenation. The script connects to a database and lists out the title row of a mysql table into a html table. The idea is to make the titles clickable (html linking) by id. When I click on the title I will be directed to another page wich should view the content of that title i just clicked on.

    PHP Code:
    <?php
    $connect 
    = new PDO("mysql:host=localhost;dbname=db"'user''pass');
    $query   $connect->query("SELECT id,title FROM text ORDER BY title ASC");
    $fetch   $query->setFetchMode(PDO::FETCH_NUM);

    echo 
    "<table border=1>";
    while (
    $rows $query->fetch()) {
     echo 
    "<tr>";
     foreach(
    $rows as $row) {
      print 
    "<td><a href=rn.php?id=$row>$row</a></td>";
     }
     echo 
    "</tr>";
    }
    echo 
    "</table>";
    ?>
    http://chrisdee.bandcamp.com
  20. #11
  21. No Profile Picture
    Registered User
    Devshed Newbie (0 - 499 posts)

    Join Date
    May 2013
    Posts
    12
    Rep Power
    0
    Originally Posted by christdi
    Hi again. I just post here instead of making a new thread.

    I'm in the process of updating my mysql/php code using PDO. I wonder if the code below is more or less secure and modern compared to the old mysql wich is about to be depricated ?

    A short explenation. The script connects to a database and lists out the title row of a mysql table into a html table. The idea is to make the titles clickable (html linking) by id. When I click on the title I will be directed to another page wich should view the content of that title i just clicked on.

    PHP Code:
    <?php
    $connect 
    = new PDO("mysql:host=localhost;dbname=db"'user''pass');
    $query   $connect->query("SELECT id,title FROM text ORDER BY title ASC");
    $fetch   $query->setFetchMode(PDO::FETCH_NUM);

    echo 
    "<table border=1>";
    while (
    $rows $query->fetch()) {
     echo 
    "<tr>";
     foreach(
    $rows as $row) {
      print 
    "<td><a href=rn.php?id=$row>$row</a></td>";
     }
     echo 
    "</tr>";
    }
    echo 
    "</table>";
    ?>
    Of course it's better than before but i recommend you to use prepared statements with PDO for better security
  22. #12
  23. Sarcky
    Devshed Supreme Being (6500+ posts)

    Join Date
    Oct 2006
    Location
    Pennsylvania, USA
    Posts
    10,692
    Rep Power
    6351
    Prepared statements are indeed better...if there's variables in them or you'll be executing them more than once. Neither is the case for this query, so it's correct.

    You should quote and escape your HTML tag attributes though.
    Last edited by ManiacDan; May 30th, 2013 at 08:59 AM.
    HEY! YOU! Read the New User Guide and Forum Rules

    "They that can give up essential liberty to obtain a little temporary safety deserve neither liberty nor safety." -Benjamin Franklin

    "The greatest tragedy of this changing society is that people who never knew what it was like before will simply assume that this is the way things are supposed to be." -2600 Magazine, Fall 2002

    Think we're being rude? Maybe you asked a bad question or you're a Help Vampire. Trying to argue intelligently? Please read this.
  24. #13
  25. No Profile Picture
    Contributing User
    Devshed Newbie (0 - 499 posts)

    Join Date
    Sep 2004
    Location
    Trondheim, Norway
    Posts
    251
    Rep Power
    10
    Thanks guys.

    So the code below works, but I wonder if am doing it correct ?

    PHP Code:
    <?php
    $connect 
    = new PDO("mysql:host=localhost;dbname=db"'user''pass');
    $query   $connect->prepare("SELECT id,title FROM text ORDER BY title ASC");
    $query->execute();
    $fetch   $query->setFetchMode(PDO::FETCH_NUM);

    echo 
    "<table border='1'>";
    while (
    $rows $query->fetch()) {
     echo 
    "<tr>";
     foreach(
    $rows as $key => $row) {
      print 
    "<td><a href='read.php?id=$rows[0]'>$row</a></td>";
     }
     echo 
    "<td><a href=edit.php?id=$rows[0]>Edit</a> | <a href=del.php?id=$rows[0]>Del</a></td>";
     echo 
    "</tr>";
    }
    echo 
    "</table>";
    ?>
    Last edited by christdi; June 5th, 2013 at 07:16 AM.
    http://chrisdee.bandcamp.com
  26. #14
  27. --
    Devshed Expert (3500 - 3999 posts)

    Join Date
    Jul 2012
    Posts
    3,920
    Rep Power
    1045
    You should have listened to ManiacDan, not jeapie.

    Your query is constant, it doesn't have any input. So there's absolutely no reason to use a prepared statement. You do understand the purpose of prepared statements, right?

    Actually, it's not even an actual prepared statement, because you haven't set up PDO correctly. Use this configuration:

    PHP Code:
    $db_options = array( 
        
    PDO::ATTR_EMULATE_PREPARES => false                     // important! use actual prepared statements (default: emulate prepared statements) 
        
    PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION           // throw exceptions on errors (default: stay silent) 
        
    PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC      // fetch associative arrays (default: mixed arrays) 
    ); 
    $database = new PDO('mysql:host=localhost;dbname=YOURDB;charset=utf8''YOURUSER''YOURPW'$db_options);    // important! specify the character encoding in the DSN string, don't use SET NAMES 
    You must escape values before inserting them into HTML. ManiacDan already said that. See The 6 worst sins of security on how to do proper HTML escaping.

    Last but not least, you should start giving your variables meaningful names. "connect", "query", "row", "fetch" doesn't tell you anything about the actual content of the variable, which makes the code hard to understand. "connect" to what? The Internet? The local network of your company? Your sister's iPad? But if you name it "database", everybody knows what it means. Use sensible, concrete names. Don't just say "query". Name it "blog_articles_query" or whatever you're dealing with.

    Also note that you can use a PDOStatement directly in a foreach loop. You don't need this cumbersome while ( fetch() ) construct:

    PHP Code:
    <?php

    $texts_query 
    $database->query('
        SELECT
            text_id
            , title
        FROM
            texts
        ORDER BY
            title ASC
    '
    );

    foreach (
    $texts_query as $text) {
        
    // ...
    }
    The 6 worst sins of security ē How to (properly) access a MySQL database with PHP

    Why canít I use certain words like "drop" as part of my Security Question answers?
    There are certain words used by hackers to try to gain access to systems and manipulate data; therefore, the following words are restricted: "select," "delete," "update," "insert," "drop" and "null".
  28. #15
  29. No Profile Picture
    Contributing User
    Devshed Newbie (0 - 499 posts)

    Join Date
    Sep 2004
    Location
    Trondheim, Norway
    Posts
    251
    Rep Power
    10
    Originally Posted by Jacques1
    You should have listened to ManiacDan, not jeapie.

    Your query is constant, it doesn't have any input. So there's absolutely no reason to use a prepared statement. You do understand the purpose of prepared statements, right?
    Ok. I will do that. Thanks for your answer.

    I have to read up more on prepared statements befor I attack that. Don't quite understand the concept yet other than what php.net says (a statement that is used to execute the same statement repeatedly with high efficiency).

    PHP.net also says the same degree of security can be achieved with non-prepared statements, if input values are escaped correctly.

    So why not just use non prepared statements and remember to escape input values ?
    http://chrisdee.bandcamp.com
Page 1 of 2 12 Last
  • Jump to page:

IMN logo majestic logo threadwatch logo seochat tools logo