5
\$\begingroup\$

I am a 3rd-year computer science undergraduate. One of my university lecturers has developed his own page for students to submit work. It came up that one student was accused of hacking (sic) by the system. The problem was the characters used in the comment field.

The lecturer claimed that he needed to filter out certain characters in order to stop attacks, and that it was a known problem with PHP. (His system had been broken by students before). I am suspecting poor programming, as he claims that the strings are being executed by PHP.

He told me to try writing a PHP program with a form text field, then input PHP code into the text form. He expects that there to be a vulnerability without filtering the input for bad chars.

<?php

if(isset($_POST['data']) ){
    if( file_exists("../post.sqlite") ){
        $db = new SQLite3("../post.sqlite");    

        if( isset($_POST['password']) && $_POST['password'] === "correct horse battery staple" ){
            $statement = $db->prepare("INSERT INTO posts( id, post ) VALUES ( NULL, ? )");
            $statement->bindValue(1, $_POST['data'], SQLITE3_TEXT);
            $statement->execute();
        }

    }
    else {
        $db = new SQLite3("../post.sqlite");

        $db->exec("CREATE TABLE posts( id INTEGER PRIMARY KEY, post TEXT )");
    }
}

function posts(){
    if( file_exists("../post.sqlite") ){
        $db = new SQLite3("../post.sqlite");    

        $query = $db->query("SELECT post from posts ORDER BY id DESC");

        while( $row = $query->fetchArray(SQLITE3_ASSOC)){
            print( "<pre>" );
            print( htmlentities( $row['post'] ) );
            print( "</pre><hr />" );
        }
    }
}

?>
<!DOCTYPE html>
<html>
    <head><title>Try This</title></head>
    <body>
        <form action="post.php" method="post">
            <textarea rows="10" cols="80" name="data"></textarea><br />
            <input type="submit" value="Post" /><br />
            Secret key: <input type="text" name="password" />
        </form>
        <hr />
        <div id="posts">
            <?php posts(); ?>
        </div>
    </body>
</html>

Without the htmlentities function, I know it would have client side vulnerabilities (running malicious scripts). SQL injection is covered (prepared statements). I don't see how the program can be exploited server side. What (if anything) am I missing?

Improve this question
\$\endgroup\$
1
  • \$\begingroup\$ Not going to leave an actual answer because I've not done enough research in security to make any answer of mine educated enough. However, I am concerned about your passing any sort of POST data directly to your program, not to be confused with $_POST['data'] though that is the main concern here. If you have PHP version >= 5.2 check out filter_input() \$\endgroup\$
    – mseancole
    May 9, 2012 at 13:20

2 Answers 2

Reset to default
6
\$\begingroup\$

It seems fine for me.

Some small changes:

  1. I'd change the action to 

    <form action="<?php echo htmlentities($_SERVER['PHP_SELF']); ?>" method="post">

    Using PHP_SELF in the action field of a form

  2. I'd consider storing escaped data in the database instead of escaping them on every page requests for performance reasons if the page has lots of visitors.

  3. "../post.sqlite" should be a constant.

  4. I'd reorganize the code a little bit to use only one SQLite3 instance:

    define("DB_FILE", "../post.sqlite");

$dbExists = file_exists(DB_FILE);
$db = new SQLite3(DB_FILE);
if (!$dbExists) {
   $db->exec("CREATE TABLE posts( id INTEGER PRIMARY KEY, post TEXT )");
}

if (isset($_POST['data'])) {
    if( isset($_POST['password']) && $_POST['password'] === "correct horse battery staple" ){
        $statement = $db->prepare("INSERT INTO posts( id, post ) VALUES ( NULL, ? )");
        $statement->bindValue(1, $_POST['data'], SQLITE3_TEXT);
        $statement->execute();
    }
}

function posts($db) {
    $query = $db->query("SELECT post FROM posts ORDER BY id DESC");

    while ($row = $query->fetchArray(SQLITE3_ASSOC)) {
        print( "<pre>" );
        print( htmlentities( $row['post'] ) );
        print( "</pre><hr />" );
    }
}

?>
[...]

            <?php posts($db); ?>
[...]

<?php
    $db->close();
?>

    It may be worth creating the table with IF NOT EXISTS in every execution instead of the file_exists check for simplicity: Creating an SQLite table only if it doesn't already exist

Improve this answer
\$\endgroup\$
1
  • \$\begingroup\$ Using $_SERVER['SCRIPT_NAME'] instead of PHP_SELF is safer as it won't pass the extended path info. \$\endgroup\$
    – user38176
    Mar 5, 2014 at 21:26
2
\$\begingroup\$

My two cents:

As you said, from a security point of view your script looks OK, as it's defended from SQLi and XSS.

You should, however, check all other scripts in your application, or other scripts that access the same database (if any).

That's because the application could still be vulnerable to Second Order Injection, if someone else wrote another script that:

  • uses the data in your table using non-parameterized queries (unlikely in your case, but worth mentioning)
  • displays the data from your table without using htmlentities or another proper encoding mechanism
Improve this answer
\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.