Very true Don, and there are plenty of other quick fixes (of which we've implemented one).
However, I believe this is a flaw that should be addressed on the core level. It's ticketed now anyway, so we'll see what the symfony devs say. On 10 May 2010 15:56, Don Pinkster <[email protected]> wrote: > 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]<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]<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
