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 2 down vote favorite

I have a small social networking site built in CodeIgniter. Any registered user can send messages to others by visiting their profile.

Today I noticed that one user sent bulk messages to 200 users. How he was able to do that?

Suggestions to make the code secure are welcome.

I have a textarea and a send button on the profile page.

jQuery code on profile page (View)

$("#send").click(function(event){ 
    var msg=$("#quick_message").val();
    var uid=$(this).attr('uid');

    if(msg.length > 0) {
        $("#msg_status").html('<span id="loading_content"></span>');

        $.post("<?=base_url()?>message/send_message", {"ids":uid,"msg":msg}, function(data){

        $("#msg_status").html('<span class="errorsuc">Message sent.</span>');
        $("#quick_message").val('');
        });
    } else {

    $("#msg_status").html('<span class="errormsg">Write something to send message.</span>');
    }
});

Here is my controller

  // send message
   function send_message()
 {
    if (!$this->users->is_logged_in()) {
    redirect('signin');
    }

    $user_id=$this->session->userdata('user_id');
    $ids=trim($this->input->post('ids'));
    $msg=trim($this->input->post('msg'));
    $msg=htmlspecialchars($msg);
    $msg=$this->replaceTolink($msg);
    $msg=$this->replaceTowinks($msg);

    $pieces=explode(",", $ids);
    foreach ($pieces as &$user_id2) {
    $this->db->insert('messages', array('user_id1' => $user_id,'user_id2' => $user_id2,'message' => $msg)); 
    }

    return true;
}

What I need to improve in my code and how to protect the code to send bulk messages?

share|improve this question
add comment

migrated from stackoverflow.com Jun 11 '12 at 13:59

This question came from our site for professional and enthusiast programmers.

3 Answers

up vote 1 down vote accepted

The send_message controller is vulnerable to automation.

If I can discover a list of user ids I could create the following html page:

<!DOCTYPE html>
<html>
<head>
    <meta charset="utf-8">
    <title>Bulk Sender</title>
</head>
<body>
    <form action="yoursite/send_message" method="POST">
        <input type="text" id="ids" name="ids" value="1,2,3,4,5,6,7,8,9,..."><br>
        <textarea id="msg" name="msg"></textarea>
        <input type="submit" value="Send">
    </form>
</body>
</html>

To use that, a user would simply need to sign in to your site and then load up this page. Worse, an attacker who has discovered potential victims on your site may be able to fool one victim into opening a session and then loading a page similar to this that automatically sends a hidden form on behalf of the victim. This is commonly known as a Cross Site Request Forgery.

Dealing with CSRFs is not entirely trivial. The easiest way to deal with it is to protect the vulnerable areas with a form of expiring validation that is unique to the form. I like to use a timestamp and hash, like this:

//in jquery method:
<?php
    $ts = generate_timestamp()
?>
$.post("<?=base_url()?>message/send_message", {"ids":uid,
    "msg":msg,"ts": "<?=$ts?>",
    "csrfhash": "<?=csrf_hash($ts)?>},

//elsewhere
function csrf_hash($ts) {
    return hash('sha256', $ts . //timestamp limits potential csrf window
        $this->session->userdata('session_id') . //session id provides decent base
                                                 //entropy, but is available in the 
                                                 //user's cookie so potentially XSS
        $this->users->get_user_salt()); //the user's password salt will stop XSS attempts
}

//in send_message
$ts = $this->input->post('ts');
if(csrf_hash($ts) != $this->input->post('csrfhash') && date($ts) > now()->add_minutes(-5)) {
    return error('Please refresh the page and try sending the message again. ' .
        'It was caught by the spam filter due to taking more than 5 minutes to write. ' .
        'You may select the text and copy it before refreshing the page if you wish.');
}

(after substituting or writing the code that I didn't provide; you can probably do better than the error message I added as well)

CodeIgnitor does have built in CSRF protection (scroll to bottom), but I don't know how to use it (I've never written any significant php before and have never used this project) and the documentation is lacking (as evident by the comment in the source of that page).

share|improve this answer
add comment
up vote 3 down vote

First of all you should use site_url() in your jquery code instead of base_url() as it provides url flexibility. For more info visit http://codeigniter.com/user_guide/helpers/url_helper.html

And in your controller code i suggest you to use bulk insert instead of loop insert. So you may use this piece of code:

foreach( $pieces as $user_id2)
{
    $insert_arr[] = array('user_id1'=> $user_id, 'user_id2'=> $user_id2, 'message'=>$msg);
}

$this->db->insert_batch('messages', $insert_arr);
share|improve this answer
add comment
up vote 2 down vote

Today I noticed that one of user sent bulk messages to 200 users. How he was able to do that? Suggestions to make code secure are welcome.

  $pieces=explode(",", $ids);
    foreach ($pieces as &$user_id2) {
    $this->db->insert('messages', array('user_id1' => $user_id,'user_id2' => $user_id2,'message' => $msg)); 
    }

You controller is splitting the recipients user_id by comma. It doesn't have any limit on how many user_ids can be entered and processed by the controller. You can impose that limit to be just one recipient user by using $pieces[0] instead of using foreach loop. 

$pieces=explode(",", $ids);   
    $this->db->insert('messages', array('user_id1' => $user_id,'user_id2' => $pieces[0],'message' => $msg));

You can use count and for loop to limit the number of recipients. Like here it is 10.

$pieces=explode(",", $ids); 
$recipients = count($pieces);

if ($recipients <= 10) {
        for ($i = 0; $i <= $recipients; $i++) {
$this->db->insert('messages', array('user_id1' => $user_id,'user_id2' => $pieces[i],'message' => $msg)); 
}
    } else {

 for ($i = 0; $i <= 10; $i++) {
$this->db->insert('messages', array('user_id1' => $user_id,'user_id2' => $pieces[i],'message' => $msg)); 
}
}
share|improve this answer
    
Bad idea. This doesn't stop the CSRF; it only makes it slightly more annoying for an attacker. It also doesn't reduce your network load like the answer provided by @ShayanHusaini –  Bill Barry Jun 11 '12 at 22:58
    
@BillBarry - That's right, this a basic rate limit functionality. I didn't consider CSRF token without knowing if he had that ability or no. :) –  Kapish M Jun 12 '12 at 4:07
    
@FirstnameLastname, CSRF in codeigniter is built in, which can be used easily. So there is no point of ability here. –  Jadzia Jun 13 '12 at 3:24
add comment

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.