Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Addressing Mass Assignment vulnerabilities with @NoBind annotation for domain objects [SPR-13835] #18408

Open
spring-projects-issues opened this issue Jan 4, 2016 · 4 comments
Labels
in: web type: enhancement
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Jan 4, 2016

Anton Abashkin opened SPR-13835 and commented

Most modern frameworks provide a convenience mechanism for binding form data to a domain object. Spring MVC is no exception.

While such a function is certainly indispensable, it exposes the application to the risk of a mass assignment vulnerability http://www.hpenterprisesecurity.com/vulncat/en/vulncat/java/mass_assignment_sensitive_field_exposure.html

A very serious vulnerability, this is actually how GitHub got hacked back in 2012 http://www.infoq.com/news/2012/03/GitHub-Compromised

Currently, Spring MVC does address this issue by providing two methods:

http://docs.spring.io/autorepo/docs/spring-framework/3.1.x/javadoc-api/org/springframework/validation/DataBinder.html#setAllowedFields%28java.lang.String...%29

http://docs.spring.io/autorepo/docs/spring-framework/3.1.x/javadoc-api/org/springframework/validation/DataBinder.html#setDisallowedFields%28java.lang.String...%29

While obviously a step in the right direction, this approach is not always optimal, the reason being the developer must set this in each controller using @InitBinder. This duplicates effort and is error prone.

One alternative for the developer is to create Data Transfer Objects (DTOs). While this does indeed provide protection, as well as other architectural benefits, it is not practiced by many developers.

I believe the optimal way to address this is to do something similar to what the the Play Framework does by providing a @NoBind annotation: https://www.playframework.com/documentation/1.4.x/controllers#nobinding

public class User extends Model {
@NoBinding("profile") public boolean isAdmin;
@As("dd, MM yyyy") Date birthDate;
public String name;
}

public static void editProfile(@As("profile") User user) {

}

Expressing these constraints directly in the domain object using annotations seems optimal from both a development and security auditing point of view

Thoughts?


Issue Links:

  • #12403 Provide support for configuring the bindable properties of a form-backing object using field-level annotations
  • #19337 Ability to suppress "rejectedValue" in error responses

2 votes, 5 watchers

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jan 4, 2016

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jan 5, 2016

Juergen Hoeller commented

We have been considering such a mechanism before - either whitelisting bindable fields or blacklisting non-binding fields through an annotation as you suggest - but never got around to work out the details. I'll take your request as an opportunity to revisit this topic for 4.3...

Juergen

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jan 5, 2016

Juergen Hoeller commented

Technically, we would implement such an auto-disallow for specifically annotated fields at the DataBinder level, which is also where the present "allowedFields" / "disallowedFields" mechanism resides. We can easily make the annotation type configurable and simply provide a convenience @NoBind (whatever the name) annotation that Spring MVC configures by default.

The lower-level BeanWrapper has a different mission and should remain capable of binding to any property.

Juergen

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Mar 20, 2016

Juergen Hoeller commented

Unfortunately, this turns out out to be non-trivial to implement at the DataBinder level, in particular with respect to the traversal of nested paths. We'll have to revisit the BeanWrapper versus DataBinder relatonship in a more comprehensive fashion, so I'm afraid this is rather a 5.0 topic in general... and we're very close to our 4.3 deadline already.

Juergen

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web type: enhancement
Projects
None yet
Development

No branches or pull requests

1 participant