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]<symfony-devs%[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]<symfony-devs%[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]<symfony-devs%[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]<symfony-devs%[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]<symfony-devs%[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]<symfony-devs%[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
