If it's worth anything, I agree with you Tom :-) The goal of CSRF is to have a hidden value in the session. I do not see any practical use to adding the class_name to this hidden value and I see how it can be annoying. In my opinion this was a zealous effort which reminds me of the following ticket, where an effort to generate a really really unique token results in an actually not unique at all token ! http://trac.symfony-project.org/ticket/7018
Since in security matters you cannot afford an error, I think we developers should fight the desire to mix thousands of parameters in a sha1 to make it seem more "secret/unique" and just use the simplest but most efficient method. Fabrice Bernhard -- http://www.theodo.fr On Feb 25, 6:58 pm, Tom Boutell <[email protected]> wrote: > Hi Kris, > > I did much the same thing. But I think it's worth questioning whether > adding the form class adds much security (I'd say "no") and therefore > whether it makes sense to do it. Even if that's just a question for > 2.0 at this point. > > On Thu, Feb 25, 2010 at 12:33 PM, Kris Wallsmith > > > > > > <[email protected]> wrote: > > Hi Tom, > > The forms are built with the assumption that the same form class which > > renders will be the class that cleans the submitted data. I think this is > > the case most of the time, and the CSRF token is as secure as it can be > > within those constraints. > > You can of course disable CSRF protection and implement it yourself, if you > > choose. I've done this in the past by creating an include_csrf_field() > > helper and then calling $request->checkCSRFProtection() in the action. > > You can also customize the CSRF behavior in BaseForm as of symfony 1.3. For > > example, I've overriden and added some methods to expose a static > > BaseForm::setCSRFSalt() method, which may what you want to do in the > > instance you describe. > > Hope that helps! > > Kris > > > -- > > Kris Wallsmith | Release Manager > > [email protected] > > Portland, Oregon USA > >http://twitter.com/kriswallsmith > > On Feb 25, 2010, at 9:17 AM, Andrei Dziahel wrote: > > > Hi. > > > Well, I think it's not worth it. But it's better ho hear some words from > > core team people. May they will enlighten us? > > > 2010/2/25 Tom Boutell <[email protected]> > > >> No replies to this at all... does anyone have an opinion on whether > >> including the class name in CSRF verification is worth the trouble it > >> creates? > > >> On Feb 8, 2:46 pm, Tom Boutell <[email protected]> wrote: > >> > I implemented support for Symfony's excellent CSRF protection in forms > >> > today. We had some forms we were rendering field by field, and so of > >> > course we had problems because we weren't calling > >> > $form->renderHiddenFields where we didn't think we had any. No > >> > problem, I've taken care of that, and I appreciate what a great > >> > feature this is to have "in the box" in Symfony. > > >> > But I still had problems with a few actions. These are cases where I > >> > need the user to supply some information on the first pass, such as > >> > uploading several images, and then provide more details on a second > >> > pass (you don't want to edit descriptions of images you aren't even > >> > looking at yet). > > >> > My solution to this in the past has been to use two different form > >> > classes (often subclasses of the same parent), with different but > >> > compatible field sets. The first form takes a subset of the fields > >> > required for the second, so everything works. I use an extra > >> > "first_pass" parameter to clue in the second form that it shouldn't > >> > generate huffy validation errors, just quietly request all the > >> > as-yet-missing info. I suspect this is a fairly common pattern. > > >> > Unfortunately I ran into a snag with CSRF. Turns out the CSRF token is > >> > built like this: > > >> > md5($secret.session_id().get_class($this)); > > >> > There are several pieces used here to arrive at the key that's hashed: > > >> > The secret, unique to the application as a whole (usually) > >> > The user's PHP session ID > >> > The form class > > >> > The third one is what bit me. I worked around it by removing > >> > .get_class($this) in my own override of this method in the form > >> > classes where I need to be able to submit data first to one form class > >> > and then to another. > > >> > This leads to some questions: > > >> > 1. Why include the form class in the key? The number of form classes > >> > in a particular application is relatively small. This only helps when > >> > form data for a currently live session has already been intercepted > >> > (which would mean you probably have the cookies too anyway) AND the > >> > form you're hoping to CSRF is a different form. That seems like a > >> > pretty narrow case, and it causes problems for legitimate applications > >> > like mine. > > >> > 2. Just for curiosity: why have an application secret either? The PHP > >> > session ID is already unique, and you can't discover it unless you > >> > have the user's cookie for the Symfony-based site, which you won't if > >> > you're just dumping carefully crafted links and forms into an > >> > unrelated site to go phishing. I guess there's a small risk if you're > >> > using an incrementing MySQL ID as the session ID and it's very early > >> > in the lifetime of the site. The application secret certainly isn't > >> > hurting anything, I'm just curious about this part. > > >> > -- > >> > 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 > > > -- > > With the best regards, Andrei. > > > -- > > 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
