0

I'm running this query to fetch a post, if the member has access to it. Is it secure enough to just check if($row) ?

$sql = "SELECT f_title
            FROM fields
            INNER JOIN members
                ON m_group_id = f_group_id
                AND m_u_id = " . mysqli_real_escape_string($db_link, $_SESSION['u_id']) . "
                AND m_status > 0
            WHERE f_id = " . mysqli_real_escape_string($db_link, $_POST['id']) . "
            LIMIT 1";
$result = mysqli_query($db_link, $sql) or die(mysqli_error($db_link));
$row = mysqli_fetch_assoc($result);
if($row)
{
    echo 'Permission granted!';
}
else
{
    echo 'Permission NOT granted!';
}
SeaBass
  • 1,584
  • 4
  • 20
  • 46
  • 1
    Your query is not secure at all. You are wide open for SQL injection. Since you're using mysqli, take advantage of [prepared statements](http://php.net/manual/en/mysqli.quickstart.prepared-statements.php) and [bind_param](http://php.net/manual/en/mysqli-stmt.bind-param.php). – aynber Aug 24 '17 at 18:23
  • Well, in terms of *security* your code has a potential SQL injection vulnerability. Use prepared statements with query parameters. As for what you're asking, what is the *actual logic* of the "security" you're implementing? What you're doing seems to depend on assumptions, and those assumptions may or may not be true. Best to check the results explicitly than to just check whether a result object exists at all. – David Aug 24 '17 at 18:23
  • @aynber So running mysqli_real_escape_string() is not good enought? – SeaBass Aug 24 '17 at 18:25
  • No. There's a lot of information out there, but I don't have any links handy. Escaping the string still allows certain SQL injections to pass through. The best way to do it is to ignore escaping the data and use straight prepared statements and parameter binding. – aynber Aug 24 '17 at 18:26
  • @David Would that mean that I'll do something like `SELECT f_title, m_status` and then `if($row && $row['m_status'] > 0)` or still not secure? – SeaBass Aug 24 '17 at 18:27
  • @SeaBass: Before you write the code, *define the logic*. First of all, you'll want to handle unsuccessful responses from the database. (When `$result` is false.) That would be when the query failed entirely and there's a technical problem. For *successful* responses, what form would a "positive" or "negative" result take? Specifically check for those indications. – David Aug 24 '17 at 18:29
  • @aynber Interesting. Do you have any examples of how such an injection could look like? Using anything else than regular ', " and characters like that? – SeaBass Aug 24 '17 at 18:32
  • **WARNING**: When using `mysqli` you should be using [parameterized queries](http://php.net/manual/en/mysqli.quickstart.prepared-statements.php) and [`bind_param`](http://php.net/manual/en/mysqli-stmt.bind-param.php) to add user data to your query. **DO NOT** use manual escaping and string interpolation or concatenation to accomplish this because you will create severe [SQL injection bugs](http://bobby-tables.com/). Accidentally unescaped data is a serious risk. Using bound parameters is less verbose and easier to review to check you’re doing it properly. – tadman Aug 24 '17 at 18:34
  • Note: The object-oriented interface to `mysqli` is significantly less verbose, making code easier to read and audit, and is not easily confused with the obsolete `mysql_query` interface. Before you get too invested in the procedural style it’s worth switching over. Example: `$db = new mysqli(…)` and `$db->prepare("…”)` The procedural interface is an artifact from the PHP 4 era when `mysqli` API was introduced and should not be used in new code. – tadman Aug 24 '17 at 18:34
  • If you only care if the thing exists or not, use `SELECT COUNT(*) ...` instead of `SELECT * ... LIMIT 1` then check that you got a non-zero value back. – tadman Aug 24 '17 at 18:35
  • @tadman Thanks. I'm fetching values as well, but checking for permission in the same query. – SeaBass Aug 24 '17 at 18:37
  • If you're using the data it's fine. You can also check the number of rows fetched before actually getting anything as well. – tadman Aug 24 '17 at 18:38

0 Answers0