>
> Stephen, can you elaborate a little?
>
I added a sfValidatorPropelUnique to a "display_name" field. But
sfValidatorPropelUnique requires the PK to be in the tainted values array in
order to exclude the record being edited from the check.
See line 136,
if (!isset($values[$column]) or $object->$method() != $values[$column])
So instead I added this to BaseFormPropel (all our PKs are ID),
public function bind(array $taintedValues = null, array $taintedFiles =
null)
{
if (array_key_exists('id', $taintedValues) &&
$this->getObject() instanceof BaseObject &&
method_exists($this->getObject(), 'isNew') &&
!$this->getObject()->isNew())
{
$taintedValues['id'] = $this->getObject()->getId();
}
parent::bind($taintedValues, $taintedFiles);
}
CSRF protection is no help against a user who has limited legitimate
> privileges carrying out a malicious act to take control of an object
> that is not theirs.
>
Exactly my point. This is nothing to do with CSRF. Even the default
behaviour of the admin generator lets you do this.
I could edit other peoples profile (that was the hole in our app), I could
edit records hidden from me, even though the checks were being done on the
object retrieved by sfRouting.
This is a very sneaky hole a lot of developers will miss I fear.
Certainly. I have a user object with a property "display_name"
On 10 May 2010 15:15, Tom Boutell <[email protected]> wrote:
> Stephen, can you elaborate a little? Did you add that validator
> yourself for some reason? If it's a validator on the id field and you
> unset the id field from the form, it shouldn't fire.
>
> On Mon, May 10, 2010 at 10:05 AM, Stephen Melrose <[email protected]>
> wrote:
> > I tried removing the ID field, but sfValidatorPropelUnique stopped
> > working correctly.
> >
> > Instead, we put some code in the BaseFormPropel that changed the
> > binded ID back to the ID of the object, effectively rendering it read
> > only.
> >
> > On 10 May, 13:12, Tom Boutell <[email protected]> wrote:
> >> 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.
> >>
> >> 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 athttp://
> 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
>
--
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
