Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

SecurityComponent & HtmlHelper check select values. #1967

Closed
lh-import opened this Issue · 21 comments

4 participants

@lh-import

Created by Sam Mousa, 6th Jun 2012. (originally Lighthouse ticket #2938):


What I did

  • Enabled the securitycomponent
  • Created a form containing a SELECT field:
$this->Html->input('test', array('type' => 'select', 'options' => array(1, 2, 3)));
  • Tested security with FireBug.

What happened

  • I changed the submitted value to 5 and it did not blackhole my request.

What I expected to happen

  • Since the SecurityComponent does check hidden fields for their contents I would expect it to check any fields for which it knows the possible inputs.

I realize this might not be a bug but instead a feature request. Possible additions to the security component include:

  • Check HTML5 patterns (reg exps) for validity.

Possible solution

  • Serialize the options array for each select field and add it to the form as a hidden field.
  • Upon submission of the form the validation unserialize the hidden field and check if the related select field has a valid value.

This does not introduce any need for persistent storage, it does however increase computations done at the time of submission and generation of the form.

@lh-import

6th Jun 2012, ADmad said:


Patches with test cases are welcome.

@lh-import

6th Jun 2012, Sam Mousa said:


Also noted some weird / unnecessary behavior while looking into the source of the formhelper:

Starting at line 540 of /lib/Cake/View/Helper/FormHelper.php:

$out = $this->hidden('_Token.fields', array(
            'value' => urlencode($fields . ':' . $locked),
            'id' => 'TokenFields' . mt_rand()
        ));
        $out .= $this->hidden('_Token.unlocked', array(
            'value' => urlencode($unlocked),
            'id' => 'TokenUnlocked' . mt_rand()
        ));

The fields' get a randomly assigned ID however this seems to serve no purpose (the ids are not even sent back to the server)... Anyone know why this is done?

-- Will try to at least create some failing test cases.

@lh-import

6th Jun 2012, Sam Mousa said:


So... created the failing test case.
-- See it in action at http://test.befound.nl/test.php

Test case:

public function testSecureSelect() {

        // Run startup.
        $this->Controller->Security->startup($this->Controller);    
        // Create a form.
        $this->Form->create('Model');
        // Array with values for select.
        $options = array(1, 2, 3);
        $this->Form->input('selectInput', array('type' => 'select', 'options' => $options));
        $text = $this->Form->secure($this->Form->fields);
//        $this->assertEmpty($text, $text);
        // Get the hash.
        $hash = substr($text, 83, strpos($text, '"', 83)- 82);

        $key = $this->Controller->request->params['_Token']['key'];
//        $this->assertEmpty($hash, $hash);
        $fields = $hash;
        $unlocked = '';
        $data = array(
            'Model' => array('selectInput' => '10'),
            '_Token' => compact('key', 'fields', 'unlocked')
        );
        $this->Controller->request->data = $data;
        $this->assertFalse($this->Controller->Security->validatePost($this->Controller), "Incorrect value for select input was accepted.");
     }

Find the full file including setup and tear down in the attachment.

@lh-import

6th Jun 2012, Sam Mousa said:


Oh, sorry for the shameless copying of the original test file...

Let me know if it is possible / desirable to get the test case into the core test without a patch!

Will look into a patch if I've got time but don't count on it.

@lh-import

6th Jun 2012, Mark Story said:


For the love of all that is good do not use serialize() with data that is sent to the client. Its a huge security issue to send serialized data to a browser and then deserialize it upon submission.

Also SecurityComponent is not meant to replace proper validation, which is what you should be doing with your posted data. All tests need to be passing in order to be merged in, so I don't think you'll be able to get a failing test for an un-implemented feature merged in.

@lh-import

6th Jun 2012, Sam Mousa said:


Mark, serialization is no security issue since that is exactly what the security component already secures... the contents of hidden fields...

If the argument is it does not replace other validations then why do we even check the contents of hidden fields? The argument should be used consistently or not at all.

@lh-import

6th Jun 2012, Mark Story said:


No serialize() is a security concern. The hidden field + locked field data is encoded with json which is not dangerous.

@lh-import

7th Jun 2012, Sam Mousa said:


Please explain the attack vector you are suggesting... I am pretty sure you are not getting what I'm suggesting.

If the serialized data is added a hidden field and subsequently a hash of it's value is added there is no way to change the data and therefore it is guaranteed to be unchanged...

@lh-import

7th Jun 2012, Sam Mousa said:


The keyword is unchecked user input... its not unchecked since it's signed with a salted hash... guys this is what the securitycomponent does....

@lh-import

7th Jun 2012, Mark Story said:


Or you could just use JSON and not have to worry about either screwing that up, or accidentally opening holes. Seems that's simpler and safer than using serialize().

@lh-import

8th Jun 2012, Sam Mousa said:


Anyway, we're getting side-tracked. I'm fine with using json.. my use of the word serialize referred more to the "turning it into a string" part than specifically requiring the use of the PHP serialize() function.

Still the question remains: do we need / want this kind of validation?

As noted before, input validation is already done at the model level. However authorization is usually not done at that level; consider the case where I as a user am allowed to create a new Image but only in my own Gallery...
The gallery_id field must be numerical (which is checked in the Image model); however checking which galleries I am allowed to use is done at the controller level; it is done for creating the select element... why require the developer to run the same check twice?

As long as it is clear what the SecurityComponent does and does not secure this seems like an improvement to me...

-- As a side note: Isn't it cleaner to use a separate library for securing the form and validating the post?
-- Currently if I were to make any changes I would have to change both the FormHelper and the SecurityComponent. It seems to me that consolidating these functions (creating the verification data and verifying it) decreases the cohesion between the otherwise unrelated classes FormHelper and SecurityComponent.

I'll try to work on a patch this weekend.

@lh-import

8th Jun 2012, Mark Story said:


I don't want or need it. I think its a waste of CPU and limits the ability of developers to do thing with Javascript to populate select elements. The same could be said for the hidden input locking, however hidden inputs are generally not modified by users and that feature already exists. I'm a big -1 for this.

@mrahmadt

Hello Everyone,

if Cakephp developers think this is not required, then I think at least you should make a note about this in cakephp document, because most of people think cakephp will also validate the select value changes and issue blackhole.

@markstory
Owner

I don't think SecurityComponent should ever replace validation which is what I feel this issue is really wanting. The only values we can efficiently and properly ensure are not changed are those in hidden inputs.

@mrahmadt
@markstory
Owner

But you can prevent people from doing that by enforcing validation.

@mrahmadt
@mrahmadt
@markstory
Owner

Ok, I can clarify the docs around what tampering prevention means.

@ravage84

I think we can close this after @markstory 's improvements in the docs.

As an option, we could add to the new note, that

preventing select options from being added/changed

should be secured through data validation rules.

@markstory markstory closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.