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
