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.

I am building a classifieds website here in Portugal, and I'm now in the security phase. So until now, I already made what I think is a good measure against SQL injection:

$firstname = chunk_split(mysql_real_escape_string($_POST[firstname]),1,'.');
$lastname = chunk_split(mysql_real_escape_string($_POST[lastname]),1,'.');
$email = chunk_split(mysql_real_escape_string($_POST[email]),1,'.');

etc...

This will save valid and invalid emails, for example, like this:

good email: u.s.e.r.@.e.m.a.i.l.h.o.s.t...c.o.m.
bad email: 

Example of text in the email field:

Y';
 UPDATE table
  SET email = 'hacker@ymail.com'
  WHERE email = 'joe@ymail.com';

and after stored in the database:

Y.\.'.;. .U.P.D.A.T.E. .t.a.b.l.e. .S.E.T. .e.m.a.i.l. .=. .\.'.h.a.c.k.e.r.@.y.m.a.i.l...c.o.m.\'. .W.H.E.R.E. .e.m.a.i.l. .=. .\.'.j.o.e.@.y.m.a.i.l...c.o.m.\.'.;.

So, I don't care if the size of the stored string is bigger because I think this is a solid approach and a very fast one. And if I had to use other ways, the scripts would probably consume the same time that is here exchanged by size.

But now I have the serious problem of XSS attacks. I'm trying to prevent only attacks based on text, not images or JavaScript because I will not have untrusted data there.

The solution I find is based on the last example, and looks like this:

$str = "<script>alert('XSS attack');</script>";
$ad_title = chunk_split($str,1,"<span style='font-size:0px;'>.</span>");

So when the page loads the users will see the inserted text and the alert will not work:

<script>alert('fabio');</script>

This is the source code:

<<span style='font-size:0px;'>.</span>s<span style='font-size:0px;'>.</span>c<span style='font-size:0px;'>.</span>r<span style='font-size:0px;'>.</span>i<span style='font-size:0px;'>.</span>p<span style='font-size:0px;'>.</span>t<span style='font-size:0px;'>.</span>><span style='font-size:0px;'>.</span>a<span style='font-size:0px;'>.</span>l<span style='font-size:0px;'>.</span>e<span style='font-size:0px;'>.</span>r<span style='font-size:0px;'>.</span>t<span style='font-size:0px;'>.</span>(<span style='font-size:0px;'>.</span>'<span style='font-size:0px;'>.</span>f<span style='font-size:0px;'>.</span>a<span style='font-size:0px;'>.</span>b<span style='font-size:0px;'>.</span>i<span style='font-size:0px;'>.</span>o<span style='font-size:0px;'>.</span>'<span style='font-size:0px;'>.</span>)<span style='font-size:0px;'>.</span>;<span style='font-size:0px;'>.</span><<span style='font-size:0px;'>.</span>/<span style='font-size:0px;'>.</span>s<span style='font-size:0px;'>.</span>c<span style='font-size:0px;'>.</span>r<span style='font-size:0px;'>.</span>i<span style='font-size:0px;'>.</span>p<span style='font-size:0px;'>.</span>t<span style='font-size:0px;'>.</span>><span style='font-size:0px;'>.</span>

Without being concerned about all the code it produces, my question is: is this enough or actually safe?

share|improve this question

2 Answers 2

up vote 16 down vote accepted

Is it safe? Maybe. Is it the right way to do it? No.

Even if it were safe, there’s a big problem with the SQL, and that’s that you’re inserting the dots after escaping, rather than before. I notice that for the input ', it will be escaped to \', and then your dot-insertion turns it into \.'.. Before, the \ was escaping the ', but now it isn’t. Now it’s escaping the .. I don’t know if a malicious entity could do anything bad with that, but I wouldn’t take any chances.

And here are some problems with the HTML:

  • Copy-and-pasting will catch those dots.
  • Search engines will catch those dots.
  • You break any Unicode characters, e.g. you turn 日本語 into ?.?.?.?.?.?.?.?.?.. (But I only have an old version of PHP installed; maybe newer versions are more intelligent, but in that case, you might have other problems like putting a combining character onto your >)

And that’s not to mention the gigantic size increase, and potentially having invalid HTML… all around, it’s just not a very good way to do it.

So what’s the right way to do it? Well, for inserting data into the database, you should be using PDO with prepared statements. Then you don’t have to deal with SQL escaping at all: you prepare a query with placeholders, and send in the placeholder data, and since the data doesn’t have to touch the query, you don’t have to worry about that at all.

Sidebar: using PDO

You said that it’d be a lot of work to use PDO. Well, I’ve never found it particularly difficult. Your code perhaps looks like this:

mysql_connect('localhost', 'myapp', 'letmein');
mysql_select_db('myapp');
// ...
mysql_query("insert into users (firstname, lastname, email) values ('$firstname', '$lastname', '$email')");

But it’s not actually that hard to use PDO. That code could be translated to use prepared statements and PDO like so:

$db = new PDO('mysql:host=localhost;dbname=myapp', 'myapp', 'letmein');
// ...
$stmt = $db->prepare('insert into users (firstname, lastname, email) values (:firstname, :lastname, :email)');
$stmt->bindValue('firstname', $_POST['firstname']);  // look ma, no escaping!
$stmt->bindValue('lastname', $_POST['lastname']);
$stmt->bindValue('email', $_POST['email']);
$stmt->execute();

And besides being more secure, you’re also moving onto something that the PHP developers have committed to keep in place: the mysql_* functions will be removed, as the PHP documentation says:

Warning: This extension is deprecated as of PHP 5.5.0, and will be removed in the future. Instead, the MySQLi or PDO_MySQL extension should be used. See also MySQL: choosing an API guide and related FAQ for more information.

End of sidebar

On the HTML side, using htmlentities or htmlspecialchars is the standard way to do it.

Then, finally, I’d like to point out one thing about how you’re accessing POST data: you’re currently using $_POST[firstname]. It’s supposed to be an expression that’s between the brackets, but you have the name of the field literally in there. It turns out that that’s okay currently, as undefined constants evaluate to their name. But that does generate a notice, and depending on your error reporting level, you will probably see lots of notices in your error log saying that this is bad and/or deprecated. You should explicitly quote it: $_POST['firstname'].

share|improve this answer
1  
@Fábio: There’s not a whole lot to say about the htmlentities thing; that’s something you’ll just have to deal with. But PDO isn’t that much more difficult than the mysql_* functions; I’ve included an example in my answer to demonstrate. –  icktoofay Aug 18 at 1:13
2  
@FábioLinhares: Both XSS and SQLi are solved problems. There's no need to reinvent the wheel here for a solution. The best approach for when it comes to security is to keep it simple, then you can prove to yourself that it is safe. Adding extra complexity in the hope that it fools attackers is foolish - they will find a way around. –  SilverlightFox Aug 18 at 7:59
1  
@FábioLinhares: If it was so simple you would not have to post this question in the first place to ask whether it was secure. In the trade that is known as security by obscurity - to an experienced hacker it might slow them down a little but you shouldn't think for a second that it will bring them to a halt - quite the opposite in fact - they would find nothing more fun that to exploit such a flaw in your roll your own security system. –  SilverlightFox Aug 18 at 10:30
1  
@FábioLinhares: Also adding lots of unneeded <span>s is arguably as heavy on the client side. You have obviously already decided your approach is a good one (maybe out of pride). My advice is unfortunately to scrap it and do it the standard way. –  SilverlightFox Aug 18 at 10:34
1  
@FábioLinhares: A similar technique has been discussed on on sec.se that you may find interesting. –  SilverlightFox Aug 18 at 10:42

I agree with everything @icktoofay said, and you should definitely use PDO and htmlspecialchars.

Just a couple of additional points:

Security Issues

  • You should never roll your own security, it's just a bad idea in general.
  • chunk_split is vulnerable in some php versions, see here and here
  • @SilverlightFox already linked to it in the comments: you don't know for sure how different browsers will handle your solution, so there might be security concerns in some obscure situations.
  • As @icktoofay noted, mysql_real_escape_string is useless as it escapes the dots, not the characters it should escape, which might cause security problems

Usability

SQL

your solution makes it really difficult to use the database.

  • If I want to query for a specific username/email I first have to run chunk_split on it.
  • If I want to display that email, I first have to remove the dots.
  • It increases size

XSS/HTML

  • Your code is not valid HTML code (you can check html validity here) and as such it is not predictable how browsers will handle it
  • It causes a giant overhead (bad for performance)
  • The points @icktoofay has (breaking copy/paste, breaking search engines, breaking unicode chars)
  • If your only concerned about attacks on text output, not images or js, htmlspecialchars is a perfectly acceptable solution

Performance

Test this for you individual setup. I found different benchmarks, some saying PDO is faster, some saying MySQLi is faster (see here, here, here, and here for example).

Keep in mind that you have to include all the chunk_split(mysql_real_escape_string($data,1,'.'); calls (for input and the equivalent for removing the dots from the output), as they take time as well.

share|improve this answer
    
"You should never roll your own security, it's just a bad idea in general." why ? Chunk_split is not vulnerable in my php version. Works fine in all browsers (tested). I already started to change that detail: mysql real escape string. Usability ? It is far easier than you think, basic copy and past and change the values accordingly, very easy. XSS/HTML: i already said to @icktoofay "So, i am more interested in XSS, and i think i will follow your advice on using htmlspecialchars". So basicly i will not use Chunk_split for XSS, mainly because of search engines, but i will keep using it for mysql –  Fábio Linhares Aug 18 at 13:40
1  
And your code is never going to run on a different server? Also, chunk_split wasn't the only possible weak point. But the main reason is that you are just one person, and probably not a security expert. A lot of experts spent years collectively developing current best practices, and attackers spent years trying to find vulnerabilities in them. Your own security mechanisms will never get this kind of exposure and testing (as mentioned in the linked question). I mean, you can do it if you want to, but I wouldn't recommend it. –  tim Aug 18 at 13:43

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.