As far as I see, a quick fix is to unset the id key in your parameters array before you bind it to your form. That way it will not override the id with the merge.
2010/5/10 Stephen Melrose <[email protected]>: > I've submitted a ticket for this, > > http://trac.symfony-project.org/ticket/8639 > > Thank you for your feedback. Feel free to explain things better than I have > as I'm useless at explaining a point sometimes. > > As Tom stated, I think the confusion over where the vulnerability lies is > justification enough that most developers will miss this and it might need > addressing in the core. > > > On 10 May 2010 15:36, Stephen Melrose <[email protected]> wrote: >> >> The problem being you can't remove the ID for some of the built in >> validators as they expect the ID to be in the tainted values. >> >> Plus, most developers won't be aware this is happening as they will do all >> their checks on the object before it goes into the form. >> >> I'm going to report this on trac as I don't believe it's right. >> >> On 10 May 2010 15:33, Vincent Jousse <[email protected]> wrote: >>> >>> Le 10/05/10 15:54, Tom Boutell a écrit : >>>> >>>> It says hidden fields are not removed. The id is a hidden field. >>>> >>> >>> The patch of Thomas removes the hidden fields. Btw the actual >>> implementation of useFields does not. >>> >>> To avoid this kind of problem, I'm now using swFormHelper::useOnly from >>> Thomas's swFormExtraPlugin. >>> You should give it a try. >>>> >>>> On Mon, May 10, 2010 at 9:43 AM, Thomas Rabaix<[email protected]> >>>> wrote: >>>> >>>>> >>>>> You should read this : http://trac.symfony-project.org/ticket/6100 >>>>> >>>>> On Mon, May 10, 2010 at 3:35 PM, Tom Boutell<[email protected]> wrote: >>>>> >>>>>> >>>>>> Actually, you don't set $this->Blog in the code you showed either, so >>>>>> I really have no idea whether your code is secure. (: >>>>>> >>>>>> 2010/5/10 Michał Piotrowski<[email protected]>: >>>>>> >>>>>>> >>>>>>> 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 >>>>>>> >>>>>>> >>>>>> >>>>>> >>>>>> -- >>>>>> 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 >>>>>> >>>>> >>>>> >>>>> -- >>>>> Thomas Rabaix >>>>> http://rabaix.net >>>>> >>>>> -- >>>>> 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 >> > > -- > 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
