Is this unsafe? PHP - PDO

Question

Alright, so I've set up a small system where I can add pages through an administration panel and for them to appear on the main site. As well as html pages that are made in the admin panel I have also got about two PHP pages with queries that are stored in the database.

Anyways I am calling these by using 'Eval' which I've read that it is unsafe.

Although since its only html codes going in from the administration panel [php codes are disallowed and wont function if posted in these pages] and the PHP pages are unediable unless access to the database, is this safe?

One PHP page involves user comments but all HTML and PHP codes are stripped from the form. I've tested it involving a few exploiting techniques but none seemed to succeed.

But is using eval for my purpose safe? Is there a better work around?

Code:

<?php   

if (isset($_GET['p']))
{
    $stmt = $dbh->prepare('SELECT * FROM pages WHERE shortname = :p');
    if (!$stmt->execute(array(':p' => $_GET['p'])))
    {//
        exit('Could not exec query with param: '.$_GET['p']);
    }


    while($row = $stmt->fetch(PDO::FETCH_ASSOC))
    {

        eval(" ?>".$row["content"]."<?php ");
        echo '</div>';
    }
}
//ends connection
$row->dbh = null;
    ?>

Most exploitable code was written by programmers who thought they didn't leave holes. You have to be extremely careful to ensure that code using `eval` is safe. It sounds like you've been pretty diligent, but it would probably be better to find some other solution. — Barmar, Feb 03 '14 at 12:29
Show your code and lets hope you are using prepared statements — Liam Sorsby, Feb 03 '14 at 12:29
There is basicly no code to show other then `eval(" ?>".$row["content"]." — user3219007, Feb 03 '14 at 12:30
Full code if needed: `prepare('SELECT * FROM pages WHERE shortname = :p'); if (!$stmt->execute(array(':p' => $_GET['p']))) {// exit('Could not exec query with param: '.$_GET['p']); } while($row = $stmt->fetch(PDO::FETCH_ASSOC)) { eval(" ?>".$row["content"]."'; } } //ends connection $row->dbh = null; ?>` — user3219007, Feb 03 '14 at 12:33
If the php code is non-editable, why do you keep it in the database? And if you don't need to keep it there, put it in a file, and completely get rid of your eval blocks. — Tularis, Feb 03 '14 at 12:33
Sorry, added my code into the question. @Tularis Its the way I've set it up to show my pages. Id need to create a whole separate page for the PHP if I dont use eval. But instead i'd rather keep it in the same page. — user3219007, Feb 03 '14 at 12:35
So you end up with 1 extra file; is that so bad? All you need to do is include() in the right place and that's it... Sounds pretty simple to me? — Tularis, Feb 03 '14 at 12:40
Look for sql injection - reading this link http://stackoverflow.com/questions/1582161/how-does-a-preparedstatement-avoid-or-prevent-sql-injection tells me you are vulnerable to sql injection, since you are using the $_GET[''] value directly, change your link to "yourpage.com/?p=shotname' OR 1=1 --" see what happens — Arrok, Feb 03 '14 at 12:42
@Tularis The way I am doing it is instead of having millions of pages, my pages are created from using the url of something like index.php?p=sample By getting rid of eval for PHP codes it means that I can no longer fully use ?p= and makes the system confusing. — user3219007, Feb 03 '14 at 12:56
You can still use that same technique. If you have a string that does not contain PHP code that needs to be evaluated, you don't need to use eval (but can just echo or print it). So you wouldn't be stuck with the eval problem. Alternatively, you could also alter your database-table to include a column which states whether the contents are php code or html. In case of HTML you just echo the value, and with PHP you eval it. This way you prevent any type of injection via regular pages. — Tularis, Feb 03 '14 at 12:59
@Tularis But regular pages cant be injected into anyways. They are just plain html. No PHP or any queries involved, and only way to add PHP into it is by database in which no one should have access to anyways. — user3219007, Feb 03 '14 at 13:01
I think you should have got an error or something, I don;t know what you have in your db..you should create a test table call it sqlinjectiontest and access "yourpage.com/?p=Robert'); DROP TABLE sqlinjectiontest; --" and after that check if the sqlinjectiontest is still there, if it's not..you are vulnerable to sql injection. — Arrok, Feb 03 '14 at 13:20
@Arrok he uses the prepared statements correctly. He does not mix user given content with the sql command. In the error he should not echo the _GET param though. Doing that makes a nice XSS security hole. — Veda, Feb 03 '14 at 14:31

Veda · Answer 1 · 2014-02-03T14:40:17.693

0

Sometimes writing secure code is more than being sure that it is safe. Maybe we all look at your code and think that it is safe, but oversee something small and obvious that will make a big security hole.

Better safe than sorry. You say [php codes are disallowed and wont function if posted in these pages] So why do you use eval then?

Back to your code. The parts that you have posted look safe to me (as for the eval part). But what if there is some small sql injection hole somewhere else in your application that lets the attacker change rows? The attacker will be able to put php code in your database and later execute it with your eval statement.

I would say: No, this code is not safe.

Also, do not echo user given content in your errors, this can lead to an xss vulnerability.

edited Feb 03 '14 at 14:40

answered Feb 03 '14 at 13:13

Veda

2,025
1
18
34

With the quote you have of what I said. That is for HTML pages that are created from administration panel. What I am saying is you cant add PHP codes into the admin panel forms as it wont let you. – user3219007 Feb 03 '14 at 13:15
The thing is, you have to be really sure there are not other problems. Sometimes a weakness itself (like using eval() ) does not leave a security problem, but combined with some other harmless weakness it will leave a problem. Most of the time, when i think some code might potentially leave a security problem, I replace it with some code that I am more sure of does not have security problems. Although this looks safe, I would advice you to replace it with something safer (if that is possible). – Veda Feb 03 '14 at 13:21
1

@user How "won't it let you" exactly? What exactly are you doing to ensure that? – deceze Feb 03 '14 at 13:35
The form strips it before submitting. There is no way at all. – user3219007 Feb 03 '14 at 22:29
@user "The form"? Do you mean Javascript? – deceze Feb 04 '14 at 07:26
Never, ever trust anything that comes from the client. Client side stripping is not safe! It is ok if you use javascript to validate or block things, as long as you implement it on the server side as well. – Veda Feb 04 '14 at 08:27

Is this unsafe? PHP - PDO

1 Answers1