CODEWITHSUNDEEP
phpsecurityeval
Georgy Nemtsov
Georgy Nemtsov

Reputation: 846

Is it safe to use eval such way?

Let's say I have a file called query.sql with the following content in it:

SELECT * FROM `users` WHERE `id`!=".$q->Num($_POST['id'])."

And in my php-script, which has a html form with input named "id" in it, I do the following trick:

$sql=file_get_contents('query.sql');
$query= eval("return \"$sql\";");
//here follows something like $mysqli->query($query); and so on..

I am not concerned about sql-injections since I'm using prepared statements and $q->Num performs is_int check. But is it safe to use eval such way?

As far as I understand, what is actually eval-ed here is "${_POST['id']}" and it evals to some string value the user entered. And this becomes dangerous only if I eval this string second time. While I eval string only once user's input is just string and can not be interpreted as php-code by compiler and no php-injection is possible.

UPDATE Thank you for proposing different methodologies and stressing need to use prepared statements. But this not my question at all. My question is all about php-injections. Is such use of eval bad? If yes, why?

Upvotes: 3

Views: 489

Answers (4)

hek2mgl
hek2mgl

Reputation: 157990

The eval statement - although I would try to find a way without using eval - is not vulnerable for PHP injection because $sql is enclosed in double quotes "; One can not ending this quoting with a prepared variable in PHP.

I am not conserned about sql-injections since I'm using prepared statements

Aren't you? ;) You are!

Why do you add the $id to the query using the '.' operator (string manipulation)? If you really use the benefits from prepared statements I would expect something like a bindParam()

Note how prepared statements prevent from SQL injections: The SQL query syntax is been kept separate from arguments. So the server would

  1. parse the SQL query
  2. apply arguments

As the query has been already parsed before arguments will been applied, the query syntax cannot be manipulated by the arguments.

If you prepare a MySQL query that has been created using '.' and external inputs you are potentially vulnerable against SQL injections

What you are doing defeats the principals of prepared statements

Upvotes: 2

Adam Hopkinson
Adam Hopkinson

Reputation: 28795

There is no need to use eval - put in a token, and replace it:

// file query.sql
SELECT * FROM `users` WHERE `id`!="{id}";

//php
$sql = file_get_contents('query.sql');
$query = str_replace("{id}", $_POST['id'], $sql);

Update No, it's not safe. Someone could edit your query.sql script to do anything you want. You may say "the app is internal only", or "i have permissions locked down" or whatever - but at the end of the day there are no guarantees.

Upvotes: 2

SDC
SDC

Reputation: 14222

Let's consider the following code:

$sql=file_get_contents('query.sql');
$query= eval("return \"$sql\";");

The danger points at this are:

  • if the file query.sql is modified from what you expect, then it could be used to execute any arbitrary code in your program.

  • the name of the file is shown hard-coded; if this isn't the case, then a malicious user could find a way to load an unexpected file (possibly even one from a remote site), again resulting in arbitrary code execution.

  • the only reason to use a file for this (rather than hard coding the SQL code directly in the program) would be because you want to use it as a config file. The problem here is that the syntax in this file is invalid for both SQL and PHP. Due to the way it's run in the eval(), it also requires that the syntax is exactly correct. Use the wrong quotes or miss one out, and it'll blow up. This is likely to result in brittle code, that fails badly rather than gracefully when the config is marginally incorrect.

There doesn't appear to be a direct SQL injection attack here, but that's really the least of your worries when it comes to eval().

I have personally worked in projects where code existed that worked pretty much exactly the way you've described. There were some very nasty bugs in the system as a direct result of this, and they have been difficult to rectify without wholesale rewrites. I would strongly recommend stepping back from this idea and using a sensible templating mechanism instead as recommended by others in the comments.

Upvotes: -1

Rush
Rush

Reputation: 715

Reference from this Answer.

https://stackoverflow.com/a/951868/627868

The main problems with eval() are:

Potential unsafe input. Passing an untrusted parameter is a way to fail. It is often not a trivial task to make sure that a parameter (or part of it) is fully trusted.

Trickyness. Using eval() makes code clever, therefore more difficult to follow. To quote Brian Kernighan "Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it"

Upvotes: 1

Related Questions