Does the ModelDriven interface poses a security explot in struts2?

background: I coded a struts2 ActionSupport class with ModelDriven. It's a hibernate/spring web app, using OSIV and attached entities in the view (JSP).

I received this email today from the architect 'punishing' me for putting an object that had a reference to an attached entity on the struts2 valuestack via the ModelDriven<E> interface. Is he correct or what? Obviously, this is a serious thing I am doing but I am not following what he is saying, and I really don't feel like taking up his offer and visiting him at his desk after this. oh boy. Time to change careers.

--- from the architect ---

Billy, as we previously discussed, you are still making the same mistakes in your code over and over again. This is the forth time you have made this error and I'm concerned about the quality of your work. It's one thing to make this once or even twice, but after the forth time, I am wondering if you are unable to comprehend what I am saying. The following will spell it out for you. If you don't get it after reading this email, then come to my desk and we'll go over it. This has to stop immediately, and I want all your code refactored before the end of the day correcting this mistake. If any code like this bleeds into production, we'll have a serious security problem on our hands. Also note that I am copying Dave on this so that a proper reprimand can be issued. I am also going to recommend to Dave that you be moved from a Level III to Level II developer. Read the following and please learn it, and refactor all your code as I've indicated.

About the binding objects:

When a Struts2 action class is marked with ModelDriven interface, the model will be bound to the form elements in the HTML page. For example, if an HTML form has a field called userName and an action class is defined as:

public class UserAction extends ActionSupport implements ModelDriven

And UserModel is a POJO as follows:

public class UserModel {
  private String userName;

  public String getUserName() {
      return userName;
  }

  public void setUserName(String userName) { 
      this.userName = userName;
  }
}

When the form is submitted, as long as the Action contains an instance of UserModel, struts2 will bind the field userName to UserModel.userName, automagically populating the value.

This simplicity has a high cost for malicious users, however. If an object is declared as ModelDriven, the end-user, the browsing user that is, has access to the models graph via the models setters. Take this case for example:

public class UserAction extends ActionSupport implements ModelDriven

and...

public class UserModel {
  private String userName;
  private UserEntity userEntity;

  public String getUserName() {
      return userName;
  }

  public void setUserName(String userName) { 
      this.userName = userName;
  }

  pubic UserEntity getUserEntity() {
      return userEntity;
  }
}

and...

@Entity
public class UserEntity {
    private String password;

    public String getPassword() {
        return password;
    }

    public void setPassword(String password) {
        this.password开发者_如何转开发 = password;
    }
 }

assuming the OSIV pattern is being used, and the entity UserEntity is attached.

A crafty user with a bit of fore knowledge or time on his hands may:

/myform?userName=billy&userEntity.password=newpassword

assuming the Entity is saved at the end of the session, the above results in changing billy's password.

The point is, the object graph is available!

When using ModelDriven, and using the alternative is a horrible approach, you must define fine grained models that are placed on the valuestack, and then copy from the model to the target object before sending the response and allowing the transaction to commit.

Your architect is right, putting objects with access to sensitive information on the ValueStack poses a potential security risk. A malicious user could indeed reset the password with the above attack.

BUT:

Since he is an architect he should have designed ways for proper validation/restriction of input parameters. Using the ParamsInterceptor in Struts2 it's fairly easy to only allow specific parameters to be passed to an action. Thus, it's not your work that sucks, it's your system's architecture. Developers should be able to focus on implementing business logic. The infrastructure must be provided by the architect.

Cheers,

w

ModelDriven interceptor is blind

Yes Model Interface could be a source of security issue, if you do not handle incoming parameters you will face security holes.

You have to use parameter interceptor.

In the struts.xml change your params interceptor as:

<interceptor-ref name="params">
    <param name="excludeParams">\w+((\.\w+)|(\[\d+\])|(\(\d+\))|(\['\w+'\])|(\('\w+'\)))*</param>
</interceptor-ref>

Then in your action implement ParameterNameAware and write acceptableParameterName.

public class sample implements ParameterNameAware(){
        public boolean acceptableParameterName(String parameterName) {  
       if (("username".equals(parameterName) || 
            "firstname".equals(parameterName) ||
            "lastname".equals(parameterName))
            return true;
        else
           return false;
    }

}  

The above is important, if your User pojo has lots of other properties and only some of them should be get from user.

If you use lots of ModelDriven actions you can make it general.

Create an base action which extends the ParameterNameAware . Then try to develop a general approach to have a list of your action and valid parameters :

We used spring to read the list of actions and their acceptable parameters. In the spring xml we added:

<util:properties id="actionsValidParameters"
    location="classpath:/configs/actions-valid-parameters.properties" />

The actions-valid-parameters.properties is as :

save-user=username,description,firstname,lastname
save-address=zipcode,city,detail,detail.addLine1,detail.addLine2,detail.no

Hint, If Address Object has an Detail object and you want to fill some properties in the Detail object, make sure you include the 'detail' in above list.

The action is as

public class BaseActionSupport extends ActionSupport implements ParameterNameAware
{

@Resource(name = "actionsValidParameters")
public Properties actionsValidParameters;

@Override
public boolean acceptableParameterName(String parameterName) {

    String actionName = ActionContext.getContext().getName();
     String validParams = (String) actionsValidParameters.get(actionName);

    //If the action is not defined in the list, it is assumed that the action  can accept all parameters. You can return false so if the action is not in the list no parameter is accepeted. It is up to you!
    if(StringUtils.isBlank(validParams))
        return true;
    // Search all the list of parameters. 
            //You can split the validParams to array and search array.  
    Pattern pattern = Pattern.compile("(?<=^|,)" + parameterName
            + "(?=,|$)");
    Matcher matcher = pattern.matcher(validParams);
    boolean accepeted = matcher.find();
    LOG.debug(
            "The {} parameter is {} in action {}, Position (excluding the action name) {} , {} , mathced {} ",
            parameterName, accepeted, actionName, matcher.start(), matcher.end(),
            matcher.group());
    return accepeted;
    }

}

Now wrtie your actions as

  public class UserAction extends BaseActionSupport implements  
        ModelDriven<User>{


}