It says hidden fields are not removed. The id is a hidden field. 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 >
-- 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
