current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 7 down vote favorite

Scenario

In order to prevent SQL injection, I'm converting every character of a string to be inserted in the database into its ASCII value before performing the query; in order to read the value of the string from the database, I'm reversing the operation.

The code

<?php
function toDatabase($string){
    $split = str_split(htmlspecialchars($string));
    $ascii = "";
    foreach($split as $letter){
        $ascii .= ord($letter).'-';
    }
    return $ascii;
}
function fromDatabase($string){
    $explode = explode("-",$string);
    $phrase = "";
    foreach($explode as $ascii_char){
        $phrase .= chr($ascii_char);
    }
    return $phrase;
}
$toBeInserted = toDatabase($_POST['comment']);
$connect = mysqli_connect("","","","");
$query = mysqli_query($connect,"INSERT INTO comments(comment) VALUES ('".$toBeInserted."')");
if(!$query){ die('Error!'); }
$fetch_query = mysqli_query($connect,"SELECT comment FROM comments");
if(!$fetch_query){ die('Error!'); }
while($assoc = mysqli_fetch_assoc($fetch_query)){
    echo fromDatabase($assoc['comment']).'<hr>';
}
?>

My question

Is this method safe?

share|improve this question
14  
Why not just use parameterized queries? –  Mat's Mug♦ Feb 20 at 20:26
    
@Mat'sMug thanks for your comment. As I generally have to perform just a query per page, parametrized queries will require to reach the database twice instead of once per page. Moreover, while parametrized queries may be, as today, the safest way to prevent SQL injection, they might not be safe 100% (even if I don't doubt on the fact that inserting just numbers might not be safe 100% too) –  Stubborn Feb 20 at 20:34
11  
Parameterized queries are safer because they let the server deal with understanding parameter types and values. If it's your own code concatenating the values into the query, it's inherently unsafe, no matter how clever the "injection-prevention" algorithm. –  Mat's Mug♦ Feb 20 at 20:39
1  
@Mat'sMug thanks for the clarification –  Stubborn Feb 20 at 20:41
2  
@Mat'sMug it should be noted that eg PDO by default only emulates prepared statements for MySQL (internally, it concatenates the values into the query), so the advantage doesn't always exist in practice. I would still choose it over this approach though. –  tim Feb 20 at 20:47

2 Answers 2

up vote 18 down vote accepted

There's a common credo believed when evaluating the security of software:

If it's homemade, it's unlikely secure.

Sec.SE has a Q&A about homemade algorithms, which is somewhat germane to your circumstance.

I suggest you look into preparing your queries, as that would be your best action to take in this situation. It's essentially what you're trying to do anyways. Again, the PHP docs contain more information on mysqli::prepare.

I just noticed your comment to Mat's Mug, and prepared queries will not reach the database twice as you've said. When they're implemented correctly, they will be 100% secure (in doing their job, you might have to protect against other faults).

If you're worried about performance or efficiency, you might find the following select quotes helpful:

Prepare is followed by execute. During execute the client binds parameter values and sends them to the server.

The database is not reached twice.

A prepared statement can be executed repeatedly. Upon every execution the current value of the bound variable is evaluated and sent to the server. The statement is not parsed again. The statement template is not transferred to the server again.

I know you said it's only a single query per page, but things change in the future, and it's better to know you'll be safe then too.

Prepared statements are using the so called binary protocol. The MySQL server sends result set data "as is" in binary format. Results are not serialized into strings before sending. The client libraries do not receive strings only. Instead, they will receive binary data and try to convert the values into appropriate PHP data types.

If you happen to be curious as to how and why paramterized queries are so much safer!

These quotes are from the PHP manual on prepared statements.

share|improve this answer
    
Thanks for your answer and for the clarification about the fact that the database will not be reached twice –  Stubborn Feb 20 at 20:42
    
@Stubborn I've made some edits, which will hopefully make things even more clear. –  Alex L Feb 20 at 20:48
    
Thanks very much, they actually did clarify more –  Stubborn Feb 20 at 20:54
up vote 7 down vote

Is this method safe?

Well, assuming ord is safe, it should be safe, because you will only ever have strings containing numbers in your query.

But what is also safe are prepared statements, and they are well tested over the years.

And prepared statements don't have the downsides of your solution:

  • untested in practice
  • bad performance time wise (additional loop and encoding for each character)
  • bad performance space wise (eg test becomes 116-101-115-116-)
  • bad for portability
  • bad for searches

Also, why htmlspecialchars? This doesn't seem necessary at all and might cause problems.

share|improve this answer
    
Thanks for your answer. I was using htmlspecialchars as the string (which was a comment posted by some random person who could have written some code in the comment), after being re-converted from the database, was being displayed directly to the user –  Stubborn Feb 20 at 20:50
    
Well, you are using it before inserting the data into the database, which seems like a bad idea (XSS will not hurt the database, data in database should be clear but encoding changes data, you should prevent XSS when echoing anyways because you never know where the data might be from (your database could import data from different sources, etc)) –  tim Feb 20 at 20:53
    
You're right, thanks for your suggestions –  Stubborn Feb 20 at 20:54

Your Answer

 
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.