0

Making an ad manager plugin for WordPress, so the advertisement code can be almost anything, from good code to dirty, even evil.

I'm using simple sanitization like:

$get_content = '<script>/*code to destroy the site*/</script>';
//insert into db
$sanitized_code = addslashes( $get_content );

When viewing:

$fetched_data = /*slashed code*/;
//show as it's inserted
echo stripslashes( $fetched_data );

I'm avoiding base64_encode() and base64_decode() as I learned their performance is a bit slow.

Is that enough?
if not, what else I should ensure to protect the site and/or db from evil attack using bad ad code?

I'd love to get your explanation why you are suggestion something - it'll help deciding me the right thing in future too. Any help would be greatly appreciated.

Mayeenul Islam
  • 4,532
  • 5
  • 49
  • 102

1 Answers1

4

addslashes then removeslashes is a round trip. You are echoing the original string exactly as it was submitted to you, so you are not protected at all from anything. '<script>/*code to destroy the site*/</script>' will be output exactly as-is to your web page, allowing your advertisers to do whatever they like in your web page's security context.

Normally when including submitted content in a web page, you should be using htmlspecialchars so that everything comes out as plain text and < just means a less then sign.

If you want an advertiser to be able to include markup, but not dangerous constructs like <script> then you need to parse the HTML, only allowing tags and attributes you know to be safe. This is complicated and difficult. Use an existing library such as HTMLPurifier to do it.

If you want an advertiser to be able to include markup with scripts, then you should put them in an iframe served from a different domain name, so they can't touch what's in your own page. Ads are usually done this way.

I don't know what you're hoping to do with addslashes. It is not the correct form of escaping for any particular injection context and it doesn't even remove difficult characters. There is almost never any reason to use it.

If you are using it on string content to build a SQL query containing that content then STOP, this isn't the proper way to do that and you will also be mangling your strings. Use parameterised queries to put data in the database. (And if you really can't, the correct string literal escape function would be mysql_real_escape_string or other similarly-named functions for different databases.)

bobince
  • 528,062
  • 107
  • 651
  • 834
  • Bull's eye. I never thought like that: *`addslashes` then `removeslashes` is a round trip.*, but it's actually true - a harmful script is not a threat for db when it's stored but for a db when it's executed on the visual-end. A very good answer and a direction also. Thanks a lot. – Mayeenul Islam Sep 21 '14 at 16:39
  • BTW `mysql_real_escape_string` is deprecated. :( – Mayeenul Islam Sep 21 '14 at 17:05
  • I don't think `mysql_real_escape_string` is deprecated: http://us1.php.net/manual/fa/function.mysql-real-escape-string.php . – Claude Sep 21 '14 at 21:14
  • All of `mysql_` is deprecated in PHP 5.5. That link is an out-of-date (incomplete Farsi translation) version of the manual. There is still `http://php.net/manual/en/mysqli.real-escape-string.php`, but proper parameterised queries are generally better. – bobince Sep 23 '14 at 11:03