MyBB Community Forums
Injection or bad in anyway? - Printable Version

+- MyBB Community Forums (https://community.mybb.com)
+-- Forum: Extensions (https://community.mybb.com/forum-201.html)
+--- Forum: Plugins (https://community.mybb.com/forum-73.html)
+---- Forum: Plugin Development (https://community.mybb.com/forum-68.html)
+---- Thread: Injection or bad in anyway? (/thread-185532.html)

Injection or bad in anyway? - sleeping - 2015-10-23

I need to programmatically create a thread with some data in it and currently I achieve it in the following way:

            // Set up posthandler.
            require_once MYBB_ROOT."inc/datahandlers/post.php";
            $posthandler = new PostDataHandler("insert");
            $posthandler->action = "thread";

            $usename = $db->escape_string($product['name']);
            $safename = Slug($product['name']);
            $tmessage = htmlspecialchars_uni("[color=#ff9933][size=x-large]some text {$usename}![/size][/color]\n\nmore text\n\n[url={$mybb->settings['bburl']}/{$product['id']}-{$safename}/][color=lightyellow]more text![/color][/url]");

            if ($product['type'] == 'BETA')
            {
                $mybb->input['prefix'] = 13;
            }
            elseif($product['type'] == 'PAID')
            {
                $mybb->input['prefix'] = 11;
            }
            elseif($product['type'] == 'FREE')
            {
                $mybb->input['prefix'] = 10;
            }
            else
            {
                $mybb->input['prefix'] = 0;
            }

            // Set the thread data that came from the input to the $thread array.
            $new_thread = array(
                "fid" => $product['pid'],
                "subject" => $db->escape_string($product['name']."'s support!"),
                "prefix" => $mybb->input['prefix'],
                "icon" => 3,
                "uid" => $product['uid'],
                "username" => $db->escape_string($product['username']),
                "message" => $tmessage,
                "lastposter" => $db->escape_string($product['username']),
                "views" => 0,
                "replies" => 0,
                "visible" => 0,
                "notes" => ''
            );

            $new_thread['options'] = array(
                "signature" => 1,
                "subscriptionmethod" => 1,
                "disablesmilies" => 0
            );
            $posthandler->set_data($new_thread);
            $valid_thread = $posthandler->validate_thread();

            $post_errors = array();
            $errors = array();
            // Fetch friendly error messages if this is an invalid thread
            if(!$valid_thread)
            {
                $post_errors = $posthandler->get_friendly_errors();
                if(count($post_errors) > 0)
                {
                    $errors[] = inline_error($post_errors);
                }
            }

            if (empty($errors))
            {
                $thread_info = $posthandler->insert_thread();
                $tid = $thread_info['tid'];
            }

It does work, and I only have 1 concern, I am not using "escape_string" on the message field, the message field is mainly predefined by me with the exception of 2 entries the URL and the username.

I do use:
            $usename = $db->escape_string($product['name']);
            $safename = Slug($product['name']);

Just in case this is my Slug function:
function Slug($string)
{
    return strtolower(trim(preg_replace('~[^0-9a-z]+~i', '-', html_entity_decode(preg_replace('~&([a-z]{1,2})(?:acute|cedil|circ|grave|lig|orn|ring|slash|th|tilde|uml);~i', '$1', htmlentities($string, ENT_QUOTES, 'UTF-8')), ENT_QUOTES, 'UTF-8')), '-'));
}
To make both the username and product name safe and I was wondering if interpolating it inside my $tmessage could cause a security issue.

Thanks.

RE: Injection or bad in anyway? - Omar G. - 2015-10-23

Escape it.

RE: Injection or bad in anyway? - Nathan Malcolm - 2015-10-24

Escape everything which goes into the database. No exceptions.

RE: Injection or bad in anyway? - sleeping - 2015-10-24

(2015-10-23, 11:21 PM)Omar G. Wrote: Escape it.

When I escape $tmessage I can never get it to display the newlines, the thread doesn't read it or parse it after posted. How can I make the new lines work?

Like it creates the thread fine but newlines show up as \n\n since the escape will escape em.

RE: Injection or bad in anyway? - dragonexpert - 2015-10-24

Use nl2br to make new lines actually show a new line.

RE: Injection or bad in anyway? - sleeping - 2015-10-24

(2015-10-24, 01:04 PM)dragonexpert Wrote: Use nl2br to make new lines actually show a new line.

Well I don't want to change the code of how a thread is read, I want to figure out how to properly insert the data without having to manipulate where it is read.

Otherwise I would have a major headache figuring out why the whole forum is not reading threads in a different way it was.

When I manually changed my $tmessage from \n to <br> it simple "parsed" it as it is considered as HTML and result was &qt;br&lt;, which is technically what the nl2br would do, turn it in <br>.

I further checked the datahandler and function_post and I don't see nl2br being used at all.

RE: Injection or bad in anyway? - Omar G. - 2015-10-24

Did you tried..
/*~~*/
$tmessage = "[color=#ff9933][size=x-large]some text {$usename}![/size][/color]

more text

[url={$mybb->settings['bburl']}/{$product['id']}-{$safename}/][color=lightyellow]more text![/color][/url]";
/*~~*/


RE: Injection or bad in anyway? - sleeping - 2015-10-24

yes, it becomes escaped \n as well, I am trying to find what my insertion code is missing that properly makes new lines accepted.
Ok I think I understood it? This is from the newthread.php:

	// Set up posthandler.
	require_once MYBB_ROOT."inc/datahandlers/post.php";
	$posthandler = new PostDataHandler("insert");
	$posthandler->action = "thread";

	// Set the thread data that came from the input to the $thread array.
	$new_thread = array(
		"fid" => $forum['fid'],
		"subject" => $mybb->get_input('subject'),
		"prefix" => $mybb->get_input('threadprefix', MyBB::INPUT_INT),
		"icon" => $mybb->get_input('icon', MyBB::INPUT_INT),
		"uid" => $uid,
		"username" => $username,
		"message" => $mybb->get_input('message'),
		"ipaddress" => $session->packedip,
		"posthash" => $mybb->get_input('posthash')
	);
$posthandler->set_data($new_thread);

escapes are not used at all in there, then further looking at the post handler the escaping is automatically done there.

			$this->post_insert_data = array(
				"subject" => $db->escape_string($thread['subject']),
				"icon" => (int)$thread['icon'],
				"username" => $db->escape_string($thread['username']),
				"dateline" => (int)$thread['dateline'],
				"message" => $db->escape_string($thread['message']),
				"ipaddress" => $db->escape_binary(my_inet_pton(get_ip())),
				"includesig" => $thread['options']['signature'],
				"smilieoff" => $thread['options']['disablesmilies'],
				"visible" => $visible
			);

So I am technically double escaping it hence why it doesn't work. So in the outside layer I don't have to worry about escaping it and when I send it to the post handler it will do all the work?

Did I get it right?

RE: Injection or bad in anyway? - Omar G. - 2015-10-25

Yes I think you are correct. The insert_post method already escapes everything.