Testing filtering of certain characters

Question 1

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?

Question 2

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()

Question 3

It seems fine for me.

Some small changes:

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

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.
"../post.sqlite" should be a constant.

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

Question 4

Using $_SERVER['SCRIPT_NAME'] instead of PHP_SELF is safer as it won't pass the extended path info.

Question 5

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

palacsint palacsint 30.3k9 gold badges81 silver badges157 bronze badges · Accepted Answer · 2012-05-09 22:43:55Z

It seems fine for me.

Some small changes:

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

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.
"../post.sqlite" should be a constant.

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

Using $_SERVER['SCRIPT_NAME'] instead of PHP_SELF is safer as it won't pass the extended path info.

Stack Exchange Network

Testing filtering of certain characters

2 Answers 2

Hot Network Questions

Testing filtering of certain characters

2 Answers 2

Related

Hot Network Questions