2010/5/10 Tom Boutell <[email protected]>:
> This is a dangerous behavior of the standard Symfony Doctrine and
> Propel CRUD module generators (and the admin generator as well).
>
> The Doctrine admin generator and CRUD generator (presumably Propel
> too) generate redundant code in which the object is fetched based on
> an ID in the route, but the form still contains an ID.
>
> Sure, the generated code gets away with this because it has no per-row
> validation anyway, but it still does not make sense and it is
> dangerous because the conspicuous IDs in the generated CRUD code mask
> the presence of the "sneaky," redundant ID field in the form. The
> minute you try to add any validation of who's supposed to be doing
> what at the controller level, you've got a big security hole and you
> don't know it.
Hmmm... I'm doing things like
public function editCommon($request)
{
[..]
$this->logged_user_id = $this->UserData['logged_user_id'];
[..]
$this->forward404Unless($this->logged_user_id == $this->Blog->getUserId());
[..]
}
public function executeEdit(sfWebRequest $request)
{
$this->editCommon($request);
$this->form = new BlogForm($this->Blog);
}
public function executeUpdate(sfWebRequest $request)
{
$this->forward404Unless($request->isMethod(sfRequest::POST) ||
$request->isMethod(sfRequest::PUT));
$this->editCommon($request);
$this->form = new BlogForm($this->Blog);
$this->processForm($request, $this->form);
$this->setTemplate('edit');
}
I'm always checking logged user_id against user_id of record creator.
I don't see vulnerability here.
>
> The CRUD generator and admin generator should generate form subclasses
> that unset the ID field, unset it directly at the action level, or
> verify that it's the same as the ID coming from the route and ignore
> it entirely in the create action, IMHO. Otherwise it's misleading and
> a danger in exactly the way you suggest.
>
> In our own projects we unset($this['id']) in the configure method of
> our Doctrine forms.
>
> 2010/5/10 Michał Piotrowski <[email protected]>:
>> Hi,
>>
>> 2010/5/10 Stephen Melrose <[email protected]>:
>>> Hi,
>>>
>>> We have discovered what could be a potential flaw in the form
>>> framework. The reason I'm discussing this here is because I'm in mixed
>>> feelings as to whether this is actually bug or not, or rather poor
>>> implementation on our part. Either way, I'm also saying this flaw
>>> should be safe guarded against.
>>>
>>> We discovered that a malicious user can use the forms generated by the
>>> form framework to edit content they shouldn't be able to.
>>>
>>> They do this by replacing the primary ID in the hidden form field with
>>> that of the record they want to edit. When they hit save, the
>>> validation is run, and the Object is updated with the new ID, so when
>>> the save() is called, the other row is updated.
>>>
>>> Now, if we (as in developers) want to restrict editing of content for
>>> certain users, then it is our responsibility to make sure we put safe
>>> guards in place. I'm not arguing this fact.
>>>
>>> The reason I believe this to be a problem is how users will actually
>>> guard their code. Most people (including myself) run all the safe
>>> guard checks before the Object is passed into the Form on
>>> construction. I don't then expect the POST data to override the
>>> primary key of the Object on save. Infact, I can't think of an
>>> instance I would ever want this to happen.
>>>
>>> I therefore propose that some sort of restriction/block is put in
>>> place by default that stops the PK of an Object being altered on
>>> bind().
>>>
>>> Thoughts?
>>
>> I create a methods like newCommon() or editCommon() with all safe
>> checks and call them from new/create, edit/update.
>>
>> The main reason for this is that you _always_ _need_ to perform the
>> same checks in new and create as well in edit and update. Why?
>>
>> For example - user want to create a comment to blog post
>> - new method is called - all safe checks pass well
>> - form is rendered
>> - user write his comment
>> - other user delete his blog post
>> - user tries to write his comment
>>
>> If you wont do the same check in create method you failed :)
>>
>> IMHO it's a security vulnerability, but it's not symfony fault.
>>
>> And BTW. CSRF protection should do the trick for form protection
>>
>>>
>>> Stephen Melrose
>>>
>>
>> Regards,
>> Michal
>>
>> --
>> If you want to report a vulnerability issue on symfony, please send it to
>> security at symfony-project.com
>>
>> You received this message because you are subscribed to the Google
>> Groups "symfony developers" group.
>> To post to this group, send email to [email protected]
>> To unsubscribe from this group, send email to
>> [email protected]
>> For more options, visit this group at
>> http://groups.google.com/group/symfony-devs?hl=en
>>
>
>
>
> --
> Tom Boutell
> P'unk Avenue
> 215 755 1330
> punkave.com
> window.punkave.com
>
> --
> If you want to report a vulnerability issue on symfony, please send it to
> security at symfony-project.com
>
> You received this message because you are subscribed to the Google
> Groups "symfony developers" group.
> To post to this group, send email to [email protected]
> To unsubscribe from this group, send email to
> [email protected]
> For more options, visit this group at
> http://groups.google.com/group/symfony-devs?hl=en
>
--
If you want to report a vulnerability issue on symfony, please send it to
security at symfony-project.com
You received this message because you are subscribed to the Google
Groups "symfony developers" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to
[email protected]
For more options, visit this group at
http://groups.google.com/group/symfony-devs?hl=en
