java - Does the ModelDriven interface poses a security explot in struts2?

Question

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.

Answer 1

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