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] For more options, visit this group at http://groups.google.com/group/symfony-devs?hl=en
