Re: Potential Security Flaw in Struts MVC

Mon, 07 May 2001 14:15:18 -0700

Yes I think that's a problem; interesting that you would do it that way, I never saw it from that perspective. But I believe the intent of Struts (e.g. the examples, etc) is that your ActionForms are really just forms -- conduits for moving field values between HTML forms and Java primitives. They shouldn't really involve themselves with your model at all. Keep your model interactions in your Action perform()'s.

Other comments I've seen in this vein are with regards to validation. In your ActionForms' validate methods, you should validate the data for basic type-correctness (e.g. did the user really enter a date in the date field?). But for business-model validation, save it for the Action class (e.g. did the user enter a date not already booked?). This is sort of a two-phase validation approach which separates basic validation logic from business model logic.

Bryan



Jeff Trent wrote:
018501c0d739$fa597bd0$6401a8c0@PROVIDENCE">
Ah, this maybe a problem in the way I've adapted Struts.  I reflect all UserForm method calls directly into the contained User object owned by the UserForm.  So for instance, I have
 
public class UserForm extends ActionsForm
{
    protected User user;
 
    ...
 
    public String getName()
    {
        return user.getName();
    }
 
    public void setName(String name)
    {
        user.setName(name);
    }
 
    ...
 
}
 
Now can you begin to see my original concern?  Maybe I need to separate the model from the form a little more than what I have.
 
- jeff
 
----- Original Message -----
Sent: Monday, May 07, 2001 4:38 PM
Subject: Re: Potential Security Flaw in Struts MVC

Either you are misunderstanding Struts, or I am misunderstanding you.

Struts will populate your UserForm for you, prior to your UserAction being called. However, it is your responsibility to, within UserAction, copy the values from UserForm to User.

Bryan


Jeff Trent wrote:
00bd01c0d728$40864960$6401a8c0@PROVIDENCE" type="cite">
Bryan,
 
This is good advice.  However, I thought the beans are populated off of the request outside of the control of my Action class derivation.  Therefore, copyProperties() doesn't pertain.
 
- jeff
----- Original Message -----
Sent: Monday, May 07, 2001 1:14 PM
Subject: Re: Potential Security Flaw in Struts MVC

There is a security risk here as you describe, if (and only if) you are using a generic introspection-based function (like Struts' PropertyUtils.copyBean) to copy the values from the UserForm object to the User object. There are several ways to avoid this --

1. Don't put an admin flag "setter" method in your User class.
2. In UserAction, don't use the generic bean copy utility -- instead, manually copy the values, excluding the admin flag.
3. As a smarter alternative to #2, don't use a generic bean copy utility -- instead, write an intelligent copy function in the User class, which "knows" that it's copying FROM a UserForm, TO a User, and therefore, skip the copying of the Admin flag.

Bryan



Jeff Trent wrote:
002501c0d70b$9df009a0$6401a8c0@PROVIDENCE" type="cite">
I may be wrong about this (only been working w/ Struts for a week now).  But I do see a potential security flaw in struts that I would like to hear from others regarding.
 
Consider a simple set of struts classes that represent a user in a system. You would probably have classes that look something like this:
    User                (the model representing the user)
    UserForm        (an enrollment form for a new user)
    UserAction        (Saves the UserForm information to db, etc)
   
The User class would have accessors and modifiers like getFirstName(), setFirstName(), getAdministrativeUserFlag(), setAdministrativeUserFlag(), etc.  The basic implementation of the UserForm is to take the UI form data, introspect the beans, and call the correct modifier of the UserForm bean based on the fields contained within the UI submission/form.  A developer of course would not expose the "Administrative User Flag" option on the UI for enrollment (that would be found possibly in some other administrative-level module).  However, if someone is familiar with the db schema and the naming convention the developer used, that user could subvert the application by writing his own version of the UI which contains an "Administrative User Flag" field (or any other field for that matter) and the basic form processing in Struts will kindly honor the request and set the "Administrative Flag" on the user.  Unless, of course, the developer makes special provisions to prevent this behavior.  However, its not entirely obvious to the struts user (in my opinion) that this is even a concern.  Am I making sense here?
 
- jeff
 



Reply via email to