vb.org Archive - Create Secure Mods

vb.org Archive (https://vborg.vbsupport.ru/index.php)

- Programming Articles (https://vborg.vbsupport.ru/forumdisplay.php?f=188)

- - Create Secure Mods (https://vborg.vbsupport.ru/showthread.php?t=154411)

Create Secure Mods

In this post we'll go over some basic modification vulnerabilities and how to prevent them.

Preparing Data

Whenever you accept input from a user, you must understand that you have no control over what is entered, period. The short version is that anything client-side can be manipulated. Even if you use Javascript or HTML limitations, they can all easily be avoided. This input includes but is not limited to:

Entered form input
Hidden form input (!)
URL query string
User Agent

In your application it is your responsibility to ensure the validity of the data they enter.

Use the vBulletin Input Cleaner!

I'm going to assume you know how to use it, even though sometimes you choose not to use it. If not, dig up a tutorial on it.

A common example is fetching a row from a table using an ID in the query string. I often see this:

PHP Code:


		
			
$row = $db->query_first("
    SELECT *
    FROM " . TABLE_PREFIX . "table
    WHERE id = '$_GET[id]'
");

Not good.

Here is what you should use...

PHP Code:


		
			
$id = $vbulletin->input->clean_gpc('g', 'id', TYPE_UINT);

$row = $db->query_first("
    SELECT *
    FROM " . TABLE_PREFIX . "table
    WHERE id = $id
");

The TYPE_UINT ensures that the variable is an integer, and is >= 0. You can also use $id in the rest of your application, more confidently. But wait... if they don't enter a valid integer here, it will just be 0! We can prevent that query from running in this case. Though no harm would be done, it is good to do as little processing as possible to keep things running fast.

PHP Code:


		
			
$id = $vbulletin->input->clean_gpc('g', 'id', TYPE_UINT);

if (!$id)
{
    standard_error('Please enter a valid product ID to view.');
}

$row = $db->query_first("
    SELECT *
    FROM " . TABLE_PREFIX . "table
    WHERE id = $id
");

That's better. The same concept applies to a lot of different types of data. If you expect them to enter an email address, then you can use the is_valid_email() function. If you expect them to enter one of a few specific answers, have those answers in an array and ensure that it matches. Example,

HTML Code:

<select name="meal">

    <option value="1">Breakfast</option>

    <option value="2">Lunch</option>

    <option value="3">Dinner</option>

</select>

PHP Code:


		
			
$meal = $vbulletin->input->clean_gpc('p', 'meal', TYPE_UINT);

if (!in_array($meal, array(1, 2, 3))
{
    standard_error('Invalid meal specified.');
}

If you expect them to enter data in a certain pattern - for example, a md5 hash, then you can use a regular expression to match it.

PHP Code:


		
			
$hash = $vbulletin->input->clean_gpc('p', 'hash', TYPE_STR);

if (!preg_match('/^([a-f0-9]{32})$/', $hash))
{
    standard_error('Please enter a valid md5 hash.');
}

Not a great example, but you should really learn to use regular expressions to match the validity of data. An EXCELLENT tool to have. If you use the vBulletin datamanager for your custom application, be sure to use the error checking functionality in it. Very flexible.

Point: ensure the data you expect is what you expect. If it's not, do not let them continue. False data can do a number of things of varying damage:

Display a PHP error. Not a big deal, but it may disclose your script names and path. The less information they know about it the better.
Display SQL error. Injection, anyone?
Get into DB . It looks horrible to visitors when they see random junk on your website. Ohter side effects include: breaking search capabilities, false statistics, even DB errors down the road! Injection round two!

SQL Injection

What is SQL injection? It's when your script does not properly clean user input, and they can actually manipulate the query using the input value.

Example,

A script to update a user's email address. Here is the query:

Code:

UPDATE user SET email = '$value' WHERE userid = 1;

You'd expect the user to enter an email address, but what if they don't? If the value they enter contains a single quote, it will end the string email value in the query, and it will be parsed as SQL. This is injection. What could happen from this? It really depends on the query, but it can be devastating. If I enter

Code:

my@email.com', username = 'HAHA', password = ''#

The final query is:

Code:

UPDATE user SET email = 'my@email.com', username = 'HAHA', password = ''#' WHERE userid = 1;

Since a # is a MySQL comment starter (one of the few ways), here is the query:

Code:

UPDATE user SET email = 'my@email.com', username = 'HAHA', password = '';

This affects the query in two ways:

Adds two new columns and values into the query
Removes the WHERE clause

In English - wiped out your entire user table.

Preventing Injection

It is very easy to prevent injection. Generally, users either input text (string) or a number (int/float). For the latter, they should already be safe assuming you used the input cleaner as suggested. For strings, however, you must use the $db->escape_string() function. What does this do? In a standard MySQL setup, it's just a wrapper for mysql_real_escape_string which in simpliest terms, prepends a backslash to any characters which may alter query. Remember to use $db->escape_string() instead of the actual function, because not every board runs MySQL!

Automation

I highly recommend using a function to generate queries. This is how the datamanager works, and also many of my own private hacks. How does this make it more secure? - by not relying on the coder to escape it. If you do so, you can wrap each value in the $db->sql_prepare() function, which makes it SQL safe.

Here is what the function does to...

Strings, returns it escaped, and wrapped in single quotes
Numbers, returns them as is
Booleans, returns them in their integer form (1 and 0)
Anything else, returns it escaped, and wrapped in single quotes

Very useful tool.

XSS (Cross Site Scripting)

Cross site scripting is when a user injects HTML into any text that he posts, and in that HTML uses Javascript to communicate with his own server. What can this do? He can steal your cookie information and take over your online identity. Imagine someone doing this to an administrative account. Uh oh!

Like SQL injection, XSS is actually very easy to prevent. You have two options:

1) Use TYPE_NOHTML instead of TYPE_STR when accepting string input.This has a few (minor) downsides. If you want to allow searching, or for any other reason want to retain the exact information the user enters. The alternative, is

2) Clean it using htmlspecialchars_uni() when displaying the text. This is where it gets tricky. If you display it all over the place, it can be difficult to remember to clean it every time.

So what does this do exactly? Any HTML characters, such as "<", ">", and "&" are parsed as HTML, so if you convert them to their entity form ("<", ">", "&", the browser knows to display that character, and not parse it as HTML. Now it is impossible for any hackers to inject HTML into your site.

Summary

Only accept input that is what you expect.
Escape strings when using them inside queries.
Convert all HTML characters to entities before displaying them.

If you have any further questions, you may contact me at vBlogetin.com, or at SirAdrian.com

You may not copy this article without my permission.

Excellant article. Very very helpful. Great job Sir Adrian

Good Articles. Thanks Sir Adrian.

Very Nice Article..

Thanks !

Nice article SirAdrian ;)

Thanks...

superb article SirAdrian :up:

A hghly recommended must read article.

Quote:

Originally Posted by Princeton (Post 1311912)

superb article SirAdrian :up:

A hghly recommended must read article.

Couldn't have said it better Princeton!

Thank you SirAdrian :up:

Great article ! Exactly what I was asking in another thread !

Addition: Use fetch_query_sql() whenever it makes sense.

Quote:

Originally Posted by Andreas (Post 1311962)

Addition: Use fetch_query_sql() whenever it makes sense.

This looks like a function for reading from the database. I assume that once the data is there, it can be trusted. Does this function has any security implications ? (after all, this is the theme of this article)

Quote:

I assume that once the data is there, it can be trusted.

Wrong assumption.

Example: A post that contains

Quote:

Now, if you parse this text or run htmlspecialchars_uni() on it you are safe.
However, if you just read pagetext from table post and output it, then you just created an XSS issue.

Quote:

Originally Posted by vBulletin functions.php

PHP Code:


		
			
/**
* Returns an UPDATE or INSERT query string for use in big queries with loads of fields...
*
* @param    array    Array of fieldname = value pairs - array('userid' => 21, 'username' => 'John Doe')
* @param    string    Name of the table into which the data should be saved
* @param    string    SQL condition to add to the query string
* @param    array    Array of field names that should be ignored from the $queryvalues array
*
* @return    string
*/

As you can see, it runs escape_string() automatically

fetch_query_sql() was what I meant by automation, thouh I actually use my own function that combines the $db->query_write() call and the fetch_query_sql.

Thanks for pointing that out - I'll add it in later.

Quote:

Originally Posted by Andreas (Post 1311973)

Wrong assumption.

Example: A post that contains

Now, if you parse this text or run htmlspecialchars_uni() on it you are safe.
However, if you just read pagetext from table post and output it, then you just created an XSS issue.

As you can see, it runs escape_string() automatically

But you should escape data *before* entering the database. You should make sure that whatever enters the database is clean. The reason is that usually the data is put into the database once and read many times thus it is better to do all validations when putting the data into the database and not when reading it.

XSS and SQL injection are two different vulnerabilities.

Escaping it will avoid injection, but the XSS threat still remains unless you TYPE_NOHTML or htmlspecialchars_uni() before displaying it.

Quote:

Originally Posted by SirAdrian (Post 1312381)

XSS and SQL injection are two different vulnerabilities.

Escaping it will avoid injection, but the XSS threat still remains unless you TYPE_NOHTML or htmlspecialchars_uni() before displaying it.

But what if I run "htmlspecialchars_uni()" before putting the data in the database ?

very helpful SirAdrian, thank you. So it seems as if it should be a standard practive to use $db->sql_prepare() as opposed to $db->escape_string() since the former seems to do a bit more "cleaning" of the user input. Are there any instances where this might not be the case? I ask this because I tend to see $db->escape_string() more frequently than $db->sql_prepare() in vb code (default and mods). Or is one no better than the other?

They do different things.

sql_prepare basically checks the data type, and adds quotes/escapes it if necessary. escape_string just escapes it.

Why is it neccessary to clean admincp code?

Quote:

Originally Posted by EnIgMa1234 (Post 1315558)

Why is it neccessary to clean admincp code?

It's just good coding practice to do so. Say for instance you have a moderator or someone whom you allow access to the admin CP (a business partner for instance) and you have a falling out - he/she then injects something nasty theough the AdminCP and BOOM!

Sir Adrian,
ok, i'm learning this, thanks. So after escaping it, if I want to display it in a <textarea> type input box for user to edit it, it's showing as follows:

Quote:

Posted the banner on my myspace profile. Also posted their video on my blog, etc... \r\n\r\nOh yes i did.\r\n\r\nThat\'s what I am talking about. "oh yeah" i said

Which is safe, however for display in the textarea, i want it to display as entered which was like this:

Quote:

Posted the banner on my myspace profile. Also posted their video on my blog, etc...

Oh yes i did.

That's what I am talking about. "oh yeah" i said

When I display it, I can qet the " to parse correctly, but all the /n/r/n/r i can't get to display correctly. Can you help? Thanks :)

Why are you displaying it after escaping it? It goes into the query escaped, and when you read from the DB again it should be OK.

It goes into query escaped before being put into database. Then if user wants to edit the text, it's displayed again, within the <textarea> box for them to modify.

I'd be fine if the initial cleaning of the data totally stripped out all the /n/r/n/r stuff as well since i have no need for them to post data on new lines. Am i missing something or do i need to create a regex before passing data to the update or save query?

Only escape just before using it in a query. That's all it is for. You should never display it after it has been escaped; that doesn't make sense!

Thanks for your help on this issue SirAdrian.
So you're telling me that somewhere I am escaping it before displaying it in the <textarea> box?

Yeah you must be. Post your code here (or make a new thread in programming / coders discussion and I will take a look).

ok will post it in programming discussions - thanks

Excellent article SirAdrian, was a great read. :)

Quote:

Originally Posted by Kirk Y (Post 1324567)

Excellent article SirAdrian, was a great read. :)

Definitely. Thank you for taking the time to write this article, Adrian. :up:

Nice article, thanks for taking the time to write this out.

Good article, AJ. Congrats. :)

Excellent article, thanks for writing it.

PHP Code:


		
			
$vbulletin->input->clean_gpc('g', 'id', TYPE_UINT);

What's 'g' mean?
I've seen some code has 'r' or 'p'?

They are the some of the super globals:

g = $_GET, p = $_POST, r = $_REQUEST, and also c = $_COOKIE.

The above are all valid and alters what content the cleanser sanitizes.

More information can be found in the PHP manual:

http://us.php.net/variables.predefined

vB doesn't seem to use htmlspecialchars_uni when displaying the fields it uses htmlspecialchars_uni only during the update/insert of the TYPE_STR fields, so if we do that we are on the safe side right ?

I am asking cause some of the opinions in this thread really messed up with mine :erm:

1. Use the vBulletin Input Cleaner!
2. Use the htmlspecialchars_uni to clean the TYPE_STR vars from the vBulletin Input Cleaner
3. Use the $db->escape_string() to insert/update/replace values

If you use again the htmlspecialchars_uni during display or in the inputs things get ugly.

The only exception in this rule is the text columns that you might want to use html instead of bbcode. In this case you don't use htmlspecialchars_uni to clean the $vbulletin->GPC['message'] but you use the htmlspecialchars_uni during the edit on the textarea.

Phrase Groups Available:

X vBulletin 3.8.12 by vBS Debug Information
Page Generation 0.07834 seconds Memory Usage 1,850KB Queries Executed 10 (?)
More Information
Template Usage: (1)ad_footer_end (1)ad_footer_start (1)ad_header_end (1)ad_header_logo (1)ad_navbar_below (4)bbcode_code_printable (1)bbcode_html_printable (7)bbcode_php_printable (11)bbcode_quote_printable (1)footer (1)gobutton (1)header (1)headinclude (6)option (1)post_thanks_navbar_search (1)printthread (35)printthreadbit (1)spacer_close (1)spacer_open Phrase Groups Available: global postbit showthread	Included Files: ./printthread.php ./global.php ./includes/init.php ./includes/class_core.php ./includes/config.php ./includes/functions.php ./includes/class_hook.php ./includes/modsystem_functions.php ./includes/class_bbcode_alt.php ./includes/class_bbcode.php ./includes/functions_bigthree.php Hooks Called: init_startup init_startup_session_setup_start init_startup_session_setup_complete cache_permissions fetch_threadinfo_query fetch_threadinfo fetch_foruminfo style_fetch cache_templates global_start parse_templates global_setup_complete printthread_start bbcode_fetch_tags bbcode_create bbcode_parse_start bbcode_parse_complete_precache bbcode_parse_complete printthread_post printthread_complete
Messages: