Forms, Security, and Action tags

[eluser]gullah[/eluser]
Hello everybody,

pretty simple quick question. I'm looking for the most secure way to have users submit data from a form. Hidden variables can be changed in the browser so that doesn't seem to really work.

What I'm wondering is if this could be beaten.

bad guy wants to edit something that is considered 'locked' and has the id of 35

So they go to a page that isn't locked and has the id of 36 and isn't locked. This form has the action set as '/users/edit/36'. Would it be possible for an attacker to change that form action and how can I prevent this or guarantee that this is the form that they should be editing.

Thanks in advance

Drew

[eluser]Thorpe Obazee[/eluser]
Identify them and give users privileges. Have them only access the id that they can edit. Whenever they access pages that they shouldn't have access, don't show them anything except a, 'you're not allowed here' page.

[eluser]gullah[/eluser]
I have user authentication and users are only allowed to edit certain things yes. What I'm worried about is them altering form data to edit something else.

The site I'm working on is a community site and how I have it set up now they can edit generic items and they would have the form action of songs/edit/[id]. I am curious if someone could alter the action to edit a different song for example.

[eluser]gullah[/eluser]
Turns out yes, yes you can with a Dom inspector, so basically I need to come up with a database flag or something to say if they are editable.

[eluser]Thorpe Obazee[/eluser]
[quote author="drewtown" date="1228119930"]Turns out yes, yes you can with a Dom inspector, so basically I need to come up with a database flag or something to say if they are editable.[/quote]

Yep. using a database flag is the way to go.

[eluser]Future Webs[/eluser]
what i do in this situation is ..

and im guessing that you have the users id stored along with the data that is being edited.

first up, when the page loads cross reference the users id from the session with the users id of the data being edited. if they dont match dont show the form and either redirect or show a notice of "your not allowed here, your IP has been logged etc"

I would also not store the users id as a hidden field in your form as people could see that in the source and maybe change it. Instead do this in the model or controller wherever you are building the array to pass to the update. Nobody has access to the model or controller and the array is not listening for any POST values for the users id and instead takes them from the session

The less you put in the form the better. If you can hard code it into the controller or model rather then passing it back and forth as a POST the less chance there is of anything being changed that should not be

[eluser]Michael Wales[/eluser]
You say you already have a user authentication system ensuring they have access to view the current edit form, let's say: /users/edit/36

If you make the form submit to itself (/users/edit/36) you can kill two birds with one stone.

First, you are removing the hidden form field with the ID - you can now use the ID from the URL. Secondly, your authentication testing for that user accessing that particular controller method will prevent them from altering other records.

A code example:

Code:

function edit($id = NULL) {
  // Check to make sure they provided an ID
  // If they did, use our model to ensure this user has access to this ID
  // Our user_id is 1 but you would pull this from session or something
  if (($id === NULL) || (!$this->user->has_access('1', $id))) {
    // Send them to a list of all entries - they can't be here
    redirect('user/view');
    return;
  }

  // Come this point we are sure the user is allowed to be here, handle the form

  $this->validation->set_rules();
  $this->validation->set_fields();

  if ($this->validation->run()) {
    // Form was submitted, passed validation, do our database stuff
  }

  $this->load->view('entry/edit');
}

[eluser]gullah[/eluser]
Thanks for the replies but I'm afraid Michael what you were suggesting is what I'm trying to avoid. I had the form action set like that and it is possible to change that. I left the action with the id in it but I added security in the function, here is what I did.

let me know if you see any problems with it.

Code:

function lyrics()
    {
        //check if user is logged in to add/submit lyrics
        $this->freakauth_light->check();
        
        //load form_validation class
        $this->load->library('form_validation');
        
        //get the action and the songId from the URI
        $action = $this->uri->segment(3);
        $songId = $this->uri->segment(4);
        
        //run the query to get album, artist, and song information
        $this->db->join('albums', 'albums.album_id = songs.album_id');
        $this->db->join('artists', 'artists.artist_id = songs.artist_id');
        $this->db->where('song_id', $songId);
        $query = $this->db->get('songs');

        //if the song exists do this
        if($query->num_rows() > 0)
        {    
            //get this information ready to go to the view
            $row = $query->row();
            $data['artist'] = $row->artist;
            $data['album'] = $row->album;
            $data['song'] = $row->song;
            $data['songId'] = $row->song_id;
            
            //if there has been a post we will go in here
            if(isset($_POST['Lyrics']))
            {
                //set the rules for lyrics
                $this->form_validation->set_rules('Lyrics', 'Lyrics', 'trim|required|xss_clean|alpha_dash|min_length[30]');
        
                //run the validation
                if($this->form_validation->run() == FALSE)
                {
                    //validation failed
                    $data['title'] = 'Error in your submission';
                    $this->template->load('template_main', 'songs/lyrics', $data);
                } else {
                    //validation passed
                    $ok = false;
                    
                    //check to see if this song already exists in the lyrics table
                    $this->db->where('song_id', $data['songId']);
                    $lyricsQuery = $this->db->get('lyrics');
                    
                    //if it doesn't set ok to true
                    if($lyricsQuery->num_rows() == 0)
                    {
                        $ok = true;
                    }  
                    
                    //if it does make sure verified is not set to 1 --this here prevents users from altering form to populate another 'visible' song
                    $lyricsRow = $lyricsQuery->row();
                    if($ok == true || $lyricsRow->lyrics_verified == -1)
                    {
                        //change the lyrics \n\r to <br />'s
                        $this->load->library('lyrics');
                        $cleanLyrics = $this->lyrics->addBreaks($this->input->post('Lyrics'));    
                        
                        //load the submit model
                        $this->load->model('submitmodel');
                        $created_by = $this->db_session->userdata('user_name');
                        
                        $this->submitmodel->submitLyrics($cleanLyrics, $data['songId'], $created_by);
                        
                        //set the flash to let them know we will review it
                        $this->db_session->set_flashdata('flashMessage', 'Thank you for your submission. The lyrics will be reviewed in the next 24 hours');
                        redirect('songs/view/' . $data['artist'] . '/' . $data['album'] . '/' . $data['song']);    

                    } else {
                        //if they end up here we know they changed the form
                        $data['error'] = 'You really had to try to get here, therefore you IP, username, and e-mail have all been logged';
                        $this->template->load('template_main', '404', $data);
                    }
                }
            
            } else {
                
                //if there is no post data it will display the form
                
                $action = $this->uri->segment(3);
                $song = $this->uri->segment(4);
                if($action == 'add')
                {
                    $data['title'] = 'Add Lyrics';
                    $this->template->load('template_main', 'songs/lyrics', $data);
                }
            }
        } else {
            //if the song doesn't exist send them to a 404 page
            $data['error'] = 'Song does not exist';
            $this->template->load('template_main', '404', $data);
        }
        $this->output->enable_profiler(TRUE);    
    }