up vote 3 down vote favorite 
<?php

include_once('../dbInfo.php');

function getReport($user_table) {
    $tables = array(
        "day"        => "p_day", 
        "month"      => "p_month"
        ... etc. .....
    );

    $table = $tables[$user_table];

    if(!$table) {
        die(json_encode(array("error" => "bad table name")));
    }

    $con = getConnection(); // getConnection is in '../dbInfo.php'

    $query = "select * from " . $table;
    $res = mysql_query($query, $con);

    if(!$res) {
        die(json_encode(array("error" => "no results from table")));
    }

    $fields_num = mysql_num_fields($res);
    $fields = array();
    for($i=0; $i < $fields_num; $i++) {
        $field = mysql_fetch_field($res);
        $fields[$i] = $field->name;
    }

    $i = 0;
    while($row = mysql_fetch_array($res)) {
        $rows[$i] = $row;
        $i++;
    }
    $json = array("rows" => $rows, "headers" => $fields);

    $jsontext = json_encode($json);
    return $jsontext;
}

?>

What this code is doing:

  • access the database, selecting rows from a table, and returning them as a serialized json object
  • a table name is looked up in $tables -- the keys are acceptable user input, the values are actual table/view names in the database
  • data is selected from the table
  • the data is put into a big hash
  • the hash is serialized as a json string and returned

Specific issues I'm concerned about:

  1. security -- is my DB connection info safe? This file is in the root directory of public content, so dbiInfo.php, with the database connection information, is not publicly accessible (I think)
  2. security -- am I open to SQL injection attacks? I build a SQL query with string concatenation
  3. security -- $user_table is untrusted input; is it safe? It's only used as a key to look up trusted input ...
  4. error handling -- have I dealt with all error conditions
  5. there are lots of versions of PHP functions -- am I using the right ones?

General issues:

  • following conventions
  • quality/readability/comments

Edit: the data is publicly available -- I'm worried about somebody getting more than read access to one of the listed tables, or any access to any other table in the DB.

share|improve this question

2 Answers

up vote 3 down vote accepted

This: 

$tables = array(
    "day"        => "p_day", 
    "month"      => "p_month"
    ... etc. .....
);

$table = $tables[$user_table];

if(!$table) {
    die(json_encode(array("error" => "bad table name")));
}

will throw a notice if $user_table is not a valid array key, something you should have already tested. Rewrite as:

$tables = array(
    "day"        => "p_day", 
    "month"      => "p_month"
    ... etc. .....
);

if(!array_key_exists($user_table, $tables)) {
    die(json_encode(array("error" => "bad table name")));
}

$table = $tables[$user_table];

I really hate it when functions die(), and in your case there's no point in that. You could simply do: 

if(!$table) {
    return json_encode(array("error" => "bad table name"));
}

since the function is expected to return a json formatted string. If you really want to die() you should do that where you call the function and the returned string is an error. Or you could just return false when an error occurs and the raw array when everything works, and when calling the function do something like: 

$result = json_encode(getReport("some_table"));

That way getReport() will be useful even when you don't need json encoded output. But that's up to you and how you actually use it.

As @LokiAstari mentions mysql_* functions are deprecated and should be avoided. I would skip mysqli_* functions too and use PDO which will give you the extra bonus of switching to another database engine if you ever need to and it has a nice OO interface.

For everything else, I'm with @LokiAstari.

share|improve this answer
up vote 2 down vote

If the output is json you may want to set the content type:

This is what I nievely do:

$type   ="text/json";
if ($_GET{"test"})
{
    $type   = "text/plain";
}
header("Content-type: $type");

I also use mysql_query() but that is because my PHP is not good (and I have not done it for a while). But apparently this DB module is deprecated in favour of "mysqli(mproved)" module. http://us.php.net/manual/en/book.mysqli.php

security -- is my DB connection info safe? This file is in the root directory of public content, so dbiInfo.php, with the database connection information, is not publicly accessible (I think)

Don't think it makes a difference.
If your Web server is set up correctly it will never deliver php files (only execute them).

security -- am I open to SQL injection attacks? I build a SQL query with string concatenation

Potentially. All input from insecure location must be escaped.

Also you print error message that contain information about your tables into the response. This is definitely a security problem. The only response to the user should be a failed message (not why it failed), potentially with an ID so you can use this to look it up.

All the error should be put into the log file (with the query that generated them). You can then use the ID provided in the error message to look up the query and the error message it produced.

security -- $user_table is untrusted input; is it safe? It's only used as a key to look up trusted input ...

And data from this table must be escaped before use.

error handling -- have I dealt with all error conditions there are lots of versions of PHP functions -- am I using the right ones?

OK. Not an expert here so others may know better but I like to put the error checking and command on the same line.

$res = mysql_query($query, $con);

if(!$res) {
    die(json_encode(array("error" => "no results from table")));
}

// For me this would be
$res = mysql_query($query, $con) or LogErrorAndDie(array("error" => "no results from table"));
share|improve this answer
Thanks for the advice. I'm not sure that I need to escape the user input, though -- I don't use the user input to build the SQL query, I use it to look up a trusted string in a table, which then goes into the query. Is data from the database untrusted -- it needs to be escaped too? I also don't understand how the error messages are a security problem -- could you clarify? Thanks again for the help! – Matt Fenwick Nov 23 '11 at 20:50
The problem with security is you can never know what can potentially be usefull to an attacker. Thus you should never give away anything. On the other hand a normal user usually does not care why it went wrong (they are not in a position to fix it) so providing this information is redundant. So both cases are best handled with non specific data. So best is just to apologize and provide a reference (GUID) the user can use when complaining. You can use the reference to look up the problem in the log files. – Loki Astari Nov 23 '11 at 21:02

Your Answer

 
or
required, but never shown
discard

By posting your answer, you agree to the privacy policy and terms of service.

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